On Tue, Aug 20, 2024 at 8:59 PM Rong Qianfeng <rongqianfeng@xxxxxxxx> wrote:
>
> Replace devm_clk_get() and clk_prepare_enable() with
> devm_clk_get_enabled() that also disables and unprepares it on
> driver detach.
>
> Signed-off-by: Rong Qianfeng <rongqianfeng@xxxxxxxx>
> ---
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 13 +++----------
> drivers/gpu/drm/sun4i/sun6i_drc.c | 15 ++++-----------
> drivers/gpu/drm/sun4i/sun8i_mixer.c | 13 +++----------
> 3 files changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index ab6c0c6cd0e2..057dceaf079e 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -284,16 +284,11 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> return PTR_ERR(fsl_dev->regmap);
> }
>
> - fsl_dev->clk = devm_clk_get(dev, "dcu");
> + fsl_dev->clk = devm_clk_get_enabled(dev, "dcu");
> if (IS_ERR(fsl_dev->clk)) {
> dev_err(dev, "failed to get dcu clock\n");
> return PTR_ERR(fsl_dev->clk);
> }
> - ret = clk_prepare_enable(fsl_dev->clk);
> - if (ret < 0) {
> - dev_err(dev, "failed to enable dcu clk\n");
> - return ret;
> - }
>
> pix_clk_in = devm_clk_get(dev, "pix");
> if (IS_ERR(pix_clk_in)) {
> @@ -311,8 +306,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> div_ratio_shift, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL);
> if (IS_ERR(fsl_dev->pix_clk)) {
> dev_err(dev, "failed to register pix clk\n");
> - ret = PTR_ERR(fsl_dev->pix_clk);
> - goto disable_clk;
> + return PTR_ERR(fsl_dev->pix_clk);
> }
>
> fsl_dev->tcon = fsl_tcon_init(dev);
> @@ -341,8 +335,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> drm_dev_put(drm);
> unregister_pix_clk:
> clk_unregister(fsl_dev->pix_clk);
> -disable_clk:
> - clk_disable_unprepare(fsl_dev->clk);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_drc.c b/drivers/gpu/drm/sun4i/sun6i_drc.c
> index 0d342f43fa93..f263ad282828 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_drc.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_drc.c
> @@ -42,33 +42,28 @@ static int sun6i_drc_bind(struct device *dev, struct device *master,
> return ret;
> }
>
> - drc->bus_clk = devm_clk_get(dev, "ahb");
> + drc->bus_clk = devm_clk_get_enabled(dev, "ahb");
devm_* is actually not the correct thing to use in this case, as this
is the component bind function, not the probe function. The lifetimes
are not the same.
We get away with devm_*_get because it's just a reference count,
but devm_*_get_enabled is going to be worse, because the clock
or whatever will not get disabled upon unbind. Same for sun8i_mixer.
I just got bitten by this in an ASoC component driver, but the
problem is similar.
ChenYu
> if (IS_ERR(drc->bus_clk)) {
> dev_err(dev, "Couldn't get our bus clock\n");
> ret = PTR_ERR(drc->bus_clk);
> goto err_assert_reset;
> }
> - clk_prepare_enable(drc->bus_clk);
>
> - drc->mod_clk = devm_clk_get(dev, "mod");
> + drc->mod_clk = devm_clk_get_enabled(dev, "mod");
> if (IS_ERR(drc->mod_clk)) {
> dev_err(dev, "Couldn't get our mod clock\n");
> ret = PTR_ERR(drc->mod_clk);
> - goto err_disable_bus_clk;
> + goto err_assert_reset;
> }
>
> ret = clk_set_rate_exclusive(drc->mod_clk, 300000000);
> if (ret) {
> dev_err(dev, "Couldn't set the module clock frequency\n");
> - goto err_disable_bus_clk;
> + goto err_assert_reset;
> }
>
> - clk_prepare_enable(drc->mod_clk);
> -
> return 0;
>
> -err_disable_bus_clk:
> - clk_disable_unprepare(drc->bus_clk);
> err_assert_reset:
> reset_control_assert(drc->reset);
> return ret;
> @@ -80,8 +75,6 @@ static void sun6i_drc_unbind(struct device *dev, struct device *master,
> struct sun6i_drc *drc = dev_get_drvdata(dev);
>
> clk_rate_exclusive_put(drc->mod_clk);
> - clk_disable_unprepare(drc->mod_clk);
> - clk_disable_unprepare(drc->bus_clk);
> reset_control_assert(drc->reset);
> }
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index bd0fe2c6624e..ebf00676a76d 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -507,19 +507,18 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> return ret;
> }
>
> - mixer->bus_clk = devm_clk_get(dev, "bus");
> + mixer->bus_clk = devm_clk_get_enabled(dev, "bus");
> if (IS_ERR(mixer->bus_clk)) {
> dev_err(dev, "Couldn't get the mixer bus clock\n");
> ret = PTR_ERR(mixer->bus_clk);
> goto err_assert_reset;
> }
> - clk_prepare_enable(mixer->bus_clk);
>
> - mixer->mod_clk = devm_clk_get(dev, "mod");
> + mixer->mod_clk = devm_clk_get_enabled(dev, "mod");
> if (IS_ERR(mixer->mod_clk)) {
> dev_err(dev, "Couldn't get the mixer module clock\n");
> ret = PTR_ERR(mixer->mod_clk);
> - goto err_disable_bus_clk;
> + goto err_assert_reset;
> }
>
> /*
> @@ -530,8 +529,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
> if (mixer->cfg->mod_rate)
> clk_set_rate(mixer->mod_clk, mixer->cfg->mod_rate);
>
> - clk_prepare_enable(mixer->mod_clk);
> -
> list_add_tail(&mixer->engine.list, &drv->engine_list);
>
> base = sun8i_blender_base(mixer);
> @@ -592,8 +589,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>
> return 0;
>
> -err_disable_bus_clk:
> - clk_disable_unprepare(mixer->bus_clk);
> err_assert_reset:
> reset_control_assert(mixer->reset);
> return ret;
> @@ -606,8 +601,6 @@ static void sun8i_mixer_unbind(struct device *dev, struct device *master,
>
> list_del(&mixer->engine.list);
>
> - clk_disable_unprepare(mixer->mod_clk);
> - clk_disable_unprepare(mixer->bus_clk);
> reset_control_assert(mixer->reset);
> }
>
> --
> 2.39.0
>
