* [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 9:42 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-20 9:42 UTC (permalink / raw)
To: Neil Armstrong, Kevin Hilman
Cc: Jerome Brunet, Martin Blumenstingl, Guillaume Tucker, kernel-team,
dri-devel, linux-amlogic, linux-arm-kernel, linux-kernel
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);
--
2.28.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 9:42 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-20 9:42 UTC (permalink / raw)
To: Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel, Jerome Brunet
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);
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 9:42 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-20 9:42 UTC (permalink / raw)
To: Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel, Jerome Brunet
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);
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
2020-11-20 9:42 ` Marc Zyngier
(?)
(?)
@ 2020-11-20 12:26 ` Neil Armstrong
-1 siblings, 0 replies; 40+ messages in thread
From: Neil Armstrong @ 2020-11-20 12:26 UTC (permalink / raw)
To: Marc Zyngier, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel, Jerome Brunet
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>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 12:26 ` Neil Armstrong
0 siblings, 0 replies; 40+ messages in thread
From: Neil Armstrong @ 2020-11-20 12:26 UTC (permalink / raw)
To: Marc Zyngier, Kevin Hilman
Cc: Jerome Brunet, Martin Blumenstingl, Guillaume Tucker, kernel-team,
dri-devel, linux-amlogic, linux-arm-kernel, linux-kernel
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>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 12:26 ` Neil Armstrong
0 siblings, 0 replies; 40+ messages in thread
From: Neil Armstrong @ 2020-11-20 12:26 UTC (permalink / raw)
To: Marc Zyngier, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel, Jerome Brunet
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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-20 12:26 ` Neil Armstrong
0 siblings, 0 replies; 40+ messages in thread
From: Neil Armstrong @ 2020-11-20 12:26 UTC (permalink / raw)
To: Marc Zyngier, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel, Jerome Brunet
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>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
2020-11-20 9:42 ` Marc Zyngier
(?)
(?)
@ 2020-11-23 14:03 ` Jerome Brunet
-1 siblings, 0 replies; 40+ messages in thread
From: Jerome Brunet @ 2020-11-23 14:03 UTC (permalink / raw)
To: Marc Zyngier, Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel
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);
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:03 ` Jerome Brunet
0 siblings, 0 replies; 40+ messages in thread
From: Jerome Brunet @ 2020-11-23 14:03 UTC (permalink / raw)
To: Marc Zyngier, Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, kernel-team, dri-devel,
linux-amlogic, linux-arm-kernel, linux-kernel
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);
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:03 ` Jerome Brunet
0 siblings, 0 replies; 40+ messages in thread
From: Jerome Brunet @ 2020-11-23 14:03 UTC (permalink / raw)
To: Marc Zyngier, Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel
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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:03 ` Jerome Brunet
0 siblings, 0 replies; 40+ messages in thread
From: Jerome Brunet @ 2020-11-23 14:03 UTC (permalink / raw)
To: Marc Zyngier, Neil Armstrong, Kevin Hilman
Cc: Martin Blumenstingl, Guillaume Tucker, linux-kernel, dri-devel,
linux-amlogic, kernel-team, linux-arm-kernel
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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
2020-11-23 14:03 ` Jerome Brunet
(?)
(?)
@ 2020-11-23 14:15 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-23 14:15 UTC (permalink / raw)
To: Jerome Brunet
Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-kernel,
dri-devel, Guillaume Tucker, linux-amlogic, kernel-team,
linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:15 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-23 14:15 UTC (permalink / raw)
To: Jerome Brunet
Cc: Neil Armstrong, Kevin Hilman, Martin Blumenstingl,
Guillaume Tucker, kernel-team, dri-devel, linux-amlogic,
linux-arm-kernel, linux-kernel
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:15 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-23 14:15 UTC (permalink / raw)
To: Jerome Brunet
Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-kernel,
dri-devel, Guillaume Tucker, linux-amlogic, kernel-team,
linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
@ 2020-11-23 14:15 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-11-23 14:15 UTC (permalink / raw)
To: Jerome Brunet
Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-kernel,
dri-devel, Guillaume Tucker, linux-amlogic, kernel-team,
linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread