[PATCH v2 0/2] gpiolib: Fix gpio_lookup_flags mess and add Return sections

[PATCH v2 0/2] gpiolib: Fix gpio_lookup_flags mess and add Return sections

[Andy Shevchenko]

While reviewing another patch I have realised that enum
gpio_lookup_flags messes up with GPIO_* flags definitions.
Hence the first patch fix. Second one is an updated version
of adding Return sections to the kernel-doc.

The series is made in assumption that the patches will land in this
order, but if applied separately, e.g., one to for-current and one to
for-next branches, no conflicts should appear.

In v2:
- new patch 1
- replaces error-code by errno (Randy)
- added tags to patch 2 (Randy)

Andy Shevchenko (2):
  gpiolib: Fix a mess with the GPIO_* flags
  gpiolib: Update the kernel documentation - add Return sections

 drivers/gpio/gpiolib-acpi.c                   |  22 ++-
 drivers/gpio/gpiolib-cdev.c                   |   8 +-
 drivers/gpio/gpiolib-devres.c                 |  44 ++++-
 drivers/gpio/gpiolib-legacy.c                 |   3 +
 drivers/gpio/gpiolib-of.c                     |  53 ++++--
 drivers/gpio/gpiolib-swnode.c                 |   4 +-
 drivers/gpio/gpiolib-sysfs.c                  |   6 +-
 drivers/gpio/gpiolib.c                        | 165 ++++++++++++++----
 .../broadcom/brcm80211/brcmsmac/led.c         |   2 +-
 include/linux/gpio/driver.h                   |   3 +-
 include/linux/gpio/machine.h                  |  20 +--
 11 files changed, 245 insertions(+), 85 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

The GPIO_* flag definitions are *almost* duplicated in two files
(with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
on one set of definitions while the rest is on the other. Clean up
this mess by providing only one source of the definitions to all.

Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-of.c                     |  5 ++---
 drivers/gpio/gpiolib.c                        |  8 +++-----
 .../broadcom/brcm80211/brcmsmac/led.c         |  2 +-
 include/linux/gpio/driver.h                   |  3 +--
 include/linux/gpio/machine.h                  | 20 +++++--------------
 5 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index cb0cefaec37e..2f251b08173c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -434,7 +434,7 @@ int of_get_named_gpio(const struct device_node *np, const char *propname,
 }
 EXPORT_SYMBOL_GPL(of_get_named_gpio);
 
-/* Converts gpio_lookup_flags into bitmask of GPIO_* values */
+/* Converts of_gpio_flags into bitmask of GPIO_* values */
 static unsigned long of_convert_gpio_flags(enum of_gpio_flags flags)
 {
 	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
@@ -708,8 +708,7 @@ struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id,
  * @chip:	GPIO chip whose hog is parsed
  * @idx:	Index of the GPIO to parse
  * @name:	GPIO line name
- * @lflags:	bitmask of gpio_lookup_flags GPIO_* values - returned from
- *		of_find_gpio() or of_parse_own_gpio()
+ * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
  * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 69542c2a5b70..cb66506bebde 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2432,7 +2432,7 @@ static inline const char *function_name_or_default(const char *con_id)
 struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    unsigned int hwnum,
 					    const char *label,
-					    enum gpio_lookup_flags lflags,
+					    unsigned long lflags,
 					    enum gpiod_flags dflags)
 {
 	struct gpio_desc *desc = gpiochip_get_desc(gc, hwnum);
@@ -4348,8 +4348,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional);
  * gpiod_configure_flags - helper function to configure a given GPIO
  * @desc:	gpio whose value will be assigned
  * @con_id:	function within the GPIO consumer
- * @lflags:	bitmask of gpio_lookup_flags GPIO_* values - returned from
- *		of_find_gpio() or of_get_gpio_hog()
+ * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
  * Return 0 on success, -ENOENT if no GPIO has been assigned to the
@@ -4475,8 +4474,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index_optional);
  * gpiod_hog - Hog the specified GPIO desc given the provided flags
  * @desc:	gpio whose value will be assigned
  * @name:	gpio line name
- * @lflags:	bitmask of gpio_lookup_flags GPIO_* values - returned from
- *		of_find_gpio() or of_get_gpio_hog()
+ * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  */
 int gpiod_hog(struct gpio_desc *desc, const char *name,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
index 9540a05247c2..be07d9ba8283 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
@@ -60,8 +60,8 @@ int brcms_led_register(struct brcms_info *wl)
 		&sprom->gpio1,
 		&sprom->gpio2,
 		&sprom->gpio3 };
+	unsigned long lflags = GPIO_ACTIVE_HIGH;
 	int hwnum = -1;
-	enum gpio_lookup_flags lflags = GPIO_ACTIVE_HIGH;
 
 	/* find radio enabled LED */
 	for (i = 0; i < BRCMS_LED_NO; i++) {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index f8617eaf08ba..0c506c7485bd 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -31,7 +31,6 @@ struct gpio_chip;
 struct gpio_desc;
 struct gpio_device;
 
-enum gpio_lookup_flags;
 enum gpiod_flags;
 
 union gpio_irq_fwspec {
@@ -789,7 +788,7 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc)
 struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    unsigned int hwnum,
 					    const char *label,
-					    enum gpio_lookup_flags lflags,
+					    unsigned long lflags,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 44e5f162973e..8221ee91c6f2 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -2,21 +2,11 @@
 #ifndef __LINUX_GPIO_MACHINE_H
 #define __LINUX_GPIO_MACHINE_H
 
+#include <dt-bindings/gpio/gpio.h> /* for GPIO_* flags */
 #include <linux/types.h>
 
-enum gpio_lookup_flags {
-	GPIO_ACTIVE_HIGH		= (0 << 0),
-	GPIO_ACTIVE_LOW			= (1 << 0),
-	GPIO_OPEN_DRAIN			= (1 << 1),
-	GPIO_OPEN_SOURCE		= (1 << 2),
-	GPIO_PERSISTENT			= (0 << 3),
-	GPIO_TRANSITORY			= (1 << 3),
-	GPIO_PULL_UP			= (1 << 4),
-	GPIO_PULL_DOWN			= (1 << 5),
-	GPIO_PULL_DISABLE		= (1 << 6),
-
-	GPIO_LOOKUP_FLAGS_DEFAULT	= GPIO_ACTIVE_HIGH | GPIO_PERSISTENT,
-};
+/* Additional GPIO_* flags for internal use */
+#define GPIO_LOOKUP_FLAGS_DEFAULT	(GPIO_ACTIVE_HIGH | GPIO_PERSISTENT)
 
 /**
  * struct gpiod_lookup - lookup table
@@ -27,7 +17,7 @@ enum gpio_lookup_flags {
  *              U16_MAX to indicate that @key is a GPIO line name
  * @con_id: name of the GPIO from the device's point of view
  * @idx: index of the GPIO in case several GPIOs share the same name
- * @flags: bitmask of gpio_lookup_flags GPIO_* values
+ * @flags: bitmask of GPIO_* values
  *
  * gpiod_lookup is a lookup table for associating GPIOs to specific devices and
  * functions using platform data.
@@ -51,7 +41,7 @@ struct gpiod_lookup_table {
  * @chip_label: name of the chip the GPIO belongs to
  * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
  * @line_name: consumer name for the hogged line
- * @lflags: bitmask of gpio_lookup_flags GPIO_* values
+ * @lflags: bitmask of GPIO_* values
  * @dflags: GPIO flags used to specify the direction and value
  */
 struct gpiod_hog {
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on wireless-next/main wireless/main linus/master v6.9-rc3 next-20240408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/gpiolib-Fix-a-mess-with-the-GPIO_-flags/20240409-071911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240408231727.396452-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240409/202404091205.rnu1DOaq-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240409/202404091205.rnu1DOaq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404091205.rnu1DOaq-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/geode.h:12,
                    from arch/x86/platform/geode/alix.c:28:
>> include/linux/cs5535.h:149: warning: "GPIO_PULL_UP" redefined
     149 | #define GPIO_PULL_UP            0x18
         | 
   In file included from include/linux/gpio/machine.h:5,
                    from arch/x86/platform/geode/alix.c:25:
   include/dt-bindings/gpio/gpio.h:37: note: this is the location of the previous definition
      37 | #define GPIO_PULL_UP 16
         | 
>> include/linux/cs5535.h:150: warning: "GPIO_PULL_DOWN" redefined
     150 | #define GPIO_PULL_DOWN          0x1C
         | 
   include/dt-bindings/gpio/gpio.h:40: note: this is the location of the previous definition
      40 | #define GPIO_PULL_DOWN 32
         | 


vim +/GPIO_PULL_UP +149 include/linux/cs5535.h

f060f27007b393 Andres Salomon 2009-12-14  141  
5f0a96b044d8ed Andres Salomon 2009-12-14  142  /* GPIOs */
5f0a96b044d8ed Andres Salomon 2009-12-14  143  #define GPIO_OUTPUT_VAL		0x00
5f0a96b044d8ed Andres Salomon 2009-12-14  144  #define GPIO_OUTPUT_ENABLE	0x04
5f0a96b044d8ed Andres Salomon 2009-12-14  145  #define GPIO_OUTPUT_OPEN_DRAIN	0x08
5f0a96b044d8ed Andres Salomon 2009-12-14  146  #define GPIO_OUTPUT_INVERT	0x0C
5f0a96b044d8ed Andres Salomon 2009-12-14  147  #define GPIO_OUTPUT_AUX1	0x10
5f0a96b044d8ed Andres Salomon 2009-12-14  148  #define GPIO_OUTPUT_AUX2	0x14
5f0a96b044d8ed Andres Salomon 2009-12-14 @149  #define GPIO_PULL_UP		0x18
5f0a96b044d8ed Andres Salomon 2009-12-14 @150  #define GPIO_PULL_DOWN		0x1C
5f0a96b044d8ed Andres Salomon 2009-12-14  151  #define GPIO_INPUT_ENABLE	0x20
5f0a96b044d8ed Andres Salomon 2009-12-14  152  #define GPIO_INPUT_INVERT	0x24
5f0a96b044d8ed Andres Salomon 2009-12-14  153  #define GPIO_INPUT_FILTER	0x28
5f0a96b044d8ed Andres Salomon 2009-12-14  154  #define GPIO_INPUT_EVENT_COUNT	0x2C
5f0a96b044d8ed Andres Salomon 2009-12-14  155  #define GPIO_READ_BACK		0x30
5f0a96b044d8ed Andres Salomon 2009-12-14  156  #define GPIO_INPUT_AUX1		0x34
5f0a96b044d8ed Andres Salomon 2009-12-14  157  #define GPIO_EVENTS_ENABLE	0x38
5f0a96b044d8ed Andres Salomon 2009-12-14  158  #define GPIO_LOCK_ENABLE	0x3C
5f0a96b044d8ed Andres Salomon 2009-12-14  159  #define GPIO_POSITIVE_EDGE_EN	0x40
5f0a96b044d8ed Andres Salomon 2009-12-14  160  #define GPIO_NEGATIVE_EDGE_EN	0x44
5f0a96b044d8ed Andres Salomon 2009-12-14  161  #define GPIO_POSITIVE_EDGE_STS	0x48
5f0a96b044d8ed Andres Salomon 2009-12-14  162  #define GPIO_NEGATIVE_EDGE_STS	0x4C
5f0a96b044d8ed Andres Salomon 2009-12-14  163  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on wireless-next/main wireless/main linus/master v6.9-rc3 next-20240409]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/gpiolib-Fix-a-mess-with-the-GPIO_-flags/20240409-071911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240408231727.396452-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags
config: i386-buildonly-randconfig-002-20240409 (https://download.01.org/0day-ci/archive/20240409/202404091557.rf8NTu9B-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240409/202404091557.rf8NTu9B-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404091557.rf8NTu9B-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/x86/platform/geode/net5501.c:25:
   In file included from arch/x86/include/asm/geode.h:12:
>> include/linux/cs5535.h:149:9: warning: 'GPIO_PULL_UP' macro redefined [-Wmacro-redefined]
     149 | #define GPIO_PULL_UP            0x18
         |         ^
   include/dt-bindings/gpio/gpio.h:37:9: note: previous definition is here
      37 | #define GPIO_PULL_UP 16
         |         ^
   In file included from arch/x86/platform/geode/net5501.c:25:
   In file included from arch/x86/include/asm/geode.h:12:
>> include/linux/cs5535.h:150:9: warning: 'GPIO_PULL_DOWN' macro redefined [-Wmacro-redefined]
     150 | #define GPIO_PULL_DOWN          0x1C
         |         ^
   include/dt-bindings/gpio/gpio.h:40:9: note: previous definition is here
      40 | #define GPIO_PULL_DOWN 32
         |         ^
   2 warnings generated.


vim +/GPIO_PULL_UP +149 include/linux/cs5535.h

f060f27007b393 Andres Salomon 2009-12-14  141  
5f0a96b044d8ed Andres Salomon 2009-12-14  142  /* GPIOs */
5f0a96b044d8ed Andres Salomon 2009-12-14  143  #define GPIO_OUTPUT_VAL		0x00
5f0a96b044d8ed Andres Salomon 2009-12-14  144  #define GPIO_OUTPUT_ENABLE	0x04
5f0a96b044d8ed Andres Salomon 2009-12-14  145  #define GPIO_OUTPUT_OPEN_DRAIN	0x08
5f0a96b044d8ed Andres Salomon 2009-12-14  146  #define GPIO_OUTPUT_INVERT	0x0C
5f0a96b044d8ed Andres Salomon 2009-12-14  147  #define GPIO_OUTPUT_AUX1	0x10
5f0a96b044d8ed Andres Salomon 2009-12-14  148  #define GPIO_OUTPUT_AUX2	0x14
5f0a96b044d8ed Andres Salomon 2009-12-14 @149  #define GPIO_PULL_UP		0x18
5f0a96b044d8ed Andres Salomon 2009-12-14 @150  #define GPIO_PULL_DOWN		0x1C
5f0a96b044d8ed Andres Salomon 2009-12-14  151  #define GPIO_INPUT_ENABLE	0x20
5f0a96b044d8ed Andres Salomon 2009-12-14  152  #define GPIO_INPUT_INVERT	0x24
5f0a96b044d8ed Andres Salomon 2009-12-14  153  #define GPIO_INPUT_FILTER	0x28
5f0a96b044d8ed Andres Salomon 2009-12-14  154  #define GPIO_INPUT_EVENT_COUNT	0x2C
5f0a96b044d8ed Andres Salomon 2009-12-14  155  #define GPIO_READ_BACK		0x30
5f0a96b044d8ed Andres Salomon 2009-12-14  156  #define GPIO_INPUT_AUX1		0x34
5f0a96b044d8ed Andres Salomon 2009-12-14  157  #define GPIO_EVENTS_ENABLE	0x38
5f0a96b044d8ed Andres Salomon 2009-12-14  158  #define GPIO_LOCK_ENABLE	0x3C
5f0a96b044d8ed Andres Salomon 2009-12-14  159  #define GPIO_POSITIVE_EDGE_EN	0x40
5f0a96b044d8ed Andres Salomon 2009-12-14  160  #define GPIO_NEGATIVE_EDGE_EN	0x44
5f0a96b044d8ed Andres Salomon 2009-12-14  161  #define GPIO_POSITIVE_EDGE_STS	0x48
5f0a96b044d8ed Andres Salomon 2009-12-14  162  #define GPIO_NEGATIVE_EDGE_STS	0x4C
5f0a96b044d8ed Andres Salomon 2009-12-14  163  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The GPIO_* flag definitions are *almost* duplicated in two files
> (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> on one set of definitions while the rest is on the other. Clean up
> this mess by providing only one source of the definitions to all.
>
> Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-of.c                     |  5 ++---
>  drivers/gpio/gpiolib.c                        |  8 +++-----
>  .../broadcom/brcm80211/brcmsmac/led.c         |  2 +-
>  include/linux/gpio/driver.h                   |  3 +--
>  include/linux/gpio/machine.h                  | 20 +++++--------------
>  5 files changed, 12 insertions(+), 26 deletions(-)
>

I don't think ./dt-bindings/gpio/gpio.h is the right source of these
defines for everyone - including non-OF systems. I would prefer the
ones in include/linux/gpio/machine.h be the upstream source but then
headers in include/dt-bindings/ cannot include them so my second-best
suggestion is to rename the ones in include/linux/gpio/machine.h and
treewide too. In general values from ./dt-bindings/gpio/gpio.h should
only be used in DTS sources and gpiolib-of code.

Bart

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The GPIO_* flag definitions are *almost* duplicated in two files
> > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > on one set of definitions while the rest is on the other. Clean up
> > this mess by providing only one source of the definitions to all.
> >
> > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib-of.c                     |  5 ++---
> >  drivers/gpio/gpiolib.c                        |  8 +++-----
> >  .../broadcom/brcm80211/brcmsmac/led.c         |  2 +-
> >  include/linux/gpio/driver.h                   |  3 +--
> >  include/linux/gpio/machine.h                  | 20 +++++--------------
> >  5 files changed, 12 insertions(+), 26 deletions(-)
> 
> I don't think ./dt-bindings/gpio/gpio.h is the right source of these
> defines for everyone - including non-OF systems. I would prefer the
> ones in include/linux/gpio/machine.h be the upstream source but then
> headers in include/dt-bindings/ cannot include them so my second-best
> suggestion is to rename the ones in include/linux/gpio/machine.h and
> treewide too. In general values from ./dt-bindings/gpio/gpio.h should
> only be used in DTS sources and gpiolib-of code.

Then, please fix that your way. It's quite annoying issue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 2:51 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > The GPIO_* flag definitions are *almost* duplicated in two files
> > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > > on one set of definitions while the rest is on the other. Clean up
> > > this mess by providing only one source of the definitions to all.
> > >
> > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/gpio/gpiolib-of.c                     |  5 ++---
> > >  drivers/gpio/gpiolib.c                        |  8 +++-----
> > >  .../broadcom/brcm80211/brcmsmac/led.c         |  2 +-
> > >  include/linux/gpio/driver.h                   |  3 +--
> > >  include/linux/gpio/machine.h                  | 20 +++++--------------
> > >  5 files changed, 12 insertions(+), 26 deletions(-)
> >
> > I don't think ./dt-bindings/gpio/gpio.h is the right source of these
> > defines for everyone - including non-OF systems. I would prefer the
> > ones in include/linux/gpio/machine.h be the upstream source but then
> > headers in include/dt-bindings/ cannot include them so my second-best
> > suggestion is to rename the ones in include/linux/gpio/machine.h and
> > treewide too. In general values from ./dt-bindings/gpio/gpio.h should
> > only be used in DTS sources and gpiolib-of code.
>
> Then, please fix that your way. It's quite annoying issue.
>

This is not difficult in itself but it's a tree-wide change so we will
probably have to send it to Torvalds at the end of the merge window in
a separate pull-request.

I don't really have time now, I'll be travelling for 5 weeks in a row.
I'll see closer to the merge window. Or next release cycle.

Bart

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 02:55:20PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 2:51 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > The GPIO_* flag definitions are *almost* duplicated in two files
> > > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > > > on one set of definitions while the rest is on the other. Clean up
> > > > this mess by providing only one source of the definitions to all.
> > > >
> > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > >  drivers/gpio/gpiolib-of.c                     |  5 ++---
> > > >  drivers/gpio/gpiolib.c                        |  8 +++-----
> > > >  .../broadcom/brcm80211/brcmsmac/led.c         |  2 +-
> > > >  include/linux/gpio/driver.h                   |  3 +--
> > > >  include/linux/gpio/machine.h                  | 20 +++++--------------
> > > >  5 files changed, 12 insertions(+), 26 deletions(-)
> > >
> > > I don't think ./dt-bindings/gpio/gpio.h is the right source of these
> > > defines for everyone - including non-OF systems. I would prefer the
> > > ones in include/linux/gpio/machine.h be the upstream source but then
> > > headers in include/dt-bindings/ cannot include them so my second-best
> > > suggestion is to rename the ones in include/linux/gpio/machine.h and
> > > treewide too. In general values from ./dt-bindings/gpio/gpio.h should
> > > only be used in DTS sources and gpiolib-of code.
> >
> > Then, please fix that your way. It's quite annoying issue.
> 
> This is not difficult in itself

I'm not sure, what about enum gpio_lookup_flags? Shall we resurrect it?
I see that you have better vision anyway. Consider my patch as a problem
report (and as bonus you have already list of Fixes tags :-).

> but it's a tree-wide change so we will
> probably have to send it to Torvalds at the end of the merge window in
> a separate pull-request.

WFM!

> I don't really have time now, I'll be travelling for 5 weeks in a row.
> I'll see closer to the merge window. Or next release cycle.

But can you prioritize this? It's a carefully planted minefield with already
a bug and confusion here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Linus Walleij]

On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The GPIO_* flag definitions are *almost* duplicated in two files
> (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> on one set of definitions while the rest is on the other. Clean up
> this mess by providing only one source of the definitions to all.
>
> Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The way the line lookup flags ("lflags") were conceived was through
support for non-DT systems using descriptor tables, and that is how
enum gpio_lookup_flags came to be.

When OF support was added it was bolted on on the side, in essence
assuming that the DT/OF ABI was completely separate (and they/we
sure like to think about it that way...) and thus needed translation from
OF flags to kernel-internal enum gpio_lookup_flags.

The way *I* thought about this when writing it was certainly that the
DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist
at the time I think) and that translation from OF to kernel-internal
lflags would happen in *one* place.

The main reasoning still holds: the OF define is an ABI, so it can
*never* be changed, but the enum gpio_lookup_flags is subject to
Documentation/process/stable-api-nonsense.rst and that means
that if we want to swap around the order of the definitions we can.

But admittedly this is a bit over-belief in process and separation of
concerns and practical matters may be something else...

Yours,
Linus Walleij

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Fri, Apr 12, 2024 at 10:20:24AM +0200, Linus Walleij wrote:
> On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > The GPIO_* flag definitions are *almost* duplicated in two files
> > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > on one set of definitions while the rest is on the other. Clean up
> > this mess by providing only one source of the definitions to all.
> >
> > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> The way the line lookup flags ("lflags") were conceived was through
> support for non-DT systems using descriptor tables, and that is how
> enum gpio_lookup_flags came to be.
> 
> When OF support was added it was bolted on on the side, in essence
> assuming that the DT/OF ABI was completely separate (and they/we
> sure like to think about it that way...) and thus needed translation from
> OF flags to kernel-internal enum gpio_lookup_flags.
> 
> The way *I* thought about this when writing it was certainly that the
> DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist
> at the time I think) and that translation from OF to kernel-internal
> lflags would happen in *one* place.
> 
> The main reasoning still holds: the OF define is an ABI, so it can
> *never* be changed, but the enum gpio_lookup_flags is subject to
> Documentation/process/stable-api-nonsense.rst and that means
> that if we want to swap around the order of the definitions we can.
> 
> But admittedly this is a bit over-belief in process and separation of
> concerns and practical matters may be something else...

Got it. But we have a name clash and the mess added to the users.
I can redo this to separate these entities.

Note, that there is code in the kernel that *does* use
#include <dt-bindings/*.h>
for Linux internals.

$ git grep -lw '^#include <dt-bindings/.*\.h>' -- drivers/ | xargs dirname | cut -f 1,2 -d '/' | sort -u
drivers/bus
drivers/clk
drivers/clocksource
drivers/cpufreq
drivers/dma
drivers/firmware
drivers/gpio
drivers/gpu
drivers/hwtracing
drivers/i2c
drivers/iio
drivers/input
drivers/interconnect
drivers/iommu
drivers/irqchip
drivers/leds
drivers/mailbox
drivers/media
drivers/memory
drivers/mfd
drivers/net
drivers/phy
drivers/pinctrl
drivers/platform
drivers/pmdomain
drivers/power
drivers/pwm
drivers/regulator
drivers/remoteproc
drivers/reset
drivers/rtc
drivers/soc
drivers/spmi
drivers/thermal
drivers/tty
drivers/video
drivers/watchdog

P.S>
One of the patch this tries to fix is yours IIRC :-)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski]

On Fri, Apr 12, 2024 at 5:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Apr 12, 2024 at 10:20:24AM +0200, Linus Walleij wrote:
> > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > The GPIO_* flag definitions are *almost* duplicated in two files
> > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > > on one set of definitions while the rest is on the other. Clean up
> > > this mess by providing only one source of the definitions to all.
> > >
> > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > The way the line lookup flags ("lflags") were conceived was through
> > support for non-DT systems using descriptor tables, and that is how
> > enum gpio_lookup_flags came to be.
> >
> > When OF support was added it was bolted on on the side, in essence
> > assuming that the DT/OF ABI was completely separate (and they/we
> > sure like to think about it that way...) and thus needed translation from
> > OF flags to kernel-internal enum gpio_lookup_flags.
> >
> > The way *I* thought about this when writing it was certainly that the
> > DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist
> > at the time I think) and that translation from OF to kernel-internal
> > lflags would happen in *one* place.
> >
> > The main reasoning still holds: the OF define is an ABI, so it can
> > *never* be changed, but the enum gpio_lookup_flags is subject to
> > Documentation/process/stable-api-nonsense.rst and that means
> > that if we want to swap around the order of the definitions we can.
> >
> > But admittedly this is a bit over-belief in process and separation of
> > concerns and practical matters may be something else...
>
> Got it. But we have a name clash and the mess added to the users.
> I can redo this to separate these entities.
>
> Note, that there is code in the kernel that *does* use
> #include <dt-bindings/*.h>
> for Linux internals.
>

Well, then they are wrong. We should convert them to using
linux/gpio/machine.h first. Or even put these defines elsewhere
depending on what these drivers are using it for in general. Maybe
machine.h is not the right place. Then once that's figured out, we can
start renaming the constants.

IIUC include/dt-bindings/ headers should only be used by DT sources
and code that parses the OF properties.

But it seems to me that we need to inspect these users, we cannot just
automatically convert them at once, this is asking for trouble IMO.

Bart

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Linus Walleij]

On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> IIUC include/dt-bindings/ headers should only be used by DT sources
> and code that parses the OF properties.

That's what I have come to understand as well.

I wonder if there is something that can be done to enforce it?

Ideally the code that parses OF properties should have to
opt in to get access to the <dt-bindings/*> namespace.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote:
> On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > IIUC include/dt-bindings/ headers should only be used by DT sources
> > and code that parses the OF properties.
> 
> That's what I have come to understand as well.
> 
> I wonder if there is something that can be done to enforce it?
> 
> Ideally the code that parses OF properties should have to
> opt in to get access to the <dt-bindings/*> namespace.

Whatever you, guys, come up with as a solution, can it be fixed sooner than later?
I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't
need to look into this again.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski]

On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote:
> > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > IIUC include/dt-bindings/ headers should only be used by DT sources
> > > and code that parses the OF properties.
> >
> > That's what I have come to understand as well.
> >
> > I wonder if there is something that can be done to enforce it?
> >
> > Ideally the code that parses OF properties should have to
> > opt in to get access to the <dt-bindings/*> namespace.
>
> Whatever you, guys, come up with as a solution, can it be fixed sooner than later?
> I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't
> need to look into this again.
>

I'm not sure you got what I was saying. I don't think this can be
fixed quickly. This is just another bunch of technical debt that will
have to be addressed carefully on a case-by-case basis and run through
autobuilders in all possible configurations.

This type of include-related issues is always brittle and will lead to
build failures if we don't consider our moves.

Bart

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote:
> > > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > IIUC include/dt-bindings/ headers should only be used by DT sources
> > > > and code that parses the OF properties.
> > >
> > > That's what I have come to understand as well.
> > >
> > > I wonder if there is something that can be done to enforce it?
> > >
> > > Ideally the code that parses OF properties should have to
> > > opt in to get access to the <dt-bindings/*> namespace.
> >
> > Whatever you, guys, come up with as a solution, can it be fixed sooner than later?
> > I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't
> > need to look into this again.
> 
> I'm not sure you got what I was saying. I don't think this can be
> fixed quickly. This is just another bunch of technical debt that will
> have to be addressed carefully on a case-by-case basis and run through
> autobuilders in all possible configurations.
> 
> This type of include-related issues is always brittle and will lead to
> build failures if we don't consider our moves.

I proposed a quick fix which was rejected. I think this is still doable in a
few steps:

- align constant values in DT and enum
- drop usage of DT in the kernel code (That's what you want IIUC, however
  I disagree with this from technical perspective as DT constants can be used
  in the code as long as they are mapped 1:1 to what code does. That's current
  state of affairs. OTOH semantically this may be an issue.)
- restore enum usage treewide (?)

Again, the problem now is only in open source / open drain configurations
and there are only a few users of these flags _in kernel_. I do not see
why it can not be done in one or two evenings time range.

P.S>
Most of the time I spent when prepared the proposed fix is digging the history
and trying to understand how comes that we have desynchronisation of the values
over the time. The output of that is the list of Fixes tags.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski]

On Wed, Apr 17, 2024 at 10:45 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote:
> > > > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > > IIUC include/dt-bindings/ headers should only be used by DT sources
> > > > > and code that parses the OF properties.
> > > >
> > > > That's what I have come to understand as well.
> > > >
> > > > I wonder if there is something that can be done to enforce it?
> > > >
> > > > Ideally the code that parses OF properties should have to
> > > > opt in to get access to the <dt-bindings/*> namespace.
> > >
> > > Whatever you, guys, come up with as a solution, can it be fixed sooner than later?
> > > I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't
> > > need to look into this again.
> >
> > I'm not sure you got what I was saying. I don't think this can be
> > fixed quickly. This is just another bunch of technical debt that will
> > have to be addressed carefully on a case-by-case basis and run through
> > autobuilders in all possible configurations.
> >
> > This type of include-related issues is always brittle and will lead to
> > build failures if we don't consider our moves.
>
> I proposed a quick fix which was rejected. I think this is still doable in a
> few steps:
>

You having proposed a quick fix is not a reason for me to either apply
it immediately OR equally promptly provide a proper solution myself.

> - align constant values in DT and enum
> - drop usage of DT in the kernel code (That's what you want IIUC, however
>   I disagree with this from technical perspective as DT constants can be used
>   in the code as long as they are mapped 1:1 to what code does. That's current
>   state of affairs. OTOH semantically this may be an issue.)

It's against the convention of only using dt-bindings headers as I
described before. I disagree with your proposition and it seems to me
Linus backed me up on this.

> - restore enum usage treewide (?)
>
> Again, the problem now is only in open source / open drain configurations
> and there are only a few users of these flags _in kernel_. I do not see
> why it can not be done in one or two evenings time range.
>

So you know what needs doing. I'm at a conference now, I'll be off for
a week in April and I also have another conference scheduled for May.
If you believe this needs addressing urgently, then I suggest you do
it right. Otherwise, I'll get to it when I have the time.
Unfortunately my TODO list runneth over. :(

But I have to say, I suspect it won't be as easy as you present it
because we have so many build configs that may fail.

> P.S>
> Most of the time I spent when prepared the proposed fix is digging the history
> and trying to understand how comes that we have desynchronisation of the values
> over the time. The output of that is the list of Fixes tags.
>

Thank you, this history is useful and should make its way into git
history when we fix this issue.

Bart

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Wed, Apr 17, 2024 at 08:39:52PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 17, 2024 at 10:45 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote:

...

> > Again, the problem now is only in open source / open drain configurations
> > and there are only a few users of these flags _in kernel_. I do not see
> > why it can not be done in one or two evenings time range.
> 
> So you know what needs doing. I'm at a conference now, I'll be off for
> a week in April and I also have another conference scheduled for May.
> If you believe this needs addressing urgently, then I suggest you do
> it right. Otherwise, I'll get to it when I have the time.
> Unfortunately my TODO list runneth over. :(

I have started already as you may have noticed.

> But I have to say, I suspect it won't be as easy as you present it
> because we have so many build configs that may fail.

Let's see (with a hope)...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Linus Walleij]

On Wed, Apr 17, 2024 at 8:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Unfortunately my TODO list runneth over. :(

When in situations like this, patch the objective into
drivers/gpio/TODO so others can pick it up, that's why
I created the file, and it has actually helped a bit!

IMO you don't even need to send edits to this file for
review, it's just a work document. Just edit and commit
it in your tree.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Andy Shevchenko]

On Fri, Apr 19, 2024 at 03:29:13PM +0200, Linus Walleij wrote:
> On Wed, Apr 17, 2024 at 8:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > Unfortunately my TODO list runneth over. :(
> 
> When in situations like this, patch the objective into
> drivers/gpio/TODO so others can pick it up, that's why
> I created the file, and it has actually helped a bit!
> 
> IMO you don't even need to send edits to this file for
> review, it's just a work document. Just edit and commit
> it in your tree.

Btw, good point and thanks for the reminder, I believe I also can use it,
but in my case I probably need to send a formal patch :-)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Andy Shevchenko]

$ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
67

Fix these by adding Return sections. While at it, make sure all of
Return sections use the same style.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>
---
 drivers/gpio/gpiolib-acpi.c   |  22 +++--
 drivers/gpio/gpiolib-cdev.c   |   8 +-
 drivers/gpio/gpiolib-devres.c |  44 +++++++++-
 drivers/gpio/gpiolib-legacy.c |   3 +
 drivers/gpio/gpiolib-of.c     |  48 ++++++++---
 drivers/gpio/gpiolib-swnode.c |   4 +-
 drivers/gpio/gpiolib-sysfs.c  |   6 +-
 drivers/gpio/gpiolib.c        | 157 +++++++++++++++++++++++++++-------
 8 files changed, 233 insertions(+), 59 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 7f140df40f35..9aac44f0816d 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -136,8 +136,12 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, const void *data)
  * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
  * @pin:	ACPI GPIO pin number (0-based, controller-relative)
  *
- * Return: GPIO descriptor to use with Linux generic GPIO API, or ERR_PTR
- * error value. Specifically returns %-EPROBE_DEFER if the referenced GPIO
+ * Returns:
+ * GPIO descriptor to use with Linux generic GPIO API.
+ * If the GPIO cannot be translated or there is an error an ERR_PTR is
+ * returned.
+ *
+ * Specifically returns -EPROBE_DEFER if the referenced GPIO
  * controller does not have GPIO chip registered at the moment. This is to
  * support probe deferral.
  */
@@ -207,6 +211,9 @@ EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
  *			       I/O resource or return False if not.
  * @ares:	Pointer to the ACPI resource to fetch
  * @agpio:	Pointer to a &struct acpi_resource_gpio to store the output pointer
+ *
+ * Returns:
+ * true if GpioIo resource is found, false otherwise.
  */
 bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
 			       struct acpi_resource_gpio **agpio)
@@ -859,7 +866,9 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
  * that case @index is used to select the GPIO entry in the property value
  * (in case of multiple).
  *
- * If the GPIO cannot be translated or there is an error, an ERR_PTR is
+ * Returns:
+ * GPIO descriptor to use with Linux generic GPIO API.
+ * If the GPIO cannot be translated or there is an error an ERR_PTR is
  * returned.
  *
  * Note: if the GPIO resource has multiple entries in the pin list, this
@@ -910,6 +919,8 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
  * resource with the relevant information from a data-only ACPI firmware node
  * and uses that to obtain the GPIO descriptor to return.
  *
+ * Returns:
+ * GPIO descriptor to use with Linux generic GPIO API.
  * If the GPIO cannot be translated or there is an error an ERR_PTR is
  * returned.
  */
@@ -1023,7 +1034,8 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
  * The GPIO is considered wake capable if the GpioInt resource specifies
  * SharedAndWake or ExclusiveAndWake.
  *
- * Return: Linux IRQ number (> %0) on success, negative errno on failure.
+ * Returns:
+ * Linux IRQ number (> 0) on success, negative errno on failure.
  */
 int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, int index,
 				  bool *wake_capable)
@@ -1406,7 +1418,7 @@ static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
  * @fwnode:	firmware node of the GPIO consumer
  * @con_id:	function within the GPIO consumer
  *
- * Return:
+ * Returns:
  * The number of GPIOs associated with a firmware node / function or %-ENOENT,
  * if no GPIO has been assigned to the requested function.
  */
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d09c7d728365..23dad419cade 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2726,7 +2726,9 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
  * gpio_chrdev_open() - open the chardev for ioctl operations
  * @inode: inode for this chardev
  * @file: file struct for storing private data
- * Returns 0 on success
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int gpio_chrdev_open(struct inode *inode, struct file *file)
 {
@@ -2792,7 +2794,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
  * gpio_chrdev_release() - close chardev after ioctl operations
  * @inode: inode for this chardev
  * @file: file struct for storing private data
- * Returns 0 on success
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int gpio_chrdev_release(struct inode *inode, struct file *file)
 {
diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 4987e62dcb3d..8a0371678ed2 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -52,6 +52,11 @@ static int devm_gpiod_match_array(struct device *dev, void *res, void *data)
  * Managed gpiod_get(). GPIO descriptors returned from this function are
  * automatically disposed on driver detach. See gpiod_get() for detailed
  * information about behavior and return values.
+ *
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
 					      const char *con_id,
@@ -70,6 +75,11 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get);
  * Managed gpiod_get_optional(). GPIO descriptors returned from this function
  * are automatically disposed on driver detach. See gpiod_get_optional() for
  * detailed information about behavior and return values.
+ *
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, NULL if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check devm_gpiod_get_optional(struct device *dev,
 						       const char *con_id,
@@ -89,6 +99,11 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get_optional);
  * Managed gpiod_get_index(). GPIO descriptors returned from this function are
  * automatically disposed on driver detach. See gpiod_get_index() for detailed
  * information about behavior and return values.
+ *
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 						    const char *con_id,
@@ -141,8 +156,10 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get_index);
  * GPIO descriptors returned from this function are automatically disposed on
  * driver detach.
  *
- * On successful request the GPIO pin is configured in accordance with
- * provided @flags.
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
 					      struct fwnode_handle *fwnode,
@@ -182,6 +199,11 @@ EXPORT_SYMBOL_GPL(devm_fwnode_gpiod_get_index);
  * function are automatically disposed on driver detach. See
  * gpiod_get_index_optional() for detailed information about behavior and
  * return values.
+ *
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, NULL if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check devm_gpiod_get_index_optional(struct device *dev,
 							     const char *con_id,
@@ -207,6 +229,12 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get_index_optional);
  * Managed gpiod_get_array(). GPIO descriptors returned from this function are
  * automatically disposed on driver detach. See gpiod_get_array() for detailed
  * information about behavior and return values.
+ *
+ * Returns:
+ * The GPIO descriptors corresponding to the function @con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function,
+ * or another IS_ERR() code if an error occurred while trying to acquire
+ * the GPIOs.
  */
 struct gpio_descs *__must_check devm_gpiod_get_array(struct device *dev,
 						     const char *con_id,
@@ -243,6 +271,12 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get_array);
  * function are automatically disposed on driver detach.
  * See gpiod_get_array_optional() for detailed information about behavior and
  * return values.
+ *
+ * Returns:
+ * The GPIO descriptors corresponding to the function @con_id of device
+ * dev, NULL if no GPIO has been assigned to the requested function,
+ * or another IS_ERR() code if an error occurred while trying to acquire
+ * the GPIOs.
  */
 struct gpio_descs *__must_check
 devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
@@ -337,6 +371,9 @@ static void devm_gpio_release(struct device *dev, void *res)
  *      same arguments and performs the same function as
  *      gpio_request().  GPIOs requested with this function will be
  *      automatically freed on driver detach.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
 {
@@ -366,6 +403,9 @@ EXPORT_SYMBOL_GPL(devm_gpio_request);
  *	@gpio:	the GPIO number
  *	@flags:	GPIO configuration as specified by GPIOF_*
  *	@label:	a literal description string of this GPIO
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int devm_gpio_request_one(struct device *dev, unsigned gpio,
 			  unsigned long flags, const char *label)
diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index 616d60187f85..0774afa8abde 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -22,6 +22,9 @@ EXPORT_SYMBOL_GPL(gpio_free);
  * @label:	a literal description string of this GPIO
  *
  * **DEPRECATED** This function is deprecated and must not be used in new code.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 {
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2f251b08173c..495dc24275b9 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -46,16 +46,19 @@ enum of_gpio_flags {
  * @propname:	property name containing gpio specifier(s)
  *
  * The function returns the count of GPIOs specified for a node.
- * Note that the empty GPIO specifiers count too. Returns either
- *   Number of gpios defined in property,
- *   -EINVAL for an incorrectly formed gpios property, or
- *   -ENOENT for a missing gpios property
+ * NOTE: The empty GPIO specifiers count too.
  *
- * Example:
- * gpios = <0
- *          &gpio1 1 2
- *          0
- *          &gpio2 3 4>;
+ * Returns:
+ * Either number of GPIOs defined in the property, or
+ * *  -EINVAL for an incorrectly formed "gpios" property, or
+ * *  -ENOENT for a missing "gpios" property.
+ *
+ * Example::
+ *
+ *     gpios = <0
+ *              &gpio1 1 2
+ *              0
+ *              &gpio2 3 4>;
  *
  * The above example defines four GPIOs, two of which are not specified.
  * This function will return '4'
@@ -77,6 +80,11 @@ static int of_gpio_named_count(const struct device_node *np,
  * "gpios" for the chip select lines. If we detect this, we redirect
  * the counting of "cs-gpios" to count "gpios" transparent to the
  * driver.
+ *
+ * Returns:
+ * Either number of GPIOs defined in the property, or
+ * *  -EINVAL for an incorrectly formed "gpios" property, or
+ * *  -ENOENT for a missing "gpios" property.
  */
 static int of_gpio_spi_cs_get_count(const struct device_node *np,
 				    const char *con_id)
@@ -365,7 +373,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
  * @index:	index of the GPIO
  * @flags:	a flags pointer to fill in
  *
- * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
+ * Returns:
+ * GPIO descriptor to use with Linux GPIO API, or one of the errno
  * value on the error condition. If @flags is not NULL the function also fills
  * in flags for the GPIO.
  */
@@ -417,7 +426,8 @@ static struct gpio_desc *of_get_named_gpiod_flags(const struct device_node *np,
  *
  * **DEPRECATED** This function is deprecated and must not be used in new code.
  *
- * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
+ * Returns:
+ * GPIO number to use with Linux generic GPIO API, or one of the errno
  * value on the error condition.
  */
 int of_get_named_gpio(const struct device_node *np, const char *propname,
@@ -711,7 +721,8 @@ struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id,
  * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
- * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
+ * Returns:
+ * GPIO descriptor to use with Linux GPIO API, or one of the errno
  * value on the error condition.
  */
 static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
@@ -779,7 +790,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
  * @chip:	gpio chip to act on
  * @hog:	device node describing the hogs
  *
- * Returns error if it fails otherwise 0 on success.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
 {
@@ -813,7 +825,9 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
  *
  * This is only used by of_gpiochip_add to request/set GPIO initial
  * configuration.
- * It returns error if it fails otherwise 0 on success.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 {
@@ -926,6 +940,9 @@ struct notifier_block gpio_of_notifier = {
  * This is simple translation function, suitable for the most 1:1 mapped
  * GPIO chips. This function performs only one sanity check: whether GPIO
  * is less than ngpios (that is specified in the gpio_chip).
+ *
+ * Returns:
+ * GPIO number (>= 0) on success, negative errno on failure.
  */
 static int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
@@ -975,6 +992,9 @@ static int of_gpio_simple_xlate(struct gpio_chip *gc,
  * If succeeded, this function will map bank's memory and will
  * do all necessary work for you. Then you'll able to use .regs
  * to manage GPIOs from the callbacks.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int of_mm_gpiochip_add_data(struct device_node *np,
 			    struct of_mm_gpio_chip *mm_gc,
diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index fa52bdb1a29a..7298f1247dd3 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -96,8 +96,8 @@ struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
  *		system-global GPIOs
  * @con_id:	function within the GPIO consumer
  *
- * Return:
- * The number of GPIOs associated with a device / function or %-ENOENT,
+ * Returns:
+ * The number of GPIOs associated with a device / function or -ENOENT,
  * if no GPIO has been assigned to the requested function.
  */
 int swnode_gpio_count(const struct fwnode_handle *fwnode, const char *con_id)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6853ecd98bcb..772ad900ffac 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -568,7 +568,8 @@ static struct class gpio_class = {
  * will see "direction" sysfs attribute which may be used to change
  * the gpio's direction.  A "value" attribute will always be provided.
  *
- * Returns zero on success, else an error.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
@@ -667,7 +668,8 @@ static int match_export(struct device *dev, const void *desc)
  * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN
  * node. Caller is responsible for unlinking.
  *
- * Returns zero on success, else an error.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cb66506bebde..105029627d39 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -216,6 +216,9 @@ EXPORT_SYMBOL_GPL(desc_to_gpio);
  * This function is unsafe and should not be used. Using the chip address
  * without taking the SRCU read lock may result in dereferencing a dangling
  * pointer.
+ *
+ * Returns:
+ * Address of the GPIO chip backing this device.
  */
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
@@ -326,7 +329,8 @@ static int gpiochip_find_base_unlocked(int ngpio)
  * gpiod_get_direction - return the current direction of a GPIO
  * @desc:	GPIO to get the direction of
  *
- * Returns 0 for output, 1 for input, or an error code in case of error.
+ * Returns:
+ * 0 for output, 1 for input, or an error code in case of error.
  *
  * This function may sleep if gpiod_cansleep() is true.
  */
@@ -383,8 +387,8 @@ EXPORT_SYMBOL_GPL(gpiod_get_direction);
  * Add a new chip to the global chips list, keeping the list of chips sorted
  * by range(means [base, base + ngpio - 1]) order.
  *
- * Return -EBUSY if the new chip overlaps with some other chip's integer
- * space.
+ * Returns:
+ * -EBUSY if the new chip overlaps with some other chip's integer space.
  */
 static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
 {
@@ -1504,6 +1508,9 @@ static unsigned int gpiochip_child_offset_to_irq_noop(struct gpio_chip *gc,
  * This function is a wrapper that calls gpiochip_lock_as_irq() and is to be
  * used as the activate function for the &struct irq_domain_ops. The host_data
  * for the IRQ domain must be the &struct gpio_chip.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int gpiochip_irq_domain_activate(struct irq_domain *domain,
 					struct irq_data *data, bool reserve)
@@ -1648,6 +1655,9 @@ static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
  * This function will set up the mapping for a certain IRQ line on a
  * gpiochip by assigning the gpiochip as chip data, and using the irqchip
  * stored inside the gpiochip.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
 			    irq_hw_number_t hwirq)
@@ -1882,6 +1892,9 @@ static int gpiochip_irqchip_add_allocated_domain(struct gpio_chip *gc,
  * @gc: the GPIO chip to add the IRQ chip to
  * @lock_key: lockdep class for IRQ lock
  * @request_key: lockdep class for IRQ request
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
  */
 static int gpiochip_add_irqchip(struct gpio_chip *gc,
 				struct lock_class_key *lock_key,
@@ -2017,6 +2030,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)
  * @domain: the irqdomain to add to the gpiochip
  *
  * This function adds an IRQ domain to the gpiochip.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
 				struct irq_domain *domain)
@@ -2053,6 +2069,9 @@ static inline void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc)
  * gpiochip_generic_request() - request the gpio function for a pin
  * @gc: the gpiochip owning the GPIO
  * @offset: the offset of the GPIO to request for GPIO function
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiochip_generic_request(struct gpio_chip *gc, unsigned int offset)
 {
@@ -2086,6 +2105,9 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
  * @gc: the gpiochip owning the GPIO
  * @offset: the offset of the GPIO to apply the configuration
  * @config: the configuration to be applied
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config)
@@ -2112,6 +2134,9 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_config);
  * pinctrl driver is DEPRECATED. Please see Section 2.1 of
  * Documentation/devicetree/bindings/gpio/gpio.txt on how to
  * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,
@@ -2163,13 +2188,13 @@ EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
  * @npins: the number of pins from the offset of each pin space (GPIO and
  *	pin controller) to accumulate in this range
  *
- * Returns:
- * 0 on success, or a negative error-code on failure.
- *
  * Calling this function directly from a DeviceTree-supported
  * pinctrl driver is DEPRECATED. Please see Section 2.1 of
  * Documentation/devicetree/bindings/gpio/gpio.txt on how to
  * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
  */
 int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 			   unsigned int gpio_offset, unsigned int pin_offset,
@@ -2573,7 +2598,8 @@ static int gpio_set_bias(struct gpio_desc *desc)
  * The function calls the certain GPIO driver to set debounce timeout
  * in the hardware.
  *
- * Returns 0 on success, or negative error code otherwise.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 {
@@ -2589,7 +2615,8 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
  * Set the direction of the passed GPIO to input, such as gpiod_get_value() can
  * be called safely on it.
  *
- * Return 0 in case of success, else an error code.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_direction_input(struct gpio_desc *desc)
 {
@@ -2696,7 +2723,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
  * be called safely on it. The initial value of the output must be specified
  * as raw value on the physical line without regard for the ACTIVE_LOW status.
  *
- * Return 0 in case of success, else an error code.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
 {
@@ -2715,7 +2743,8 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
  * as the logical value of the GPIO, i.e. taking its ACTIVE_LOW status into
  * account.
  *
- * Return 0 in case of success, else an error code.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_direction_output(struct gpio_desc *desc, int value)
 {
@@ -2788,7 +2817,8 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
  * @desc: GPIO to enable.
  * @flags: Flags related to GPIO edge.
  *
- * Return 0 in case of success, else negative error code.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_enable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
@@ -2820,7 +2850,8 @@ EXPORT_SYMBOL_GPL(gpiod_enable_hw_timestamp_ns);
  * @desc: GPIO to disable.
  * @flags: Flags related to GPIO edge, same value as used during enable call.
  *
- * Return 0 in case of success, else negative error code.
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_disable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
@@ -2912,7 +2943,8 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
  * gpiod_is_active_low - test whether a GPIO is active-low or not
  * @desc: the gpio descriptor to test
  *
- * Returns 1 if the GPIO is active-low, 0 otherwise.
+ * Returns:
+ * 1 if the GPIO is active-low, 0 otherwise.
  */
 int gpiod_is_active_low(const struct gpio_desc *desc)
 {
@@ -3127,7 +3159,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
  * gpiod_get_raw_value() - return a gpio's raw value
  * @desc: gpio whose value will be returned
  *
- * Return the GPIO's raw value, i.e. the value of the physical line disregarding
+ * Returns:
+ * The GPIO's raw value, i.e. the value of the physical line disregarding
  * its ACTIVE_LOW status, or negative errno on failure.
  *
  * This function can be called from contexts where we cannot sleep, and will
@@ -3146,7 +3179,8 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
  * gpiod_get_value() - return a gpio's value
  * @desc: gpio whose value will be returned
  *
- * Return the GPIO's logical value, i.e. taking the ACTIVE_LOW status into
+ * Returns:
+ * The GPIO's logical value, i.e. taking the ACTIVE_LOW status into
  * account, or negative errno on failure.
  *
  * This function can be called from contexts where we cannot sleep, and will
@@ -3179,11 +3213,13 @@ EXPORT_SYMBOL_GPL(gpiod_get_value);
  * @value_bitmap: bitmap to store the read values
  *
  * Read the raw values of the GPIOs, i.e. the values of the physical lines
- * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
- * else an error code.
+ * without regard for their ACTIVE_LOW status.
  *
  * This function can be called from contexts where we cannot sleep,
  * and it will complain if the GPIO chip functions potentially sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_get_raw_array_value(unsigned int array_size,
 			      struct gpio_desc **desc_array,
@@ -3206,10 +3242,13 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_array_value);
  * @value_bitmap: bitmap to store the read values
  *
  * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
- * into account.  Return 0 in case of success, else an error code.
+ * into account.
  *
  * This function can be called from contexts where we cannot sleep,
  * and it will complain if the GPIO chip functions potentially sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_get_array_value(unsigned int array_size,
 			  struct gpio_desc **desc_array,
@@ -3497,6 +3536,9 @@ EXPORT_SYMBOL_GPL(gpiod_set_value);
  *
  * This function can be called from contexts where we cannot sleep, and will
  * complain if the GPIO chip functions potentially sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_set_raw_array_value(unsigned int array_size,
 			      struct gpio_desc **desc_array,
@@ -3522,6 +3564,9 @@ EXPORT_SYMBOL_GPL(gpiod_set_raw_array_value);
  *
  * This function can be called from contexts where we cannot sleep, and will
  * complain if the GPIO chip functions potentially sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_set_array_value(unsigned int array_size,
 			  struct gpio_desc **desc_array,
@@ -3540,6 +3585,8 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value);
  * gpiod_cansleep() - report whether gpio value access may sleep
  * @desc: gpio to check
  *
+ * Returns:
+ * 0 for non-sleepable, 1 for sleepable, or an error code in case of error.
  */
 int gpiod_cansleep(const struct gpio_desc *desc)
 {
@@ -3552,6 +3599,9 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
  * gpiod_set_consumer_name() - set the consumer name for the descriptor
  * @desc: gpio to set the consumer name on
  * @name: the new consumer name
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
 {
@@ -3565,8 +3615,8 @@ EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
  * gpiod_to_irq() - return the IRQ corresponding to a GPIO
  * @desc: gpio whose IRQ will be returned (already requested)
  *
- * Return the IRQ corresponding to the passed GPIO, or an error code in case of
- * error.
+ * Returns:
+ * The IRQ corresponding to the passed GPIO, or an error code in case of error.
  */
 int gpiod_to_irq(const struct gpio_desc *desc)
 {
@@ -3620,6 +3670,9 @@ EXPORT_SYMBOL_GPL(gpiod_to_irq);
  *
  * This is used directly by GPIO drivers that want to lock down
  * a certain GPIO line to be used for IRQs.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
 {
@@ -3771,7 +3824,8 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent);
  * gpiod_get_raw_value_cansleep() - return a gpio's raw value
  * @desc: gpio whose value will be returned
  *
- * Return the GPIO's raw value, i.e. the value of the physical line disregarding
+ * Returns:
+ * The GPIO's raw value, i.e. the value of the physical line disregarding
  * its ACTIVE_LOW status, or negative errno on failure.
  *
  * This function is to be called from contexts that can sleep.
@@ -3788,7 +3842,8 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_value_cansleep);
  * gpiod_get_value_cansleep() - return a gpio's value
  * @desc: gpio whose value will be returned
  *
- * Return the GPIO's logical value, i.e. taking the ACTIVE_LOW status into
+ * Returns:
+ * The GPIO's logical value, i.e. taking the ACTIVE_LOW status into
  * account, or negative errno on failure.
  *
  * This function is to be called from contexts that can sleep.
@@ -3818,10 +3873,12 @@ EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
  * @value_bitmap: bitmap to store the read values
  *
  * Read the raw values of the GPIOs, i.e. the values of the physical lines
- * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
- * else an error code.
+ * without regard for their ACTIVE_LOW status.
  *
  * This function is to be called from contexts that can sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
 				       struct gpio_desc **desc_array,
@@ -3845,9 +3902,12 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_array_value_cansleep);
  * @value_bitmap: bitmap to store the read values
  *
  * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
- * into account.  Return 0 in case of success, else an error code.
+ * into account.
  *
  * This function is to be called from contexts that can sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_get_array_value_cansleep(unsigned int array_size,
 				   struct gpio_desc **desc_array,
@@ -3910,6 +3970,9 @@ EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
  * without regard for their ACTIVE_LOW status.
  *
  * This function is to be called from contexts that can sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 				       struct gpio_desc **desc_array,
@@ -3952,6 +4015,9 @@ void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n)
  * into account.
  *
  * This function is to be called from contexts that can sleep.
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_set_array_value_cansleep(unsigned int array_size,
 				   struct gpio_desc **desc_array,
@@ -4285,9 +4351,12 @@ EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
 
 /**
  * gpiod_count - return the number of GPIOs associated with a device / function
- *		or -ENOENT if no GPIO has been assigned to the requested function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
+ *
+ * Returns:
+ * The number of GPIOs associated with a device / function or -ENOENT if no
+ * GPIO has been assigned to the requested function.
  */
 int gpiod_count(struct device *dev, const char *con_id)
 {
@@ -4314,7 +4383,8 @@ EXPORT_SYMBOL_GPL(gpiod_count);
  * @con_id:	function within the GPIO consumer
  * @flags:	optional GPIO initialization flags
  *
- * Return the GPIO descriptor corresponding to the function con_id of device
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
  * dev, -ENOENT if no GPIO has been assigned to the requested function, or
  * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
@@ -4334,6 +4404,11 @@ EXPORT_SYMBOL_GPL(gpiod_get);
  * This is equivalent to gpiod_get(), except that when no GPIO was assigned to
  * the requested function it will return NULL. This is convenient for drivers
  * that need to handle optional GPIOs.
+ *
+ * Returns:
+ * The GPIO descriptor corresponding to the function @con_id of device
+ * dev, NULL if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
 						  const char *con_id,
@@ -4351,7 +4426,8 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional);
  * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
- * Return 0 on success, -ENOENT if no GPIO has been assigned to the
+ * Returns:
+ * 0 on success, -ENOENT if no GPIO has been assigned to the
  * requested function and/or index, or another IS_ERR() code if an error
  * occurred while trying to acquire the GPIO.
  */
@@ -4426,7 +4502,8 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
  * This variant of gpiod_get() allows to access GPIOs other than the first
  * defined one for functions that define several GPIOs.
  *
- * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
+ * Returns:
+ * A valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
  * requested function and/or index, or another IS_ERR() code if an error
  * occurred while trying to acquire the GPIO.
  */
@@ -4454,6 +4531,11 @@ EXPORT_SYMBOL_GPL(gpiod_get_index);
  * This is equivalent to gpiod_get_index(), except that when no GPIO with the
  * specified index was assigned to the requested function it will return NULL.
  * This is convenient for drivers that need to handle optional GPIOs.
+ *
+ * Returns:
+ * A valid GPIO descriptor, NULL if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occurred while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check gpiod_get_index_optional(struct device *dev,
 							const char *con_id,
@@ -4476,6 +4558,9 @@ EXPORT_SYMBOL_GPL(gpiod_get_index_optional);
  * @name:	gpio line name
  * @lflags:	bitmask of GPIO_* values - returned from *_find_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * Returns:
+ * 0 on success, or negative errno on failure.
  */
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 	      unsigned long lflags, enum gpiod_flags dflags)
@@ -4532,9 +4617,11 @@ static void gpiochip_free_hogs(struct gpio_chip *gc)
  *
  * This function acquires all the GPIOs defined under a given function.
  *
- * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
- * no GPIO has been assigned to the requested function, or another IS_ERR()
- * code if an error occurred while trying to acquire the GPIOs.
+ * Returns:
+ * The GPIO descriptors corresponding to the function @con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function,
+ * or another IS_ERR() code if an error occurred while trying to acquire
+ * the GPIOs.
  */
 struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 						const char *con_id,
@@ -4660,6 +4747,12 @@ EXPORT_SYMBOL_GPL(gpiod_get_array);
  *
  * This is equivalent to gpiod_get_array(), except that when no GPIO was
  * assigned to the requested function it will return NULL.
+ *
+ * Returns:
+ * The GPIO descriptors corresponding to the function @con_id of device
+ * dev, NULL if no GPIO has been assigned to the requested function,
+ * or another IS_ERR() code if an error occurred while trying to acquire
+ * the GPIOs.
  */
 struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
 							const char *con_id,
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 02:12:51AM +0300, Andy Shevchenko wrote:
> $ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
> 67
> 
> Fix these by adding Return sections. While at it, make sure all of
> Return sections use the same style.

Since there shouldn't be hard dependency to the first one, can you consider
applying this one, so it unblocks me?

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 2:52 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 09, 2024 at 02:12:51AM +0300, Andy Shevchenko wrote:
> > $ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
> > 67
> >
> > Fix these by adding Return sections. While at it, make sure all of
> > Return sections use the same style.
>
> Since there shouldn't be hard dependency to the first one, can you consider
> applying this one, so it unblocks me?
>

I'm not sure what the resolution is for % and HTML <font> tags in the end?

Bart

Re: [PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 04:01:43PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 2:52 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Apr 09, 2024 at 02:12:51AM +0300, Andy Shevchenko wrote:
> > > $ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
> > > 67
> > >
> > > Fix these by adding Return sections. While at it, make sure all of
> > > Return sections use the same style.
> >
> > Since there shouldn't be hard dependency to the first one, can you consider
> > applying this one, so it unblocks me?
> 
> I'm not sure what the resolution is for % and HTML <font> tags in the end?

Most of the constants are without %, so less churn now is to drop %.
If you think otherwise, please, fix it and I will rebase my patches later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 4:06 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 09, 2024 at 04:01:43PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 9, 2024 at 2:52 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Apr 09, 2024 at 02:12:51AM +0300, Andy Shevchenko wrote:
> > > > $ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
> > > > 67
> > > >
> > > > Fix these by adding Return sections. While at it, make sure all of
> > > > Return sections use the same style.
> > >
> > > Since there shouldn't be hard dependency to the first one, can you consider
> > > applying this one, so it unblocks me?
> >
> > I'm not sure what the resolution is for % and HTML <font> tags in the end?
>
> Most of the constants are without %, so less churn now is to drop %.
> If you think otherwise, please, fix it and I will rebase my patches later.
>

I'm not sure I get the logic of it. If the kernel-wide standard is to
use %, then we should work towards using it across the GPIO code even
if we do it a few lines at a time instead of going backwards just for
consistency in drivers/gpio/, no? We don't need to fix everything now
but if you're touching this code, then I'd go with %.

Also: what about the s/error-code/error code/g issue? While we should
always say "active-low", I think error code looks better as two words.

Bart

Re: [PATCH v2 2/2] gpiolib: Update the kernel documentation - add Return sections

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 04:18:46PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 4:06 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Apr 09, 2024 at 04:01:43PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Apr 9, 2024 at 2:52 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Apr 09, 2024 at 02:12:51AM +0300, Andy Shevchenko wrote:
> > > > > $ scripts/kernel-doc -v -none -Wall drivers/gpio/gpiolib* 2>&1 | grep -w warning | wc -l
> > > > > 67
> > > > >
> > > > > Fix these by adding Return sections. While at it, make sure all of
> > > > > Return sections use the same style.
> > > >
> > > > Since there shouldn't be hard dependency to the first one, can you consider
> > > > applying this one, so it unblocks me?
> > >
> > > I'm not sure what the resolution is for % and HTML <font> tags in the end?
> >
> > Most of the constants are without %, so less churn now is to drop %.
> > If you think otherwise, please, fix it and I will rebase my patches later.
> >
> 
> I'm not sure I get the logic of it. If the kernel-wide standard is to
> use %, then we should work towards using it across the GPIO code even
> if we do it a few lines at a time instead of going backwards just for
> consistency in drivers/gpio/, no? We don't need to fix everything now
> but if you're touching this code, then I'd go with %.
> 
> Also: what about the s/error-code/error code/g issue? While we should
> always say "active-low", I think error code looks better as two words.

I also have no much time for these details. :(
Let's drop this series then. Feel free to consider this as a problem report.

-- 
With Best Regards,
Andy Shevchenko