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
