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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |