Quoting Douglas Anderson (2022-04-08 19:36:23)
As talked about in the kerneldoc for "struct dp_aux_ep_client" in this patch and also in the past in commit a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the DP AUX bus properly we really need two "struct device"s. One "struct device" is in charge of providing the DP AUX bus and the other is where we'll try to get a reference to the newly probed endpoint devices.
In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver is already broken up into several "struct devices" anyway because it also provides a PWM and some GPIOs. Adding one more wasn't that difficult / ugly.
When I tried to do the same solution in parade-ps8640, it felt like I was copying too much boilerplate code. I made the realization that I didn't _really_ need a separate "driver" for each person that wanted to do the same thing. By putting all the "driver" related code in a common place then we could save a bit of hassle. This change effectively adds a new "ep_client" driver that can be used by anyone. The devices instantiated by this driver will just call through to the probe/remove/shutdown calls provided.
At the moment, the "ep_client" driver is backed by the Linux auxiliary bus (unfortunate naming--this has nothing to do with DP AUX). I didn't want to expose this to clients, though, so as far as clients are concerned they get a vanilla "struct device".
Signed-off-by: Douglas Anderson dianders@chromium.org
Is it correct that the struct dp_aux_ep_client is largely equivalent to a struct dp_aux_ep_device? What's the difference? I think it is a fully probed dp_aux_ep_device? Or a way to tell the driver that calls of_dp_aux_populate_ep_devices() that the endpoint has probed now?
I read the commit text but it didn't click until I read the next patch that this is solving a probe ordering/loop problem. The driver that creates the 'struct drm_dp_aux' and populates devices on the DP aux bus with of_dp_aux_populate_ep_devices() wants to be sure that the drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in edp-panel is going to be there before calling drm_of_get_bridge().
of_dp_aux_populate_ep_devices(); [No idea if the bridge is registered yet] drm_of_get_bridge();
The solution is to retry the drm_of_get_bridge() until 'struct dp_aux_ep_driver' probes and registers the next bridge. It looks like a wait_for_completion() on top of drm_of_get_bridge() implemented through driver probe and -EPROBE_DEFER; no disrespect!
Is there another problem here that the driver that creates the 'struct drm_dp_aux' and populates devices on the DP aux bus with of_dp_aux_populate_ep_devices() wants to use that same 'struct drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was populated? And the 'struct dp_aux_ep_device' may either not be probed and bound to a 'struct dp_aux_ep_driver' or it may not be powered on because it went to runtime PM suspend?
Put another way, I worry that the owner of 'struct drm_dp_aux' wants to use some function in include/drm/dp/drm_dp_helper.h that takes the 'struct drm_dp_aux' directly without considering the device model state of the endpoint device (the 'struct dp_aux_ep_device'). That would be a similar problem as waiting for the endpoint to be powered on and probed. Solving that through a sub-driver feels awkward.
What if we had some function like drm_dp_get_aux_ep() that took a 'struct drm_dp_aux' and looked for the endpoint device (there can only be one?), waited for it to be probed, and then powered it up via some pm_runtime_get_sync()? My understanding is that with edp-panel we have two power "domains" of importance, the panel power domain and the DP/eDP power domain. Access to the AUX bus via 'struct drm_dp_aux' needs both power domains to be powered on. If the 'struct dp_aux_ep_device' is in hand, then both power domains can be powered on by making sure the power state of the 'struct dp_aux_ep_device::dev' is enabled. If only the 'struct drm_dp_aux' is in hand then we'll need to do something more like find the child device and power it on.
Could the 'struct drm_dp_aux' functions in drm_dp_helper.h do this automatically somehow? Maybe we can drop a variable in 'struct drm_dp_aux' when of_dp_aux_populate_ep_devices() is called that the other side may not be powered up. Then if something tries to transfer on that aux channel it powers on all children of 'struct drm_dp_aux::dev' that are on the 'dp_aux_bus_type' or bails out if those devices haven't probed yet or can't be powered on.
diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h index 4f19b20b1dd6..ecf68b6873bd 100644 --- a/include/drm/dp/drm_dp_aux_bus.h +++ b/include/drm/dp/drm_dp_aux_bus.h @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv, struct module *owner); void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
+/**
- struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
dp_aux_ep_client?
- The DP AUX bus can be a bit tricky to use properly. Usually, the way
- things work is that:
- The DP controller driver provides the DP AUX bus and would like to probe
- the endpoints on the DP AUX bus (AKA the panel) as part of its probe
- routine.
- The DP controller driver would also like to acquire a reference to the
- DP AUX endpoints (AKA the panel) as part of its probe.
- The problem is that the DP AUX endpoints aren't guaranteed to complete their
- probe right away. They could be probing asynchronously or they simply might
- fail to acquire some resource and return -EPROBE_DEFER.
- The best way to solve this is to break the DP controller's probe into
- two parts. The first part will create the DP AUX bus. The second part will
- acquire the reference to the DP AUX endpoint. The first part can complete
- finish with no problems and be "done" even if the second part ends up
s/finish//
- deferring while waiting for the DP AUX endpoint.
- The dp_aux_ep_client structure and associated functions help with managing
- this common case. They will create/add a second "struct device" for you.
create and add a second
- In the probe for this second "struct device" (known as the "clientdev" here)
- you can acquire references to the AUX DP endpoints and can freely return
- -EPROBE_DEFER if they're not ready yet.
- A few notes:
- The parent of the clientdev is guaranteed to be aux->dev
- The of_node of the clientdev is guaranteed to be the same as the of_node
- of aux->dev, copied with device_set_of_node_from_dev().
- If you're doing "devm" type things in the clientdev's probe you should
- use the clientdev. This makes lifetimes be managed properly.
- NOTE: there's no requirement to use these helpers if you have solved the
- problem described above in some other way.
- */