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

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(-)

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.

From an API point of view I think it makes more sense to define the
state after disable to be inactive, that is high for normal PWM and low
for inversed PWM. The reason for this is that the disabled state is
effectively what the reset state of the PWM would be. If the PWM is
inverted, typically the board design has a pull-up somewhere to avoid a
backlight or LED from getting enabled by default. In all other cases a
PWM pin is likely just going to default to low when disabled.

This also matches what all other drivers, and users, assume.

Thierry

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.)

> From an API point of view I think it makes more sense to define the
> state after disable to be inactive, that is high for normal PWM and low
> for inversed PWM. The reason for this is that the disabled state is
> effectively what the reset state of the PWM would be.

The problem is this is where theory and reality differ. On i.MX if the
pwm clock is disabled the pin goes to zero independent of being inverted
or not. And before pinmuxing is done the reset state of the PWMs I know
and work with is an input.

So the only way the PWM API could be implemented fulfilling your
conditions on i.MX is that .disable() is a noop. And the pwm clock must
not be disabled until pwm_put() is called.

So the semantic of .disable() currently is "disable the pwm clock if this
keeps the pin at the expected level". For some---maybe even
most---hardware implementations this if condition is a no-brainer, but
for others it is relevant.

I want to simplify the semantic of .disable to "disable the pwm clock".
This downside is that this exposes hardware details and we need to teach
the pwm API users that this might have a side effect that isn't expected
today. But the expressiveness of the API isn't restricted because
with calling only pwm_config(pwm, 0, 100) you can still get the intended
result.

> If the PWM is inverted, typically the board design has a pull-up
> somewhere to avoid a backlight or LED from getting enabled by default.

Experience shows that it's a bad idea to rely on the hardware guys to do
everything we software guys consider sane. If the API is only suitable
for typical scenarios that's a bad report. My suggestion doesn't solve
this completely, but it makes the behaviour in the "non-typical"
situations better without complicating stuff in the good cases. A
complete win.

> In all other cases a PWM pin is likely just going to default to low
> when disabled.

So you suggest to specify "calling pwm_disable() is likely to drive the
pin to zero"? For sure you don't as this is completely useless.

> This also matches what all other drivers, and users, assume.

I don't know the hardware details for all PWMs that have a Linux driver
but I'd be surprised if the PWMs in i.MX SoCs were the only hardware
implementations that don't comply with your idea of a PWM.

And users are fixable.

Best regards
Uwe

Hi Thierry,

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. And if you
look at existing drivers, it shouldn't be that hard to patch those that
support this optimization since they most of the time have a separate
disable function which could be called from the their config function
if needed.
On the other hand, if we do it the other way around, and expect
pwm_disable() to keep the pin in the state it was after setting a duty
of 0, it becomes more complicated (impossible?) for PWM blocks that do
not support specifying a default pin state when the PWM is disabled.

Regards,

Boris

On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> 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. And if you
> look at existing drivers, it shouldn't be that hard to patch those that
> support this optimization since they most of the time have a separate
> disable function which could be called from the their config function
> if needed.
> On the other hand, if we do it the other way around, and expect
> pwm_disable() to keep the pin in the state it was after setting a duty
> of 0, it becomes more complicated (impossible?) for PWM blocks that do
> not support specifying a default pin state when the PWM is disabled.

Of course you could argue (as Thierry does) that .disable should be a
noop then. Thinking that further .disable could be a noop for every hw
driver then and we could remove pwm_disable() completely (together with
.enabled in pwm_state).

Best regards
Uwe

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.)
 
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.)

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".

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.

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.

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);
 - abstract inversed pwms in the framework instead of each hw driver;
 - 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)

Best regards
Uwe

[1] the only example I know is the pwm-backlight driver that could
    disable the pwm without the inactive level guarantee when the
    enable-gpio is inactive.

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

On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> 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. And if you
> look at existing drivers, it shouldn't be that hard to patch those that
> support this optimization since they most of the time have a separate
> disable function which could be called from the their config function
> if needed.

There's nothing in the API that prevents you from doing so and I never
said that this couldn't be done in pwm_config(). If you know it's safe
to disable the PWM if the duty cycle goes to zero, then by all means
do it.

That doesn't mean we necessarily have to go and change the API. There
may be drivers that prefer to do it in pwm_disable() and I don't see a
reason why we shouldn't be able to have both.

Quite frankly I think this whole discussion is moot. We already have a
different API (atomic) that was created specifically to solve these
kinds of issues. So if there's any driver issues with the legacy API,
then those drivers should be moved to the new API. All the public-facing
APIs already use those if available anyway. I don't see the point in
rev'ing an API that's deprecated anyway.

Also, the whole discussion here started because Uwe reported that the
i.MX driver wasn't expecting according to the assumptions from PWM
consumers (i.e. the PWM pin goes low for inverted PWM). So really no
amount of API changes is going to magically fix that problem.

> On the other hand, if we do it the other way around, and expect
> pwm_disable() to keep the pin in the state it was after setting a duty
> of 0, it becomes more complicated (impossible?) for PWM blocks that do
> not support specifying a default pin state when the PWM is disabled.

I think that'd be an unrealistic expectation. If you look at your
typical hardware design, engineers will have put it together in a way
that with the (hardware) defaults, your device will behave normally.
Those hardware defaults, or at least something as close to it as
possible, are what you should be programming your hardware blocks with
before you relinquish control in the driver. For pretty much all the
hardware devices that I'm familiar with, the "off" or "no power" state
is that hardware default.

In other words, when you boot a device the fans, backlights and LEDs
will be off by default. Therefore I think pwm_disable() should put the
PWM back into that state. That's evidently not what is happening for
i.MX currently, otherwise Uwe wouldn't be seeing that problem.

Furthermore, there is another patch currently under review that does
attempt to fix this in a proper way. So I just don't understand why we
keep going around in circles here.

Thierry

Hello Thierry,

On Thu, Oct 18, 2018 at 04:44:14PM +0200, Thierry Reding wrote:
> On Thu, Oct 18, 2018 at 10:44:05AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> > > 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.

Ah, you're talking about drivers making use of the PWM API, I thought
we're talking about the people working with PWMs.

The pwm-backlight driver only calls pwm_disable() after
pwm_config(pb->pwm, 0, pb->period). Same for leds-pwm. So I'd claim
their expectation is that pwm_disable() freezes the pwm, not that it
stops at idle. pwm-fan is strange, mixing the old API with
pwm_apply_state, assuming the maximal allowed duty cycle is period - 1,
so I'm not taking that one into account to argue.

> > 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...

You wrote above that pwm_disable is equivalent to duty-cycle = 0. So we
are at an agreement finally it seems.

And yes, I asked you again for an example because "there is a difference
and some might care and use that difference" isn't an argument that is
getting us further. If the statement is true it would be more
cooperative to explain the difference and why it matters. As is it only
thwarts my efforts without a possibility to effectively accept or refute
it.

> > 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.

No driver *wants* the pin go to an undefined state. But it might
indicate that doing so is acceptable without interfering with its
intentions. The example I had was a display that has both an enable GPIO
and a PWM for backlight. When the driver disables the display using the
GPIO the state of the PWM doesn't matter and it could call pwm_disable()
with the semantic I want pwm_disable to have.

> > 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?

see above.

Do you think it's sensible to keep pwm_disable() with the current semantic
(i.e. same result as pwm_config(pwm, 0, period))? If not, what is in
your eyes the best that should be done with the findings of this thread?

> > 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.

It's better to have code that handles identical situations for several
drivers in a single place. Even if the code in question is simple.
Having only the stuff in a hardware driver that cannot be shared
simplifies understanding what it does, increases code coverage,
simplifies identifying further stuff that can be factored into the core.
And if you later notice that the simple stuff needs an adaption because
it isn't that simple after all or should cover another use case, you
only touch a single place. Then testing this change on a single
implementation gives more confidence that the change is also right for
the other affected drivers than when you fix each individual instance
individually.

> 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.

I agree, if the hardware has a means to detect when the new period
started, that should be used instead of the generic sleep. That's why
the sleep in the core should only be used by the others.

> So why would you want to add code to the core that's highly device
> dependent?

I want to add code to the core that is generic enough to be used by
several drivers. For that to be useful it's not necessary that all
drivers benefit from that.

> 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?

To simplify the hardware drivers and not solve the same problem (even if
it's easy) in a single place. And note, I want pwm_config to return only
after the settings are active, I didn't talk about the callback that hw
drivers have to implement. That's because ideally the PWM framework
isn't only a shim layer between the hardware and PWM users but it
implements things that are useful for both sides. So the things that a
caller of pwm_config should be able to rely on doesn't necessarily need
to be inherited to the implementers of the .config callback.

> >  - 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.

Right, you claimed there is a difference. I argued that nobody can make
use of that difference because it's only a shift and as the period
borders are not communicated back to the PWM user there is nothing that
can be synced to it. Can you contradict that?

Also, when changing the polarity glitches can happen, even more so when
the PWM is disabled. Take for example an uninverted PWM running at 50%
that is then inverted and disabled at A and then enabled again at B:
     ____      ____            ____
    /    \____/    \____/\____/
    ^         ^         ^^
                      A  B

It might result in a peek which depending on the use case might be bad.

> >  - 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.

I don't care much how this is done. The clk API explicitly states that
clk_set_rate might not exactly set the desired rate and provides a
function (clk_round_rate) to prophesy the result. That's what I had in
mind and what might make sense for PWM, too. After all most drivers have
to implement rounding already now if the granularity of period and duty
isn't a divisor of 1 ns.

Best regards
Uwe