Re: [PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Friday, 6 June 2025 05:27:13 Central European Summer Time Pei Xiao wrote:
> 
> 在 2025/6/5 01:42, Nicolas Frattaroli 写道:
> > On Wednesday, 4 June 2025 05:13:30 Central European Summer Time Pei Xiao wrote:
> >> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> >> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> >> replaced by devm_clk_get_enabled() when driver enables the clocks for the
> >> whole lifetime of the device. Moreover, it is no longer necessary to
> >> unprepare and disable the clocks explicitly.
> >>
> >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> >> ---
> >>  sound/soc/rockchip/rockchip_sai.c | 8 +-------
> >>  1 file changed, 1 insertion(+), 7 deletions(-)
> >>
> >> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
> >> index 79b04770da1c..2ec675708681 100644
> >> --- a/sound/soc/rockchip/rockchip_sai.c
> >> +++ b/sound/soc/rockchip/rockchip_sai.c
> >> @@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
> >>  				     "Failed to get mclk\n");
> >>  	}
> >>  
> >> -	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> >> +	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
> >>  	if (IS_ERR(sai->hclk)) {
> >>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
> >>  				     "Failed to get hclk\n");
> >>  	}
> >>  
> >> -	ret = clk_prepare_enable(sai->hclk);
> >> -	if (ret)
> >> -		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> >> -
> >>  	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
> >>  
> >>  	ret = rockchip_sai_init_dai(sai, res, &dai);
> >> @@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
> >>  	if (pm_runtime_put(&pdev->dev))
> >>  		rockchip_sai_runtime_suspend(&pdev->dev);
> >>  err_disable_hclk:
> >> -	clk_disable_unprepare(sai->hclk);
> >> -
> >>  	return ret;
> >>  }
> >>  
> >>
> > Please get rid of the err_disable_hclk label, and change the
> >
> >     goto err_disable_hclk;
> >
> > in the resume failure condition to a 
> >
> >     return ret;
> May I ask, could we use the dev_err_probe function instead?
> 
> return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");

Absolutely, dev_err_probe states the following in its documentation:

    Using this helper in your probe function is totally fine even if @err
    is known to never be -EPROBE_DEFER.

This means you can use it for every error case in the probe function.

> 
> 
> @@ -1441,28 +1441,22 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>                                      "Failed to get mclk\n");
>         }
>  
> -       sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> +       sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
>         if (IS_ERR(sai->hclk)) {
>                 return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>                                      "Failed to get hclk\n");
>         }
>  
> -       ret = clk_prepare_enable(sai->hclk);
> -       if (ret)
> -               return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> -
>         regmap_read(sai->regmap, SAI_VERSION, &sai->version);
>  
>         ret = rockchip_sai_init_dai(sai, res, &dai);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to initialize DAI: %d\n", ret);
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");
>         }

This would now be a one-line statement in the if branch, so checkpatch will
advise you to remove the redundant { } from the if, so that it becomes

    if (ret)
            return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");

You can run `./scripts/checkpatch.pl` on either a git commit range or a
patch file, and it'll let you know. If you use b4, it'll run that
script with some recommended flags on all commits in your series with
`b4 prep --check`.

>  
>         ret = rockchip_sai_parse_paths(sai, node);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to parse paths: %d\n", ret);
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to parse paths\n");
>         }

Same here

>  
>         /*
> @@ -1475,8 +1469,7 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>         pm_runtime_get_noresume(&pdev->dev);
>         ret = rockchip_sai_runtime_resume(&pdev->dev);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to resume device: %pe\n", ERR_PTR(ret));
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to resume device\n");
>         }

Same here

>  
>         ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> @@ -1504,8 +1497,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>         /* If we're !CONFIG_PM, we get -ENOSYS and disable manually */
>         if (pm_runtime_put(&pdev->dev))
>                 rockchip_sai_runtime_suspend(&pdev->dev);
> -err_disable_hclk:
> -       clk_disable_unprepare(sai->hclk);
>  
>         return ret;
>  }
> 
> thanks!
> 
> Pei.

Kind regards,
Nicolas Frattaroli

> 
> > Other than that, patch tested to be working fine.
> >
> > Kind regards,
> > Nicolas Frattaroli
> >
> >
>