Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Oct 18, 2018 at 10:44:05AM +0200, Uwe Kleine-König wrote:
> Hello Boris,
> 
> On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> > On Tue, 9 Oct 2018 11:04:01 +0200
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > 
> > > Hello Thierry,
> > > 
> > > thanks for taking time to reply in this thread.
> > > 
> > > On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > > > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > > > Contrarily to the common assumption that pwm_disable() stops the
> > > > > output at the state where it just happens to be, this isn't what the
> > > > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > > > 0 level instead.
> > > > > 
> > > > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > > > The backend driver is free to implement potential power savings
> > > > > already when the duty cycle changes to one of these two cases so this
> > > > > doesn't come at an increased runtime power cost (once the respective
> > > > > drivers are fixed).
> > > > > 
> > > > > In return this allows to adhere to the API more easily for drivers that
> > > > > currently cannot know if it's ok to disable the hardware on
> > > > > pwm_disable() because the caller might or might not rely on the pin
> > > > > stopping at a certain level.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  Documentation/pwm.txt | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > > > 
> > > > I don't think this is correct. An API whose result is an undefined state
> > > > is useless in my opinion. So either we properly define what the state
> > > > should be after a disable operation, or we might as well just remove the
> > > > disable operation altogether.  
> > > 
> > > Explicitly not defining some details makes it easier for hardware
> > > drivers to comply with the API. Sure it would be great if all hardware
> > > would allow to implement a "stronger" API but this isn't how reality looks
> > > like.
> > > 
> > > You say it's bad that .disable() should imply less than it did up to
> > > now. I say .disable() should only imply that the PWM stops toggling, so
> > > .disable has a single purpose that each hardware design should be able
> > > to implement.
> > > And this weaker requirement/result is still good enough to implement all
> > > use cases. (You had doubts here in the past, but without an example. I
> > > cannot imagine there is one.) In my eyes this makes the API better not
> > > worse.
> > > 
> > > What we have now in the API is redundancy, which IMHO is worse. If I
> > > want the pwm pin to stay low I can say:
> > > 
> > > 	pwm_config(pwm, 0, 100);
> > > 
> > > or I can say:
> > > 
> > > 	pwm_config(pwm, 0, 100);
> > > 	pwm_disable(pwm);
> > > 
> > > The expected result is the same. Now you could argue that not disabling
> > > the pwm is a bug because it doesn't save some energy. But really this is
> > > a weakness of the API. There are two ways to express "Set the pin to
> > > constant 0" and if there is an opportunity to save some energy on a
> > > certain hardware implementation, just calling pwm_config() with duty=0
> > > should be enough to benefit from it. This makes the API easier to use
> > > because there is a single command to get to the state the pwm user wants
> > > the pwm to be in. (This is two advantages: A single way to reach the
> > > desired state and only one function to call.)
> > 
> > I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> > duty=100%" logic should, IMHO, be a HW-specific optimization.
> 
> Note that according to Thierry the behaviour of
> 
> 	pwm_config(pwm, 100, 100);
> 	pwm_disable(pwm);
> 
> (and similarily of
> 
> 	pwm_apply_state({.period = 100, .duty_cycle = 100, .enabled = false});
> 
> ) should (assuming an uninverted pwm) result in the output stopping at 0
> (i.e. it's inactive level which is the same as after pwm_config(pwm, 0, 100)).
> 
> So there isn't anything special about "disable PWM on duty=0% or
> duty=100%". "disable PWM" in Thierry's eyes is always "stop at inactive
> level" independant of the previously configured duty cycle.
> 
> You talking about the specific cases 0% and 100% makes me believe that
> you are an example (BTW, I'm another) that makes Thierry's claim
> 
> 	The current assumption by all users is more that [after
> 	pwm_disable()] the pin would stop at the no power state, which
> 	would be 0 for normal PWM and 1 for inverted.
> 
> wrong. (See mail with Message-ID: 20181012095349.GA9162@ulmo in this
> thread.)

Let's be specific here: the vast majority of PWM consumers currently
are pwm-backlight, leds-pwm and pwm-fan. All of them call pwm_disable()
when the devices are turned off (backlight and LED turn off, fan stops)
which is equivalent to duty-cycle = 0.

> To get forward here: Thierry, can you please confirm that there is no
> semantical difference between "pwm_disable()" and "pwm_config(pwm, 0,
> period)". (Or alternatively come up with a use case where a difference
> really matters. I'm sure this is impossible though.)

And here I was hoping that we were making progress on this. Now you're
taking us back to square one...

> Then I can start simplifying API users and adapt the pwm
> hardware drivers. When this is done there will be no user left of
> pwm_disable and struct pwm_state::enabled because nobody needs it with
> the current semantic. After that we can either rip the concept of
> pwm_disable or shift it's semantic to my initial suggestion of "no
> guarantees about the output level" and use that in the few drivers that
> really don't care about the output level[1] and so give the hardware the
> possibility to disable the clock even if that results in something
> different than "inactive".

I have repeatedly told you that not making any guarantees is completely
useless. I can't think of a reason why any PWM consumer would ever want
to have the pin go into an undefined state.

> As a first step I'd suggest to explicitly state the expectations on
> hardware driver implementations and pwm-API users to get all affected
> parties in line. These are in my eyes:
> 
>  - configuring the duty cycle should (if possible) not result in
>    glitches, so the currently running period should be completed and
>    after that a new period with the new configuration should start.
>    (Is this reasonable to expect this from all implementations? If not
>    we should maybe introduce a way to let the users know?)
> 
>  - The function to configure the duty cycle should only return when the
>    period with the new configuration actually started.

Yes, those two are reasonable expectations in my opinion.

> If the concept of pwm_disable() stays, we should document the expected
> behaviour of the pin. Something like:
> 
>  - After disabling the pwm you must not make any assumptions about the
>    pin level. It might be low, or inactive, or even high-z. So don't
>    disable the pwm if you for example care about a display backlight to
>    stay off.

Like I said multiple times, that's completely useless. Can you please
provide an example where a consumer would possible want that behavior?

> If desired we might even keep the behaviour for the sysfs implementation
> that disable there for historic reasons keeps the semantic of
> duty-cycle=0.
> 
> Further things that could be done (eventually after discussing they are
> really sensible) are:
> 
>  - for pwms that can be programmed now for the next period implement the
>    necessary sleep in the PWM-Framework (mxs, atmel, sun4i at least);

I fail to see the point in this. It's incredibly trivial to do this in
the drivers. And a sleep is perhaps the simplest way to implement this
and could technically be made generic and handled in the core, but the
hardware could just as well have a mechanism to signal the beginning of
the next period in some bit in a register. So why would you want to add
code to the core that's highly device dependent? What do you gain from
it? If we already have the expectations documented that ->config() must
only return after the settings have been applied, then why move some of
the responsibility back into the core?

>  - abstract inversed pwms in the framework instead of each hw driver;

What exactly are you propsing here? We've had this discussion just a
couple of days ago and I already mentioned why the duty cycle inversion
is not the same thing as the signal inversion.

>  - let hw drivers correct *state on apply
>    (if the user requests period=1ms and the driver can only provide
>    multiples of 0.6667 ms because the input clock is fixed at 1500 Hz)

That's not a very good idea. If the user requests a configuration that
can't be met we should return an error, not silently apply something
that we think is good enough and hope that the user noticed that and
tries a different configuration instead.

Thierry

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