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

From: Thierry Reding <thierry.reding@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>,
	devicetree@vger.kernel.org, linux-pwm@vger.kernel.org,
	Paul Barker <pbarker@konsulko.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Marcel Ziswiler <marcel.ziswiler@toradex.com>,
	Igor Opaniuk <igor.opaniuk@toradex.com>,
	Philippe Schenker <philippe.schenker@toradex.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/7] dt-bindings: pwm: document the PWM polarity flag
Date: Thu, 19 Mar 2020 18:04:55 +0100	[thread overview]
Message-ID: <20200319170455.GC3354541@ulmo> (raw)
In-Reply-To: <20200319070510.gc6hr53gn7n2osvb@pengutronix.de>

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

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@toradex.com>
> > > > > ---
> > > > > 
> > > > >  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.

Thierry

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