Re: [RFC PATCH 2/7] dt-bindings: pwm: document the PWM polarity flag

[Thierry Reding]

On Tue, Mar 17, 2020 at 10:30:56PM +0100, Uwe Kleine-KÃnig wrote:
> Hello Thierry,
> 
> On Tue, Mar 17, 2020 at 06:43:44PM +0100, Thierry Reding wrote:
> > On Tue, Mar 17, 2020 at 02:32:26PM +0200, Oleksandr Suvorov wrote:
> > > Add the description of PWM_POLARITY_NORMAL flag.
> > > 
> > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@xxxxxxxxxxx>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/pwm/pwm.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > index 084886bd721e..440c6b9a6a4e 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > @@ -46,6 +46,7 @@ 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_POLARITY_NORMAL: don't invert the PWM signal polarity
> > 
> > This doesn't make sense. PWM_POLARITY_NORMAL is not part of the DT ABI.
> 
> "is not part of the DT ABI" is hardly a good reason. If it's sensible to
> be used, it is sensible to define it.

That's exactly it. It's not sensible at all to use it. If you define it
here it means people are allowed to do stuff like this:

	pwms = <&pwm 1234 PWM_POLARITY_INVERTED | PWM_POLARITY_NORMAL>;

which doesn't make sense. What's more, it impossible for the code to
even notice that you're being silly because | PWM_POLARITY_NORMAL is
just | 0 and that's just a nop.

Thierry

> > The third cell of the specifier is a bitmask of flags.
> > 
> > PWM_POLARITY_NORMAL is an enumeration value that evaluates to 0, so it
> > makes absolutely no sense as a flag.
> 
> Using 0 or PWM_POLARITY_NORMAL doesn't have an effect on the compiled
> device tree, that's true. But having the term PWM_POLARITY_NORMAL (in
> contrast to a plain 0) in a dts file is useful in my eyes for human
> readers.

Yes, I suppose that's true.

> > PWM signals are considered to be "normal" by default, so no flag is
> > necessary to specify that.
> 
> GPIOs are considered to be active high by default, still there is
> GPIO_ACTIVE_HIGH (which also evaluates to 0). Also there is
> IRQ_TYPE_NONE.

I'm aware of these. They carry the same risks as I mentioned above,
though. You can easily make mistakes that no software will be able to
detect. If you make a GPIO GPIO_ACTIVE_HIGH | GPIO_ACTIVE_LOW, it
becomes really confusing as to what that means. It really means that the
GPIO will be active-low because GPIO_ACTIVE_HIGH doesn't do anything.
But just reading that may make you think that perhaps HIGH is better
(because, well, it's high, or perhaps because it is listed first).
Having both may also be interpreted as "don't care".

In my opinion things become much easier when you don't have any of the
alternatives. If you don't specify PWM_POLARITY_INVERTED, well, it's
just not going to be inverted, it's going to be normal. No need to be
extra verbose about that.

Thierry

Attachment: signature.asc
Description: PGP signature