[PATCH] pwm: lpc3200: Don't modify HW state in .remove callback

[PATCH] pwm: lpc3200: Don't modify HW state in .remove callback

[Uwe Kleine-König]

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-lpc32xx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index c42cc314c170..2834a0f001d3 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -136,10 +136,6 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 static int lpc32xx_pwm_remove(struct platform_device *pdev)
 {
 	struct lpc32xx_pwm_chip *lpc32xx = platform_get_drvdata(pdev);
-	unsigned int i;
-
-	for (i = 0; i < lpc32xx->chip.npwm; i++)
-		pwm_disable(&lpc32xx->chip.pwms[i]);
 
 	return pwmchip_remove(&lpc32xx->chip);
 }
-- 
2.30.2

Re: [PATCH] pwm: lpc3200: Don't modify HW state in .remove callback

[Vladimir Zapolskiy]

Hi Uwe,

On 3/29/21 9:41 AM, 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). 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-lpc32xx.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> index c42cc314c170..2834a0f001d3 100644
> --- a/drivers/pwm/pwm-lpc32xx.c
> +++ b/drivers/pwm/pwm-lpc32xx.c
> @@ -136,10 +136,6 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
>   static int lpc32xx_pwm_remove(struct platform_device *pdev)
>   {
>   	struct lpc32xx_pwm_chip *lpc32xx = platform_get_drvdata(pdev);
> -	unsigned int i;
> -
> -	for (i = 0; i < lpc32xx->chip.npwm; i++)
> -		pwm_disable(&lpc32xx->chip.pwms[i]);
>   
>   	return pwmchip_remove(&lpc32xx->chip);
>   }
> 

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

I've briefly checked other PWM drivers, some of them do contain the same
logic, for instance please check pwm-stm32.c, pwm-stm32-lp.c

Many other drivers disable at least the controller clock, I believe there
should be a clear agreed policy, it makes no sense to vary the behaviour
among the drivers, thus likely all the drivers should be checked and fixed
at one series, thanks.

--
Best wishes,
Vladimir

Re: [PATCH] pwm: lpc3200: Don't modify HW state in .remove callback

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Mon, Mar 29, 2021 at 08:41:11AM +0200, 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). 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-lpc32xx.c | 4 ----
>  1 file changed, 4 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]