drm/rockchip: dsi: convert to dev_err_probe in probe function

[PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Heiko Stuebner 2 weeks, 1 day ago

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

DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a number
of lines too, so convert the error prints for the dsi-driver.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 80 ++++++-------------
 1 file changed, 26 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 58a44af0e9ad..3224ab749352 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -1356,17 +1356,15 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
 	struct resource *res;
 	const struct rockchip_dw_dsi_chip_data *cdata =
 				of_device_get_match_data(dev);
-	int ret, i;
+	int i;
 
 	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
 	if (!dsi)
 		return -ENOMEM;
 
 	dsi->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(dsi->base)) {
-		DRM_DEV_ERROR(dev, "Unable to get dsi registers\n");
-		return PTR_ERR(dsi->base);
-	}
+	if (IS_ERR(dsi->base))
+		return dev_err_probe(dev, PTR_ERR(dsi->base), "Unable to get dsi registers\n");
 
 	i = 0;
 	while (cdata[i].reg) {
@@ -1378,67 +1376,47 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
 		i++;
 	}
 
-	if (!dsi->cdata) {
-		DRM_DEV_ERROR(dev, "no dsi-config for %s node\n", np->name);
-		return -EINVAL;
-	}
+	if (!dsi->cdata)
+		return dev_err_probe(dev, -EINVAL, "No dsi-config for %s node\n", np->name);
 
 	/* try to get a possible external dphy */
 	dsi->phy = devm_phy_optional_get(dev, "dphy");
-	if (IS_ERR(dsi->phy)) {
-		ret = PTR_ERR(dsi->phy);
-		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(dsi->phy))
+		return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi dphy\n");
 
 	dsi->pclk = devm_clk_get(dev, "pclk");
-	if (IS_ERR(dsi->pclk)) {
-		ret = PTR_ERR(dsi->pclk);
-		DRM_DEV_ERROR(dev, "Unable to get pclk: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(dsi->pclk))
+		return dev_err_probe(dev, PTR_ERR(dsi->pclk), "Unable to get pclk\n");
 
 	dsi->pllref_clk = devm_clk_get(dev, "ref");
 	if (IS_ERR(dsi->pllref_clk)) {
-		if (dsi->phy) {
+		if (dsi->phy)
 			/*
 			 * if external phy is present, pll will be
 			 * generated there.
 			 */
 			dsi->pllref_clk = NULL;
-		} else {
-			ret = PTR_ERR(dsi->pllref_clk);
-			DRM_DEV_ERROR(dev,
-				      "Unable to get pll reference clock: %d\n",
-				      ret);
-			return ret;
-		}
+		else
+			return dev_err_probe(dev, PTR_ERR(dsi->pllref_clk),
+					     "Unable to get pll reference clock\n");
 	}
 
 	if (dsi->cdata->flags & DW_MIPI_NEEDS_PHY_CFG_CLK) {
 		dsi->phy_cfg_clk = devm_clk_get(dev, "phy_cfg");
-		if (IS_ERR(dsi->phy_cfg_clk)) {
-			ret = PTR_ERR(dsi->phy_cfg_clk);
-			DRM_DEV_ERROR(dev,
-				      "Unable to get phy_cfg_clk: %d\n", ret);
-			return ret;
-		}
+		if (IS_ERR(dsi->phy_cfg_clk))
+			return dev_err_probe(dev, PTR_ERR(dsi->phy_cfg_clk),
+					     "Unable to get phy_cfg_clk\n");
 	}
 
 	if (dsi->cdata->flags & DW_MIPI_NEEDS_GRF_CLK) {
 		dsi->grf_clk = devm_clk_get(dev, "grf");
-		if (IS_ERR(dsi->grf_clk)) {
-			ret = PTR_ERR(dsi->grf_clk);
-			DRM_DEV_ERROR(dev, "Unable to get grf_clk: %d\n", ret);
-			return ret;
-		}
+		if (IS_ERR(dsi->grf_clk))
+			return dev_err_probe(dev, PTR_ERR(dsi->grf_clk), "Unable to get grf_clk\n");
 	}
 
 	dsi->grf_regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
-	if (IS_ERR(dsi->grf_regmap)) {
-		DRM_DEV_ERROR(dev, "Unable to get rockchip,grf\n");
-		return PTR_ERR(dsi->grf_regmap);
-	}
+	if (IS_ERR(dsi->grf_regmap))
+		return dev_err_probe(dev, PTR_ERR(dsi->grf_regmap), "Unable to get rockchip,grf\n");
 
 	dsi->dev = dev;
 	dsi->pdata.base = dsi->base;
@@ -1451,24 +1429,18 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
 	mutex_init(&dsi->usage_mutex);
 
 	dsi->dphy = devm_phy_create(dev, NULL, &dw_mipi_dsi_dphy_ops);
-	if (IS_ERR(dsi->dphy)) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to create PHY\n");
-		return PTR_ERR(dsi->dphy);
-	}
+	if (IS_ERR(dsi->dphy))
+		return dev_err_probe(dev, PTR_ERR(dsi->dphy), "Failed to create PHY\n");
 
 	phy_set_drvdata(dsi->dphy, dsi);
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	if (IS_ERR(phy_provider))
-		return PTR_ERR(phy_provider);
+		return dev_err_probe(dev, PTR_ERR(phy_provider),
+				     "Failed to register phy-provider\n");
 
 	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
-	if (IS_ERR(dsi->dmd)) {
-		ret = PTR_ERR(dsi->dmd);
-		if (ret != -EPROBE_DEFER)
-			DRM_DEV_ERROR(dev,
-				      "Failed to probe dw_mipi_dsi: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(dsi->dmd))
+		return dev_err_probe(dev, PTR_ERR(dsi->dmd), "Failed to probe dw_mipi_dsi\n");
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Diederik de Haas 1 week, 6 days ago

Hi Heiko,

On Fri Nov 8, 2024 at 3:44 PM CET, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
>
> DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a number
> of lines too, so convert the error prints for the dsi-driver.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>  .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 80 ++++++-------------
>  1 file changed, 26 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 58a44af0e9ad..3224ab749352 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -1356,17 +1356,15 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	const struct rockchip_dw_dsi_chip_data *cdata =
>  				of_device_get_match_data(dev);
> -	int ret, i;
> +	int i;
>  
>  	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi)
>  		return -ENOMEM;
>  
>  	dsi->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> -	if (IS_ERR(dsi->base)) {
> -		DRM_DEV_ERROR(dev, "Unable to get dsi registers\n");
> -		return PTR_ERR(dsi->base);
> -	}
> +	if (IS_ERR(dsi->base))
> +		return dev_err_probe(dev, PTR_ERR(dsi->base), "Unable to get dsi registers\n");
>  
>  	i = 0;
>  	while (cdata[i].reg) {
> @@ -1378,67 +1376,47 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> -	if (!dsi->cdata) {
> -		DRM_DEV_ERROR(dev, "no dsi-config for %s node\n", np->name);
> -		return -EINVAL;
> -	}
> +	if (!dsi->cdata)
> +		return dev_err_probe(dev, -EINVAL, "No dsi-config for %s node\n", np->name);
>  
>  	/* try to get a possible external dphy */
>  	dsi->phy = devm_phy_optional_get(dev, "dphy");
> -	if (IS_ERR(dsi->phy)) {
> -		ret = PTR_ERR(dsi->phy);
> -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->phy))
> +		return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi dphy\n");

One small remark/question: wouldn't the recently introduced [1]
dev_warn_probe be more appropriate here, given that "dphy" is optional?

But besides that, the 'conversion' to use dev_err_probe was done
consistently and in line with other dev_err_probe conversions and looks
much cleaner and thus better then the previous code. Thanks!
Feel free to add my

Reviewed-by: Diederik de Haas <didi.debian@cknow.org>

[1] https://lore.kernel.org/linux-rockchip/2be0a28538bb2a3d1bcc91e2ca1f2d0dc09146d9.1727601608.git.dsimic@manjaro.org/
>  
>  	dsi->pclk = devm_clk_get(dev, "pclk");
> -	if (IS_ERR(dsi->pclk)) {
> -		ret = PTR_ERR(dsi->pclk);
> -		DRM_DEV_ERROR(dev, "Unable to get pclk: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->pclk))
> +		return dev_err_probe(dev, PTR_ERR(dsi->pclk), "Unable to get pclk\n");
>  
>  	dsi->pllref_clk = devm_clk_get(dev, "ref");
>  	if (IS_ERR(dsi->pllref_clk)) {
> -		if (dsi->phy) {
> +		if (dsi->phy)
>  			/*
>  			 * if external phy is present, pll will be
>  			 * generated there.
>  			 */
>  			dsi->pllref_clk = NULL;
> -		} else {
> -			ret = PTR_ERR(dsi->pllref_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get pll reference clock: %d\n",
> -				      ret);
> -			return ret;
> -		}
> +		else
> +			return dev_err_probe(dev, PTR_ERR(dsi->pllref_clk),
> +					     "Unable to get pll reference clock\n");
>  	}
>  
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_PHY_CFG_CLK) {
>  		dsi->phy_cfg_clk = devm_clk_get(dev, "phy_cfg");
> -		if (IS_ERR(dsi->phy_cfg_clk)) {
> -			ret = PTR_ERR(dsi->phy_cfg_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get phy_cfg_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->phy_cfg_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->phy_cfg_clk),
> +					     "Unable to get phy_cfg_clk\n");
>  	}
>  
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_GRF_CLK) {
>  		dsi->grf_clk = devm_clk_get(dev, "grf");
> -		if (IS_ERR(dsi->grf_clk)) {
> -			ret = PTR_ERR(dsi->grf_clk);
> -			DRM_DEV_ERROR(dev, "Unable to get grf_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->grf_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->grf_clk), "Unable to get grf_clk\n");
>  	}
>  
>  	dsi->grf_regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> -	if (IS_ERR(dsi->grf_regmap)) {
> -		DRM_DEV_ERROR(dev, "Unable to get rockchip,grf\n");
> -		return PTR_ERR(dsi->grf_regmap);
> -	}
> +	if (IS_ERR(dsi->grf_regmap))
> +		return dev_err_probe(dev, PTR_ERR(dsi->grf_regmap), "Unable to get rockchip,grf\n");
>  
>  	dsi->dev = dev;
>  	dsi->pdata.base = dsi->base;
> @@ -1451,24 +1429,18 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  	mutex_init(&dsi->usage_mutex);
>  
>  	dsi->dphy = devm_phy_create(dev, NULL, &dw_mipi_dsi_dphy_ops);
> -	if (IS_ERR(dsi->dphy)) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to create PHY\n");
> -		return PTR_ERR(dsi->dphy);
> -	}
> +	if (IS_ERR(dsi->dphy))
> +		return dev_err_probe(dev, PTR_ERR(dsi->dphy), "Failed to create PHY\n");
>  
>  	phy_set_drvdata(dsi->dphy, dsi);
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  	if (IS_ERR(phy_provider))
> -		return PTR_ERR(phy_provider);
> +		return dev_err_probe(dev, PTR_ERR(phy_provider),
> +				     "Failed to register phy-provider\n");
>  
>  	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> -	if (IS_ERR(dsi->dmd)) {
> -		ret = PTR_ERR(dsi->dmd);
> -		if (ret != -EPROBE_DEFER)
> -			DRM_DEV_ERROR(dev,
> -				      "Failed to probe dw_mipi_dsi: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->dmd))
> +		return dev_err_probe(dev, PTR_ERR(dsi->dmd), "Failed to probe dw_mipi_dsi\n");
>  
>  	return 0;
>  }

Re: [PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Dragan Simic 1 week, 6 days ago

Hello Heiko,

Thanks for the patch.  Please, see a couple of comments below.

On 2024-11-08 15:44, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a 
> number
> of lines too, so convert the error prints for the dsi-driver.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>  .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 80 ++++++-------------
>  1 file changed, 26 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 58a44af0e9ad..3224ab749352 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -1356,17 +1356,15 @@ static int dw_mipi_dsi_rockchip_probe(struct
> platform_device *pdev)
>  	struct resource *res;
>  	const struct rockchip_dw_dsi_chip_data *cdata =
>  				of_device_get_match_data(dev);
> -	int ret, i;
> +	int i;
> 
>  	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi)
>  		return -ENOMEM;
> 
>  	dsi->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> -	if (IS_ERR(dsi->base)) {
> -		DRM_DEV_ERROR(dev, "Unable to get dsi registers\n");
> -		return PTR_ERR(dsi->base);
> -	}
> +	if (IS_ERR(dsi->base))
> +		return dev_err_probe(dev, PTR_ERR(dsi->base), "Unable to get dsi 
> registers\n");
> 
>  	i = 0;
>  	while (cdata[i].reg) {
> @@ -1378,67 +1376,47 @@ static int dw_mipi_dsi_rockchip_probe(struct
> platform_device *pdev)
>  		i++;
>  	}
> 
> -	if (!dsi->cdata) {
> -		DRM_DEV_ERROR(dev, "no dsi-config for %s node\n", np->name);
> -		return -EINVAL;
> -	}
> +	if (!dsi->cdata)
> +		return dev_err_probe(dev, -EINVAL, "No dsi-config for %s node\n", 
> np->name);
> 
>  	/* try to get a possible external dphy */
>  	dsi->phy = devm_phy_optional_get(dev, "dphy");
> -	if (IS_ERR(dsi->phy)) {
> -		ret = PTR_ERR(dsi->phy);
> -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->phy))
> +		return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi 
> dphy\n");

After spending a couple of days thinking about it, I think this
particular change should be extracted into a separate patch and
submitted additionally for the inclusion into stable kernels.

My reasoning behind that is the "spamming" of the kernel log with
multiple suspiciously looking error messages when getting the dphy
results in deferred probing, which IMHO looks really bad and may
cause false impression of underlying issues.

If you agree with this suggestion, please feel free to use the
relevant parts of the description of the patch I submitted and
withdrawn earlier, [*] to provide the required rationale for the
inclusion into stable kernels.

[*] 
https://lore.kernel.org/linux-rockchip/559094275c3e41cae7c89e904341f89a1240a51a.1731073565.git.dsimic@manjaro.org/

>  	dsi->pclk = devm_clk_get(dev, "pclk");
> -	if (IS_ERR(dsi->pclk)) {
> -		ret = PTR_ERR(dsi->pclk);
> -		DRM_DEV_ERROR(dev, "Unable to get pclk: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->pclk))
> +		return dev_err_probe(dev, PTR_ERR(dsi->pclk), "Unable to get 
> pclk\n");
> 
>  	dsi->pllref_clk = devm_clk_get(dev, "ref");
>  	if (IS_ERR(dsi->pllref_clk)) {
> -		if (dsi->phy) {
> +		if (dsi->phy)
>  			/*
>  			 * if external phy is present, pll will be
>  			 * generated there.
>  			 */
>  			dsi->pllref_clk = NULL;
> -		} else {
> -			ret = PTR_ERR(dsi->pllref_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get pll reference clock: %d\n",
> -				      ret);
> -			return ret;
> -		}
> +		else
> +			return dev_err_probe(dev, PTR_ERR(dsi->pllref_clk),
> +					     "Unable to get pll reference clock\n");
>  	}
> 
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_PHY_CFG_CLK) {
>  		dsi->phy_cfg_clk = devm_clk_get(dev, "phy_cfg");
> -		if (IS_ERR(dsi->phy_cfg_clk)) {
> -			ret = PTR_ERR(dsi->phy_cfg_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get phy_cfg_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->phy_cfg_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->phy_cfg_clk),
> +					     "Unable to get phy_cfg_clk\n");
>  	}
> 
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_GRF_CLK) {
>  		dsi->grf_clk = devm_clk_get(dev, "grf");
> -		if (IS_ERR(dsi->grf_clk)) {
> -			ret = PTR_ERR(dsi->grf_clk);
> -			DRM_DEV_ERROR(dev, "Unable to get grf_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->grf_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->grf_clk), "Unable to get 
> grf_clk\n");
>  	}
> 
>  	dsi->grf_regmap = syscon_regmap_lookup_by_phandle(np, 
> "rockchip,grf");
> -	if (IS_ERR(dsi->grf_regmap)) {
> -		DRM_DEV_ERROR(dev, "Unable to get rockchip,grf\n");
> -		return PTR_ERR(dsi->grf_regmap);
> -	}
> +	if (IS_ERR(dsi->grf_regmap))
> +		return dev_err_probe(dev, PTR_ERR(dsi->grf_regmap), "Unable to get 
> rockchip,grf\n");
> 
>  	dsi->dev = dev;
>  	dsi->pdata.base = dsi->base;
> @@ -1451,24 +1429,18 @@ static int dw_mipi_dsi_rockchip_probe(struct
> platform_device *pdev)
>  	mutex_init(&dsi->usage_mutex);
> 
>  	dsi->dphy = devm_phy_create(dev, NULL, &dw_mipi_dsi_dphy_ops);
> -	if (IS_ERR(dsi->dphy)) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to create PHY\n");
> -		return PTR_ERR(dsi->dphy);
> -	}
> +	if (IS_ERR(dsi->dphy))
> +		return dev_err_probe(dev, PTR_ERR(dsi->dphy), "Failed to create 
> PHY\n");
> 
>  	phy_set_drvdata(dsi->dphy, dsi);
>  	phy_provider = devm_of_phy_provider_register(dev, 
> of_phy_simple_xlate);
>  	if (IS_ERR(phy_provider))
> -		return PTR_ERR(phy_provider);
> +		return dev_err_probe(dev, PTR_ERR(phy_provider),
> +				     "Failed to register phy-provider\n");
> 
>  	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> -	if (IS_ERR(dsi->dmd)) {
> -		ret = PTR_ERR(dsi->dmd);
> -		if (ret != -EPROBE_DEFER)
> -			DRM_DEV_ERROR(dev,
> -				      "Failed to probe dw_mipi_dsi: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->dmd))
> +		return dev_err_probe(dev, PTR_ERR(dsi->dmd), "Failed to probe 
> dw_mipi_dsi\n");
> 
>  	return 0;
>  }

Regardless of the above-suggested split into a patch series
ending up accepted or not, the introduced changes are looking
good to me, so please feel free to include my

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Re: [PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Diederik de Haas 2 weeks, 1 day ago

On Fri Nov 8, 2024 at 3:44 PM CET, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
>
> DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a number
> of lines too, so convert the error prints for the dsi-driver.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>

Should this have a Fixes tag?
Because in the PineTab2 case it reported an error, which was actually
just a deferred probe.

> ---
>  .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 80 ++++++-------------
>  1 file changed, 26 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 58a44af0e9ad..3224ab749352 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> ...
> @@ -1378,67 +1376,47 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> -	if (!dsi->cdata) {
> -		DRM_DEV_ERROR(dev, "no dsi-config for %s node\n", np->name);
> -		return -EINVAL;
> -	}
> +	if (!dsi->cdata)
> +		return dev_err_probe(dev, -EINVAL, "No dsi-config for %s node\n", np->name);
>  
>  	/* try to get a possible external dphy */
>  	dsi->phy = devm_phy_optional_get(dev, "dphy");
> -	if (IS_ERR(dsi->phy)) {
> -		ret = PTR_ERR(dsi->phy);
> -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->phy))
> +		return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi dphy\n");

I think from this line.

Cheers,
  Diederik

>  
>  	dsi->pclk = devm_clk_get(dev, "pclk");
> -	if (IS_ERR(dsi->pclk)) {
> -		ret = PTR_ERR(dsi->pclk);
> -		DRM_DEV_ERROR(dev, "Unable to get pclk: %d\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(dsi->pclk))
> +		return dev_err_probe(dev, PTR_ERR(dsi->pclk), "Unable to get pclk\n");
>  
>  	dsi->pllref_clk = devm_clk_get(dev, "ref");
>  	if (IS_ERR(dsi->pllref_clk)) {
> -		if (dsi->phy) {
> +		if (dsi->phy)
>  			/*
>  			 * if external phy is present, pll will be
>  			 * generated there.
>  			 */
>  			dsi->pllref_clk = NULL;
> -		} else {
> -			ret = PTR_ERR(dsi->pllref_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get pll reference clock: %d\n",
> -				      ret);
> -			return ret;
> -		}
> +		else
> +			return dev_err_probe(dev, PTR_ERR(dsi->pllref_clk),
> +					     "Unable to get pll reference clock\n");
>  	}
>  
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_PHY_CFG_CLK) {
>  		dsi->phy_cfg_clk = devm_clk_get(dev, "phy_cfg");
> -		if (IS_ERR(dsi->phy_cfg_clk)) {
> -			ret = PTR_ERR(dsi->phy_cfg_clk);
> -			DRM_DEV_ERROR(dev,
> -				      "Unable to get phy_cfg_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->phy_cfg_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->phy_cfg_clk),
> +					     "Unable to get phy_cfg_clk\n");
>  	}
>  
>  	if (dsi->cdata->flags & DW_MIPI_NEEDS_GRF_CLK) {
>  		dsi->grf_clk = devm_clk_get(dev, "grf");
> -		if (IS_ERR(dsi->grf_clk)) {
> -			ret = PTR_ERR(dsi->grf_clk);
> -			DRM_DEV_ERROR(dev, "Unable to get grf_clk: %d\n", ret);
> -			return ret;
> -		}
> +		if (IS_ERR(dsi->grf_clk))
> +			return dev_err_probe(dev, PTR_ERR(dsi->grf_clk), "Unable to get grf_clk\n");
>  	}
>  
>  	dsi->grf_regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> -	if (IS_ERR(dsi->grf_regmap)) {
> -		DRM_DEV_ERROR(dev, "Unable to get rockchip,grf\n");
> -		return PTR_ERR(dsi->grf_regmap);
> -	}
> +	if (IS_ERR(dsi->grf_regmap))
> +		return dev_err_probe(dev, PTR_ERR(dsi->grf_regmap), "Unable to get rockchip,grf\n");
>  
>  	dsi->dev = dev;
>  	dsi->pdata.base = dsi->base;
> ...

Re: [PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Heiko Stübner 2 weeks, 1 day ago

Am Freitag, 8. November 2024, 16:21:24 CET schrieb Diederik de Haas:
> On Fri Nov 8, 2024 at 3:44 PM CET, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> >
> > DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a number
> > of lines too, so convert the error prints for the dsi-driver.
> >
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Should this have a Fixes tag?
> Because in the PineTab2 case it reported an error, which was actually
> just a deferred probe.

A deferred-probe is an error ;-)   -517 in fact  ... just that convention
nowadays is to not actively report on it but "fail" silently.

So personally I don't really consider it a fix, but more a style thing.
I guess I'll let others chime in for that.


Heiko

> 
> > ---
> >  .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 80 ++++++-------------
> >  1 file changed, 26 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > index 58a44af0e9ad..3224ab749352 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > ...
> > @@ -1378,67 +1376,47 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
> >  		i++;
> >  	}
> >  
> > -	if (!dsi->cdata) {
> > -		DRM_DEV_ERROR(dev, "no dsi-config for %s node\n", np->name);
> > -		return -EINVAL;
> > -	}
> > +	if (!dsi->cdata)
> > +		return dev_err_probe(dev, -EINVAL, "No dsi-config for %s node\n", np->name);
> >  
> >  	/* try to get a possible external dphy */
> >  	dsi->phy = devm_phy_optional_get(dev, "dphy");
> > -	if (IS_ERR(dsi->phy)) {
> > -		ret = PTR_ERR(dsi->phy);
> > -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
> > -		return ret;
> > -	}
> > +	if (IS_ERR(dsi->phy))
> > +		return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi dphy\n");
> 
> I think from this line.
> 
> Cheers,
>   Diederik
> 
> >  
> >  	dsi->pclk = devm_clk_get(dev, "pclk");
> > -	if (IS_ERR(dsi->pclk)) {
> > -		ret = PTR_ERR(dsi->pclk);
> > -		DRM_DEV_ERROR(dev, "Unable to get pclk: %d\n", ret);
> > -		return ret;
> > -	}
> > +	if (IS_ERR(dsi->pclk))
> > +		return dev_err_probe(dev, PTR_ERR(dsi->pclk), "Unable to get pclk\n");
> >  
> >  	dsi->pllref_clk = devm_clk_get(dev, "ref");
> >  	if (IS_ERR(dsi->pllref_clk)) {
> > -		if (dsi->phy) {
> > +		if (dsi->phy)
> >  			/*
> >  			 * if external phy is present, pll will be
> >  			 * generated there.
> >  			 */
> >  			dsi->pllref_clk = NULL;
> > -		} else {
> > -			ret = PTR_ERR(dsi->pllref_clk);
> > -			DRM_DEV_ERROR(dev,
> > -				      "Unable to get pll reference clock: %d\n",
> > -				      ret);
> > -			return ret;
> > -		}
> > +		else
> > +			return dev_err_probe(dev, PTR_ERR(dsi->pllref_clk),
> > +					     "Unable to get pll reference clock\n");
> >  	}
> >  
> >  	if (dsi->cdata->flags & DW_MIPI_NEEDS_PHY_CFG_CLK) {
> >  		dsi->phy_cfg_clk = devm_clk_get(dev, "phy_cfg");
> > -		if (IS_ERR(dsi->phy_cfg_clk)) {
> > -			ret = PTR_ERR(dsi->phy_cfg_clk);
> > -			DRM_DEV_ERROR(dev,
> > -				      "Unable to get phy_cfg_clk: %d\n", ret);
> > -			return ret;
> > -		}
> > +		if (IS_ERR(dsi->phy_cfg_clk))
> > +			return dev_err_probe(dev, PTR_ERR(dsi->phy_cfg_clk),
> > +					     "Unable to get phy_cfg_clk\n");
> >  	}
> >  
> >  	if (dsi->cdata->flags & DW_MIPI_NEEDS_GRF_CLK) {
> >  		dsi->grf_clk = devm_clk_get(dev, "grf");
> > -		if (IS_ERR(dsi->grf_clk)) {
> > -			ret = PTR_ERR(dsi->grf_clk);
> > -			DRM_DEV_ERROR(dev, "Unable to get grf_clk: %d\n", ret);
> > -			return ret;
> > -		}
> > +		if (IS_ERR(dsi->grf_clk))
> > +			return dev_err_probe(dev, PTR_ERR(dsi->grf_clk), "Unable to get grf_clk\n");
> >  	}
> >  
> >  	dsi->grf_regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > -	if (IS_ERR(dsi->grf_regmap)) {
> > -		DRM_DEV_ERROR(dev, "Unable to get rockchip,grf\n");
> > -		return PTR_ERR(dsi->grf_regmap);
> > -	}
> > +	if (IS_ERR(dsi->grf_regmap))
> > +		return dev_err_probe(dev, PTR_ERR(dsi->grf_regmap), "Unable to get rockchip,grf\n");
> >  
> >  	dsi->dev = dev;
> >  	dsi->pdata.base = dsi->base;
> > ...
> 
>

Re: [PATCH] drm/rockchip: dsi: convert to dev_err_probe in probe function

Posted by Diederik de Haas 2 weeks, 1 day ago

On Fri Nov 8, 2024 at 5:31 PM CET, Heiko Stübner wrote:
> Am Freitag, 8. November 2024, 16:21:24 CET schrieb Diederik de Haas:
> > On Fri Nov 8, 2024 at 3:44 PM CET, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > >
> > > DRM_DEV_ERROR is deprecated and using dev_err_probe saves quite a number
> > > of lines too, so convert the error prints for the dsi-driver.
> > >
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > Should this have a Fixes tag?
> > Because in the PineTab2 case it reported an error, which was actually
> > just a deferred probe.
>
> A deferred-probe is an error ;-)   -517 in fact  ... just that convention
> nowadays is to not actively report on it but "fail" silently.

Good to know, thanks :)

> So personally I don't really consider it a fix, but more a style thing.
> I guess I'll let others chime in for that.

Then I agree that it should not have a Fixes tag.

Cheers,
  Diederik