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

[Rob Herring]

On Thu, Mar 19, 2020 at 06:04:55PM +0100, Thierry Reding wrote:
> On Thu, Mar 19, 2020 at 08:05:10AM +0100, Uwe Kleine-König wrote:
> > On Thu, Mar 19, 2020 at 12:05:39AM +0100, Thierry Reding wrote:
> > > 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 think the argument is "It is not sensible to be used." then please
> > say so and don't add "PWM_POLARITY_NORMAL is not part of the DT ABI."
> > which seems to be irrelevant now.
> 
> I did say that, didn't I? I said that it doesn't make sense because it
> isn't part of the ABI. And since this is the document that defines the
> DT ABI, why should we add something that isn't part of the ABI?
> 
> Now, if you want to make this part of the ABI, then that should be done
> as part of this patch so that the patch is actually consistent.
> 
> > > 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.
> > 
> > I would hope that a human reader would catch this.
> 
> Maybe. At the same time we're now moving towards automatic checking of
> device trees against a binding. These tools will only ever see the
> binary representation, so won't be able to spot this nonsense. The whole
> purpose of having these tools is so that we don't have to do the tedious
> work of manually inspecting all device tree files. It's also not
> guaranteed that we'll even be seeing all of the device tree files ever
> written against these bindings.
> 
> > 
> > > 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.
> > 
> > I think this argument is a bad one. Whenever you introduce a
> > function or symbol you can use it in a wrong way. With this argument you
> > can also say GPIO_ACTIVE_LOW doesn't make sense because
> > 
> > 	pwms = <&pwm 1234 GPIO_ACTIVE_LOW>;
> > 
> > is silly.
> 
> Yes, it's also obviously silly to try and eat a hammer. I was assuming
> people have at least /some/ sense and try not to use GPIO related flags
> with PWM specifiers. And because I think that people aren't totally
> stupid, I think they'll be capable of understanding that by default a
> PWM will be "normal" and only if they want to deviate from "normal" do
> they need to do something special, like specify PWM_POLARITY_INVERTED.
> 
> I'm growing tired of this discussion, to be honest. So if you really
> absolutely must have PWM_POLARITY_NORMAL so that you can read DT files
> without having to think, then fine, I'll take a patch that adds that.
> But please let's not confuse the definitions for DT with the polarity
> enum in the API.

I agree with Thierry here. I'd give my reasons, but I've got 200 other 
patches to review.

Rob