Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag

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

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

On Tue, Apr 13, 2021 at 01:51:15PM +0200, Thierry Reding wrote:
> On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > > Add the flag and corresponding documentation for PWM_USAGE_POWER.
> > 
> > My concern here in the previous round was that PWM_USAGE_POWER isn't a
> > name that intuitively suggests its semantic. Do you disagree?
> 
> I suggested PWM_USAGE_POWER because I think it accurately captures what
> we want here.
> 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > > ---
> > >  Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > >  include/dt-bindings/pwm/pwm.h                 | 1 +
> > >  2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > index 084886bd721e..fe3a28f887c0 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > @@ -46,6 +46,9 @@ period in nanoseconds.
> > >  Optionally, the pwm-specifier can encode a number of flags (defined in
> > >  <dt-bindings/pwm/pwm.h>) in a third cell:
> > >  - PWM_POLARITY_INVERTED: invert the PWM signal polarity
> > > +- PWM_USAGE_POWER: Only care about the power output of the signal. This
> > > +  allows drivers (if supported) to optimize the signals, for example to
> > > +  improve EMI and reduce current spikes.
> > 
> > IMHO there are too many open questions about which freedom this gives to
> > the lowlevel driver. If the consumer requests .duty_cycle = 25ns +
> > .period = 100ns, can the driver provide .duty_cycle = 25s + .period =
> > 100s which nominally has the same power output? Let's not introduce more
> > ambiguity than there already is.
> 
> The freedom given to the driver should be to adjust the signal within
> reasonable bounds. Changing the time unit by a factor of 1000000000 is
> not within reason, and I doubt anyone would interpret it that way, even
> if we didn't document this at all.

Please define a rule that allows to judge if any given implementation is
correct or not. For the record neither "within reasonable bounds" nor "a
factor of 1000000000 is not within reason" is good enough.

This is not only important to be able to review drivers that implement
it, but also for consumers, because they should know what to expect.

> To be frank I think that quest of yours to try and rid the PWM API of
> all ambiguity is futile.

I consider my quest about rounding reasonable. And I think this is
painful because when the PWM framework was introduced it was too much ad
hoc and the APIs were not thought through enough. And because I don't
want to have that repeated, I express my concerns here.

> I've been trying to be lenient because you seem
> motivated, but I think you're taking this too far. There are always
> going to be cases that aren't completely clear-cut and where drivers
> need the flexibility to cheat in order to be useful at all. If we get to
> a point where everything needs to be 100% accurate, the majority of the
> PWM controllers won't be usable at all.
> 
> Don't let perfect be the enemy of good.

I admit here I don't have a constructive idea how to define what is
needed.

For example if we only care about the relative duty cycle, a consumer
requests

	.period = 1045
	.duty_cyle = 680

and the driver can provide multiples of 100 ns for both .period and
.duty_cycle, the candidates that might be sensible to chose from are
(IMHO):

 - exact relative duty:

	.period = 104500
	.duty_cycle = 68000

 - round both values in the same direction, minimizing error

 	.period = 1100
	.duty_cycle = 700

   (requested relative duty = 65.07%, implemented = 63.64%; when
   rounding both down we get 60%)

 - round both values mathematically: 

 	.period = 1000
	.duty_cycle = 700

   (yielding a relative duty of 70% instead of the requested 65.07%)

 - Maybe

 	.period = 1000
	.duty_cycle = 600

   might also be preferable for some consumers?! (60%)

 - Maybe

 	.period = 2000
	.duty_cycle = 1300

   is a good compromise because the relative duty is nearly exactly
   matched and the period is only stretched by a factor < 2.

In my eyes a driver author should be told which of these options should
be picked. Do you consider it obvious which of these options is the
objective best? If so why? Do you agree that we should tell driver
authors how to implement this before we have several drivers that all
implement their own ideas and getting this in a consistent state is
another pain?

(My bet is you are lax and don't consider consistency among drivers soo
important. In this case we don't agree. I think it's important for
consumer driver authors to be able to rely on some expectations
independently which lowlevel driver is in use.)

Best regards
Uwe

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

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