[1/2] pwm: stm32-lp: Don't modify HW state in .remove callback

A consumer is expected to disable a PWM before calling pwm_put(). And if
they didn't there is hopefully a good reason (or the consumer needs
fixing). Also if disabling an enabled PWM was the right thing to do,
this should better be done in the framework instead of in each low level
driver.

So drop the hardware modification from the .remove() callback.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32-lp.c | 2 --
 1 file changed, 2 deletions(-)


base-commit: a6efb35019d00f483a0e5f188747723371d659fe

On 5/5/21 6:28 PM, Uwe Kleine-König wrote:
> A consumer is expected to disable a PWM before calling pwm_put(). And if
> they didn't there is hopefully a good reason (or the consumer needs
> fixing).

Hi Uwe,

As you pointed out, (ideally) consumers need to disable the PWM before
undind or remove of the driver. Calling pwm_disable in the remove is a
"fail safe".

> Also if disabling an enabled PWM was the right thing to do,
> this should better be done in the framework instead of in each low level
> driver.

Not doing so, in case the driver gets unbind when the PWM is enabled,
then bind again, it leads to unbalanced clock enable count.
There's no change to recover the balance after it.

Also, I'm also wondering if it's a good idea to let a free running PWM
channel? (That's probably a more general discussion).

> 
> So drop the hardware modification from the .remove() callback.

For now, I'd prefer to keep the current implementation, e.g. not to
simply drop this fail safe.

Is there a reason to address only the STM32 LP Timer pwm driver in your
patch ?
(I see there are other drivers around, doing the same. I agree this
could better be addressed by the framework, for all drivers).

Best Regards,
Fabrice

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32-lp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
> index af08f564ef1d..2464f7a24983 100644
> --- a/drivers/pwm/pwm-stm32-lp.c
> +++ b/drivers/pwm/pwm-stm32-lp.c
> @@ -224,8 +224,6 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev)
>  {
>  	struct stm32_pwm_lp *priv = platform_get_drvdata(pdev);
>  
> -	pwm_disable(&priv->chip.pwms[0]);
> -
>  	return pwmchip_remove(&priv->chip);
>  }
>  
> 
> base-commit: a6efb35019d00f483a0e5f188747723371d659fe
>

Hello Fabrice,

On Wed, May 12, 2021 at 09:41:45AM +0200, Fabrice Gasnier wrote:
> On 5/5/21 6:28 PM, Uwe Kleine-König wrote:
> > A consumer is expected to disable a PWM before calling pwm_put(). And if
> > they didn't there is hopefully a good reason (or the consumer needs
> > fixing).
> 
> As you pointed out, (ideally) consumers need to disable the PWM before
> undind or remove of the driver. Calling pwm_disable in the remove is a
> "fail safe".
> 
> > Also if disabling an enabled PWM was the right thing to do,
> > this should better be done in the framework instead of in each low level
> > driver.
> 
> Not doing so, in case the driver gets unbind when the PWM is enabled,
> then bind again, it leads to unbalanced clock enable count.

Ah, the clk must indeed be properly freed, hmm. I will respin the patch.

> There's no change to recover the balance after it.
> 
> Also, I'm also wondering if it's a good idea to let a free running PWM
> channel? (That's probably a more general discussion).

It might make sense, e.g. if you don't want your backlight to go out
for a reboot because the display shows a "I'm rebooting" message.

> > So drop the hardware modification from the .remove() callback.
> 
> For now, I'd prefer to keep the current implementation, e.g. not to
> simply drop this fail safe.
> 
> Is there a reason to address only the STM32 LP Timer pwm driver in your
> patch ?

Whenever I find a driver I fix it. So I already fixed some other drivers
accordingly. See

	git lg --grep="modify HW state in .remove"

Best regards
Uwe

Hello Fabrice,

On Fri, May 14, 2021 at 04:08:43PM +0200, Uwe Kleine-König wrote:
> On Wed, May 12, 2021 at 09:41:45AM +0200, Fabrice Gasnier wrote:
> > On 5/5/21 6:28 PM, Uwe Kleine-König wrote:
> > > A consumer is expected to disable a PWM before calling pwm_put(). And if
> > > they didn't there is hopefully a good reason (or the consumer needs
> > > fixing).
> > 
> > As you pointed out, (ideally) consumers need to disable the PWM before
> > undind or remove of the driver. Calling pwm_disable in the remove is a
> > "fail safe".
> > 
> > > Also if disabling an enabled PWM was the right thing to do,
> > > this should better be done in the framework instead of in each low level
> > > driver.
> > 
> > Not doing so, in case the driver gets unbind when the PWM is enabled,
> > then bind again, it leads to unbalanced clock enable count.
> 
> Ah, the clk must indeed be properly freed, hmm. I will respin the patch.

I took a deeper look now, and I don't agree any more to what I wrote
here. The clock is provided by the parent device, so .probe() doesn't
call clk_get() (or one of its variants). The clock is only enabled when
the .apply() callback is called with .enabled = true. So indeed if the
driver is unloaded while the PWM is still running, the enable count is
non-zero at the end. The unbalanced clock enable count isn't a big
problem (it just never reaches zero any more, which is harmless compared
to becoming negative).

I don't think this should be fixed or handled at the driver level.
Consumers should be fixed to not produce such a leak and if we are sure
that this is really always wrong, a check (and maybe a call to
pwm_disable()) should be added to the PWM core code.

So I'm in favour of applying the patch as is.

Best regards
Uwe

Hi Thierry,

this thread is marked as "Rejected" in patchwork without a word of you.
Did this happen on purpose? Assuming it did: I would welcome a word from
you in such a case. From my POV the patch set is still fine and should
be applied.

Best regards
Uwe