| Message ID | 20201120094205.525228-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | More meson HDMI fixes | expand |
On 20/11/2020 10:42, Marc Zyngier wrote: > The HDMI driver request clocks early, but never disable them, leaving > the clocks on even when the driver is removed. > > Fix it by slightly refactoring the clock code, and register a devm > action that will eventually disable/unprepare the enabled clocks. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c > index 7f8eea494147..29623b309cb1 100644 > --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c > @@ -145,8 +145,6 @@ struct meson_dw_hdmi { > struct reset_control *hdmitx_apb; > struct reset_control *hdmitx_ctrl; > struct reset_control *hdmitx_phy; > - struct clk *hdmi_pclk; > - struct clk *venci_clk; > struct regulator *hdmi_supply; > u32 irq_stat; > struct dw_hdmi *hdmi; > @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data) > regulator_disable(data); > } > > +static void meson_disable_clk(void *data) > +{ > + clk_disable_unprepare(data); > +} > + > +static int meson_enable_clk(struct device *dev, char *name) > +{ > + struct clk *clk; > + int ret; > + > + clk = devm_clk_get(dev, name); > + if (IS_ERR(clk)) { > + dev_err(dev, "Unable to get %s pclk\n", name); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (!ret) > + ret = devm_add_action_or_reset(dev, meson_disable_clk, clk); > + > + return ret; > +} > + > static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > void *data) > { > @@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > if (IS_ERR(meson_dw_hdmi->hdmitx)) > return PTR_ERR(meson_dw_hdmi->hdmitx); > > - meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr"); > - if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) { > - dev_err(dev, "Unable to get HDMI pclk\n"); > - return PTR_ERR(meson_dw_hdmi->hdmi_pclk); > - } > - clk_prepare_enable(meson_dw_hdmi->hdmi_pclk); > + ret = meson_enable_clk(dev, "isfr"); > + if (ret) > + return ret; > > - meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci"); > - if (IS_ERR(meson_dw_hdmi->venci_clk)) { > - dev_err(dev, "Unable to get venci clk\n"); > - return PTR_ERR(meson_dw_hdmi->venci_clk); > - } > - clk_prepare_enable(meson_dw_hdmi->venci_clk); > + ret = meson_enable_clk(dev, "venci"); > + if (ret) > + return ret; > > dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi, > &meson_dw_hdmi_regmap_config); > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
On Fri 20 Nov 2020 at 10:42, Marc Zyngier <maz@kernel.org> wrote: > The HDMI driver request clocks early, but never disable them, leaving > the clocks on even when the driver is removed. > > Fix it by slightly refactoring the clock code, and register a devm > action that will eventually disable/unprepare the enabled clocks. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c > index 7f8eea494147..29623b309cb1 100644 > --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c > @@ -145,8 +145,6 @@ struct meson_dw_hdmi { > struct reset_control *hdmitx_apb; > struct reset_control *hdmitx_ctrl; > struct reset_control *hdmitx_phy; > - struct clk *hdmi_pclk; > - struct clk *venci_clk; > struct regulator *hdmi_supply; > u32 irq_stat; > struct dw_hdmi *hdmi; > @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data) > regulator_disable(data); > } > > +static void meson_disable_clk(void *data) > +{ > + clk_disable_unprepare(data); > +} > + > +static int meson_enable_clk(struct device *dev, char *name) > +{ > + struct clk *clk; > + int ret; > + > + clk = devm_clk_get(dev, name); > + if (IS_ERR(clk)) { > + dev_err(dev, "Unable to get %s pclk\n", name); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (!ret) > + ret = devm_add_action_or_reset(dev, meson_disable_clk, > clk); Thanks for fixing this Marc. FYI, while it is fine to declare a function to disable the clocks, a quick cast may avoid it devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, clk); > + > + return ret; > +} > + > static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > void *data) > { > @@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > if (IS_ERR(meson_dw_hdmi->hdmitx)) > return PTR_ERR(meson_dw_hdmi->hdmitx); > > - meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr"); > - if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) { > - dev_err(dev, "Unable to get HDMI pclk\n"); > - return PTR_ERR(meson_dw_hdmi->hdmi_pclk); > - } > - clk_prepare_enable(meson_dw_hdmi->hdmi_pclk); > + ret = meson_enable_clk(dev, "isfr"); > + if (ret) > + return ret; > > - meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci"); > - if (IS_ERR(meson_dw_hdmi->venci_clk)) { > - dev_err(dev, "Unable to get venci clk\n"); > - return PTR_ERR(meson_dw_hdmi->venci_clk); > - } > - clk_prepare_enable(meson_dw_hdmi->venci_clk); > + ret = meson_enable_clk(dev, "venci"); > + if (ret) > + return ret; > > dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi, > &meson_dw_hdmi_regmap_config);
On 2020-11-23 14:03, Jerome Brunet wrote: > On Fri 20 Nov 2020 at 10:42, Marc Zyngier <maz@kernel.org> wrote: > >> The HDMI driver request clocks early, but never disable them, leaving >> the clocks on even when the driver is removed. >> >> Fix it by slightly refactoring the clock code, and register a devm >> action that will eventually disable/unprepare the enabled clocks. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 >> ++++++++++++++++++--------- >> 1 file changed, 29 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c >> b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> index 7f8eea494147..29623b309cb1 100644 >> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c >> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> @@ -145,8 +145,6 @@ struct meson_dw_hdmi { >> struct reset_control *hdmitx_apb; >> struct reset_control *hdmitx_ctrl; >> struct reset_control *hdmitx_phy; >> - struct clk *hdmi_pclk; >> - struct clk *venci_clk; >> struct regulator *hdmi_supply; >> u32 irq_stat; >> struct dw_hdmi *hdmi; >> @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data) >> regulator_disable(data); >> } >> >> +static void meson_disable_clk(void *data) >> +{ >> + clk_disable_unprepare(data); >> +} >> + >> +static int meson_enable_clk(struct device *dev, char *name) >> +{ >> + struct clk *clk; >> + int ret; >> + >> + clk = devm_clk_get(dev, name); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "Unable to get %s pclk\n", name); >> + return PTR_ERR(clk); >> + } >> + >> + ret = clk_prepare_enable(clk); >> + if (!ret) >> + ret = devm_add_action_or_reset(dev, meson_disable_clk, >> clk); > > Thanks for fixing this Marc. > > FYI, while it is fine to declare a function to disable the clocks, a > quick > cast may avoid it > > devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, > clk); While this works for now, a change to the clk_disable_unprepare() prototype (such as adding a second argument) would now go completely unnoticed (after all, you've cast the function, it *must* be correct, right?), and someone would spend a few hours trying to track down memory corruption or some other interesting results. Yes, casting C functions can be hilarious. I can see a few uses of this hack in the tree, and I have my pop-corn ready. Thanks, M.
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c index 7f8eea494147..29623b309cb1 100644 --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c @@ -145,8 +145,6 @@ struct meson_dw_hdmi { struct reset_control *hdmitx_apb; struct reset_control *hdmitx_ctrl; struct reset_control *hdmitx_phy; - struct clk *hdmi_pclk; - struct clk *venci_clk; struct regulator *hdmi_supply; u32 irq_stat; struct dw_hdmi *hdmi; @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data) regulator_disable(data); } +static void meson_disable_clk(void *data) +{ + clk_disable_unprepare(data); +} + +static int meson_enable_clk(struct device *dev, char *name) +{ + struct clk *clk; + int ret; + + clk = devm_clk_get(dev, name); + if (IS_ERR(clk)) { + dev_err(dev, "Unable to get %s pclk\n", name); + return PTR_ERR(clk); + } + + ret = clk_prepare_enable(clk); + if (!ret) + ret = devm_add_action_or_reset(dev, meson_disable_clk, clk); + + return ret; +} + static int meson_dw_hdmi_bind(struct device *dev, struct device *master, void *data) { @@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, if (IS_ERR(meson_dw_hdmi->hdmitx)) return PTR_ERR(meson_dw_hdmi->hdmitx); - meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr"); - if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) { - dev_err(dev, "Unable to get HDMI pclk\n"); - return PTR_ERR(meson_dw_hdmi->hdmi_pclk); - } - clk_prepare_enable(meson_dw_hdmi->hdmi_pclk); + ret = meson_enable_clk(dev, "isfr"); + if (ret) + return ret; - meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci"); - if (IS_ERR(meson_dw_hdmi->venci_clk)) { - dev_err(dev, "Unable to get venci clk\n"); - return PTR_ERR(meson_dw_hdmi->venci_clk); - } - clk_prepare_enable(meson_dw_hdmi->venci_clk); + ret = meson_enable_clk(dev, "venci"); + if (ret) + return ret; dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi, &meson_dw_hdmi_regmap_config);
The HDMI driver request clocks early, but never disable them, leaving the clocks on even when the driver is removed. Fix it by slightly refactoring the clock code, and register a devm action that will eventually disable/unprepare the enabled clocks. Signed-off-by: Marc Zyngier <maz@kernel.org> --- drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++++++++++++++++++--------- 1 file changed, 29 insertions(+), 14 deletions(-)