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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

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