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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200319070510.gc6hr53gn7n2osvb@pengutronix.de>
Date:   Thu, 19 Mar 2020 08:05:10 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Oleksandr Suvorov <oleksandr.suvorov@...adex.com>,
        devicetree@...r.kernel.org, linux-pwm@...r.kernel.org,
        Paul Barker <pbarker@...sulko.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        Igor Opaniuk <igor.opaniuk@...adex.com>,
        Philippe Schenker <philippe.schenker@...adex.com>,
        Rob Herring <robh+dt@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/7] dt-bindings: pwm: document the PWM polarity flag

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@...adex.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.

> 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.

> 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.

Keep well and fit,
Uwe

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