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

[Fabrice Gasnier]

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 at 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
>