| Message ID | 20200317123231.2843297-4-oleksandr.suvorov@toradex.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Tue, 17 Mar 2020 14:32:27 +0200 Oleksandr Suvorov <oleksandr.suvorov@toradex.com> wrote: > PWM can have a normal polarity and a reverted one. The reverted polarity > value is defined. > Define the PWM_POLARITY_NORMAL to be used further. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> s/reverted/inverted/
On Tue, Mar 17, 2020 at 3:37 PM Paul Barker <pbarker@konsulko.com> wrote: > > On Tue, 17 Mar 2020 14:32:27 +0200 > Oleksandr Suvorov <oleksandr.suvorov@toradex.com> wrote: > > > PWM can have a normal polarity and a reverted one. The reverted polarity > > value is defined. > > Define the PWM_POLARITY_NORMAL to be used further. > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > s/reverted/inverted/ Thank you, Paul, for so fast review! I'll fix it in the next version of the patchset. > > -- > Paul Barker > Konsulko Group
On Tue, Mar 17, 2020 at 02:32:27PM +0200, Oleksandr Suvorov wrote: > PWM can have a normal polarity and a reverted one. The reverted polarity > value is defined. > Define the PWM_POLARITY_NORMAL to be used further. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > --- > > include/dt-bindings/pwm/pwm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h > index ab9a077e3c7d..6b58caa6385e 100644 > --- a/include/dt-bindings/pwm/pwm.h > +++ b/include/dt-bindings/pwm/pwm.h > @@ -10,6 +10,7 @@ > #ifndef _DT_BINDINGS_PWM_PWM_H > #define _DT_BINDINGS_PWM_PWM_H > > +#define PWM_POLARITY_NORMAL 0 > #define PWM_POLARITY_INVERTED (1 << 0) Maybe define PWM_POLARITY_NORMAL as (0 << 0) to make it more obvious that it is the inverse of PWM_POLARITY_INVERTED? But even when kept as is I like hafing PWM_POLARITY_NORMAL in the binding definitions. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
Hi Oleksandr, Thank you for the patch. On Tue, Mar 17, 2020 at 02:32:27PM +0200, Oleksandr Suvorov wrote: > PWM can have a normal polarity and a reverted one. The reverted polarity > value is defined. As mentioned by Paul, I'd use "inverted" instead of "reverted". Your patch series is trying to standardized on "inverted", let's not add another term :-) I would squash this patch with 2/7, apart from that it looks fine. However, I also agree with Thierry that the PWM cell that contains this value is a bitmask, so once we get more flags it may get a bit awkward. Will we have one macro for each flag that will evaluate to 0 to report that the flag isn't set ? Or should we define a single PWM_FLAG_NONE (or similarly named) macro ? In retrospect, maybe PWM_POLARITY_INVERTED should have been named PWM_FLAG_POLARITY_INVERTED. > Define the PWM_POLARITY_NORMAL to be used further. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > --- > > include/dt-bindings/pwm/pwm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h > index ab9a077e3c7d..6b58caa6385e 100644 > --- a/include/dt-bindings/pwm/pwm.h > +++ b/include/dt-bindings/pwm/pwm.h > @@ -10,6 +10,7 @@ > #ifndef _DT_BINDINGS_PWM_PWM_H > #define _DT_BINDINGS_PWM_PWM_H > > +#define PWM_POLARITY_NORMAL 0 > #define PWM_POLARITY_INVERTED (1 << 0) > > #endif
Hello Laurent, On Wed, Mar 18, 2020 at 12:56:56AM +0200, Laurent Pinchart wrote: > On Tue, Mar 17, 2020 at 02:32:27PM +0200, Oleksandr Suvorov wrote: > > PWM can have a normal polarity and a reverted one. The reverted polarity > > value is defined. > > I would squash this patch with 2/7, apart from that it looks fine. > However, I also agree with Thierry that the PWM cell that contains this > value is a bitmask, so once we get more flags it may get a bit awkward. For me the usefulness of PWM_POLARITY_NORMAL increases with more bits used. That's because if there are 5 things that can be set there and the patch author mentions only the two that are non-zero, I as a reviewer don't know if the author actually know and thought about the other three. If however they spell out PWM_POLARITY_NORMAL it's quite sure they want normal polarity. > Will we have one macro for each flag that will evaluate to 0 to report > that the flag isn't set ? Yes. Given the above mentioned advantage this is cheap enough in my eyes. > Or should we define a single PWM_FLAG_NONE (or > similarly named) macro ? I like one macro for each bit field better for the above mentioned reason. > In retrospect, maybe PWM_POLARITY_INVERTED > should have been named PWM_FLAG_POLARITY_INVERTED. Seems to be subjective. I don't see much added semantic that justifies the longer name. Best regards Uwe
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h index ab9a077e3c7d..6b58caa6385e 100644 --- a/include/dt-bindings/pwm/pwm.h +++ b/include/dt-bindings/pwm/pwm.h @@ -10,6 +10,7 @@ #ifndef _DT_BINDINGS_PWM_PWM_H #define _DT_BINDINGS_PWM_PWM_H +#define PWM_POLARITY_NORMAL 0 #define PWM_POLARITY_INVERTED (1 << 0) #endif
PWM can have a normal polarity and a reverted one. The reverted polarity value is defined. Define the PWM_POLARITY_NORMAL to be used further. Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- include/dt-bindings/pwm/pwm.h | 1 + 1 file changed, 1 insertion(+)