| Message ID | 20190727070916.2960-1-xywang.sjtu@sjtu.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: avoid disabling clk twice | expand |
Hello, On Sat, Jul 27, 2019 at 03:09:16PM +0800, Wang Xiayang wrote: > Similar to commit 63fd4b94b948 ("serial: imx: fix error handling > in console_setup"), as ddata->clk has been explicitly disabled two > lines above, it should avoid being disabled for the second time. > clk_unprepare() suits here better. > > Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> > --- > drivers/pwm/pwm-sifive.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index a7c107f19e66..00f6fab5bd3b 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -312,7 +312,7 @@ static int pwm_sifive_remove(struct platform_device *dev) > if (is_enabled) > clk_disable(ddata->clk); > > - clk_disable_unprepare(ddata->clk); > + clk_unprepare(ddata->clk); > ret = pwmchip_remove(&ddata->chip); > clk_notifier_unregister(ddata->clk, &ddata->notifier); I think this patch is wrong. After a successfull call to .probe the clock is left prepared and enabled. This is undone in the unconditional call to clk_disable_unprepare that you removed. There is however still a problem: If Linux is started with the pwm enabled (such that .get_state returns state->enabled = true) then disabling the pwm has one clk_disable too much. Best regards Uwe
----- On Jul 29, 2019, at 2:25 PM, Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote: > Hello, > > On Sat, Jul 27, 2019 at 03:09:16PM +0800, Wang Xiayang wrote: >> Similar to commit 63fd4b94b948 ("serial: imx: fix error handling >> in console_setup"), as ddata->clk has been explicitly disabled two >> lines above, it should avoid being disabled for the second time. >> clk_unprepare() suits here better. >> >> Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> >> --- >> drivers/pwm/pwm-sifive.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c >> index a7c107f19e66..00f6fab5bd3b 100644 >> --- a/drivers/pwm/pwm-sifive.c >> +++ b/drivers/pwm/pwm-sifive.c >> @@ -312,7 +312,7 @@ static int pwm_sifive_remove(struct platform_device *dev) >> if (is_enabled) >> clk_disable(ddata->clk); >> >> - clk_disable_unprepare(ddata->clk); >> + clk_unprepare(ddata->clk); >> ret = pwmchip_remove(&ddata->chip); >> clk_notifier_unregister(ddata->clk, &ddata->notifier); > > I think this patch is wrong. > > After a successfull call to .probe the clock is left prepared and > enabled. This is undone in the unconditional call to > clk_disable_unprepare that you removed. Thank you very much for pointing out the issue. I did miss the normal path:( > There is however still a problem: If Linux is started with the pwm > enabled (such that .get_state returns state->enabled = true) then > disabling the pwm has one clk_disable too much. > There is another path of double-disabling: pwm_sifive_enable() may disable ddata->clk as required by the 'enable' flag. pwm_sifive_apply() calls it and passes the flag. However, there is a clk_disable(ddata->clk) just below invoking pwm_sifive_enable() in pwm_sifive_apply().
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c index a7c107f19e66..00f6fab5bd3b 100644 --- a/drivers/pwm/pwm-sifive.c +++ b/drivers/pwm/pwm-sifive.c @@ -312,7 +312,7 @@ static int pwm_sifive_remove(struct platform_device *dev) if (is_enabled) clk_disable(ddata->clk); - clk_disable_unprepare(ddata->clk); + clk_unprepare(ddata->clk); ret = pwmchip_remove(&ddata->chip); clk_notifier_unregister(ddata->clk, &ddata->notifier);
Similar to commit 63fd4b94b948 ("serial: imx: fix error handling in console_setup"), as ddata->clk has been explicitly disabled two lines above, it should avoid being disabled for the second time. clk_unprepare() suits here better. Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> --- drivers/pwm/pwm-sifive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)