[PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()

[Biju Das]

Hi Luca,

> -----Original Message-----
> From: Luca Ceresoli <luca.ceresoli at bootlin.com>
> Sent: 28 April 2026 14:20
> Subject: Re: [PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()
> 
> On Tue Apr 28, 2026 at 1:58 PM CEST, Biju Das wrote:
> > Hi all,
> >
> >> -----Original Message-----
> >> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf
> >> Of Biju Das
> >> Sent: 28 April 2026 12:50
> >> Subject: RE: [PATCH v2 08/11] drm/bridge: adv7511: switch to
> >> of_drm_get_bridge_by_endpoint()
> >>
> >>
> >> Hi,
> >>
> >> Thanks for the patch.
> >>
> >> > -----Original Message-----
> >> > From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf
> >> > Of Luca Ceresoli
> >> > Sent: 28 April 2026 10:16
> >> > Subject: [PATCH v2 08/11] drm/bridge: adv7511: switch to
> >> > of_drm_get_bridge_by_endpoint()
> >> >
> >> > This driver calls drm_of_find_panel_or_bridge() with a NULL pointer
> >> > in the @panel parameter, thus using a reduced feature set of that function.
> >> > Replace this call with the simpler of_drm_get_bridge_by_endpoint().
> >> >
> >> > Since of_drm_get_bridge_by_endpoint() increases the refcount of the
> >> > returned bridge, ensure it is put on removal. To achieve this,
> >> > instead of adding an explicit drm_bridge_put(), migrate to the
> >> > bridge::next_bridge pointer which is
> >> automatically put when the bridge is eventually freed.
> >> >
> >> > Signed-off-by: Luca Ceresoli <luca.ceresoli at bootlin.com>
> >> > ---
> >> >  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 -
> >> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 11 +++++------
> >> >  2 files changed, 5 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> >> > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> >> > index 8be7266fd4f4..12c95198d9a4 100644
> >> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> >> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> >> > @@ -354,7 +354,6 @@ struct adv7511 {
> >> >  	enum drm_connector_status status;
> >> >  	bool powered;
> >> >
> >> > -	struct drm_bridge *next_bridge;
> >> >  	struct drm_display_mode curr_mode;
> >> >
> >> >  	unsigned int f_tmds;
> >> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> > index 6bd76c1fb007..498e38579a0f 100644
> >> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> > @@ -851,8 +851,8 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
> >> >  	struct adv7511 *adv = bridge_to_adv7511(bridge);
> >> >  	int ret = 0;
> >> >
> >> > -	if (adv->next_bridge) {
> >> > -		ret = drm_bridge_attach(encoder, adv->next_bridge, bridge,
> >> > +	if (adv->bridge.next_bridge) {
> >> > +		ret = drm_bridge_attach(encoder, adv->bridge.next_bridge,
> >> > +bridge,
> >> >  					flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >> >  		if (ret)
> >> >  			return ret;
> >> > @@ -1251,10 +1251,9 @@ static int adv7511_probe(struct i2c_client
> >> > *i2c)
> >> >
> >> >  	memset(&link_config, 0, sizeof(link_config));
> >> >
> >> > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, NULL,
> >> > -					  &adv7511->next_bridge);
> >> > -	if (ret && ret != -ENODEV)
> >> > -		return ret;
> >> > +	adv7511->bridge.next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, -1);
> >> > +	if (IS_ERR(adv7511->bridge.next_bridge) && PTR_ERR(adv7511->bridge.next_bridge) != -ENODEV)
> >> > +		return PTR_ERR(adv7511->bridge.next_bridge);
> >>
> >> Does it crash, if the error is  -EPROBE_DEFER?
> >
> > I see a crash with patch [1], which is fixed by avoiding the direct assignment.
> 
> Ah, dammit! Good catch, thanks for the quick testing and follow-up!
> 
> Indeed this driver assumes next_bridge is either NULL or a valid pointer, but due to the 'if(IS_ERR()
> && some_other_condition)' now it can also be -ENODEV (not -EPROBE_DEFER, but that's irrelevant).
> 
> This affects the msm and zynqmp_dp patches equally.
> 
> I'm sending a v3 soon with these fixed. I'm just not sure which approach to use to fix (same for all
> the 3 patches). Alternatives are:
> 
>  1. -ENODEV is accepted, set next_bridge to NULL when it happens:
> 
>       -       if (IS_ERR(adv7511->bridge.next_bridge) && PTR_ERR(adv7511->bridge.next_bridge) != -
> ENODEV)
>       -               return PTR_ERR(adv7511->bridge.next_bridge);
>       +       if (IS_ERR(adv7511->bridge.next_bridge)) {
>       +               if (PTR_ERR(adv7511->bridge.next_bridge) == -ENODEV)
>       +                       adv7511->bridge.next_bridge = NULL;
>       +               else
>       +                       return PTR_ERR(adv7511->bridge.next_bridge);

The point is you cannot return PTR_ERR as it will lead to crash, if it is
direct assignment.

   if (IS_ERR(adv7511->bridge.next_bridge)) {
	int err = PTR_ERR(adv7511->bridge.next_bridge);
	adv7511->bridge.next_bridge = NULL;
	return err;
  }

Cheers,
Biju

>  2. let nexxt_bridge hold -ENODEV but ignore it when it is used (only in
>     the attach op, for all 3 drivers):
> 
>       -       if (adv->bridge.next_bridge) {
>       +       if (!IS_ERR_OR_NULL(adv->bridge.next_bridge)) {
> 
> While the latter approach involves less code, it might let errors sneak in in case new usages of
> next_bridge are added with just a NULL check.
> 
> Opinions about the two?
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com