On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> while digging around in the pwm drivers I found
> drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls
> pwm_disable() and pwm_set_duty_cycle(pwm, 0).
> 
> I think calling pwm_disable is wrong for two reasons:
> 
>  a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should
do so very carefully and not behind consumers' backs.

>  b) .free isn't supposed to stop the pwm.

That's a bit of a gray area. I still think we need a way of stopping the
PWM at some point. Some drivers do this on ->remove() and it was in fact
discussed at one point that this should be moved to the core, so that
all channels would be automatically disabled when the PWM chip is
removed.

Disabling in ->free() should not have any effect, because users should
already have disabled it before they even call pwm_put() on it.

So perhaps a good compromise would be to document that users are
supposed to disable the PWM before they release it and then drivers (or
the core) could go and check that they're really disabled when the chip
is being removed and WARN() if not. That way we can flag all consumers
that don't behave and fix them.

> Also I'm surprised that .request calls pwm_get_duty_cycle() and writes
> the result into hardware. Still more as I think pwm_get_duty_cycle()
> always returns 0 in .request.
> 
> BTW, this is the only caller of pwm_set_duty_cycle(), so if the driver
> is simplified/fixed to not care about the duty cycle in .free and
> .request, this function can go away.
> 
> What do you think?

Yeah, it looks as if pwm_get_duty_cycle() would always return 0 because
the driver doesn't implement hardware readout. pwm_set_duty_cycle() was
introduced back at the time when sysfs support was added, but it seems
to have otherwise been unused ever since. I think it no longer makes
sense to keep it around once all consumers have adopted the atomic API.

Thierry