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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Oct 31, 2024 at 12:45:24AM +0800, 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.

There's a relevant discussion for a ti-sn65dsi86 patch, see
https://lore.kernel.org/r/20241030102846.GB14276@pendragon.ideasonboard.com

I agree it shouldn't be the responsibility of each caller to generate
unique IDs.

> 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.
> 
> 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.
> 
> 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.

-- 
Regards,

Laurent Pinchart