[PATCH v2 0/2] drm/rockchip: lvds: probe logging improvements

[PATCH v2 0/2] drm/rockchip: lvds: probe logging improvements

[Heiko Stuebner]

Getting the panel already uses dev_err_probe to stay silent, when
the panel just probes later, and the lvds defers.

But the phy needed on px30, also has the capability to probe after
the lvds. So make the rest of the lvds probe/bind logic also use
more modern logging than DRM_DEV_ERR, that is deprecated anyway.

changes in v2:
- reword the messages about getting (and preparing) pclk (Quentin)
- use a ret = dev_err_probe(dev, -EINVAL, ...) pattern
  in some (additional) places (Quentin)

Heiko Stuebner (2):
  drm/rockchip: lvds: move pclk preparation in with clk_get
  drm/rockchip: lvds: Hide scary error messages on probe deferral

 drivers/gpu/drm/rockchip/rockchip_lvds.c | 78 +++++++++---------------
 1 file changed, 28 insertions(+), 50 deletions(-)

-- 
2.47.2

[PATCH v2 1/2] drm/rockchip: lvds: move pclk preparation in with clk_get

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

The LVDS block needs a separate pclk only on some socs, so currently
requests and prepares it in the soc-specific probe function, but common
code is required to unprepare it in the error path or on driver remove.

While this works because clk_unprepare just does nothing if clk is NULL,
this mismatch of who is responsible still is not very nice.
The clock-framework already has a helper for clk-get-and-prepare even
with devres support in devm_clk_get_prepared().

This will get and prepare the clock and also unprepare it on driver
removal, saving the driver from having to handle it "manually".

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 385cf6881504..ecfae8d5da89 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -448,15 +448,13 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
 static int rk3288_lvds_probe(struct platform_device *pdev,
 			     struct rockchip_lvds *lvds)
 {
-	int ret;
-
 	lvds->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(lvds->regs))
 		return PTR_ERR(lvds->regs);
 
-	lvds->pclk = devm_clk_get(lvds->dev, "pclk_lvds");
+	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
 	if (IS_ERR(lvds->pclk)) {
-		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
+		DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");
 		return PTR_ERR(lvds->pclk);
 	}
 
@@ -480,12 +478,6 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
 		}
 	}
 
-	ret = clk_prepare(lvds->pclk);
-	if (ret < 0) {
-		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -728,20 +720,15 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, lvds);
 
 	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
-	if (ret < 0) {
+	if (ret < 0)
 		DRM_DEV_ERROR(dev, "failed to add component\n");
-		clk_unprepare(lvds->pclk);
-	}
 
 	return ret;
 }
 
 static void rockchip_lvds_remove(struct platform_device *pdev)
 {
-	struct rockchip_lvds *lvds = platform_get_drvdata(pdev);
-
 	component_del(&pdev->dev, &rockchip_lvds_component_ops);
-	clk_unprepare(lvds->pclk);
 }
 
 struct platform_driver rockchip_lvds_driver = {
-- 
2.47.2

Re: [PATCH v2 1/2] drm/rockchip: lvds: move pclk preparation in with clk_get

[Andy Yan]

Hi Heiko,


On 3/2/25 01:35, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The LVDS block needs a separate pclk only on some socs, so currently
> requests and prepares it in the soc-specific probe function, but common
> code is required to unprepare it in the error path or on driver remove.
> 
> While this works because clk_unprepare just does nothing if clk is NULL,
> this mismatch of who is responsible still is not very nice.
> The clock-framework already has a helper for clk-get-and-prepare even
> with devres support in devm_clk_get_prepared().
> 
> This will get and prepare the clock and also unprepare it on driver
> removal, saving the driver from having to handle it "manually".
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>

   Reviewed-by: Andy Yan <andy.yan@rock-chips.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_lvds.c | 19 +++----------------
>   1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 385cf6881504..ecfae8d5da89 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -448,15 +448,13 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
>   static int rk3288_lvds_probe(struct platform_device *pdev,
>   			     struct rockchip_lvds *lvds)
>   {
> -	int ret;
> -
>   	lvds->regs = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(lvds->regs))
>   		return PTR_ERR(lvds->regs);
>   
> -	lvds->pclk = devm_clk_get(lvds->dev, "pclk_lvds");
> +	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
>   	if (IS_ERR(lvds->pclk)) {
> -		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
> +		DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");
>   		return PTR_ERR(lvds->pclk);
>   	}
>   
> @@ -480,12 +478,6 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
>   		}
>   	}
>   
> -	ret = clk_prepare(lvds->pclk);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
> -		return ret;
> -	}
> -
>   	return 0;
>   }
>   
> @@ -728,20 +720,15 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
>   	dev_set_drvdata(dev, lvds);
>   
>   	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
> -	if (ret < 0) {
> +	if (ret < 0)
>   		DRM_DEV_ERROR(dev, "failed to add component\n");
> -		clk_unprepare(lvds->pclk);
> -	}
>   
>   	return ret;
>   }
>   
>   static void rockchip_lvds_remove(struct platform_device *pdev)
>   {
> -	struct rockchip_lvds *lvds = platform_get_drvdata(pdev);
> -
>   	component_del(&pdev->dev, &rockchip_lvds_component_ops);
> -	clk_unprepare(lvds->pclk);
>   }
>   
>   struct platform_driver rockchip_lvds_driver = {

[PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when
probing defer") already started hiding scary messages that are not relevant
if the requested supply just returned EPROBE_DEFER, but there are more
possible sources - like the phy.

So modernize the whole logging in the probe path by replacing the
remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe)
and drm_err calls.

The distinction here is that all messages talking about mishaps of the
lvds element use dev_err(_probe) while messages caused by interaction
with the main Rockchip drm-device use drm_err.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 61 ++++++++++--------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index ecfae8d5da89..d8b2007123fa 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
 		return PTR_ERR(lvds->regs);
 
 	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
-	if (IS_ERR(lvds->pclk)) {
-		DRM_DEV_ERROR(lvds->dev, "could not get or prepare 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 or prepare pclk_lvds\n");
 
 	lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins),
 				  GFP_KERNEL);
@@ -465,14 +464,14 @@ 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;
 		}
@@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 
 	lvds->drm_dev = drm_dev;
 	port = 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");
-		return -EINVAL;
-	}
+	if (!port)
+		return dev_err_probe(dev, -EINVAL,
+				     "can't found port point, please init lvds panel port!\n");
+
 	for_each_child_of_node(port, endpoint) {
 		child_count++;
 		of_property_read_u32(endpoint, "reg", &endpoint_id);
@@ -563,8 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (!child_count) {
-		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
-		ret = -EINVAL;
+		ret = dev_err_probe(dev, -EINVAL, "lvds port does not have any children\n");
 		goto err_put_port;
 	} else if (ret) {
 		dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
@@ -581,8 +578,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);
-		ret = lvds->output;
+		ret = dev_err_probe(dev, lvds->output, "invalid output type [%s]\n", name);
 		goto err_put_remote;
 	}
 
@@ -593,7 +589,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;
 	}
@@ -604,8 +600,8 @@ 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);
+		drm_err(drm_dev,
+			"failed to initialize encoder: %d\n", ret);
 		goto err_put_remote;
 	}
 
@@ -618,8 +614,8 @@ 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);
+			drm_err(drm_dev,
+				"failed to initialize connector: %d\n", ret);
 			goto err_free_encoder;
 		}
 
@@ -633,9 +629,9 @@ 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);
+			drm_err(drm_dev,
+				"failed to initialize bridge connector: %pe\n",
+				connector);
 			ret = PTR_ERR(connector);
 			goto err_free_encoder;
 		}
@@ -643,8 +639,7 @@ 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);
+		drm_err(drm_dev, "failed to attach encoder: %d\n", ret);
 		goto err_free_connector;
 	}
 
@@ -706,24 +701,20 @@ 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");
+		return dev_err_probe(dev, ret, "failed to add component\n");
 
-	return ret;
+	return 0;
 }
 
 static void rockchip_lvds_remove(struct platform_device *pdev)
-- 
2.47.2

Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

[Andy Yan]

Hi Heiko,


On 3/2/25 01:35, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when
> probing defer") already started hiding scary messages that are not relevant
> if the requested supply just returned EPROBE_DEFER, but there are more
> possible sources - like the phy.
> 
> So modernize the whole logging in the probe path by replacing the
> remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe)
> and drm_err calls.
> 
> The distinction here is that all messages talking about mishaps of the
> lvds element use dev_err(_probe) while messages caused by interaction
> with the main Rockchip drm-device use drm_err.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>

   Reviewed-by: Andy Yan <andy.yan@rock-chips.com>

Thanks.
> ---
>   drivers/gpu/drm/rockchip/rockchip_lvds.c | 61 ++++++++++--------------
>   1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index ecfae8d5da89..d8b2007123fa 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
>   		return PTR_ERR(lvds->regs);
>   
>   	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
> -	if (IS_ERR(lvds->pclk)) {
> -		DRM_DEV_ERROR(lvds->dev, "could not get or prepare 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 or prepare pclk_lvds\n");
>   
>   	lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins),
>   				  GFP_KERNEL);
> @@ -465,14 +464,14 @@ 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;
>   		}
> @@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>   
>   	lvds->drm_dev = drm_dev;
>   	port = 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");
> -		return -EINVAL;
> -	}
> +	if (!port)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't found port point, please init lvds panel port!\n");
> +
>   	for_each_child_of_node(port, endpoint) {
>   		child_count++;
>   		of_property_read_u32(endpoint, "reg", &endpoint_id);
> @@ -563,8 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>   		}
>   	}
>   	if (!child_count) {
> -		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> -		ret = -EINVAL;
> +		ret = dev_err_probe(dev, -EINVAL, "lvds port does not have any children\n");
>   		goto err_put_port;
>   	} else if (ret) {
>   		dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
> @@ -581,8 +578,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);
> -		ret = lvds->output;
> +		ret = dev_err_probe(dev, lvds->output, "invalid output type [%s]\n", name);
>   		goto err_put_remote;
>   	}
>   
> @@ -593,7 +589,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;
>   	}
> @@ -604,8 +600,8 @@ 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);
> +		drm_err(drm_dev,
> +			"failed to initialize encoder: %d\n", ret);
>   		goto err_put_remote;
>   	}
>   
> @@ -618,8 +614,8 @@ 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);
> +			drm_err(drm_dev,
> +				"failed to initialize connector: %d\n", ret);
>   			goto err_free_encoder;
>   		}
>   
> @@ -633,9 +629,9 @@ 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);
> +			drm_err(drm_dev,
> +				"failed to initialize bridge connector: %pe\n",
> +				connector);
>   			ret = PTR_ERR(connector);
>   			goto err_free_encoder;
>   		}
> @@ -643,8 +639,7 @@ 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);
> +		drm_err(drm_dev, "failed to attach encoder: %d\n", ret);
>   		goto err_free_connector;
>   	}
>   
> @@ -706,24 +701,20 @@ 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");
> +		return dev_err_probe(dev, ret, "failed to add component\n");
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static void rockchip_lvds_remove(struct platform_device *pdev)

Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

[Quentin Schulz]

Hi Heiko,

On 3/1/25 6:35 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when
> probing defer") already started hiding scary messages that are not relevant
> if the requested supply just returned EPROBE_DEFER, but there are more
> possible sources - like the phy.
> 
> So modernize the whole logging in the probe path by replacing the
> remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe)
> and drm_err calls.
> 
> The distinction here is that all messages talking about mishaps of the
> lvds element use dev_err(_probe) while messages caused by interaction
> with the main Rockchip drm-device use drm_err.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_lvds.c | 61 ++++++++++--------------
>   1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index ecfae8d5da89..d8b2007123fa 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
>   		return PTR_ERR(lvds->regs);
>   
>   	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
> -	if (IS_ERR(lvds->pclk)) {
> -		DRM_DEV_ERROR(lvds->dev, "could not get or prepare 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 or prepare pclk_lvds\n");
>   
>   	lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins),
>   				  GFP_KERNEL);
> @@ -465,14 +464,14 @@ 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;

Should those be dev_err if they are not breaking anything? After all, 
the device will actually probe? Maybe dev_warn would be more appropriate?

Maybe a separate patch since we had DRM_DEV_ERROR already, so switching 
to dev_err in one and then lowering the log level in a second would make 
"more" sense?

>   		}
> @@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>   
>   	lvds->drm_dev = drm_dev;
>   	port = 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");
> -		return -EINVAL;
> -	}
> +	if (!port)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't found port point, please init lvds panel port!\n");
> +
>   	for_each_child_of_node(port, endpoint) {
>   		child_count++;
>   		of_property_read_u32(endpoint, "reg", &endpoint_id);
> @@ -563,8 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>   		}
>   	}
>   	if (!child_count) {
> -		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> -		ret = -EINVAL;
> +		ret = dev_err_probe(dev, -EINVAL, "lvds port does not have any children\n");
>   		goto err_put_port;
>   	} else if (ret) {
>   		dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
> @@ -581,8 +578,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);
> -		ret = lvds->output;
> +		ret = dev_err_probe(dev, lvds->output, "invalid output type [%s]\n", name);
>   		goto err_put_remote;
>   	}
>   
> @@ -593,7 +589,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;

nipitck:

ret = dev_err_probe(dev, lvds->format, "invalid data-mapping format 
[%s]\n", name);

>   		goto err_put_remote;
>   	}
> @@ -604,8 +600,8 @@ 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);
> +		drm_err(drm_dev,
> +			"failed to initialize encoder: %d\n", ret);
>   		goto err_put_remote;
>   	}
>   
> @@ -618,8 +614,8 @@ 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);
> +			drm_err(drm_dev,
> +				"failed to initialize connector: %d\n", ret);
>   			goto err_free_encoder;
>   		}
>   
> @@ -633,9 +629,9 @@ 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);
> +			drm_err(drm_dev,
> +				"failed to initialize bridge connector: %pe\n",
> +				connector);
>   			ret = PTR_ERR(connector);
>   			goto err_free_encoder;
>   		}
> @@ -643,8 +639,7 @@ 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);
> +		drm_err(drm_dev, "failed to attach encoder: %d\n", ret);
>   		goto err_free_connector;
>   	}
>   
> @@ -706,24 +701,20 @@ 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");
> +		return dev_err_probe(dev, ret, "failed to add component\n");
>   

Should this rather be drm_error? I believe this is related to the 
Rockchip DRM main device?

Otherwise looks good to me,

Cheers,
Quentin

Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

[Heiko Stübner]

Am Dienstag, 4. März 2025, 12:46:59 MEZ schrieb Quentin Schulz:
> > @@ -465,14 +464,14 @@ 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;
> 
> Should those be dev_err if they are not breaking anything? After all, 
> the device will actually probe? Maybe dev_warn would be more appropriate?
> 
> Maybe a separate patch since we had DRM_DEV_ERROR already, so switching 
> to dev_err in one and then lowering the log level in a second would make 
> "more" sense?

I did debate a bit whether to ignore here and go directly to dev_warn,
but opted for DRM_DEV_ERROR -> dev_err -> dev_warn, to keep the commit
description intact. Otherwise people looking at this patch alone might ask
if this part was forgotten, when the commit message indicates "all".

So expect an additional patch in v3 :-) .


> > @@ -593,7 +589,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;
> 
> nipitck:
> 
> ret = dev_err_probe(dev, lvds->format, "invalid data-mapping format 
> [%s]\n", name);

you're right of course

...

> >   	ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
> >   	if (ret < 0)
> > -		DRM_DEV_ERROR(dev, "failed to add component\n");
> > +		return dev_err_probe(dev, ret, "failed to add component\n");
> >   
> 
> Should this rather be drm_error? I believe this is related to the 
> Rockchip DRM main device?

I would group this more to general device error.
Component_add happens way before the component master binds anything.

drm_err is supposed to also point to the main drm_device as its parameter,
which does not even exist at this point.


Heiko