| Message ID | 20200317123231.2843297-2-oleksandr.suvorov@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/7] pwm: rename the PWM_POLARITY_INVERSED enum | expand |
On Tue, 17 Mar 2020 14:32:25 +0200 Oleksandr Suvorov <oleksandr.suvorov@toradex.com> wrote: > The polarity enum definition PWM_POLARITY_INVERSED is misspelled. > Rename it to PWM_POLARITY_INVERTED. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> Looks good. PWM_POLARITY_INVERSED confused me when I was working in this area recently. After applying to master there's no more instances of PWM_POLARITY_INVERSED in the source tree. Reviewed-by: Paul Barker <pbarker@konsulko.com>
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 ^ > - polarity = PWM_POLARITY_INVERSED; > + polarity = PWM_POLARITY_INVERTED;
On Tue, Mar 17, 2020 at 6:27 PM <Claudiu.Beznea@microchip.com> 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;
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
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
On Tue, Mar 17, 2020 at 01:34:50PM +0000, Paul Barker wrote: > On Tue, 17 Mar 2020 14:32:25 +0200 > Oleksandr Suvorov <oleksandr.suvorov@toradex.com> wrote: > > > The polarity enum definition PWM_POLARITY_INVERSED is misspelled. > > Rename it to PWM_POLARITY_INVERTED. > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > Looks good. PWM_POLARITY_INVERSED confused me when I was working in this area > recently. Same for me. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Tue, Mar 17, 2020 at 7:41 PM Thierry Reding <thierry.reding@gmail.com> 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
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
[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
On Thu, Mar 19, 2020 at 1:00 AM Thierry Reding <thierry.reding@gmail.com> 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
Hello, [dropping Tony Prisk <linux@prisktech.co.nz> 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
On Thu, Mar 19, 2020 at 2:11 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > [dropping Tony Prisk <linux@prisktech.co.nz> 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/ |
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
On Thu, Mar 19, 2020 at 01:40:28PM +0200, Oleksandr Suvorov wrote: > On Thu, Mar 19, 2020 at 1:00 AM Thierry Reding <thierry.reding@gmail.com> 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
Hello, On Thu, Mar 19, 2020 at 05:37:00PM +0100, Thierry Reding wrote: > On Thu, Mar 19, 2020 at 07:50:39AM +0100, Uwe Kleine-König wrote: > > 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: > > > > 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. And I'm entirely on your side here. > 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. You might have missed it, but that's OK for me, too. And note that after applying the whole series the enum is gone and so the problem. (First hunk of include/linux/pwm.h in patch 5.) > Also, if inversed is a synonym of inverted, we don't loose any precision > at all. grep doesn't know about synonyms, so if I grep for stuff about inverted PWMs in the kernel I completely miss one half as it's called inversed there. (Yeah sure, I can also grep for "inversed|inverted", but therefor I have to know first that both are used interchangable here.) That's a bit like a schematic that has "RESET#" in one place and "nRESET" in an other. If you stumble about that you wonder if they are two different names for the same signal or if they are actually two different ones. Have you ever read a specification that described some property, gave it a name and then later used a synonym to describe it? In my eyes that's a bad idea. > 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. Everything you need to remember (or learn) about a subsystem makes it harder work with it. > 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. If there is a technical need to have different names that's one thing. But using synonyms to differentiate them is not optimal. Then please let's have names where looking at the identifier makes it obvious which is for the device trees and which for the API enum. > > > > > 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. I don't understand why you proved something here. I didn't doubt this. > > 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. Oh, that's worse than I expected. There are two .c files that include dt-bindings/gpio/gpio.h: drivers/rtc/rtc-omap.c drivers/tty/serial/omap-serial.c So the definition isn't even used in the gpio core to parse dt-stuff. (And both files don't use any definition of that file :-|) > 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. If a consumer or provider includes the dt-bindings file there is something fishy. (Still catching this with a compiler message better than "expected identifier before ‘(’ token" would be good.) > > > > > 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. I don't want to follow your argument that dt-bindings/pwm/pwm.h is ABI as well. device tree binaries follow an ABI (similar to machine code), but the compiler and the source code (including headers) are not. > > > 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. The other way round however is fine, isn't it? So use the dt definition in the kernel should be ok. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 5a7f6598c05f..08afbb5b98aa 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -152,7 +152,7 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) pwm->args.polarity = PWM_POLARITY_NORMAL; if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) - pwm->args.polarity = PWM_POLARITY_INVERSED; + pwm->args.polarity = PWM_POLARITY_INVERTED; return pwm; } @@ -819,7 +819,7 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode) pwm->args.polarity = PWM_POLARITY_NORMAL; if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED) - pwm->args.polarity = PWM_POLARITY_INVERSED; + pwm->args.polarity = PWM_POLARITY_INVERTED; #endif return pwm; diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index dcbc0489dfd4..b53a188479b0 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -270,7 +270,7 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev) chip->chip.of_xlate = of_pwm_xlate_with_flags; chip->chip.of_pwm_n_cells = 3; - ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERTED); if (ret) { clk_disable_unprepare(hlcdc->periph_clk); return ret; diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c index 85c53701958c..98526a286347 100644 --- a/drivers/pwm/pwm-atmel-tcb.c +++ b/drivers/pwm/pwm-atmel-tcb.c @@ -152,7 +152,7 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) /* * If duty is 0 the timer will be stopped and we have to * configure the output correctly on software trigger: - * - set output to high if PWM_POLARITY_INVERSED + * - set output to high if PWM_POLARITY_INVERTED * - set output to low if PWM_POLARITY_NORMAL * * This is why we're reverting polarity in this case. @@ -166,13 +166,13 @@ static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) /* flush old setting and set the new one */ if (index == 0) { cmr &= ~ATMEL_TC_ACMR_MASK; - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_ASWTRG_CLEAR; else cmr |= ATMEL_TC_ASWTRG_SET; } else { cmr &= ~ATMEL_TC_BCMR_MASK; - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_BSWTRG_CLEAR; else cmr |= ATMEL_TC_BSWTRG_SET; @@ -211,7 +211,7 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) /* * If duty is 0 the timer will be stopped and we have to * configure the output correctly on software trigger: - * - set output to high if PWM_POLARITY_INVERSED + * - set output to high if PWM_POLARITY_INVERTED * - set output to low if PWM_POLARITY_NORMAL * * This is why we're reverting polarity in this case. @@ -229,13 +229,13 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) cmr &= ~ATMEL_TC_ACMR_MASK; /* Set CMR flags according to given polarity */ - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_ASWTRG_CLEAR; else cmr |= ATMEL_TC_ASWTRG_SET; } else { cmr &= ~ATMEL_TC_BCMR_MASK; - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_BSWTRG_CLEAR; else cmr |= ATMEL_TC_BSWTRG_SET; @@ -249,12 +249,12 @@ static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) */ if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0) { if (index == 0) { - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_ACPA_SET | ATMEL_TC_ACPC_CLEAR; else cmr |= ATMEL_TC_ACPA_CLEAR | ATMEL_TC_ACPC_SET; } else { - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) cmr |= ATMEL_TC_BCPB_SET | ATMEL_TC_BCPC_CLEAR; else cmr |= ATMEL_TC_BCPB_CLEAR | ATMEL_TC_BCPC_SET; diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 6161e7e3e9ac..2d42f97e4b81 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -329,7 +329,7 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, } if (cmr & PWM_CMR_CPOL) - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; else state->polarity = PWM_POLARITY_NORMAL; } diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c index 1f829edd8ee7..574bec61e0ac 100644 --- a/drivers/pwm/pwm-bcm-iproc.c +++ b/drivers/pwm/pwm-bcm-iproc.c @@ -97,7 +97,7 @@ static void iproc_pwmc_get_state(struct pwm_chip *chip, struct pwm_device *pwm, if (value & BIT(IPROC_PWM_CTRL_POLARITY_SHIFT(pwm->hwpwm))) state->polarity = PWM_POLARITY_NORMAL; else - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; value = readl(ip->base + IPROC_PWM_PRESCALE_OFFSET); prescale = value >> IPROC_PWM_PRESCALE_SHIFT(pwm->hwpwm); diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c index 81da91df2529..02da511814f1 100644 --- a/drivers/pwm/pwm-bcm-kona.c +++ b/drivers/pwm/pwm-bcm-kona.c @@ -303,7 +303,7 @@ static int kona_pwmc_probe(struct platform_device *pdev) clk_disable_unprepare(kp->clk); - ret = pwmchip_add_with_polarity(&kp->chip, PWM_POLARITY_INVERSED); + ret = pwmchip_add_with_polarity(&kp->chip, PWM_POLARITY_INVERTED); if (ret < 0) dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c index 4bab73073ad7..02345b6f9fe8 100644 --- a/drivers/pwm/pwm-ep93xx.c +++ b/drivers/pwm/pwm-ep93xx.c @@ -124,7 +124,7 @@ static int ep93xx_pwm_polarity(struct pwm_chip *chip, struct pwm_device *pwm, if (ret) return ret; - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) writew(0x1, ep93xx_pwm->base + EP93XX_PWMx_INVERT); else writew(0x0, ep93xx_pwm->base + EP93XX_PWMx_INVERT); diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c index 59272a920479..75dc30978c19 100644 --- a/drivers/pwm/pwm-fsl-ftm.c +++ b/drivers/pwm/pwm-fsl-ftm.c @@ -287,7 +287,7 @@ static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc, regmap_write(fpc->regmap, FTM_CV(pwm->hwpwm), duty); reg_polarity = 0; - if (newstate->polarity == PWM_POLARITY_INVERSED) + if (newstate->polarity == PWM_POLARITY_INVERTED) reg_polarity = BIT(pwm->hwpwm); regmap_update_bits(fpc->regmap, FTM_POL, BIT(pwm->hwpwm), reg_polarity); diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c index ad205fdad372..c57a94e7da0f 100644 --- a/drivers/pwm/pwm-hibvt.c +++ b/drivers/pwm/pwm-hibvt.c @@ -120,7 +120,7 @@ static void hibvt_pwm_set_polarity(struct pwm_chip *chip, { struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip); - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) hibvt_pwm_set_bits(hi_pwm_chip->base, PWM_CTRL_ADDR(pwm->hwpwm), PWM_POLARITY_MASK, (0x1 << PWM_POLARITY_SHIFT)); else diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c index 9145f6160649..461ab2c08616 100644 --- a/drivers/pwm/pwm-imx-tpm.c +++ b/drivers/pwm/pwm-imx-tpm.c @@ -156,7 +156,7 @@ static void pwm_imx_tpm_get_state(struct pwm_chip *chip, /* get polarity */ val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); if ((val & PWM_IMX_TPM_CnSC_ELS) == PWM_IMX_TPM_CnSC_ELS_INVERSED) - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; else /* * Assume reserved values (2b00 and 2b11) to yield diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 35a7ac42269c..33d344445254 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -146,7 +146,7 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, state->polarity = PWM_POLARITY_NORMAL; break; case MX3_PWMCR_POUTC_INVERTED: - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; break; default: dev_warn(chip->dev, "can't set polarity, output disconnected"); @@ -280,7 +280,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | MX3_PWMCR_DBGEN; - if (state->polarity == PWM_POLARITY_INVERSED) + if (state->polarity == PWM_POLARITY_INVERTED) cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED); diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c index 9d78cc21cb12..67075d18561f 100644 --- a/drivers/pwm/pwm-jz4740.c +++ b/drivers/pwm/pwm-jz4740.c @@ -130,7 +130,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, case PWM_POLARITY_NORMAL: ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW; break; - case PWM_POLARITY_INVERSED: + case PWM_POLARITY_INVERTED: ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW; break; } diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 6245bbdb6e6c..2d368bfc680d 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -8,7 +8,7 @@ * N cycles for the first half period. * The hardware has no "polarity" setting. This driver reverses the period * cycles (the low length is inverted with the high length) for - * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity + * PWM_POLARITY_INVERTED. This means that .get_state cannot read the polarity * from the hardware. * Setting the duty cycle will disable and re-enable the PWM output. * Disabling the PWM stops the output immediately (without waiting for the @@ -168,7 +168,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, duty = state->duty_cycle; period = state->period; - if (state->polarity == PWM_POLARITY_INVERSED) + if (state->polarity == PWM_POLARITY_INVERTED) duty = period - duty; fin_freq = clk_get_rate(channel->clk); @@ -275,7 +275,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return -EINVAL; if (!state->enabled) { - if (state->polarity == PWM_POLARITY_INVERSED) { + if (state->polarity == PWM_POLARITY_INVERTED) { /* * This IP block revision doesn't have an "always high" * setting which we can use for "inverted disabled". diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c index 88a3c5690fea..082ccec93133 100644 --- a/drivers/pwm/pwm-omap-dmtimer.c +++ b/drivers/pwm/pwm-omap-dmtimer.c @@ -190,7 +190,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, load_value, load_value, match_value, match_value); omap->pdata->set_pwm(omap->dm_timer, - pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED, + pwm_get_polarity(pwm) == PWM_POLARITY_INVERTED, true, PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE); @@ -220,7 +220,7 @@ static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip, */ mutex_lock(&omap->mutex); omap->pdata->set_pwm(omap->dm_timer, - polarity == PWM_POLARITY_INVERSED, + polarity == PWM_POLARITY_INVERTED, true, PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE); mutex_unlock(&omap->mutex); diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 4a855a21b782..32beeb93ade1 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -108,17 +108,17 @@ static void tpu_pwm_set_pin(struct tpu_pwm_device *pwm, switch (state) { case TPU_PIN_INACTIVE: tpu_pwm_write(pwm, TPU_TIORn, - pwm->polarity == PWM_POLARITY_INVERSED ? + pwm->polarity == PWM_POLARITY_INVERTED ? TPU_TIOR_IOA_1 : TPU_TIOR_IOA_0); break; case TPU_PIN_PWM: tpu_pwm_write(pwm, TPU_TIORn, - pwm->polarity == PWM_POLARITY_INVERSED ? + pwm->polarity == PWM_POLARITY_INVERTED ? TPU_TIOR_IOA_0_SET : TPU_TIOR_IOA_1_CLR); break; case TPU_PIN_ACTIVE: tpu_pwm_write(pwm, TPU_TIORn, - pwm->polarity == PWM_POLARITY_INVERSED ? + pwm->polarity == PWM_POLARITY_INVERTED ? TPU_TIOR_IOA_0 : TPU_TIOR_IOA_1); break; } diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index 73352e6fbccb..c6158d559790 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -91,7 +91,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip, true : false; if (pc->data->supports_polarity && !(val & PWM_DUTY_POSITIVE)) - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; else state->polarity = PWM_POLARITY_NORMAL; @@ -135,7 +135,7 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, if (pc->data->supports_polarity) { ctrl &= ~PWM_POLARITY_MASK; - if (state->polarity == PWM_POLARITY_INVERSED) + if (state->polarity == PWM_POLARITY_INVERTED) ctrl |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE; else ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c index cc63f9baa481..409123405a11 100644 --- a/drivers/pwm/pwm-sifive.c +++ b/drivers/pwm/pwm-sifive.c @@ -124,7 +124,7 @@ static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm, state->period = ddata->real_period; state->duty_cycle = (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH; - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; } static int pwm_sifive_enable(struct pwm_chip *chip, bool enable) @@ -157,7 +157,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, int ret = 0; u32 frac; - if (state->polarity != PWM_POLARITY_INVERSED) + if (state->polarity != PWM_POLARITY_INVERTED) return -EINVAL; ret = clk_enable(ddata->clk); diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c index 3e3efa6c768f..7ddcdefd2a97 100644 --- a/drivers/pwm/pwm-sun4i.c +++ b/drivers/pwm/pwm-sun4i.c @@ -149,7 +149,7 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, if (val & BIT_CH(PWM_ACT_STATE, pwm->hwpwm)) state->polarity = PWM_POLARITY_NORMAL; else - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; if ((val & BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) == BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c index ab38c8203b79..b96b388f0969 100644 --- a/drivers/pwm/pwm-tiecap.c +++ b/drivers/pwm/pwm-tiecap.c @@ -118,7 +118,7 @@ static int ecap_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, value = readw(pc->mmio_base + ECCTL2); - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) /* Duty cycle defines LOW period of PWM */ value |= ECCTL2_APWM_POL_LOW; else diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c index 7b4c770ce9d6..71c337443dd5 100644 --- a/drivers/pwm/pwm-tiehrpwm.c +++ b/drivers/pwm/pwm-tiehrpwm.c @@ -193,7 +193,7 @@ static void configure_polarity(struct ehrpwm_pwm_chip *pc, int chan) aqctl_reg = AQCTLB; aqctl_mask = AQCTL_CBU_MASK; - if (pc->polarity[chan] == PWM_POLARITY_INVERSED) + if (pc->polarity[chan] == PWM_POLARITY_INVERTED) aqctl_val = AQCTL_CHANB_POLINVERSED; else aqctl_val = AQCTL_CHANB_POLNORMAL; @@ -201,7 +201,7 @@ static void configure_polarity(struct ehrpwm_pwm_chip *pc, int chan) aqctl_reg = AQCTLA; aqctl_mask = AQCTL_CAU_MASK; - if (pc->polarity[chan] == PWM_POLARITY_INVERSED) + if (pc->polarity[chan] == PWM_POLARITY_INVERTED) aqctl_val = AQCTL_CHANA_POLINVERSED; else aqctl_val = AQCTL_CHANA_POLNORMAL; diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c index 11d45e56a923..fc434965c5ed 100644 --- a/drivers/pwm/pwm-vt8500.c +++ b/drivers/pwm/pwm-vt8500.c @@ -165,7 +165,7 @@ static int vt8500_pwm_set_polarity(struct pwm_chip *chip, val = readl(vt8500->base + REG_CTRL(pwm->hwpwm)); - if (polarity == PWM_POLARITY_INVERSED) + if (polarity == PWM_POLARITY_INVERTED) val |= CTRL_INVERT; else val &= ~CTRL_INVERT; diff --git a/drivers/pwm/pwm-zx.c b/drivers/pwm/pwm-zx.c index e2c21cc34a96..dc7d20e52c52 100644 --- a/drivers/pwm/pwm-zx.c +++ b/drivers/pwm/pwm-zx.c @@ -75,7 +75,7 @@ static void zx_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, if (value & ZX_PWM_POLAR) state->polarity = PWM_POLARITY_NORMAL; else - state->polarity = PWM_POLARITY_INVERSED; + state->polarity = PWM_POLARITY_INVERTED; if (value & ZX_PWM_EN) state->enabled = true; @@ -158,7 +158,7 @@ static int zx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->polarity != cstate.polarity) zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_POLAR, - (state->polarity == PWM_POLARITY_INVERSED) ? + (state->polarity == PWM_POLARITY_INVERTED) ? 0 : ZX_PWM_POLAR); if (state->period != cstate.period || diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 2389b8669846..769ac09c56c2 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -166,7 +166,7 @@ static ssize_t polarity_show(struct device *child, polarity = "normal"; break; - case PWM_POLARITY_INVERSED: + case PWM_POLARITY_INVERTED: polarity = "inversed"; break; } @@ -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")) - polarity = PWM_POLARITY_INVERSED; + polarity = PWM_POLARITY_INVERTED; else return -EINVAL; diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 0ef808d925bb..38b7ed8ef913 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -16,13 +16,13 @@ struct pwm_chip; * @PWM_POLARITY_NORMAL: a high signal for the duration of the duty- * cycle, followed by a low signal for the remainder of the pulse * period - * @PWM_POLARITY_INVERSED: a low signal for the duration of the duty- + * @PWM_POLARITY_INVERTED: a low signal for the duration of the duty- * cycle, followed by a high signal for the remainder of the pulse * period */ enum pwm_polarity { PWM_POLARITY_NORMAL, - PWM_POLARITY_INVERSED, + PWM_POLARITY_INVERTED, }; /**
The polarity enum definition PWM_POLARITY_INVERSED is misspelled. Rename it to PWM_POLARITY_INVERTED. Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- drivers/pwm/core.c | 4 ++-- drivers/pwm/pwm-atmel-hlcdc.c | 2 +- drivers/pwm/pwm-atmel-tcb.c | 16 ++++++++-------- drivers/pwm/pwm-atmel.c | 2 +- drivers/pwm/pwm-bcm-iproc.c | 2 +- drivers/pwm/pwm-bcm-kona.c | 2 +- drivers/pwm/pwm-ep93xx.c | 2 +- drivers/pwm/pwm-fsl-ftm.c | 2 +- drivers/pwm/pwm-hibvt.c | 2 +- drivers/pwm/pwm-imx-tpm.c | 2 +- drivers/pwm/pwm-imx27.c | 4 ++-- drivers/pwm/pwm-jz4740.c | 2 +- drivers/pwm/pwm-meson.c | 6 +++--- drivers/pwm/pwm-omap-dmtimer.c | 4 ++-- drivers/pwm/pwm-renesas-tpu.c | 6 +++--- drivers/pwm/pwm-rockchip.c | 4 ++-- drivers/pwm/pwm-sifive.c | 4 ++-- drivers/pwm/pwm-sun4i.c | 2 +- drivers/pwm/pwm-tiecap.c | 2 +- drivers/pwm/pwm-tiehrpwm.c | 4 ++-- drivers/pwm/pwm-vt8500.c | 2 +- drivers/pwm/pwm-zx.c | 4 ++-- drivers/pwm/sysfs.c | 4 ++-- include/linux/pwm.h | 4 ++-- 24 files changed, 44 insertions(+), 44 deletions(-)