| Message ID | 20190402105736.80062-3-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/7] gpiolib: Indent entry values of enum gpio_lookup_flags | expand |
On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > Most of the code inside GPIO library is using enum gpio_lookup_flags. > Some of the function still operate with unsigned long. > > In order to be more consistent and better type checking, convert > gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags > instead of unsigned long. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 4 ++-- > drivers/gpio/gpiolib.h | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index a3fe41a2c6c0..ea0d38164b06 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); > * occurred while trying to acquire the GPIO. > */ > int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, > - unsigned long lflags, enum gpiod_flags dflags) > + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) I actually think unsigned long here is more correct because enum is supposed to present a single enumerated value and here we pass several of them as bitmask.
On Wed, Apr 03, 2019 at 09:36:24PM +0300, Mika Westerberg wrote: > On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > > Most of the code inside GPIO library is using enum gpio_lookup_flags. > > Some of the function still operate with unsigned long. > > > > In order to be more consistent and better type checking, convert > > gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags > > instead of unsigned long. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/gpio/gpiolib.c | 4 ++-- > > drivers/gpio/gpiolib.h | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index a3fe41a2c6c0..ea0d38164b06 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); > > * occurred while trying to acquire the GPIO. > > */ > > int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, > > - unsigned long lflags, enum gpiod_flags dflags) > > + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) > > I actually think unsigned long here is more correct because enum is > supposed to present a single enumerated value and here we pass several of > them as bitmask. There are more functions that are using enum to keep bit mask, for example, gpiochip_request_own_desc(). Are you suggesting to change them all to unsigned long?
On Thu, Apr 04, 2019 at 11:37:35AM +0300, Andy Shevchenko wrote: > On Wed, Apr 03, 2019 at 09:36:24PM +0300, Mika Westerberg wrote: > > On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > > > Most of the code inside GPIO library is using enum gpio_lookup_flags. > > > Some of the function still operate with unsigned long. > > > > > > In order to be more consistent and better type checking, convert > > > gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags > > > instead of unsigned long. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/gpio/gpiolib.c | 4 ++-- > > > drivers/gpio/gpiolib.h | 4 ++-- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index a3fe41a2c6c0..ea0d38164b06 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); > > > * occurred while trying to acquire the GPIO. > > > */ > > > int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, > > > - unsigned long lflags, enum gpiod_flags dflags) > > > + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) > > > > I actually think unsigned long here is more correct because enum is > > supposed to present a single enumerated value and here we pass several of > > them as bitmask. > > There are more functions that are using enum to keep bit mask, for example, > gpiochip_request_own_desc(). Are you suggesting to change them all to unsigned > long? No just wanted to mention it. For example enum gpiod_flags used by gpiochip_request_own_desc() encodes bitmask as part of the enumeration value which is fine but enum gpio_lookup_flags on the other hand does not. So for the latter I would prefer to use unsigned int/long instead. Up to you really. I'm fine either way :)
On Thu, Apr 04, 2019 at 11:52:20AM +0300, Mika Westerberg wrote: > On Thu, Apr 04, 2019 at 11:37:35AM +0300, Andy Shevchenko wrote: > > On Wed, Apr 03, 2019 at 09:36:24PM +0300, Mika Westerberg wrote: > > > On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > > > > Most of the code inside GPIO library is using enum gpio_lookup_flags. > > > > Some of the function still operate with unsigned long. > > > > > > > > In order to be more consistent and better type checking, convert > > > > gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags > > > > instead of unsigned long. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > drivers/gpio/gpiolib.c | 4 ++-- > > > > drivers/gpio/gpiolib.h | 4 ++-- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > index a3fe41a2c6c0..ea0d38164b06 100644 > > > > --- a/drivers/gpio/gpiolib.c > > > > +++ b/drivers/gpio/gpiolib.c > > > > @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); > > > > * occurred while trying to acquire the GPIO. > > > > */ > > > > int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, > > > > - unsigned long lflags, enum gpiod_flags dflags) > > > > + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) > > > > > > I actually think unsigned long here is more correct because enum is > > > supposed to present a single enumerated value and here we pass several of > > > them as bitmask. > > > > There are more functions that are using enum to keep bit mask, for example, > > gpiochip_request_own_desc(). Are you suggesting to change them all to unsigned > > long? > > No just wanted to mention it. For example enum gpiod_flags used by > gpiochip_request_own_desc() encodes bitmask as part of the enumeration > value which is fine but enum gpio_lookup_flags on the other hand does > not. So for the latter I would prefer to use unsigned int/long instead. Ah, okay, so, basically using enum gpio_lookup_flags as a parameter to functions doesn't feel right. Thank you for a comment. > Up to you really. I'm fine either way :)
On Thu, Apr 4, 2019 at 4:22 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:> > On Thu, Apr 04, 2019 at 11:52:20AM +0300, Mika Westerberg wrote: > > On Thu, Apr 04, 2019 at 11:37:35AM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 03, 2019 at 09:36:24PM +0300, Mika Westerberg wrote: > > > > On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > > > > > Most of the code inside GPIO library is using enum gpio_lookup_flags. > > > > > Some of the function still operate with unsigned long. > > > > > > > > > > In order to be more consistent and better type checking, convert > > > > > gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags > > > > > instead of unsigned long. > > > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > --- > > > > > drivers/gpio/gpiolib.c | 4 ++-- > > > > > drivers/gpio/gpiolib.h | 4 ++-- > > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > index a3fe41a2c6c0..ea0d38164b06 100644 > > > > > --- a/drivers/gpio/gpiolib.c > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); > > > > > * occurred while trying to acquire the GPIO. > > > > > */ > > > > > int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, > > > > > - unsigned long lflags, enum gpiod_flags dflags) > > > > > + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) > > > > > > > > I actually think unsigned long here is more correct because enum is > > > > supposed to present a single enumerated value and here we pass several of > > > > them as bitmask. > > > > > > There are more functions that are using enum to keep bit mask, for example, > > > gpiochip_request_own_desc(). Are you suggesting to change them all to unsigned > > > long? > > > > No just wanted to mention it. For example enum gpiod_flags used by > > gpiochip_request_own_desc() encodes bitmask as part of the enumeration > > value which is fine but enum gpio_lookup_flags on the other hand does > > not. So for the latter I would prefer to use unsigned int/long instead. > > Ah, okay, so, basically using enum gpio_lookup_flags as a parameter to > functions doesn't feel right. The enum gpio_lookup_flags is essentially something I came up with for <linux/gpio/machine.h> to use used by struct gpiod_lookup. It looks like it does (not just 0, 1, 2, 3... ) because it mirrors the defines for e.g. <linux/gpio.h> so if we can get rid of that legacy file we can change the values for enum gpio_lookup_flags. Maybe it was stupid of me. We could change the enum gpio_lookup_flags to just #define GPIO_ACTIVE_HIGH 0 #define GPIO_ACTIVE_LOW BIT(0) etc. And then just have some unsigned long in struct gpiod_lookup. What do you folks think makes most sense? Yours, Linus Walleij
On Thu, Apr 04, 2019 at 11:16:19PM +0700, Linus Walleij wrote: > The enum gpio_lookup_flags is essentially something I came up with > for <linux/gpio/machine.h> to use used by struct gpiod_lookup. > > It looks like it does (not just 0, 1, 2, 3... ) because it mirrors the defines > for e.g. <linux/gpio.h> so if we can get rid of that legacy file we can > change the values for enum gpio_lookup_flags. > > Maybe it was stupid of me. We could change the enum gpio_lookup_flags > to just > #define GPIO_ACTIVE_HIGH 0 > #define GPIO_ACTIVE_LOW BIT(0) > etc. > > And then just have some unsigned long in struct gpiod_lookup. > > What do you folks think makes most sense? I think it is fine to have enum gpio_lookup_flags declaring the constants: enum gpio_lookup_flags { GPIO_ACTIVE_HIGH = (0 << 0), GPIO_ACTIVE_LOW = (1 << 0), ... But when we pass combination (bitmask) of those constants to a function, say gpiod_hog(), IMHO the right type is unsigned int/long as it is already using: int gpiod_hog(struct gpio_desc *desc, const char *name, unsigned long lflags, enum gpiod_flags dflags) If it takes only a single enumerated value then using enum gpio_lookup_flags would be proper option. Same goes of course with variables and members of a struct.
On Thu, Apr 04, 2019 at 11:16:19PM +0700, Linus Walleij wrote: > On Thu, Apr 4, 2019 at 4:22 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote:> > > On Thu, Apr 04, 2019 at 11:52:20AM +0300, Mika Westerberg wrote: > > > On Thu, Apr 04, 2019 at 11:37:35AM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 03, 2019 at 09:36:24PM +0300, Mika Westerberg wrote: > > > > > On Tue, Apr 02, 2019 at 01:57:32PM +0300, Andy Shevchenko wrote: > Maybe it was stupid of me. We could change the enum gpio_lookup_flags > to just > #define GPIO_ACTIVE_HIGH 0 > #define GPIO_ACTIVE_LOW BIT(0) > etc. > > And then just have some unsigned long in struct gpiod_lookup. > > What do you folks think makes most sense? But it's not simple that, it also assumes persistent state.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index a3fe41a2c6c0..ea0d38164b06 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4078,7 +4078,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); * occurred while trying to acquire the GPIO. */ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, - unsigned long lflags, enum gpiod_flags dflags) + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) { int status; @@ -4397,7 +4397,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index_optional); * @dflags: gpiod_flags - optional GPIO initialization flags */ int gpiod_hog(struct gpio_desc *desc, const char *name, - unsigned long lflags, enum gpiod_flags dflags) + enum gpio_lookup_flags lflags, enum gpiod_flags dflags) { struct gpio_chip *chip; struct gpio_desc *local_desc; diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 078ab17b96bf..81061e5f9b22 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -231,9 +231,9 @@ struct gpio_desc { int gpiod_request(struct gpio_desc *desc, const char *label); void gpiod_free(struct gpio_desc *desc); int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, - unsigned long lflags, enum gpiod_flags dflags); + enum gpio_lookup_flags lflags, enum gpiod_flags dflags); int gpiod_hog(struct gpio_desc *desc, const char *name, - unsigned long lflags, enum gpiod_flags dflags); + enum gpio_lookup_flags lflags, enum gpiod_flags dflags); /* * Return the GPIO number of the passed descriptor relative to its chip
Most of the code inside GPIO library is using enum gpio_lookup_flags. Some of the function still operate with unsigned long. In order to be more consistent and better type checking, convert gpiod_hog() and gpiod_configure_flags() to use enum gpio_lookup_flags instead of unsigned long. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 4 ++-- drivers/gpio/gpiolib.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)