Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Neil Armstrong <neil.armstrong@linaro.org>]

On 30/10/2024 17:45, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/10/18 23:43, Dmitry Baryshkov wrote:
>> On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
>>> The assignment of the of_node to the aux bridge needs to mark the
>>> of_node as reused as well, otherwise resource providers like pinctrl will
>>> report a gpio as already requested by a different device when both pinconf
>>> and gpios property are present.
>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>
>>> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
>>> Cc: stable@vger.kernel.org      # 6.8
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>> Changes in v2:
>>> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
>>> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
>>> - Added Fixes tag and cc'ed stable
>>> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
>>> ---
>>>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> 
> Technically speaking, your driver just move the burden to its caller.
> Because this driver requires its user call drm_aux_bridge_register()
> to create an AUX child device manually, you need it call ida_alloc()
> to generate a unique id.
> 
> Functions symbols still have to leak to other subsystems, which is
> not really preserve coding sharing.

???

> 
> What's worse, the action that allocating unique device id traditionally
> is the duty of driver core. Why breaks (so called) perfect device driver
> model by moving that out of core. Especially in the DT world that the
> core knows very well how to populate device instance and manage the
> reference counter.

This has nothing to do with DT, auxiliary device is a nice way to actually
use the driver model to handle devices sub-functions without overloading
drivers. It's still young and we need to collectively solve some issues,
but it's now agreed auxiliary device helps designing multi-functions drivers.

> 
> HPD handling is traditionally belongs to connector, create standalone
> driver like this one *abuse* to both Maxime's simple bridge driver and
> Laurent's display-connector bridge driver or drm_bridge_connector or
> whatever. Why those work can't satisfy you? At least, their drivers
> are able to passing the mode setting states to the next bridge.

HPD handling is now shared along all the bridges, because it corresponds
to a reality.

It simply takes in account complex uses-cases like Type-C Altmode where
we need to describe the connection between the DP controller and the
Type-C retimers/muxes and properly propagate HPD events to synchronize
all the chain.

> 
> Basically those AUX drivers implementation abusing the definition of
> bridge, abusing the definition of connector and abusing the DT.
> Its just manually populate instances across drivers.

It abuses nothing, the DT representation of the full signal path
in the Type-C complex was required by DT bindings maintainers.

The fact we can describe an element of the Type-C Altmode DP
path is very handy, and we have the full control of the data
path unlike x86 platforms where all this handling is hidden in
closed firmwares.

If you have an issue with the aux-bridge design please open a separate
thread, because the actual patch has nothing to do with aux devices or DRM
bridge implementation.

Please do not respond to this thread except concerning this fix.

Neil

> 
> 
>