Re: [PATCH 1/1] drivers:staging: fix return value check in fsl_mx6_enable

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Mon, Jul 17, 2023 at 10:47:17PM +0800, Yuanjun Gong wrote:
> in fsl_mx6_enable, the second time of calling clk_prepare_enable()
> may fail, so its return value should also be checked. the clk enabled
> by the first invocation of clk_prepare_enable should be disabled
> on the error path.
> 
> Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@xxxxxxx>
> ---
>  drivers/staging/most/dim2/dim2.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> index 44d3252d4612..4204cfba6b48 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -944,7 +944,12 @@ static int fsl_mx6_enable(struct platform_device *pdev)
>  		}
>  
>  		writel(0x888, dev->io_base + 0x38);
> -		clk_prepare_enable(dev->clk_pll);
> +		ret = clk_prepare_enable(dev->clk_pll);
> +		if (ret) {
> +			dev_err(&pdev->dev, "%s\n", "clk_prepare_enable failed");
> +			clk_disable_unprepare(dev->clk);
> +			return ret;

This function has several wrong things.  The IS_ERR_OR_NULL() checks
should be IS_ERR().  It should propagate the error code instead of
returning -EFAULT.  Using dev_err_probe() is required as well because
the error code could be -EPROBE_DEFER.

It would be cleaner to use devm_clk_get_enabled().  That's simple enough
for the first instance but I wonder what the writel(0x888, dev->io_base + 0x38);
is for before the second enable?

Anyway, it would be better to send something like this.

regards,
dan carpenter

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 44d3252d4612..d33394d4f742 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -922,29 +922,20 @@ static int fsl_mx6_enable(struct platform_device *pdev)
 	struct dim2_hdm *dev = platform_get_drvdata(pdev);
 	int ret;
 
-	dev->clk = devm_clk_get(&pdev->dev, "mlb");
-	if (IS_ERR_OR_NULL(dev->clk)) {
-		dev_err(&pdev->dev, "unable to get mlb clock\n");
-		return -EFAULT;
-	}
-
-	ret = clk_prepare_enable(dev->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "%s\n", "clk_prepare_enable failed");
-		return ret;
-	}
+	dev->clk = devm_clk_get_enabled(&pdev->dev, "mlb");
+	if (IS_ERR(dev->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dev->clk), "unable to get mlb clock\n");
 
 	if (dev->clk_speed >= CLK_2048FS) {
 		/* enable pll */
 		dev->clk_pll = devm_clk_get(&pdev->dev, "pll8_mlb");
-		if (IS_ERR_OR_NULL(dev->clk_pll)) {
-			dev_err(&pdev->dev, "unable to get mlb pll clock\n");
-			clk_disable_unprepare(dev->clk);
-			return -EFAULT;
-		}
+		if (IS_ERR(dev->clk_pll))
+			return dev_err_probe(&pdev->dev, PTR_ERR(dev->clk_pll), "unable to get mlb pll clock\n");
 
 		writel(0x888, dev->io_base + 0x38);
-		clk_prepare_enable(dev->clk_pll);
+		ret = clk_prepare_enable(dev->clk_pll);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -956,8 +947,6 @@ static void fsl_mx6_disable(struct platform_device *pdev)
 
 	if (dev->clk_speed >= CLK_2048FS)
 		clk_disable_unprepare(dev->clk_pll);
-
-	clk_disable_unprepare(dev->clk);
 }
 
 static int rcar_gen2_enable(struct platform_device *pdev)