[PATCH -next v2 0/3] drm/rockchip: Simplified with for_each_child_of_node_scoped()

[PATCH -next v2 0/3] drm/rockchip: Simplified with for_each_child_of_node_scoped()

[Jinjie Ruan]

Use for_each_child_of_node_scoped(), __free and dev_err_probe()
to simplify code.

Changes in v2:
- Split from the patch set.
- Also use __free() and dev_err_probe() to simplify code.

Jinjie Ruan (3):
  drm/rockchip: Use for_each_child_of_node_scoped()
  drm/rockchip: Simplified with dev_err() and __free()
  drm/rockchip: Simplified with dev_err_probe()

 drivers/gpu/drm/rockchip/rockchip_lvds.c | 74 +++++++++---------------
 1 file changed, 26 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH -next v2 1/3] drm/rockchip: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 9a01aa450741..f5b3f18794dd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	struct device_node *remote = NULL;
-	struct device_node  *port, *endpoint;
+	struct device_node  *port;
 	int ret = 0, child_count = 0;
 	const char *name;
 	u32 endpoint_id = 0;
@@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 			      "can't found port point, please init lvds panel port!\n");
 		return -EINVAL;
 	}
-	for_each_child_of_node(port, endpoint) {
+	for_each_child_of_node_scoped(port, endpoint) {
 		child_count++;
 		of_property_read_u32(endpoint, "reg", &endpoint_id);
 		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
 						  &lvds->panel, &lvds->bridge);
-		if (!ret) {
-			of_node_put(endpoint);
+		if (!ret)
 			break;
-		}
 	}
 	if (!child_count) {
 		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
-- 
2.34.1

[PATCH -next v2 2/3] drm/rockchip: Simplified with dev_err() and __free()

[Jinjie Ruan]

Avoid need to manually handle of_node_put() by using __free(), and use
dev_err() to replace deprecated() DRM_DEV_ERROR(), which can simplfy
code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 38 ++++++++----------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index f5b3f18794dd..700ac730887d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -548,16 +548,14 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	struct device_node *remote = NULL;
-	struct device_node  *port;
 	int ret = 0, child_count = 0;
 	const char *name;
 	u32 endpoint_id = 0;
 
 	lvds->drm_dev = drm_dev;
-	port = of_graph_get_port_by_id(dev->of_node, 1);
+	struct device_node *port __free(device_node) = of_graph_get_port_by_id(dev->of_node, 1);
 	if (!port) {
-		DRM_DEV_ERROR(dev,
-			      "can't found port point, please init lvds panel port!\n");
+		dev_err(dev, "can't found port point, please init lvds panel port!\n");
 		return -EINVAL;
 	}
 	for_each_child_of_node_scoped(port, endpoint) {
@@ -569,13 +567,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 			break;
 	}
 	if (!child_count) {
-		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
-		ret = -EINVAL;
-		goto err_put_port;
-	} else if (ret) {
-		dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
-		goto err_put_port;
-	}
+		dev_err(dev, "lvds port does not have any children\n");
+		return -EINVAL;
+	} else if (ret)
+		return dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
 	if (lvds->panel)
 		remote = lvds->panel->dev->of_node;
 	else
@@ -587,7 +582,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 		lvds->output = rockchip_lvds_name_to_output(name);
 
 	if (lvds->output < 0) {
-		DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name);
+		dev_err(dev, "invalid output type [%s]\n", name);
 		ret = lvds->output;
 		goto err_put_remote;
 	}
@@ -599,7 +594,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 		lvds->format = rockchip_lvds_name_to_format(name);
 
 	if (lvds->format < 0) {
-		DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name);
+		dev_err(dev, "invalid data-mapping format [%s]\n", name);
 		ret = lvds->format;
 		goto err_put_remote;
 	}
@@ -610,8 +605,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 
 	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS);
 	if (ret < 0) {
-		DRM_DEV_ERROR(drm_dev->dev,
-			      "failed to initialize encoder: %d\n", ret);
+		dev_err(drm_dev->dev, "failed to initialize encoder: %d\n", ret);
 		goto err_put_remote;
 	}
 
@@ -624,8 +618,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 					 &rockchip_lvds_connector_funcs,
 					 DRM_MODE_CONNECTOR_LVDS);
 		if (ret < 0) {
-			DRM_DEV_ERROR(drm_dev->dev,
-				      "failed to initialize connector: %d\n", ret);
+			dev_err(drm_dev->dev, "failed to initialize connector: %d\n", ret);
 			goto err_free_encoder;
 		}
 
@@ -639,9 +632,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 
 		connector = drm_bridge_connector_init(lvds->drm_dev, encoder);
 		if (IS_ERR(connector)) {
-			DRM_DEV_ERROR(drm_dev->dev,
-				      "failed to initialize bridge connector: %pe\n",
-				      connector);
+			dev_err(drm_dev->dev, "failed to initialize bridge connector: %pe\n",
+				connector);
 			ret = PTR_ERR(connector);
 			goto err_free_encoder;
 		}
@@ -649,14 +641,12 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret < 0) {
-		DRM_DEV_ERROR(drm_dev->dev,
-			      "failed to attach encoder: %d\n", ret);
+		dev_err(drm_dev->dev, "failed to attach encoder: %d\n", ret);
 		goto err_free_connector;
 	}
 
 	pm_runtime_enable(dev);
 	of_node_put(remote);
-	of_node_put(port);
 
 	return 0;
 
@@ -666,8 +656,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	drm_encoder_cleanup(encoder);
 err_put_remote:
 	of_node_put(remote);
-err_put_port:
-	of_node_put(port);
 
 	return ret;
 }
-- 
2.34.1

[PATCH -next v2 3/3] drm/rockchip: Simplified with dev_err_probe()

[Jinjie Ruan]

Use dev_err_probe to replace deprecated() DRM_DEV_ERROR(), which
can simplfy code.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 30 +++++++++---------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 700ac730887d..92679e54c71d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -455,10 +455,8 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
 		return PTR_ERR(lvds->regs);
 
 	lvds->pclk = devm_clk_get(lvds->dev, "pclk_lvds");
-	if (IS_ERR(lvds->pclk)) {
-		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
-		return PTR_ERR(lvds->pclk);
-	}
+	if (IS_ERR(lvds->pclk))
+		return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), "could not get pclk_lvds\n");
 
 	lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins),
 				  GFP_KERNEL);
@@ -467,24 +465,22 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
 
 	lvds->pins->p = devm_pinctrl_get(lvds->dev);
 	if (IS_ERR(lvds->pins->p)) {
-		DRM_DEV_ERROR(lvds->dev, "no pinctrl handle\n");
+		dev_err(lvds->dev, "no pinctrl handle\n");
 		devm_kfree(lvds->dev, lvds->pins);
 		lvds->pins = NULL;
 	} else {
 		lvds->pins->default_state =
 			pinctrl_lookup_state(lvds->pins->p, "lcdc");
 		if (IS_ERR(lvds->pins->default_state)) {
-			DRM_DEV_ERROR(lvds->dev, "no default pinctrl state\n");
+			dev_err(lvds->dev, "no default pinctrl state\n");
 			devm_kfree(lvds->dev, lvds->pins);
 			lvds->pins = NULL;
 		}
 	}
 
 	ret = clk_prepare(lvds->pclk);
-	if (ret < 0) {
-		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(lvds->dev, ret, "failed to prepare pclk_lvds\n");
 
 	return 0;
 }
@@ -700,22 +696,18 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
 
 	lvds->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
 						    "rockchip,grf");
-	if (IS_ERR(lvds->grf)) {
-		DRM_DEV_ERROR(dev, "missing rockchip,grf property\n");
-		return PTR_ERR(lvds->grf);
-	}
+	if (IS_ERR(lvds->grf))
+		return dev_err_probe(dev, PTR_ERR(lvds->grf), "missing rockchip,grf property\n");
 
 	ret = lvds->soc_data->probe(pdev, lvds);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "Platform initialization failed\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Platform initialization failed\n");
 
 	dev_set_drvdata(dev, lvds);
 
 	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "failed to add component\n");
+		dev_err(dev, "failed to add component\n");
 		clk_unprepare(lvds->pclk);
 	}
 
-- 
2.34.1

Re: [PATCH -next v2 2/3] drm/rockchip: Simplified with dev_err() and __free()

[Heiko Stuebner]

Hi,

Am Dienstag, 27. August 2024, 05:03:56 CEST schrieb Jinjie Ruan:
> Avoid need to manually handle of_node_put() by using __free(), and use
> dev_err() to replace deprecated() DRM_DEV_ERROR(), which can simplfy
> code.

please make that two separate commits, one for the dev_err conversion
and one for the __free() thing.

The general rule of thumb is that if you need an "and" to describe your
changes it's somewhat likely that things should be split.

Also patch subject should be "drm/rockchip: lvds: ...."


Thanks
Heiko

> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 38 ++++++++----------------
>  1 file changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index f5b3f18794dd..700ac730887d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -548,16 +548,14 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	struct device_node *remote = NULL;
> -	struct device_node  *port;
>  	int ret = 0, child_count = 0;
>  	const char *name;
>  	u32 endpoint_id = 0;
>  
>  	lvds->drm_dev = drm_dev;
> -	port = of_graph_get_port_by_id(dev->of_node, 1);
> +	struct device_node *port __free(device_node) = of_graph_get_port_by_id(dev->of_node, 1);
>  	if (!port) {
> -		DRM_DEV_ERROR(dev,
> -			      "can't found port point, please init lvds panel port!\n");
> +		dev_err(dev, "can't found port point, please init lvds panel port!\n");
>  		return -EINVAL;
>  	}
>  	for_each_child_of_node_scoped(port, endpoint) {
> @@ -569,13 +567,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  			break;
>  	}
>  	if (!child_count) {
> -		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> -		ret = -EINVAL;
> -		goto err_put_port;
> -	} else if (ret) {
> -		dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
> -		goto err_put_port;
> -	}
> +		dev_err(dev, "lvds port does not have any children\n");
> +		return -EINVAL;
> +	} else if (ret)
> +		return dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
>  	if (lvds->panel)
>  		remote = lvds->panel->dev->of_node;
>  	else
> @@ -587,7 +582,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  		lvds->output = rockchip_lvds_name_to_output(name);
>  
>  	if (lvds->output < 0) {
> -		DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name);
> +		dev_err(dev, "invalid output type [%s]\n", name);
>  		ret = lvds->output;
>  		goto err_put_remote;
>  	}
> @@ -599,7 +594,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  		lvds->format = rockchip_lvds_name_to_format(name);
>  
>  	if (lvds->format < 0) {
> -		DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name);
> +		dev_err(dev, "invalid data-mapping format [%s]\n", name);
>  		ret = lvds->format;
>  		goto err_put_remote;
>  	}
> @@ -610,8 +605,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  
>  	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(drm_dev->dev,
> -			      "failed to initialize encoder: %d\n", ret);
> +		dev_err(drm_dev->dev, "failed to initialize encoder: %d\n", ret);
>  		goto err_put_remote;
>  	}
>  
> @@ -624,8 +618,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  					 &rockchip_lvds_connector_funcs,
>  					 DRM_MODE_CONNECTOR_LVDS);
>  		if (ret < 0) {
> -			DRM_DEV_ERROR(drm_dev->dev,
> -				      "failed to initialize connector: %d\n", ret);
> +			dev_err(drm_dev->dev, "failed to initialize connector: %d\n", ret);
>  			goto err_free_encoder;
>  		}
>  
> @@ -639,9 +632,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  
>  		connector = drm_bridge_connector_init(lvds->drm_dev, encoder);
>  		if (IS_ERR(connector)) {
> -			DRM_DEV_ERROR(drm_dev->dev,
> -				      "failed to initialize bridge connector: %pe\n",
> -				      connector);
> +			dev_err(drm_dev->dev, "failed to initialize bridge connector: %pe\n",
> +				connector);
>  			ret = PTR_ERR(connector);
>  			goto err_free_encoder;
>  		}
> @@ -649,14 +641,12 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(drm_dev->dev,
> -			      "failed to attach encoder: %d\n", ret);
> +		dev_err(drm_dev->dev, "failed to attach encoder: %d\n", ret);
>  		goto err_free_connector;
>  	}
>  
>  	pm_runtime_enable(dev);
>  	of_node_put(remote);
> -	of_node_put(port);
>  
>  	return 0;
>  
> @@ -666,8 +656,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	drm_encoder_cleanup(encoder);
>  err_put_remote:
>  	of_node_put(remote);
> -err_put_port:
> -	of_node_put(port);
>  
>  	return ret;
>  }
> 

Re: [PATCH -next v2 3/3] drm/rockchip: Simplified with dev_err_probe()

[Heiko Stuebner]

Am Dienstag, 27. August 2024, 05:03:57 CEST schrieb Jinjie Ruan:
> Use dev_err_probe to replace deprecated() DRM_DEV_ERROR(), which
> can simplfy code.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 30 +++++++++---------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 700ac730887d..92679e54c71d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c

> @@ -467,24 +465,22 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
>  
>  	lvds->pins->p = devm_pinctrl_get(lvds->dev);
>  	if (IS_ERR(lvds->pins->p)) {
> -		DRM_DEV_ERROR(lvds->dev, "no pinctrl handle\n");
> +		dev_err(lvds->dev, "no pinctrl handle\n");

you're mixing things here.

The commit message talks about dev_err_probe but you're doing
here and below a DRM_DEV_ERROR to dev_err conversion, which also
partially happens in patch 2 of this series.

Please structure things in a way that the patch content matches
the patch description :-)


Thanks a lot
Heiko

>  		devm_kfree(lvds->dev, lvds->pins);
>  		lvds->pins = NULL;
>  	} else {
>  		lvds->pins->default_state =
>  			pinctrl_lookup_state(lvds->pins->p, "lcdc");
>  		if (IS_ERR(lvds->pins->default_state)) {
> -			DRM_DEV_ERROR(lvds->dev, "no default pinctrl state\n");
> +			dev_err(lvds->dev, "no default pinctrl state\n");
>  			devm_kfree(lvds->dev, lvds->pins);
>  			lvds->pins = NULL;
>  		}
>  	}
>  
>  	ret = clk_prepare(lvds->pclk);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(lvds->dev, ret, "failed to prepare pclk_lvds\n");
>  
>  	return 0;
>  }
> @@ -700,22 +696,18 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
>  
>  	lvds->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>  						    "rockchip,grf");
> -	if (IS_ERR(lvds->grf)) {
> -		DRM_DEV_ERROR(dev, "missing rockchip,grf property\n");
> -		return PTR_ERR(lvds->grf);
> -	}
> +	if (IS_ERR(lvds->grf))
> +		return dev_err_probe(dev, PTR_ERR(lvds->grf), "missing rockchip,grf property\n");
>  
>  	ret = lvds->soc_data->probe(pdev, lvds);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "Platform initialization failed\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Platform initialization failed\n");
>  
>  	dev_set_drvdata(dev, lvds);
>  
>  	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(dev, "failed to add component\n");
> +		dev_err(dev, "failed to add component\n");
>  		clk_unprepare(lvds->pclk);
>  	}
>  
>