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

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