Add the description of PWM_POLARITY_NORMAL flag.
Signed-off-by: Oleksandr Suvorov <[email protected]>
---
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
Example with optional PWM specifier for inverse polarity
--
2.24.1
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 <[email protected]>
> ---
>
> 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.
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. PWM signals are considered to be
"normal" by default, so no flag is necessary to specify that.
Thierry
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 <[email protected]>
> > ---
> >
> > 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.
(And as far as I understand it, the term PWM_POLARITY_INVERTED isn't
part of the DT ABI either. Only the value 1 has a meaning (for some PWM
controlers).)
> 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.
> 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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Oleksandr,
Thank you for the patch.
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 <[email protected]>
> ---
>
> 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
With the previous line mentioning that the pwm-specifier can encode a
number of *flags*, this becomes confusing: reading the documentation
only and not pwm.h, one could wonder what happens if none or both of
PWM_POLARITY_INVERTED and PWM_POLARITY_NORMAL are specified :-(
>
> Example with optional PWM specifier for inverse polarity
>
--
Regards,
Laurent Pinchart
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 <[email protected]>
> > > ---
> > >
> > > 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.
> (And as far as I understand it, the term PWM_POLARITY_INVERTED isn't
> part of the DT ABI either. Only the value 1 has a meaning (for some PWM
> controlers).)
>
> > 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.
>
> > 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 those. That doesn't mean that everything needs to have
symbolic names.
Thierry
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 <[email protected]>
> > > ---
> > >
> > > 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
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 <[email protected]>
> > > > ---
> > > >
> > > > 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/ |
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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