| Message ID | 20200317123231.2843297-3-oleksandr.suvorov@toradex.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
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. 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 <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. (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
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 <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 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 >
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 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 <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 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 <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. > 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
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
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@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. I agree with Thierry here. I'd give my reasons, but I've got 200 other patches to review. Rob
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
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(+)