On Sat, 16 Apr 2022 at 00:13, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
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".
I have been thinking about your approach for quite some time. I think that enforcing a use of auxilliary device is an overkill. What do we really need is the the set callbacks in the bus struct or a notifier. We have to notify the aux_bus controller side that the client has been probed successfully or that the client is going to be removed.
It seems like these new callbacks would be nearly the same as the probe/remove callbacks in my proposal except:
- They rely on there being exactly 1 AUX device, or we make it a rule
that we wait for all AUX devices to probe (?)
Is the backlight a separate device on an AUX bus? Judging from drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just a point-to-point bus, so there is always a single client.
- We need to come up with a system for detecting when everything
probes or is going to be removed, though that's probably not too hard. I guess the DP AUX bus could just replace the panel's probe function with its own and essentially "tail patch" it. I guess it could "head patch" the remove call? ...or is there some better way you were thinking of knowing when all our children probed?
- The callback on the aux bus controller side would not be able to
DEFER. In other words trying to acquire a reference to the panel can always be the last thing we do so we know there can be no reasons to defer after. This should be doable, but at least in the ps8640 case it will require changing the code a bit. I notice that today it actually tries to get the panel side _before_ it gets the MIPI side and it potentially can return -EPROBE_DEFER if it can't find the MIPI side. I guess I have a niggling feeling that we'll find some reason in the future that we can't be last, but we can probably ignore that. ;-)
I can switch this all to normal callbacks if that's what everyone wants, but it doesn't feel significantly cleaner to me and does seem to have some (small) downsides.
And this approach would make driver's life easier, since e.g. the bus code can pm_get the EP device before calling callbacks/notifiers and pm_put_autosuspend it afterwards.
Not sure about doing the pm calls on behalf of the EP device. What's the goal there?
I think any driver can pm_runtime_get another device. The goal is to let the 'post_probe' callback to power up the panel, read the EDID, etc.
BTW: as I'm slowly diving into DP vs eDP differences. Do we need to write the EDID checksum like we do for DP? Do you have any good summary for eDP vs DP differences?