| Message ID | 20240820125840.9032-1-rongqianfeng@vivo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpu: drm: Use devm_clk_get_enabled() helpers | expand |
On Tue, Aug 20, 2024 at 8:59 PM Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> > --- > 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 >
在 2024/8/20 21:51, Chen-Yu Tsai 写道: > On Tue, Aug 20, 2024 at 8:59 PM Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> >> --- >> 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. Thanks for your reminder, I didn't notice such a difference. > > 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. Could you please post the log of the problem? > > > 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 >>
Hi Rong, On Tue, Aug 20, 2024 at 10:59 PM Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> > --- > 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/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"); > 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); Am I reading this right: is this changing the init sequence so that the clock is enabled before setting a rate? This is the sort of thing that might cause glitches and issues in some hardware, so it'd be polite to test such a change on the actual hardware before posting it. Thanks,
在 2024/8/22 8:35, Julian Calaby 写道: > [Some people who received this message don't often get email from julian.calaby@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > Hi Rong, > > On Tue, Aug 20, 2024 at 10:59 PM Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> >> --- >> 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/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"); >> 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); > Am I reading this right: is this changing the init sequence so that > the clock is enabled before setting a rate? > > This is the sort of thing that might cause glitches and issues in some > hardware, so it'd be polite to test such a change on the actual > hardware before posting it. Hi Julian , I have seen this used in other places, but the problem you raised may be different on different hardware. I wonder if anyone can clarify this? Best regards, Qianfeng > > Thanks, > > -- > Julian Calaby > > Email: julian.calaby@gmail.com > Profile: http://www.google.com/profiles/julian.calaby/
Hi Rong, On Tue, 20 Aug 2024 at 18:30, Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> > --- > 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); You can use dev_err_probe it will be fine in all cases for clocks.to get enabled. return dev_err_probe(dev, PTR_ERR(fsl_dev->clk), "failed to get duc clock\n"); Thanks -Anand > } > - 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"); > 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 > >
在 2024/8/22 13:32, Anand Moon 写道: > [Some people who received this message don't often get email from linux.amoon@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > Hi Rong, > > On Tue, 20 Aug 2024 at 18:30, Rong Qianfeng <rongqianfeng@vivo.com> 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@vivo.com> >> --- >> 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); > You can use dev_err_probe it will be fine in all cases for clocks.to > get enabled. > return dev_err_probe(dev, PTR_ERR(fsl_dev->clk), > "failed to get duc clock\n"); Good point, Thanks, I'll send v2 shortly. Best regards, Qianfeng > > Thanks > -Anand >> } >> - 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"); >> 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 >> >>
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"); 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); }
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@vivo.com> --- 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(-)