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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6885 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]