LinuxLists.cc - [RFC PATCH 1/7] pwm: rename the PWM_POLARITY

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Tue, Mar 17, 2020 at 6:27 PM <[email protected]> wrote:
>
>
>
> On 17.03.2020 14:32, Oleksandr Suvorov wrote:
> > @@ -187,7 +187,7 @@ static ssize_t polarity_store(struct device *child,
> > if (sysfs_streq(buf, "normal"))
> > polarity = PWM_POLARITY_NORMAL;
> > else if (sysfs_streq(buf, "inversed"))
>
> You may also consider this string ^

Thanks for the feedback, Claudiu.

I thought about it and decided not to change the ABI, as this change
can impact lots of user-land applications.
As a minimum, I can push this change as a separate patch to be able to
revert the change of ABI only.
What do you think?

> > - polarity = PWM_POLARITY_INVERSED;
> > + polarity = PWM_POLARITY_INVERTED;

--
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

2020-03-17 17:41:50

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> Rename it to PWM_POLARITY_INVERTED.

It isn't misspelled. "inversed" is a synonym for "inverted". Both
spellings are correct.

And as you noted in the cover letter, there's a conflict between the
macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
in the wrong order you'll get a compile error.

The enum was named this way on purpose to make it separate from the
definition for the DT bindings. Note that DT bindings are an ABI and can
never change, whereas the enum pwm_polarity is part of a Linux internal
API and doesn't have the same restrictions as an ABI.

As far as I'm concerned this is completely unnecessary churn that's
potentially going to come back and bite us, so I see no reason to accept
this.

Thierry

Attachments:

(No filename) (898.00 B)
signature.asc (849.00 B)
Download all attachments

2020-03-17 21:03:38

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

Hello,

On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > Rename it to PWM_POLARITY_INVERTED.
>
> It isn't misspelled. "inversed" is a synonym for "inverted". Both
> spellings are correct.

Some time ago I stumbled about "inversed", too. My spell checker doesn't
know it and I checked some dictionaries and none of them knew that word:

https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed

https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
having "inversed" as past participle.

Having said this I think (independent of the question if "inversed"
exists) using two similar terms for the same thing just results in
confusion. I hit that in the past already and I like it being addressed.

> And as you noted in the cover letter, there's a conflict between the
> macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> in the wrong order you'll get a compile error.

There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
first to come to my mind). I'm not aware of any problems related to
these. What am I missing?

> The enum was named this way on purpose to make it separate from the
> definition for the DT bindings.

Then please let's make it different by picking a different prefix or
something like that.

> Note that DT bindings are an ABI and can
> never change, whereas the enum pwm_polarity is part of a Linux internal
> API and doesn't have the same restrictions as an ABI.

I thought only binary device trees (dtb) are supposed to be ABI.

Best regards
Uwe

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

2020-03-17 21:34:13

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Tue, Mar 17, 2020 at 01:34:50PM +0000, Paul Barker wrote:
> On Tue, 17 Mar 2020 14:32:25 +0200
> Oleksandr Suvorov <[email protected]> wrote:
>
> > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > Rename it to PWM_POLARITY_INVERTED.
> >
> > Signed-off-by: Oleksandr Suvorov <[email protected]>
>
> Looks good. PWM_POLARITY_INVERSED confused me when I was working in this area
> recently.

Same for me.

Acked-by: Uwe Kleine-K?nig <[email protected]>

Best regards
Uwe

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

2020-03-18 11:48:50

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Tue, Mar 17, 2020 at 7:41 PM Thierry Reding <[email protected]> wrote:
>
> On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > Rename it to PWM_POLARITY_INVERTED.
>
> It isn't misspelled. "inversed" is a synonym for "inverted". Both
> spellings are correct.

> And as you noted in the cover letter, there's a conflict between the
> macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> in the wrong order you'll get a compile error.

This patch is a part of the patchset, which in result removes enum at all,
so there will be no definition conflict.

> The enum was named this way on purpose to make it separate from the
> definition for the DT bindings. Note that DT bindings are an ABI and can
> never change, whereas the enum pwm_polarity is part of a Linux internal
> API and doesn't have the same restrictions as an ABI.

AFAIU, DTS files are not a part of ABI.

I understand that enums are better than macros for some reasons.
However, I think it is dangerous to use duplicate definitions in
different places when values of these definitions use in the same
code.
So, given that the enum cannot be used in DT, I left only macros.

You personally wrote that the enum pwm_polarity can change, so the
desynchronization I quite possible.

> As far as I'm concerned this is completely unnecessary churn that's
> potentially going to come back and bite us, so I see no reason to accept
> this.

>
> Thierry
--
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

2020-03-18 23:01:42

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Tue, Mar 17, 2020 at 10:00:42PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> > On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > > Rename it to PWM_POLARITY_INVERTED.
> >
> > It isn't misspelled. "inversed" is a synonym for "inverted". Both
> > spellings are correct.
>
> Some time ago I stumbled about "inversed", too. My spell checker doesn't
> know it and I checked some dictionaries and none of them knew that word:
>
> https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
> https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
> https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed
>
> https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
> having "inversed" as past participle.

Here are the first three results from a Google query:

https://www.yourdictionary.com/inversed
https://www.dictionary.com/browse/inversed
https://en.wiktionary.org/wiki/inversed

> Having said this I think (independent of the question if "inversed"
> exists) using two similar terms for the same thing just results in
> confusion. I hit that in the past already and I like it being addressed.

I don't know. It's pretty common to use different words for the same
thing. They're called synonyms.

> > And as you noted in the cover letter, there's a conflict between the
> > macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> > in the wrong order you'll get a compile error.
>
> There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
> first to come to my mind). I'm not aware of any problems related to
> these. What am I missing?

There's currently no problem, obviously. But if for some reason the
include files end up being included in a different order (i.e. the
dt-bindings header is included before linux/pwm.h) then the macro will
be evaluated and result in something like:

enum pwm_polarity {
PWM_POLARITY_NORMAL,
1,
};

and that's not valid C, so will cause a build error.

> > The enum was named this way on purpose to make it separate from the
> > definition for the DT bindings.
>
> Then please let's make it different by picking a different prefix or
> something like that.

Again, seems to me like unnecessary churn. Feel free to propose
something, but I recall being in the same position at the time and this
was the best I could come up with.

> > Note that DT bindings are an ABI and can
> > never change, whereas the enum pwm_polarity is part of a Linux internal
> > API and doesn't have the same restrictions as an ABI.
>
> I thought only binary device trees (dtb) are supposed to be ABI.

Yes, the DTB is the ABI. dt-bindings/pwm/pwm.h is used to generate DTBs,
which basically makes it ABI as well. Yes, the symbol name may not be
part of the ABI, but changing the symbol becomes very inconvenient
because everyone that depends on it would have to change. Why bother?

My point is that enum pwm_polarity is an API in the kernel and hence its
easy to change or extend. But since that is not the same for the DTB, we
need to be careful what from the internal kernel API leaks into the DTB.
That's why they are different symbols, so that it is clear that what's
in dt-bindings/pwm/pwm.h is the ABI.

Thierry

Attachments:

(No filename) (3.42 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-19 06:52:01

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

[Dropped Tony Prisk from recipients as the address bounces]

Hello,

On Wed, Mar 18, 2020 at 11:59:53PM +0100, Thierry Reding wrote:
> On Tue, Mar 17, 2020 at 10:00:42PM +0100, Uwe Kleine-K?nig wrote:
> > Hello,
> >
> > On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> > > On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > > > Rename it to PWM_POLARITY_INVERTED.
> > >
> > > It isn't misspelled. "inversed" is a synonym for "inverted". Both
> > > spellings are correct.
> >
> > Some time ago I stumbled about "inversed", too. My spell checker doesn't
> > know it and I checked some dictionaries and none of them knew that word:
> >
> > https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
> > https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
> > https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed
> >
> > https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
> > having "inversed" as past participle.
>
> Here are the first three results from a Google query:
>
> https://www.yourdictionary.com/inversed

There is something fishy. In the Verb section it says indeed, that it is
the past participle and simple past of inverse. The entry for inverse
however only has sections that identify this word as adjective or noun;
not a verb.

> https://www.dictionary.com/browse/inversed

Not sure I'd count this as hint that inversed exists. The entry shown to
me under this URL is about "inverse" and it has

verb (used with object), in?versed, in?vers?ing.
? to invert.

Does this mean: "Did you mean invert instead?"

> https://en.wiktionary.org/wiki/inversed

Yeah, that's the one I found, too.

I still have the impression that "inversed" is in use because people
don't know better and understand the intended meaning. And this results
in leaking of this word into the references.

> > Having said this I think (independent of the question if "inversed"
> > exists) using two similar terms for the same thing just results in
> > confusion. I hit that in the past already and I like it being addressed.
>
> I don't know. It's pretty common to use different words for the same
> thing. They're called synonyms.

In literature yes, I agree. In a novel it is annoying to repeat the same
words over and over again and some variation is good. In programming
however the goal is a different one. There the goal should be to be
precise and consistent.

> > > And as you noted in the cover letter, there's a conflict between the
> > > macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> > > in the wrong order you'll get a compile error.
> >
> > There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
> > first to come to my mind). I'm not aware of any problems related to
> > these. What am I missing?
>
> There's currently no problem, obviously. But if for some reason the
> include files end up being included in a different order (i.e. the
> dt-bindings header is included before linux/pwm.h) then the macro will
> be evaluated and result in something like:
>
> enum pwm_polarity {
> PWM_POLARITY_NORMAL,
> 1,
> };
>
> and that's not valid C, so will cause a build error.

I admit I didn't look closely here and I assume you are right. If I
understand Oleksandr right this is only an intermediate step and when
the series is applied completely this issue is gone. Still it might be
worth to improve the series here.

My original question was about similar problems with GPIO_ACTIVE_HIGH.
Are you aware of problems there?

> > > Note that DT bindings are an ABI and can
> > > never change, whereas the enum pwm_polarity is part of a Linux internal
> > > API and doesn't have the same restrictions as an ABI.
> >
> > I thought only binary device trees (dtb) are supposed to be ABI.
>
> Yes, the DTB is the ABI. dt-bindings/pwm/pwm.h is used to generate DTBs,
> which basically makes it ABI as well.

We disagree here. With this argument you could fix quite some things as
ABI.

> Yes, the symbol name may not be part of the ABI, but changing the
> symbol becomes very inconvenient because everyone that depends on it
> would have to change.

Oleksandr adapted all in-tree users, so it only affects out-of-tree
users. In my book this is fine.

> Why bother?

To make the API more precise and consistent. That's a good goal in my
eyes.

Best regards
Uwe

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

2020-03-19 11:41:22

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Thu, Mar 19, 2020 at 1:00 AM Thierry Reding <[email protected]> wrote:
>
> On Tue, Mar 17, 2020 at 10:00:42PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> > > On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > > > Rename it to PWM_POLARITY_INVERTED.
> > >
> > > It isn't misspelled. "inversed" is a synonym for "inverted". Both
> > > spellings are correct.
> >
> > Some time ago I stumbled about "inversed", too. My spell checker doesn't
> > know it and I checked some dictionaries and none of them knew that word:
> >
> > https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
> > https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
> > https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed
> >
> > https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
> > having "inversed" as past participle.
>
> Here are the first three results from a Google query:
>
> https://www.yourdictionary.com/inversed
> https://www.dictionary.com/browse/inversed
> https://en.wiktionary.org/wiki/inversed
>
> > Having said this I think (independent of the question if "inversed"
> > exists) using two similar terms for the same thing just results in
> > confusion. I hit that in the past already and I like it being addressed.
>
> I don't know. It's pretty common to use different words for the same
> thing. They're called synonyms.
>
> > > And as you noted in the cover letter, there's a conflict between the
> > > macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> > > in the wrong order you'll get a compile error.
> >
> > There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
> > first to come to my mind). I'm not aware of any problems related to
> > these. What am I missing?
>
> There's currently no problem, obviously. But if for some reason the
> include files end up being included in a different order (i.e. the
> dt-bindings header is included before linux/pwm.h) then the macro will
> be evaluated and result in something like:
>
> enum pwm_polarity {
> PWM_POLARITY_NORMAL,
> 1,
> };
>
> and that's not valid C, so will cause a build error.
>
> > > The enum was named this way on purpose to make it separate from the
> > > definition for the DT bindings.
> >
> > Then please let's make it different by picking a different prefix or
> > something like that.
>
> Again, seems to me like unnecessary churn. Feel free to propose
> something, but I recall being in the same position at the time and this
> was the best I could come up with.
>
> > > Note that DT bindings are an ABI and can
> > > never change, whereas the enum pwm_polarity is part of a Linux internal
> > > API and doesn't have the same restrictions as an ABI.
> >
> > I thought only binary device trees (dtb) are supposed to be ABI.
>
> Yes, the DTB is the ABI. dt-bindings/pwm/pwm.h is used to generate DTBs,
> which basically makes it ABI as well. Yes, the symbol name may not be
> part of the ABI, but changing the symbol becomes very inconvenient
> because everyone that depends on it would have to change. Why bother?
>
> My point is that enum pwm_polarity is an API in the kernel and hence its
> easy to change or extend. But since that is not the same for the DTB, we
> need to be careful what from the internal kernel API leaks into the DTB.
> That's why they are different symbols, so that it is clear that what's
> in dt-bindings/pwm/pwm.h is the ABI.

Thierry, I see the PWM core converts the bit field "third cell" into
the polarity variable.
Now I probably understand your sight and agree that we shouldn't give
the same names to bits in bitfield (dts) and values of a variable.

But there are lots of useless "0" values of third cell of "pwms"
option in dts files.

I see 2 ways now:
- just remove all "0" "third cell" from "pwms" options in dts files. I
see this "0" confuses some people.
- convert pwm_state.polarity into pwm_state.flags and use bitfield
directly from dtb.
It simplifies the parsing logic and makes adding new flags easier.

What do think?

>
> Thierry

--
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

2020-03-19 12:12:30

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

Hello,

[dropping Tony Prisk <[email protected]> from recipients]

On Thu, Mar 19, 2020 at 01:40:28PM +0200, Oleksandr Suvorov wrote:
> Thierry, I see the PWM core converts the bit field "third cell" into
> the polarity variable.
> Now I probably understand your sight and agree that we shouldn't give
> the same names to bits in bitfield (dts) and values of a variable.
>
> But there are lots of useless "0" values of third cell of "pwms"
> option in dts files.
>
> I see 2 ways now:
> - just remove all "0" "third cell" from "pwms" options in dts files. I
> see this "0" confuses some people.

Then you have to overwrite pwm-cells of the provider. If there are two
PWMs used from the same provider and only one is inverted this won't
work. (Not entirely sure I understood your suggestion.) So I don't like
this suggestion.

And also in my eyes this isn't clearer, just more complicated to use.

> - convert pwm_state.polarity into pwm_state.flags and use bitfield
> directly from dtb.
> It simplifies the parsing logic and makes adding new flags easier.

*shrug*, I don't care much.

Best regards
Uwe

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

2020-03-19 13:05:04

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Thu, Mar 19, 2020 at 2:11 PM Uwe Kleine-König
<[email protected]> wrote:
>
> Hello,
>
> [dropping Tony Prisk <[email protected]> from recipients]
>
> On Thu, Mar 19, 2020 at 01:40:28PM +0200, Oleksandr Suvorov wrote:
> > Thierry, I see the PWM core converts the bit field "third cell" into
> > the polarity variable.
> > Now I probably understand your sight and agree that we shouldn't give
> > the same names to bits in bitfield (dts) and values of a variable.
> >
> > But there are lots of useless "0" values of third cell of "pwms"
> > option in dts files.
> >
> > I see 2 ways now:
> > - just remove all "0" "third cell" from "pwms" options in dts files. I
> > see this "0" confuses some people.
>
> Then you have to overwrite pwm-cells of the provider. If there are two
> PWMs used from the same provider and only one is inverted this won't
> work. (Not entirely sure I understood your suggestion.) So I don't like
> this suggestion.

Good point, agree. But we still have the unnamed "0".

What about renaming the dt-bindings macro PWM_POLARITY_INVERTED
and add the new one for the normal polarity?
Like PWM_FLAG_POLARITY_NORMAL / PWM_FLAG_POLARITY_INVERTED or
DT_PWM_POLARITY_NORMAL / DT_PWM_POLARITY_INVERTED?

Using different prefix will prevent interfering names of enum and
macros in the future
and will allow using the named nop-flag PWM_FLAG_POLARITY_NORMAL in dts.

> And also in my eyes this isn't clearer, just more complicated to use.
>
> > - convert pwm_state.polarity into pwm_state.flags and use bitfield
> > directly from dtb.
> > It simplifies the parsing logic and makes adding new flags easier.
>
> *shrug*, I don't care much.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |

--
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

2020-03-19 16:39:03

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Thu, Mar 19, 2020 at 07:50:39AM +0100, Uwe Kleine-König wrote:
>
> [Dropped Tony Prisk from recipients as the address bounces]
>
> Hello,
>
> On Wed, Mar 18, 2020 at 11:59:53PM +0100, Thierry Reding wrote:
> > On Tue, Mar 17, 2020 at 10:00:42PM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> > > > On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > > > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > > > > Rename it to PWM_POLARITY_INVERTED.
> > > >
> > > > It isn't misspelled. "inversed" is a synonym for "inverted". Both
> > > > spellings are correct.
> > >
> > > Some time ago I stumbled about "inversed", too. My spell checker doesn't
> > > know it and I checked some dictionaries and none of them knew that word:
> > >
> > > https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
> > > https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
> > > https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed
> > >
> > > https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
> > > having "inversed" as past participle.
> >
> > Here are the first three results from a Google query:
> >
> > https://www.yourdictionary.com/inversed
>
> There is something fishy. In the Verb section it says indeed, that it is
> the past participle and simple past of inverse. The entry for inverse
> however only has sections that identify this word as adjective or noun;
> not a verb.
>
> > https://www.dictionary.com/browse/inversed
>
> Not sure I'd count this as hint that inversed exists. The entry shown to
> me under this URL is about "inverse" and it has
>
> verb (used with object), in·versed, in·vers·ing.
> ? to invert.
>
> Does this mean: "Did you mean invert instead?"
>
> > https://en.wiktionary.org/wiki/inversed
>
> Yeah, that's the one I found, too.
>
> I still have the impression that "inversed" is in use because people
> don't know better and understand the intended meaning. And this results
> in leaking of this word into the references.
>
> > > Having said this I think (independent of the question if "inversed"
> > > exists) using two similar terms for the same thing just results in
> > > confusion. I hit that in the past already and I like it being addressed.
> >
> > I don't know. It's pretty common to use different words for the same
> > thing. They're called synonyms.
>
> In literature yes, I agree. In a novel it is annoying to repeat the same
> words over and over again and some variation is good. In programming
> however the goal is a different one. There the goal should be to be
> precise and consistent.

We also need to make sure that things don't break. It's a very bad idea
to have a macro with the same name as an enum value for reasons I stated
before. I think that's the most important thing here.

Also, if inversed is a synonym of inverted, we don't loose any precision
at all. All you have to remember is that you're dealing with a device
tree constant in one case and an API enumeration in the other.

So I think the current form is actually more precise, though I guess it
could be confusing if you don't care about the difference.

> > > > And as you noted in the cover letter, there's a conflict between the
> > > > macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> > > > in the wrong order you'll get a compile error.
> > >
> > > There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
> > > first to come to my mind). I'm not aware of any problems related to
> > > these. What am I missing?
> >
> > There's currently no problem, obviously. But if for some reason the
> > include files end up being included in a different order (i.e. the
> > dt-bindings header is included before linux/pwm.h) then the macro will
> > be evaluated and result in something like:
> >
> > enum pwm_polarity {
> > PWM_POLARITY_NORMAL,
> > 1,
> > };
> >
> > and that's not valid C, so will cause a build error.
>
> I admit I didn't look closely here and I assume you are right. If I
> understand Oleksandr right this is only an intermediate step and when
> the series is applied completely this issue is gone. Still it might be
> worth to improve the series here.

$ gcc -o /dev/null -x c - <<- EOF
> #define PWM_POLARITY_INVERTED (1 << 0)
>
> enum pwm_polarity {
> PWM_POLARITY_NORMAL,
> PWM_POLARITY_INVERTED,
> };
> EOF
<stdin>:1:35: error: expected identifier before ‘(’ token
<stdin>:5:9: note: in expansion of macro ‘PWM_POLARITY_INVERTED’

Q.E.D.

> My original question was about similar problems with GPIO_ACTIVE_HIGH.
> Are you aware of problems there?

The problem exists there equally. We're probably not running into it
because drivers don't end up including dt-bindings/gpio/gpio.h and
include/linux/gpio/machine.h at the same time. Or they end up always
including them in the right order.

For PWM the situation is slightly more complicated because we only have
one header for the kernel API, so the likelihood of including it along
with the dt-bindings header is increased compared to GPIO.

> > > > Note that DT bindings are an ABI and can
> > > > never change, whereas the enum pwm_polarity is part of a Linux internal
> > > > API and doesn't have the same restrictions as an ABI.
> > >
> > > I thought only binary device trees (dtb) are supposed to be ABI.
> >
> > Yes, the DTB is the ABI. dt-bindings/pwm/pwm.h is used to generate DTBs,
> > which basically makes it ABI as well.
>
> We disagree here. With this argument you could fix quite some things as
> ABI.

I don't understand what you're trying to say.

> > Yes, the symbol name may not be part of the ABI, but changing the
> > symbol becomes very inconvenient because everyone that depends on it
> > would have to change.
>
> Oleksandr adapted all in-tree users, so it only affects out-of-tree
> users. In my book this is fine.

There used to be a time when it was assumed that eventually device tree
sources would live outside of the kernel tree. Given that they are a HW
description, they really ought not to be relying on the Linux kernel
tree as a way of keeping them consistent. That's really only out of
convenience.

> > Why bother?
>
> To make the API more precise and consistent. That's a good goal in my
> eyes.

PWM_POLARITY_INVERTED is not part of the API. It doesn't exist for
anything other than to make the device tree more readable.

I now regret that we ever introduced this in the first place. Perhaps it
would've been better to just deal with raw numbers instead.

Thierry

Attachments:

(No filename) (6.72 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-19 16:44:51

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] pwm: rename the PWM_POLARITY_INVERSED enum

On Thu, Mar 19, 2020 at 01:40:28PM +0200, Oleksandr Suvorov wrote:
> On Thu, Mar 19, 2020 at 1:00 AM Thierry Reding <[email protected]> wrote:
> >
> > On Tue, Mar 17, 2020 at 10:00:42PM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > On Tue, Mar 17, 2020 at 06:40:43PM +0100, Thierry Reding wrote:
> > > > On Tue, Mar 17, 2020 at 02:32:25PM +0200, Oleksandr Suvorov wrote:
> > > > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled.
> > > > > Rename it to PWM_POLARITY_INVERTED.
> > > >
> > > > It isn't misspelled. "inversed" is a synonym for "inverted". Both
> > > > spellings are correct.
> > >
> > > Some time ago I stumbled about "inversed", too. My spell checker doesn't
> > > know it and I checked some dictionaries and none of them knew that word:
> > >
> > > https://www.lexico.com/search?utf8=%E2%9C%93&filter=dictionary&dictionary=en&query=inversed
> > > https://de.pons.com/%C3%BCbersetzung/englisch-deutsch/inversed
> > > https://dictionary.cambridge.org/spellcheck/english-german/?q=inversed
> > >
> > > https://en.wiktionary.org/wiki/inverse#Verb mentions "inverse" as a verb
> > > having "inversed" as past participle.
> >
> > Here are the first three results from a Google query:
> >
> > https://www.yourdictionary.com/inversed
> > https://www.dictionary.com/browse/inversed
> > https://en.wiktionary.org/wiki/inversed
> >
> > > Having said this I think (independent of the question if "inversed"
> > > exists) using two similar terms for the same thing just results in
> > > confusion. I hit that in the past already and I like it being addressed.
> >
> > I don't know. It's pretty common to use different words for the same
> > thing. They're called synonyms.
> >
> > > > And as you noted in the cover letter, there's a conflict between the
> > > > macro defined in dt-bindings/pwm/pwm.txt. If they end up being included
> > > > in the wrong order you'll get a compile error.
> > >
> > > There are also other symbols that exist twice (GPIO_ACTIVE_HIGH was the
> > > first to come to my mind). I'm not aware of any problems related to
> > > these. What am I missing?
> >
> > There's currently no problem, obviously. But if for some reason the
> > include files end up being included in a different order (i.e. the
> > dt-bindings header is included before linux/pwm.h) then the macro will
> > be evaluated and result in something like:
> >
> > enum pwm_polarity {
> > PWM_POLARITY_NORMAL,
> > 1,
> > };
> >
> > and that's not valid C, so will cause a build error.
> >
> > > > The enum was named this way on purpose to make it separate from the
> > > > definition for the DT bindings.
> > >
> > > Then please let's make it different by picking a different prefix or
> > > something like that.
> >
> > Again, seems to me like unnecessary churn. Feel free to propose
> > something, but I recall being in the same position at the time and this
> > was the best I could come up with.
> >
> > > > Note that DT bindings are an ABI and can
> > > > never change, whereas the enum pwm_polarity is part of a Linux internal
> > > > API and doesn't have the same restrictions as an ABI.
> > >
> > > I thought only binary device trees (dtb) are supposed to be ABI.
> >
> > Yes, the DTB is the ABI. dt-bindings/pwm/pwm.h is used to generate DTBs,
> > which basically makes it ABI as well. Yes, the symbol name may not be
> > part of the ABI, but changing the symbol becomes very inconvenient
> > because everyone that depends on it would have to change. Why bother?
> >
> > My point is that enum pwm_polarity is an API in the kernel and hence its
> > easy to change or extend. But since that is not the same for the DTB, we
> > need to be careful what from the internal kernel API leaks into the DTB.
> > That's why they are different symbols, so that it is clear that what's
> > in dt-bindings/pwm/pwm.h is the ABI.
>
> Thierry, I see the PWM core converts the bit field "third cell" into
> the polarity variable.

Yes. And if there were other fields in that third cell, there'd be other
variables that would be derived from those.

> Now I probably understand your sight and agree that we shouldn't give
> the same names to bits in bitfield (dts) and values of a variable.
>
> But there are lots of useless "0" values of third cell of "pwms"
> option in dts files.

These aren't useless 0 values. 0 doesn't mean "don't care", it's a very
specific value. In this case it implies that the polarity is "normal".
That's very useful if you have a PWM controller that can set the
polarity.

> I see 2 ways now:
> - just remove all "0" "third cell" from "pwms" options in dts files. I
> see this "0" confuses some people.

Some drivers already do this by supporting 2-cell and 3-cell specifiers.
Not everyone does this and they shouldn't need to. The bindings are very
clear about what to do with that third cell and how to parse it. It's
just a matter of reading and understanding the bindings. It's not
exactly rocket science, so why do we have to jump through hoops to try
and simplify it?

> - convert pwm_state.polarity into pwm_state.flags and use bitfield
> directly from dtb.
> It simplifies the parsing logic and makes adding new flags easier.

And then you have to go and and parsing code everywhere to deal with
these new flags. I prefer to have this parsing code in the core where
it's written once and used everywhere, instead of letting drivers deal
with flags that they might interpret wrongly.

Also remember that the third cell may be extended in the future and not
all data in in may end up being a simple flag.

Thierry

Attachments:

(No filename) (5.63 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-19 17:32:51