[PATCH 0/3] i.MX fsl-ldb cleanups

[PATCH 0/3] i.MX fsl-ldb cleanups

[Marco Felsch]

Hi,

just a small series to cleanup the fsl-ldb lvds bridge driver a bit.

Regards,
Marco

Marco Felsch (3):
  drm/bridge: fsl-ldb: make use of driver_private
  drm/bridge: fsl-ldb: make use of dev_err_probe
  drm/bridge: fsl-ldb: simplify device_node error handling

 drivers/gpu/drm/bridge/fsl-ldb.c | 57 ++++++++++++++------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

-- 
2.39.5

[PATCH 1/3] drm/bridge: fsl-ldb: make use of driver_private

[Marco Felsch]

Make use of the drm_bridge::driver_private data instead of
container_of() wrapper.

No functional changes.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0fc8a14fd800..fa29f2bf4031 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -99,11 +99,6 @@ static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
 	return (fsl_ldb->ch0_enabled && fsl_ldb->ch1_enabled);
 }
 
-static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
-{
-	return container_of(bridge, struct fsl_ldb, bridge);
-}
-
 static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
 {
 	if (fsl_ldb_is_dual(fsl_ldb))
@@ -115,7 +110,7 @@ static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
 static int fsl_ldb_attach(struct drm_bridge *bridge,
 			  enum drm_bridge_attach_flags flags)
 {
-	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	struct fsl_ldb *fsl_ldb = bridge->driver_private;
 
 	return drm_bridge_attach(bridge->encoder, fsl_ldb->panel_bridge,
 				 bridge, flags);
@@ -124,7 +119,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
 static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 				  struct drm_bridge_state *old_bridge_state)
 {
-	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	struct fsl_ldb *fsl_ldb = bridge->driver_private;
 	struct drm_atomic_state *state = old_bridge_state->base.state;
 	const struct drm_bridge_state *bridge_state;
 	const struct drm_crtc_state *crtc_state;
@@ -226,7 +221,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
 				   struct drm_bridge_state *old_bridge_state)
 {
-	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	struct fsl_ldb *fsl_ldb = bridge->driver_private;
 
 	/* Stop channel(s). */
 	if (fsl_ldb->devdata->lvds_en_bit)
@@ -270,7 +265,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 		   const struct drm_display_info *info,
 		   const struct drm_display_mode *mode)
 {
-	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	struct fsl_ldb *fsl_ldb = bridge->driver_private;
 
 	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
 		return MODE_CLOCK_HIGH;
@@ -309,6 +304,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	fsl_ldb->dev = &pdev->dev;
 	fsl_ldb->bridge.funcs = &funcs;
 	fsl_ldb->bridge.of_node = dev->of_node;
+	fsl_ldb->bridge.driver_private = fsl_ldb;
 
 	fsl_ldb->clk = devm_clk_get(dev, "ldb");
 	if (IS_ERR(fsl_ldb->clk))
-- 
2.39.5

[PATCH 2/3] drm/bridge: fsl-ldb: make use of dev_err_probe

[Marco Felsch]

Make use of dev_err_probe() to easily spot issues via the debugfs or
kernel log. No functional changes.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index fa29f2bf4031..e0a229c91953 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -308,11 +308,13 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 
 	fsl_ldb->clk = devm_clk_get(dev, "ldb");
 	if (IS_ERR(fsl_ldb->clk))
-		return PTR_ERR(fsl_ldb->clk);
+		return dev_err_probe(dev, PTR_ERR(fsl_ldb->clk),
+				     "Failed to get ldb clk\n");
 
 	fsl_ldb->regmap = syscon_node_to_regmap(dev->of_node->parent);
 	if (IS_ERR(fsl_ldb->regmap))
-		return PTR_ERR(fsl_ldb->regmap);
+		return dev_err_probe(dev, PTR_ERR(fsl_ldb->regmap),
+				     "Failed to get regmap\n");
 
 	/* Locate the remote ports and the panel node */
 	remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
@@ -335,12 +337,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	panel = of_drm_find_panel(panel_node);
 	of_node_put(panel_node);
 	if (IS_ERR(panel))
-		return PTR_ERR(panel);
+		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
 
 	fsl_ldb->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
 	if (IS_ERR(fsl_ldb->panel_bridge))
-		return PTR_ERR(fsl_ldb->panel_bridge);
-
+		return dev_err_probe(dev, PTR_ERR(fsl_ldb->panel_bridge),
+				     "drm panel-bridge add failed\n");
 
 	if (fsl_ldb_is_dual(fsl_ldb)) {
 		struct device_node *port1, *port2;
@@ -356,10 +358,9 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 					     "Error getting dual link configuration\n");
 
 		/* Only DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS is supported */
-		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
-			dev_err(dev, "LVDS channel pixel swap not supported.\n");
-			return -EINVAL;
-		}
+		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS)
+			return dev_err_probe(dev, -EINVAL,
+					     "LVDS channel pixel swap not supported.\n");
 	}
 
 	platform_set_drvdata(pdev, fsl_ldb);
-- 
2.39.5

[PATCH 3/3] drm/bridge: fsl-ldb: simplify device_node error handling

[Marco Felsch]

Make use of __free(device_node) to simplify the of_node_put() error
handling paths. No functional changes.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index e0a229c91953..cea9ddaa5e01 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -287,8 +287,9 @@ static const struct drm_bridge_funcs funcs = {
 static int fsl_ldb_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *panel_node;
-	struct device_node *remote1, *remote2;
+	struct device_node *panel_node __free(device_node) = NULL;
+	struct device_node *remote1 __free(device_node) = NULL;
+	struct device_node *remote2 __free(device_node) = NULL;
 	struct drm_panel *panel;
 	struct fsl_ldb *fsl_ldb;
 	int dual_link;
@@ -321,21 +322,16 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
 	fsl_ldb->ch0_enabled = (remote1 != NULL);
 	fsl_ldb->ch1_enabled = (remote2 != NULL);
-	panel_node = of_node_get(remote1 ? remote1 : remote2);
-	of_node_put(remote1);
-	of_node_put(remote2);
+	panel_node = remote1 ? remote1 : remote2;
 
-	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
-		of_node_put(panel_node);
+	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled)
 		return dev_err_probe(dev, -ENXIO, "No panel node found");
-	}
 
 	dev_dbg(dev, "Using %s\n",
 		fsl_ldb_is_dual(fsl_ldb) ? "dual-link mode" :
 		fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
 
 	panel = of_drm_find_panel(panel_node);
-	of_node_put(panel_node);
 	if (IS_ERR(panel))
 		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
 
@@ -345,14 +341,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 				     "drm panel-bridge add failed\n");
 
 	if (fsl_ldb_is_dual(fsl_ldb)) {
-		struct device_node *port1, *port2;
+		struct device_node *port1 __free(device_node) =
+			of_graph_get_port_by_id(dev->of_node, 1);
+		struct device_node *port2 __free(device_node) =
+			of_graph_get_port_by_id(dev->of_node, 2);
 
-		port1 = of_graph_get_port_by_id(dev->of_node, 1);
-		port2 = of_graph_get_port_by_id(dev->of_node, 2);
 		dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
-		of_node_put(port1);
-		of_node_put(port2);
-
 		if (dual_link < 0)
 			return dev_err_probe(dev, dual_link,
 					     "Error getting dual link configuration\n");
-- 
2.39.5

Re: [PATCH 2/3] drm/bridge: fsl-ldb: make use of dev_err_probe

[Laurent Pinchart]

Hi Marco,

Thank you for the patch.

On Thu, May 15, 2025 at 12:24:52AM +0200, Marco Felsch wrote:
> Make use of dev_err_probe() to easily spot issues via the debugfs or
> kernel log. No functional changes.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index fa29f2bf4031..e0a229c91953 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -308,11 +308,13 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  
>  	fsl_ldb->clk = devm_clk_get(dev, "ldb");
>  	if (IS_ERR(fsl_ldb->clk))
> -		return PTR_ERR(fsl_ldb->clk);
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->clk),
> +				     "Failed to get ldb clk\n");
>  
>  	fsl_ldb->regmap = syscon_node_to_regmap(dev->of_node->parent);
>  	if (IS_ERR(fsl_ldb->regmap))
> -		return PTR_ERR(fsl_ldb->regmap);
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->regmap),
> +				     "Failed to get regmap\n");
>  
>  	/* Locate the remote ports and the panel node */
>  	remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
> @@ -335,12 +337,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	panel = of_drm_find_panel(panel_node);
>  	of_node_put(panel_node);
>  	if (IS_ERR(panel))
> -		return PTR_ERR(panel);
> +		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
>  
>  	fsl_ldb->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>  	if (IS_ERR(fsl_ldb->panel_bridge))
> -		return PTR_ERR(fsl_ldb->panel_bridge);
> -
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->panel_bridge),
> +				     "drm panel-bridge add failed\n");
>  
>  	if (fsl_ldb_is_dual(fsl_ldb)) {
>  		struct device_node *port1, *port2;
> @@ -356,10 +358,9 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  					     "Error getting dual link configuration\n");
>  
>  		/* Only DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS is supported */
> -		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> -			dev_err(dev, "LVDS channel pixel swap not supported.\n");
> -			return -EINVAL;
> -		}
> +		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "LVDS channel pixel swap not supported.\n");
>  	}
>  
>  	platform_set_drvdata(pdev, fsl_ldb);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/bridge: fsl-ldb: simplify device_node error handling

[Laurent Pinchart]

Hi Marco,

On Thu, May 15, 2025 at 12:24:53AM +0200, Marco Felsch wrote:
> Make use of __free(device_node) to simplify the of_node_put() error
> handling paths. No functional changes.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index e0a229c91953..cea9ddaa5e01 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -287,8 +287,9 @@ static const struct drm_bridge_funcs funcs = {
>  static int fsl_ldb_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *panel_node;
> -	struct device_node *remote1, *remote2;
> +	struct device_node *panel_node __free(device_node) = NULL;
> +	struct device_node *remote1 __free(device_node) = NULL;
> +	struct device_node *remote2 __free(device_node) = NULL;
>  	struct drm_panel *panel;
>  	struct fsl_ldb *fsl_ldb;
>  	int dual_link;
> @@ -321,21 +322,16 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
>  	fsl_ldb->ch0_enabled = (remote1 != NULL);
>  	fsl_ldb->ch1_enabled = (remote2 != NULL);
> -	panel_node = of_node_get(remote1 ? remote1 : remote2);
> -	of_node_put(remote1);
> -	of_node_put(remote2);
> +	panel_node = remote1 ? remote1 : remote2;

This will cause a double put of panel_node, once due to __free() on
remote1 or remote2, and the second time due to __free() on panel_node.

>  
> -	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
> -		of_node_put(panel_node);
> +	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled)
>  		return dev_err_probe(dev, -ENXIO, "No panel node found");
> -	}
>  
>  	dev_dbg(dev, "Using %s\n",
>  		fsl_ldb_is_dual(fsl_ldb) ? "dual-link mode" :
>  		fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
>  
>  	panel = of_drm_find_panel(panel_node);
> -	of_node_put(panel_node);
>  	if (IS_ERR(panel))
>  		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
>  
> @@ -345,14 +341,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  				     "drm panel-bridge add failed\n");
>  
>  	if (fsl_ldb_is_dual(fsl_ldb)) {
> -		struct device_node *port1, *port2;
> +		struct device_node *port1 __free(device_node) =
> +			of_graph_get_port_by_id(dev->of_node, 1);
> +		struct device_node *port2 __free(device_node) =
> +			of_graph_get_port_by_id(dev->of_node, 2);
>  
> -		port1 = of_graph_get_port_by_id(dev->of_node, 1);
> -		port2 = of_graph_get_port_by_id(dev->of_node, 2);
>  		dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> -		of_node_put(port1);
> -		of_node_put(port2);
> -
>  		if (dual_link < 0)
>  			return dev_err_probe(dev, dual_link,
>  					     "Error getting dual link configuration\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm/bridge: fsl-ldb: make use of driver_private

[Laurent Pinchart]

Hi Marco,

Thank you for the patch.

On Thu, May 15, 2025 at 12:24:51AM +0200, Marco Felsch wrote:
> Make use of the drm_bridge::driver_private data instead of
> container_of() wrapper.

I suppose this is a personal preference, but I like usage of
container_of() better. In my opinion it conveys better that struct
fsl_ldb "unherits" from struct drm_bridge.

> No functional changes.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0fc8a14fd800..fa29f2bf4031 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -99,11 +99,6 @@ static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
>  	return (fsl_ldb->ch0_enabled && fsl_ldb->ch1_enabled);
>  }
>  
> -static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> -{
> -	return container_of(bridge, struct fsl_ldb, bridge);
> -}
> -
>  static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
>  {
>  	if (fsl_ldb_is_dual(fsl_ldb))
> @@ -115,7 +110,7 @@ static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
>  static int fsl_ldb_attach(struct drm_bridge *bridge,
>  			  enum drm_bridge_attach_flags flags)
>  {
> -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
>  
>  	return drm_bridge_attach(bridge->encoder, fsl_ldb->panel_bridge,
>  				 bridge, flags);
> @@ -124,7 +119,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
>  static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *old_bridge_state)
>  {
> -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
>  	struct drm_atomic_state *state = old_bridge_state->base.state;
>  	const struct drm_bridge_state *bridge_state;
>  	const struct drm_crtc_state *crtc_state;
> @@ -226,7 +221,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>  static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
>  				   struct drm_bridge_state *old_bridge_state)
>  {
> -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
>  
>  	/* Stop channel(s). */
>  	if (fsl_ldb->devdata->lvds_en_bit)
> @@ -270,7 +265,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>  		   const struct drm_display_info *info,
>  		   const struct drm_display_mode *mode)
>  {
> -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
>  
>  	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
>  		return MODE_CLOCK_HIGH;
> @@ -309,6 +304,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	fsl_ldb->dev = &pdev->dev;
>  	fsl_ldb->bridge.funcs = &funcs;
>  	fsl_ldb->bridge.of_node = dev->of_node;
> +	fsl_ldb->bridge.driver_private = fsl_ldb;
>  
>  	fsl_ldb->clk = devm_clk_get(dev, "ldb");
>  	if (IS_ERR(fsl_ldb->clk))

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm/bridge: fsl-ldb: make use of driver_private

[Marco Felsch]

Hi Laurent,

On 25-05-15, Laurent Pinchart wrote:
> Hi Marco,
> 
> Thank you for the patch.
> 
> On Thu, May 15, 2025 at 12:24:51AM +0200, Marco Felsch wrote:
> > Make use of the drm_bridge::driver_private data instead of
> > container_of() wrapper.
> 
> I suppose this is a personal preference, but I like usage of
> container_of() better. In my opinion it conveys better that struct
> fsl_ldb "unherits" from struct drm_bridge.

Yes, we can drop this patch if container_of() or to_fsl_ldb() is
preferred. I just saw the driver_private field and why not making use of
it since we do that a lot, same is true for container_of :)

Regards,
  Marco

> > No functional changes.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/fsl-ldb.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> > index 0fc8a14fd800..fa29f2bf4031 100644
> > --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> > @@ -99,11 +99,6 @@ static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
> >  	return (fsl_ldb->ch0_enabled && fsl_ldb->ch1_enabled);
> >  }
> >  
> > -static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> > -{
> > -	return container_of(bridge, struct fsl_ldb, bridge);
> > -}
> > -
> >  static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> >  {
> >  	if (fsl_ldb_is_dual(fsl_ldb))
> > @@ -115,7 +110,7 @@ static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> >  static int fsl_ldb_attach(struct drm_bridge *bridge,
> >  			  enum drm_bridge_attach_flags flags)
> >  {
> > -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
> >  
> >  	return drm_bridge_attach(bridge->encoder, fsl_ldb->panel_bridge,
> >  				 bridge, flags);
> > @@ -124,7 +119,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> >  static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> >  				  struct drm_bridge_state *old_bridge_state)
> >  {
> > -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
> >  	struct drm_atomic_state *state = old_bridge_state->base.state;
> >  	const struct drm_bridge_state *bridge_state;
> >  	const struct drm_crtc_state *crtc_state;
> > @@ -226,7 +221,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> >  static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
> >  				   struct drm_bridge_state *old_bridge_state)
> >  {
> > -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
> >  
> >  	/* Stop channel(s). */
> >  	if (fsl_ldb->devdata->lvds_en_bit)
> > @@ -270,7 +265,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> >  		   const struct drm_display_info *info,
> >  		   const struct drm_display_mode *mode)
> >  {
> > -	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > +	struct fsl_ldb *fsl_ldb = bridge->driver_private;
> >  
> >  	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
> >  		return MODE_CLOCK_HIGH;
> > @@ -309,6 +304,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> >  	fsl_ldb->dev = &pdev->dev;
> >  	fsl_ldb->bridge.funcs = &funcs;
> >  	fsl_ldb->bridge.of_node = dev->of_node;
> > +	fsl_ldb->bridge.driver_private = fsl_ldb;
> >  
> >  	fsl_ldb->clk = devm_clk_get(dev, "ldb");
> >  	if (IS_ERR(fsl_ldb->clk))
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 3/3] drm/bridge: fsl-ldb: simplify device_node error handling

[Marco Felsch]

Hi Laurent,

On 25-05-15, Laurent Pinchart wrote:
> Hi Marco,
> 
> On Thu, May 15, 2025 at 12:24:53AM +0200, Marco Felsch wrote:
> > Make use of __free(device_node) to simplify the of_node_put() error
> > handling paths. No functional changes.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/gpu/drm/bridge/fsl-ldb.c | 24 +++++++++---------------
> >  1 file changed, 9 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> > index e0a229c91953..cea9ddaa5e01 100644
> > --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> > @@ -287,8 +287,9 @@ static const struct drm_bridge_funcs funcs = {
> >  static int fsl_ldb_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > -	struct device_node *panel_node;
> > -	struct device_node *remote1, *remote2;
> > +	struct device_node *panel_node __free(device_node) = NULL;
> > +	struct device_node *remote1 __free(device_node) = NULL;
> > +	struct device_node *remote2 __free(device_node) = NULL;
> >  	struct drm_panel *panel;
> >  	struct fsl_ldb *fsl_ldb;
> >  	int dual_link;
> > @@ -321,21 +322,16 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> >  	remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
> >  	fsl_ldb->ch0_enabled = (remote1 != NULL);
> >  	fsl_ldb->ch1_enabled = (remote2 != NULL);
> > -	panel_node = of_node_get(remote1 ? remote1 : remote2);
> > -	of_node_put(remote1);
> > -	of_node_put(remote2);
> > +	panel_node = remote1 ? remote1 : remote2;
> 
> This will cause a double put of panel_node, once due to __free() on
> remote1 or remote2, and the second time due to __free() on panel_node.

Argh.. you're right. I drop the __free() from the panel_node.

Thanks,
  Marco

> 
> >  
> > -	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
> > -		of_node_put(panel_node);
> > +	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled)
> >  		return dev_err_probe(dev, -ENXIO, "No panel node found");
> > -	}
> >  
> >  	dev_dbg(dev, "Using %s\n",
> >  		fsl_ldb_is_dual(fsl_ldb) ? "dual-link mode" :
> >  		fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
> >  
> >  	panel = of_drm_find_panel(panel_node);
> > -	of_node_put(panel_node);
> >  	if (IS_ERR(panel))
> >  		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
> >  
> > @@ -345,14 +341,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> >  				     "drm panel-bridge add failed\n");
> >  
> >  	if (fsl_ldb_is_dual(fsl_ldb)) {
> > -		struct device_node *port1, *port2;
> > +		struct device_node *port1 __free(device_node) =
> > +			of_graph_get_port_by_id(dev->of_node, 1);
> > +		struct device_node *port2 __free(device_node) =
> > +			of_graph_get_port_by_id(dev->of_node, 2);
> >  
> > -		port1 = of_graph_get_port_by_id(dev->of_node, 1);
> > -		port2 = of_graph_get_port_by_id(dev->of_node, 2);
> >  		dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> > -		of_node_put(port1);
> > -		of_node_put(port2);
> > -
> >  		if (dual_link < 0)
> >  			return dev_err_probe(dev, dual_link,
> >  					     "Error getting dual link configuration\n");
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 2/3] drm/bridge: fsl-ldb: make use of dev_err_probe

[Alexander Stein]

Am Donnerstag, 15. Mai 2025, 00:24:52 CEST schrieb Marco Felsch:
> Make use of dev_err_probe() to easily spot issues via the debugfs or
> kernel log. No functional changes.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index fa29f2bf4031..e0a229c91953 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -308,11 +308,13 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  
>  	fsl_ldb->clk = devm_clk_get(dev, "ldb");
>  	if (IS_ERR(fsl_ldb->clk))
> -		return PTR_ERR(fsl_ldb->clk);
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->clk),
> +				     "Failed to get ldb clk\n");
>  
>  	fsl_ldb->regmap = syscon_node_to_regmap(dev->of_node->parent);
>  	if (IS_ERR(fsl_ldb->regmap))
> -		return PTR_ERR(fsl_ldb->regmap);
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->regmap),
> +				     "Failed to get regmap\n");
>  
>  	/* Locate the remote ports and the panel node */
>  	remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
> @@ -335,12 +337,12 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	panel = of_drm_find_panel(panel_node);
>  	of_node_put(panel_node);
>  	if (IS_ERR(panel))
> -		return PTR_ERR(panel);
> +		return dev_err_probe(dev, PTR_ERR(panel), "drm panel not found\n");
>  
>  	fsl_ldb->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>  	if (IS_ERR(fsl_ldb->panel_bridge))
> -		return PTR_ERR(fsl_ldb->panel_bridge);
> -
> +		return dev_err_probe(dev, PTR_ERR(fsl_ldb->panel_bridge),
> +				     "drm panel-bridge add failed\n");
>  
>  	if (fsl_ldb_is_dual(fsl_ldb)) {
>  		struct device_node *port1, *port2;
> @@ -356,10 +358,9 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  					     "Error getting dual link configuration\n");
>  
>  		/* Only DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS is supported */
> -		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> -			dev_err(dev, "LVDS channel pixel swap not supported.\n");
> -			return -EINVAL;
> -		}
> +		if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "LVDS channel pixel swap not supported.\n");
>  	}
>  
>  	platform_set_drvdata(pdev, fsl_ldb);
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 1/3] drm/bridge: fsl-ldb: make use of driver_private

[Dmitry Baryshkov]

On Thu, May 15, 2025 at 01:04:52AM +0200, Marco Felsch wrote:
> Hi Laurent,
> 
> On 25-05-15, Laurent Pinchart wrote:
> > Hi Marco,
> > 
> > Thank you for the patch.
> > 
> > On Thu, May 15, 2025 at 12:24:51AM +0200, Marco Felsch wrote:
> > > Make use of the drm_bridge::driver_private data instead of
> > > container_of() wrapper.
> > 
> > I suppose this is a personal preference, but I like usage of
> > container_of() better. In my opinion it conveys better that struct
> > fsl_ldb "unherits" from struct drm_bridge.
> 
> Yes, we can drop this patch if container_of() or to_fsl_ldb() is
> preferred. I just saw the driver_private field and why not making use of
> it since we do that a lot, same is true for container_of :)

container_of() generally is a more preferred form, because it provides
type safety. It doesn't perform blind casts. Using driver_data involves
using void pointer, which can be cast to any structure pointer. It is
easy to make hard-to-notice mistakes.

-- 
With best wishes
Dmitry