[v5] Clean and update tilcdc driver to support DRM_BRIDGE_ATTACH_NO_CONNECTOR

[PATCH v5 20/25] drm/tilcdc: Use devm_drm_of_get_bridge() helper

Posted by Kory Maincent (TI.com) 2 weeks ago

Replace drm_of_find_panel_or_bridge() with the newer
devm_drm_of_get_bridge() helper which simplifies the code by:
- Automatically handling both panel and bridge cases internally
- Managing the panel-to-bridge conversion when needed
- Using devres for resource management, eliminating manual cleanup

This removes the need for explicit panel-to-bridge conversion via
devm_drm_panel_bridge_add_typed() and the associated error handling path.

Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
---

Change in v4:
- New patch
---
 drivers/gpu/drm/tilcdc/tilcdc_encoder.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_encoder.c b/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
index a34a10337f6a8..546fe7e6ee815 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
@@ -55,15 +55,12 @@ int tilcdc_encoder_create(struct drm_device *ddev)
 	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(ddev);
 	struct tilcdc_encoder *encoder;
 	struct drm_bridge *bridge;
-	struct drm_panel *panel;
-	int ret;
 
-	ret = drm_of_find_panel_or_bridge(ddev->dev->of_node, 0, 0,
-					  &panel, &bridge);
-	if (ret == -ENODEV)
+	bridge = devm_drm_of_get_bridge(ddev->dev, ddev->dev->of_node, 0, 0);
+	if (PTR_ERR(bridge) == -ENODEV)
 		return 0;
-	else if (ret)
-		return ret;
+	else if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
 
 	encoder = drmm_simple_encoder_alloc(ddev, struct tilcdc_encoder,
 					    base, DRM_MODE_ENCODER_NONE);
@@ -73,12 +70,5 @@ int tilcdc_encoder_create(struct drm_device *ddev)
 	}
 	priv->encoder = encoder;
 
-	if (panel) {
-		bridge = devm_drm_panel_bridge_add_typed(ddev->dev, panel,
-							 DRM_MODE_CONNECTOR_DPI);
-		if (IS_ERR(bridge))
-			return PTR_ERR(bridge);
-	}
-
 	return tilcdc_attach_bridge(ddev, bridge);
 }

-- 
2.43.0

Re: [PATCH v5 20/25] drm/tilcdc: Use devm_drm_of_get_bridge() helper

Posted by Luca Ceresoli 1 week ago

Hi Kory,

On Fri Jan 23, 2026 at 5:12 PM CET, Kory Maincent (TI.com) wrote:
> Replace drm_of_find_panel_or_bridge() with the newer
> devm_drm_of_get_bridge() helper which simplifies the code by:
> - Automatically handling both panel and bridge cases internally
> - Managing the panel-to-bridge conversion when needed
> - Using devres for resource management, eliminating manual cleanup
>
> This removes the need for explicit panel-to-bridge conversion via
> devm_drm_panel_bridge_add_typed() and the associated error handling path.
>
> Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>

I'm OK with this patch, based on the v4 discussion. I have a question
however, see below.

> --- a/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_encoder.c
> @@ -55,15 +55,12 @@ int tilcdc_encoder_create(struct drm_device *ddev)
>  	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(ddev);
>  	struct tilcdc_encoder *encoder;
>  	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
> -	int ret;
>
> -	ret = drm_of_find_panel_or_bridge(ddev->dev->of_node, 0, 0,
> -					  &panel, &bridge);
> -	if (ret == -ENODEV)
> +	bridge = devm_drm_of_get_bridge(ddev->dev, ddev->dev->of_node, 0, 0);
> +	if (PTR_ERR(bridge) == -ENODEV)
>  		return 0;
> -	else if (ret)
> -		return ret;
> +	else if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);
>
>  	encoder = drmm_simple_encoder_alloc(ddev, struct tilcdc_encoder,
>  					    base, DRM_MODE_ENCODER_NONE);
> @@ -73,12 +70,5 @@ int tilcdc_encoder_create(struct drm_device *ddev)
>  	}
>  	priv->encoder = encoder;
>
> -	if (panel) {
> -		bridge = devm_drm_panel_bridge_add_typed(ddev->dev, panel,
> -							 DRM_MODE_CONNECTOR_DPI);

You are introducing a subtle difference here: while you pass the connector
type to devm_drm_panel_bridge_add_typed(), devm_drm_of_get_bridge() does
not take it and expects it to ahve been set previously and errors out if it
hasn't.

Can you ensure the connector type is alway set before this
devm_drm_of_get_bridge() call?

All the changes to the driver in the previous patches of this series make
it hard to find that out from here.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 20/25] drm/tilcdc: Use devm_drm_of_get_bridge() helper

Posted by Kory Maincent 4 days, 21 hours ago

On Fri, 30 Jan 2026 20:31:11 +0100
"Luca Ceresoli" <luca.ceresoli@bootlin.com> wrote:

> Hi Kory,
> 
> On Fri Jan 23, 2026 at 5:12 PM CET, Kory Maincent (TI.com) wrote:
> > Replace drm_of_find_panel_or_bridge() with the newer
> > devm_drm_of_get_bridge() helper which simplifies the code by:
> > - Automatically handling both panel and bridge cases internally
> > - Managing the panel-to-bridge conversion when needed
> > - Using devres for resource management, eliminating manual cleanup
> >
> > This removes the need for explicit panel-to-bridge conversion via
> > devm_drm_panel_bridge_add_typed() and the associated error handling path.

...

> >  	encoder = drmm_simple_encoder_alloc(ddev, struct tilcdc_encoder,
> >  					    base, DRM_MODE_ENCODER_NONE);
> > @@ -73,12 +70,5 @@ int tilcdc_encoder_create(struct drm_device *ddev)
> >  	}
> >  	priv->encoder = encoder;
> >
> > -	if (panel) {
> > -		bridge = devm_drm_panel_bridge_add_typed(ddev->dev, panel,
> > -
> > DRM_MODE_CONNECTOR_DPI);  
> 
> You are introducing a subtle difference here: while you pass the connector
> type to devm_drm_panel_bridge_add_typed(), devm_drm_of_get_bridge() does
> not take it and expects it to ahve been set previously and errors out if it
> hasn't.
> 
> Can you ensure the connector type is alway set before this
> devm_drm_of_get_bridge() call?

The connector type should be set by the bridge or the panel driver.

https://elixir.bootlin.com/linux/v6.18.6/source/drivers/gpu/drm/bridge/panel.c#L397
 * This function is deprecated and should not be used in new drivers. Use
 * devm_drm_panel_bridge_add() instead, and fix panel drivers as necessary if
 * they don't report a connector type.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v5 20/25] drm/tilcdc: Use devm_drm_of_get_bridge() helper

Posted by Luca Ceresoli 4 days, 14 hours ago

On Mon Feb 2, 2026 at 10:57 AM CET, Kory Maincent wrote:
> On Fri, 30 Jan 2026 20:31:11 +0100
> "Luca Ceresoli" <luca.ceresoli@bootlin.com> wrote:
>
>> Hi Kory,
>>
>> On Fri Jan 23, 2026 at 5:12 PM CET, Kory Maincent (TI.com) wrote:
>> > Replace drm_of_find_panel_or_bridge() with the newer
>> > devm_drm_of_get_bridge() helper which simplifies the code by:
>> > - Automatically handling both panel and bridge cases internally
>> > - Managing the panel-to-bridge conversion when needed
>> > - Using devres for resource management, eliminating manual cleanup
>> >
>> > This removes the need for explicit panel-to-bridge conversion via
>> > devm_drm_panel_bridge_add_typed() and the associated error handling path.
>
> ...
>
>> >  	encoder = drmm_simple_encoder_alloc(ddev, struct tilcdc_encoder,
>> >  					    base, DRM_MODE_ENCODER_NONE);
>> > @@ -73,12 +70,5 @@ int tilcdc_encoder_create(struct drm_device *ddev)
>> >  	}
>> >  	priv->encoder = encoder;
>> >
>> > -	if (panel) {
>> > -		bridge = devm_drm_panel_bridge_add_typed(ddev->dev, panel,
>> > -
>> > DRM_MODE_CONNECTOR_DPI);
>>
>> You are introducing a subtle difference here: while you pass the connector
>> type to devm_drm_panel_bridge_add_typed(), devm_drm_of_get_bridge() does
>> not take it and expects it to ahve been set previously and errors out if it
>> hasn't.
>>
>> Can you ensure the connector type is alway set before this
>> devm_drm_of_get_bridge() call?
>
> The connector type should be set by the bridge or the panel driver.
>
> https://elixir.bootlin.com/linux/v6.18.6/source/drivers/gpu/drm/bridge/panel.c#L397
>  * This function is deprecated and should not be used in new drivers. Use
>  * devm_drm_panel_bridge_add() instead, and fix panel drivers as necessary if
>  * they don't report a connector type.

Ah, right, so it means this patch might cause some regressions due to panel
drivers to be fixed. So it's fine as long as you are ready to handle such
regression, should any happen.

Also other patches did exactly the same, e.g.:

  commit 6e1853589ea6 ("drm/lcdif: switch to devm_drm_of_get_bridge")
  commit a43dd76bacd0 ("drm/vc4: dsi: Switch to devm_drm_of_get_bridge")

And so:
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com