| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200120115240.GA206171@ulmo>
Date: Mon, 20 Jan 2020 12:52:40 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Jitao Shi <jitao.shi@...iatek.com>
Cc: Uwe Kleine-Koenig <u.kleine-koenig@...gutronix.de>,
Matthias Brugger <matthias.bgg@...il.com>,
linux-pwm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, CK Hu <ck.hu@...iatek.com>,
linux-mediatek@...ts.infradead.org, sj.huang@...iatek.com
Subject: Re: [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when
disable pwm
On Tue, Dec 17, 2019 at 12:02:36PM +0800, Jitao Shi wrote:
> The clocks of pwm are still in prepare state when disable pwm.
>
> Because the clocks is prepared in mtk_disp_pwm_probe() and unprepared
> in mtk_disp_pwm_remove().
>
> clk_prepare and clk_unprepare aren't called by mtk_disp_pwm_enable()
> and mtk_disp_pwm_disable().
>
> Modified:
> So clk_enable() is instread with clk_prepare_enable().
> clk_disable() is instread with clk_disable_unprepare().
>
> And remove the clk_prepare() from mtk_disp_pwm_probe(),
> remove the clk_unprepare from mtk_disp_pwm_remove().
This commit message is basically a description of what the patch does,
which is pretty useless because I can look at the patch to see what it
does.
Use the commit message to describe why you want to make this change and
what the benefits are of doing what you're doing. Describe why and how
the patch improves the driver compared to before.
With regards to clk_prepare()/clk_enable() vs. clk_prepare_enable(),
there are valid reasons for splitting the call up like this driver did.
clk_prepare() for example may sleep, so you can't call it from interrupt
context. clk_enable() on the other hand does not sleep, so it can be
called from interrupt context. So there might be legitimate reasons for
this split in the driver.
When you say that clocks are still in prepared state when you disable
the PWM this probably means that clk_disable() doesn't do anything on
your platform and clk_unprepare() is when the clock is actually
disabled. That's perfectly valid. It should also be safe to do this now,
since a while ago the PWM API as a whole was made "sleepable", so it
should be safe to call clk_prepare_enable() and clk_disable_unprepare()
from any callbacks because users of the PWM API already need to assume
that the API can sleep.
So, I don't object to the patch, I just wanted to make sure that you've
thought about the consequences and to describe in the commit message
what you're actually trying to achieve and why it's better.
In particular it'd be interesting to know what the effects are that you
are noticing if the clock isn't off when the PWM is disabled and how you
found out. Those are the kinds of details that make the commit message
really useful because they help readers of the git log figure out what
the problems where that you encountered and why you fixed them the way
you did.
Thierry
> Signed-off-by: Jitao Shi <jitao.shi@...iatek.com>
> ---
> drivers/pwm/pwm-mtk-disp.c | 43 +++++++++++---------------------------
> 1 file changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 83b8be0209b7..c7b14acc9316 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -98,13 +98,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> high_width = div64_u64(rate * duty_ns, div);
> value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
>
> - err = clk_enable(mdp->clk_main);
> + err = clk_prepare_enable(mdp->clk_main);
> if (err < 0)
> return err;
>
> - err = clk_enable(mdp->clk_mm);
> + err = clk_prepare_enable(mdp->clk_mm);
> if (err < 0) {
> - clk_disable(mdp->clk_main);
> + clk_disable_unprepare(mdp->clk_main);
> return err;
> }
>
> @@ -124,8 +124,8 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> 0x0);
> }
>
> - clk_disable(mdp->clk_mm);
> - clk_disable(mdp->clk_main);
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
>
> return 0;
> }
> @@ -135,13 +135,13 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> int err;
>
> - err = clk_enable(mdp->clk_main);
> + err = clk_prepare_enable(mdp->clk_main);
> if (err < 0)
> return err;
>
> - err = clk_enable(mdp->clk_mm);
> + err = clk_prepare_enable(mdp->clk_mm);
> if (err < 0) {
> - clk_disable(mdp->clk_main);
> + clk_disable_unprepare(mdp->clk_main);
> return err;
> }
>
> @@ -158,8 +158,8 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
> 0x0);
>
> - clk_disable(mdp->clk_mm);
> - clk_disable(mdp->clk_main);
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
> }
>
> static const struct pwm_ops mtk_disp_pwm_ops = {
> @@ -194,14 +194,6 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
> if (IS_ERR(mdp->clk_mm))
> return PTR_ERR(mdp->clk_mm);
>
> - ret = clk_prepare(mdp->clk_main);
> - if (ret < 0)
> - return ret;
> -
> - ret = clk_prepare(mdp->clk_mm);
> - if (ret < 0)
> - goto disable_clk_main;
> -
> mdp->chip.dev = &pdev->dev;
> mdp->chip.ops = &mtk_disp_pwm_ops;
> mdp->chip.base = -1;
> @@ -210,7 +202,7 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
> ret = pwmchip_add(&mdp->chip);
> if (ret < 0) {
> dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> - goto disable_clk_mm;
> + return ret;
> }
>
> platform_set_drvdata(pdev, mdp);
> @@ -229,24 +221,13 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
> }
>
> return 0;
> -
> -disable_clk_mm:
> - clk_unprepare(mdp->clk_mm);
> -disable_clk_main:
> - clk_unprepare(mdp->clk_main);
> - return ret;
> }
>
> static int mtk_disp_pwm_remove(struct platform_device *pdev)
> {
> struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> - int ret;
> -
> - ret = pwmchip_remove(&mdp->chip);
> - clk_unprepare(mdp->clk_mm);
> - clk_unprepare(mdp->clk_main);
>
> - return ret;
> + return pwmchip_remove(&mdp->chip);
> }
>
> static const struct mtk_pwm_data mt2701_pwm_data = {
> --
> 2.21.0
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists