pwm: avoid disabling clk twice
diff mbox series

Message ID 20190727070916.2960-1-xywang.sjtu@sjtu.edu.cn
State New
Headers show
Series
  • pwm: avoid disabling clk twice
Related show

Commit Message

Wang Xiayang July 27, 2019, 7:09 a.m. UTC
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(-)

Comments

Uwe Kleine-König July 29, 2019, 6:25 a.m. UTC | #1
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
Wang Xiayang July 29, 2019, 7:35 a.m. UTC | #2
----- 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().

Patch
diff mbox series

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);