[PATCH v5 00/12] Add pinctrl support for the AAEON UP board FPGA

[PATCH v5 00/12] Add pinctrl support for the AAEON UP board FPGA

[Thomas Richard]

This is the fifth version of this series, addressing the few remaining
issues identified by Andy.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Changes in v5:
- all: improve commit messages, fix some typos and nitpicks.
- pinctrl: machine.h: add "Suggested-by: Andy Shevchenko <andy@kernel.org>"
  and "Reviewed-by: Andy Shevchenko <andy@kernel.org>" tags.
- pinctrl: core: fix kernel doc for devm_pinctrl_register_mappings().
- pinctrl: core: do not cast pointer in devm_pinctrl_unregister_mappings().
- gpio: aggregator: remove a useless check in patch 5/12.
- gpio: aggregator: fix condition to identify if the gpiochip forwarder can
  sleep or not.
- gpio: aggregator: add "Reviewed-by: Andy Shevchenko <andy@kernel.org>" tag
  in patch 10/12
- string_choices: add "Suggested-by: Andy Shevchenko <andy@kernel.org>" and
  "Reviewed-by: Andy Shevchenko <andy@kernel.org>" tags.
- string_choices: add missing parameter for str_output_input() macro.
- Link to v4: https://lore.kernel.org/r/20250429-aaeon-up-board-pinctrl-support-v4-0-b3fffc11417d@bootlin.com

Changes in v4:
- gpiolib: use positive conditonal in gpiochip_add_pin_range_with_pins().
- pinctrl: fix warning reported by kernel robot in
  include/linux/pinctrl/machine.h.
- pinctrl: add a patch to remove the extern specifier in machine.h.
- pinctrl: use devm_add_action_or_reset() in
  devm_pinctrl_register_mappings().
- string_choices: add a patch to define str_input_output() and
  str_output_input() helpers.
- gpio: aggregator: set gpiochip_fwd as opaque and define getters
  gpio_fwd_get_gpiochip() and gpio_fwd_get_data().
- gpio: aggregator: add valid_mask in gpiochip_fwd struct to track already
  registered gpio descs.
- gpio: aggregator: add gpio_fwd_gpio_free() helper.
- gpio: aggregator: add kdoc sections for exported functions.
- gpio: aggregator: fix some nitpicks.
- pinctrl-upboard: use str_input_output() helper.
- pinctrl-upboard: fix some nitpicks.
- pinctrl-upboard: add missing headers stddef.h and types.h.
- pinctrl-upboard: add intermediate cast (unsigned long) for dmi_id->driver_data.
- pinctrl-upboard: use getter gpio_fwd_get_gpiochip() and
  gpio_fwd_get_data().
- pinctrl-upboard: fix kernel robot warning 'unmet direct dependencies detected
  for GPIO_AGGREGATOR when selected by PINCTRL_UPBOARD'.
- pinctrl-upboard: use gpio_fwd_gpio_free() helper.
- Link to v3: https://lore.kernel.org/r/20250416-aaeon-up-board-pinctrl-support-v3-0-f40776bd06ee@bootlin.com

Changes in v3:
- pinctrl: add devm_pinctrl_register_mappings()
- gpiolib: rename gpiochip_add_pin_range() to
  gpiochip_add_pin_range_with_pins() and add pins parameter
- gpiolib: add stubs gpiochip_add_pin_range() and 
  gpiochip_add_sparse_pin_range()
- aggregator: split to more simpler patches
- aggregator: add a namespace for the forwarder library
- aggregator: rename header file to forwarder.h
- aggregator: add some missing headers and declaration in forwarder.h
- aggregator: forwarder.h provides consumer.h and driver.h
- aggregator: fix error code returned by gpio_fwd_request()
- pinctrl-upboard: fix order of header files
- pinctrl-upboard: fix some nitpicks
- pinctrl-upboard: rework macros to define pin groups
- pinctrl-upboard: add missing container_of.h and err.h header files
- pinctrl-upboard: handle correctly pointer returned by dmi_first_match()
- pinctrl-upboard: use devm_pinctrl_register_mappings()
- pinctrl-upboard: import GPIO_FORWARDER namespace
- Link to v2: https://lore.kernel.org/r/20250317-aaeon-up-board-pinctrl-support-v2-0-36126e30aa62@bootlin.com

Changes in v2:
- mfd: removed driver (already merged)
- led: removed driver (already merged)
- gpio-aggregator: refactor code to create a gpio-fwd library
- pinctrl: refactor gpio part to use the gpio-fwd library
- pinctrl: add pinctrl mappings for each board

---
Thomas Richard (12):
      gpiolib: add support to register sparse pin range
      pinctrl: remove extern specifier for functions in machine.h
      pinctrl: core: add devm_pinctrl_register_mappings()
      gpio: aggregator: move GPIO forwarder allocation in a dedicated function
      gpio: aggregator: refactor the code to add GPIO desc in the forwarder
      gpio: aggregator: refactor the forwarder registration part
      gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters
      gpio: aggregator: export symbols of the GPIO forwarder library
      gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd
      gpio: aggregator: add possibility to attach data to the forwarder
      lib/string_choices: Add str_input_output() helper
      pinctrl: Add pin controller driver for AAEON UP boards

 drivers/gpio/gpio-aggregator.c    |  343 +++++++++---
 drivers/gpio/gpiolib.c            |   29 +-
 drivers/pinctrl/Kconfig           |   19 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/core.c            |   29 +
 drivers/pinctrl/pinctrl-upboard.c | 1068 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h       |   51 +-
 include/linux/gpio/forwarder.h    |   48 ++
 include/linux/pinctrl/machine.h   |   18 +-
 include/linux/string_choices.h    |    6 +
 10 files changed, 1534 insertions(+), 78 deletions(-)
---
base-commit: 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
change-id: 20240930-aaeon-up-board-pinctrl-support-98fa4a030490

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH v5 01/12] gpiolib: add support to register sparse pin range

[Thomas Richard]

Add support to register for GPIO<->pin mapping using a list of non
consecutive pins. The core already supports sparse pin range (pins member
of struct pinctrl_gpio_range), but it was not possible to register one. If
pins is not NULL the core uses it, otherwise it assumes that a consecutive
pin range was registered and it uses pin_base.

The function gpiochip_add_pin_range() which allocates and fills the struct
pinctrl_gpio_range was renamed to gpiochip_add_pin_range_with_pins() and
the pins parameter was added.

Two new functions were added, gpiochip_add_pin_range() and
gpiochip_add_sparse_pin_range() to register a consecutive or sparse pins
range. Both use gpiochip_add_pin_range_with_pins().

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpiolib.c      | 29 ++++++++++++++++++--------
 include/linux/gpio/driver.h | 51 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd4fecbb41f2..b59fc4d751da 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2283,11 +2283,13 @@ int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
 
 /**
- * gpiochip_add_pin_range() - add a range for GPIO <-> pin mapping
+ * gpiochip_add_pin_range_with_pins() - add a range for GPIO <-> pin mapping
  * @gc: the gpiochip to add the range for
  * @pinctl_name: the dev_name() of the pin controller to map to
  * @gpio_offset: the start offset in the current gpio_chip number space
  * @pin_offset: the start offset in the pin controller number space
+ * @pins: the list of non consecutive pins to accumulate in this range (if not
+ *	NULL, pin_offset is ignored by pinctrl core)
  * @npins: the number of pins from the offset of each pin space (GPIO and
  *	pin controller) to accumulate in this range
  *
@@ -2299,9 +2301,12 @@ EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
  * 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,
-			   unsigned int npins)
+int gpiochip_add_pin_range_with_pins(struct gpio_chip *gc,
+				     const char *pinctl_name,
+				     unsigned int gpio_offset,
+				     unsigned int pin_offset,
+				     unsigned int const *pins,
+				     unsigned int npins)
 {
 	struct gpio_pin_range *pin_range;
 	struct gpio_device *gdev = gc->gpiodev;
@@ -2319,6 +2324,7 @@ int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 	pin_range->range.name = gc->label;
 	pin_range->range.base = gdev->base + gpio_offset;
 	pin_range->range.pin_base = pin_offset;
+	pin_range->range.pins = pins;
 	pin_range->range.npins = npins;
 	pin_range->pctldev = pinctrl_find_and_add_gpio_range(pinctl_name,
 			&pin_range->range);
@@ -2328,16 +2334,21 @@ int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 		kfree(pin_range);
 		return ret;
 	}
-	chip_dbg(gc, "created GPIO range %d->%d ==> %s PIN %d->%d\n",
-		 gpio_offset, gpio_offset + npins - 1,
-		 pinctl_name,
-		 pin_offset, pin_offset + npins - 1);
+	if (pin_range->range.pins)
+		chip_dbg(gc, "created GPIO range %d->%d ==> %s %d sparse PIN range { %d, ... }",
+			 gpio_offset, gpio_offset + npins - 1,
+			 pinctl_name, npins, pins[0]);
+	else
+		chip_dbg(gc, "created GPIO range %d->%d ==> %s PIN %d->%d\n",
+			 gpio_offset, gpio_offset + npins - 1,
+			 pinctl_name,
+			 pin_offset, pin_offset + npins - 1);
 
 	list_add_tail(&pin_range->node, &gdev->pin_ranges);
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(gpiochip_add_pin_range);
+EXPORT_SYMBOL_GPL(gpiochip_add_pin_range_with_pins);
 
 /**
  * gpiochip_remove_pin_ranges() - remove all the GPIO <-> pin mappings
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 4c0294a9104d..6142837ea403 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -784,23 +784,68 @@ struct gpio_pin_range {
 
 #ifdef CONFIG_PINCTRL
 
-int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
-			   unsigned int gpio_offset, unsigned int pin_offset,
-			   unsigned int npins);
+int gpiochip_add_pin_range_with_pins(struct gpio_chip *gc,
+				     const char *pinctl_name,
+				     unsigned int gpio_offset,
+				     unsigned int pin_offset,
+				     unsigned int const *pins,
+				     unsigned int npins);
 int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,
 			unsigned int gpio_offset, const char *pin_group);
 void gpiochip_remove_pin_ranges(struct gpio_chip *gc);
 
+static inline int
+gpiochip_add_pin_range(struct gpio_chip *gc,
+		       const char *pinctl_name,
+		       unsigned int gpio_offset,
+		       unsigned int pin_offset,
+		       unsigned int npins)
+{
+	return gpiochip_add_pin_range_with_pins(gc, pinctl_name, gpio_offset,
+						pin_offset, NULL, npins);
+}
+
+static inline int
+gpiochip_add_sparse_pin_range(struct gpio_chip *gc,
+			      const char *pinctl_name,
+			      unsigned int gpio_offset,
+			      unsigned int const *pins,
+			      unsigned int npins)
+{
+	return gpiochip_add_pin_range_with_pins(gc, pinctl_name, gpio_offset, 0,
+						pins, npins);
+}
 #else /* ! CONFIG_PINCTRL */
 
+static inline int
+gpiochip_add_pin_range_with_pins(struct gpio_chip *gc,
+				 const char *pinctl_name,
+				 unsigned int gpio_offset,
+				 unsigned int pin_offset,
+				 unsigned int npins)
+{
+	return 0;
+}
+
 static inline int
 gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 		       unsigned int gpio_offset, unsigned int pin_offset,
 		       unsigned int npins)
+{
+	return 0
+}
+
+static inline int
+gpiochip_add_sparse_pin_range(struct gpio_chip *gc,
+			      const char *pinctl_name,
+			      unsigned int gpio_offset,
+			      unsigned int const *pins,
+			      unsigned int npins)
 {
 	return 0;
 }
+
 static inline int
 gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,

-- 
2.39.5

Re: [PATCH v5 01/12] gpiolib: add support to register sparse pin range

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Richard/gpiolib-add-support-to-register-sparse-pin-range/20250506-232604
base:   8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
patch link:    https://lore.kernel.org/r/20250506-aaeon-up-board-pinctrl-support-v5-1-3906529757d2%40bootlin.com
patch subject: [PATCH v5 01/12] gpiolib: add support to register sparse pin range
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20250508/202505080122.v2Lm1rIa-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505080122.v2Lm1rIa-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/202505080122.v2Lm1rIa-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/gpio/gpiolib.c:32:
>> include/linux/gpio/driver.h:836:10: error: expected ';' after return statement
     836 |         return 0
         |                 ^
         |                 ;
   1 error generated.
--
   In file included from drivers/gpio/gpiolib-cdev.c:15:
>> include/linux/gpio/driver.h:836:10: error: expected ';' after return statement
     836 |         return 0
         |                 ^
         |                 ;
   drivers/gpio/gpiolib-cdev.c:1665:2: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
    1665 |         INIT_KFIFO(lr->events);
         |         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/kfifo.h:135:69: note: expanded from macro 'INIT_KFIFO'
     135 |         __kfifo->mask = __is_kfifo_ptr(__tmp) ? 0 : ARRAY_SIZE(__tmp->buf) - 1;\
         |                       ~                             ~~~~~~~~~~~~~~~~~~~~~~~^~~
   1 warning and 1 error generated.
--
   In file included from drivers/gpio/gpiolib-swnode.c:20:
>> include/linux/gpio/driver.h:836:10: error: expected ';' after return statement
     836 |         return 0
         |                 ^
         |                 ;
   In file included from drivers/gpio/gpiolib-swnode.c:22:
   In file included from drivers/gpio/gpiolib.h:12:
   In file included from include/linux/cdev.h:8:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/x86/include/asm/elf.h:10:
   In file included from arch/x86/include/asm/ia32.h:7:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:34:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:98:11: warning: array index 3 is past the end of the array (that has type 'unsigned long[1]') [-Warray-bounds]
      98 |                 return (set->sig[3] | set->sig[2] |
         |                         ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/gpio/gpiolib-swnode.c:22:
   In file included from drivers/gpio/gpiolib.h:12:
   In file included from include/linux/cdev.h:8:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/x86/include/asm/elf.h:10:
   In file included from arch/x86/include/asm/ia32.h:7:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:34:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:98:25: warning: array index 2 is past the end of the array (that has type 'unsigned long[1]') [-Warray-bounds]
      98 |                 return (set->sig[3] | set->sig[2] |
         |                                       ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/gpio/gpiolib-swnode.c:22:
   In file included from drivers/gpio/gpiolib.h:12:
   In file included from include/linux/cdev.h:8:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/x86/include/asm/elf.h:10:
   In file included from arch/x86/include/asm/ia32.h:7:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:34:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:99:4: warning: array index 1 is past the end of the array (that has type 'unsigned long[1]') [-Warray-bounds]
      99 |                         set->sig[1] | set->sig[0]) == 0;
         |                         ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/gpio/gpiolib-swnode.c:22:
   In file included from drivers/gpio/gpiolib.h:12:
   In file included from include/linux/cdev.h:8:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/x86/include/asm/elf.h:10:
   In file included from arch/x86/include/asm/ia32.h:7:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:34:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:101:11: warning: array index 1 is past the end of the array (that has type 'unsigned long[1]') [-Warray-bounds]
     101 |                 return (set->sig[1] | set->sig[0]) == 0;
         |                         ^        ~
   arch/x86/include/asm/signal.h:24:2: note: array 'sig' declared here
      24 |         unsigned long sig[_NSIG_WORDS];
         |         ^
   In file included from drivers/gpio/gpiolib-swnode.c:22:
   In file included from drivers/gpio/gpiolib.h:12:
   In file included from include/linux/cdev.h:8:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/x86/include/asm/elf.h:10:
   In file included from arch/x86/include/asm/ia32.h:7:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:34:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:6:
   include/linux/signal.h:114:11: warning: array index 3 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
     114 |                 return  (set1->sig[3] == set2->sig[3]) &&
         |                          ^         ~


vim +836 include/linux/gpio/driver.h

   830	
   831	static inline int
   832	gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
   833			       unsigned int gpio_offset, unsigned int pin_offset,
   834			       unsigned int npins)
   835	{
 > 836		return 0
   837	}
   838	

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

[PATCH v5 02/12] pinctrl: remove extern specifier for functions in machine.h

[Thomas Richard]

Extern is the default specifier for a function, no need to define it.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 include/linux/pinctrl/machine.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index 673e96df453b..93aceb92681b 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -153,10 +153,9 @@ struct pinctrl_map;
 
 #ifdef CONFIG_PINCTRL
 
-extern int pinctrl_register_mappings(const struct pinctrl_map *map,
-				     unsigned int num_maps);
-extern void pinctrl_unregister_mappings(const struct pinctrl_map *map);
-extern void pinctrl_provide_dummies(void);
+int pinctrl_register_mappings(const struct pinctrl_map *map, unsigned int num_maps);
+void pinctrl_unregister_mappings(const struct pinctrl_map *map);
+void pinctrl_provide_dummies(void);
 #else
 
 static inline int pinctrl_register_mappings(const struct pinctrl_map *map,

-- 
2.39.5

Re: [PATCH v5 02/12] pinctrl: remove extern specifier for functions in machine.h

[Linus Walleij]

On Tue, May 6, 2025 at 5:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Extern is the default specifier for a function, no need to define it.
>
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v5 03/12] pinctrl: core: add devm_pinctrl_register_mappings()

[Thomas Richard]

Using devm_pinctrl_register_mappings(), the core can automatically
unregister pinctrl mappings.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/core.c          | 29 +++++++++++++++++++++++++++++
 include/linux/pinctrl/machine.h | 11 +++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4bdbf6bb26e2..9046292d1360 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1530,6 +1530,35 @@ void pinctrl_unregister_mappings(const struct pinctrl_map *map)
 }
 EXPORT_SYMBOL_GPL(pinctrl_unregister_mappings);
 
+static void devm_pinctrl_unregister_mappings(void *maps)
+{
+	pinctrl_unregister_mappings(maps);
+}
+
+/**
+ * devm_pinctrl_register_mappings() - Resource managed pinctrl_register_mappings()
+ * @dev: device for which mappings are registered
+ * @maps: the pincontrol mappings table to register. Note the pinctrl-core
+ *	keeps a reference to the passed in maps, so they should _not_ be
+ *	marked with __initdata.
+ * @num_maps: the number of maps in the mapping table
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int devm_pinctrl_register_mappings(struct device *dev,
+				   const struct pinctrl_map *maps,
+				   unsigned int num_maps)
+{
+	int ret;
+
+	ret = pinctrl_register_mappings(maps, num_maps);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pinctrl_unregister_mappings, (void *)maps);
+}
+EXPORT_SYMBOL_GPL(devm_pinctrl_register_mappings);
+
 /**
  * pinctrl_force_sleep() - turn a given controller device into sleep state
  * @pctldev: pin controller device
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index 93aceb92681b..00c196061605 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -149,11 +149,15 @@ struct pinctrl_map {
 #define PIN_MAP_CONFIGS_GROUP_HOG_DEFAULT(dev, grp, cfgs)		\
 	PIN_MAP_CONFIGS_GROUP(dev, PINCTRL_STATE_DEFAULT, dev, grp, cfgs)
 
+struct device;
 struct pinctrl_map;
 
 #ifdef CONFIG_PINCTRL
 
 int pinctrl_register_mappings(const struct pinctrl_map *map, unsigned int num_maps);
+int devm_pinctrl_register_mappings(struct device *dev,
+				   const struct pinctrl_map *map,
+				   unsigned int num_maps);
 void pinctrl_unregister_mappings(const struct pinctrl_map *map);
 void pinctrl_provide_dummies(void);
 #else
@@ -164,6 +168,13 @@ static inline int pinctrl_register_mappings(const struct pinctrl_map *map,
 	return 0;
 }
 
+static inline int devm_pinctrl_register_mappings(struct device *dev,
+						 const struct pinctrl_map *map,
+						 unsigned int num_maps)
+{
+	return 0;
+}
+
 static inline void pinctrl_unregister_mappings(const struct pinctrl_map *map)
 {
 }

-- 
2.39.5

[PATCH v5 04/12] gpio: aggregator: move GPIO forwarder allocation in a dedicated function

[Thomas Richard]

Move the GPIO forwarder allocation and static initialization in a dedicated
function.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 47 ++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index d232ea865356..4268ef94914d 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -498,6 +498,36 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
 }
 #endif	/* !CONFIG_OF_GPIO */
 
+static struct gpiochip_fwd *
+devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
+{
+	const char *label = dev_name(dev);
+	struct gpiochip_fwd *fwd;
+	struct gpio_chip *chip;
+
+	fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)), GFP_KERNEL);
+	if (!fwd)
+		return ERR_PTR(-ENOMEM);
+
+	chip = &fwd->chip;
+
+	chip->label = label;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->get_direction = gpio_fwd_get_direction;
+	chip->direction_input = gpio_fwd_direction_input;
+	chip->direction_output = gpio_fwd_direction_output;
+	chip->get = gpio_fwd_get;
+	chip->get_multiple = gpio_fwd_get_multiple_locked;
+	chip->set_rv = gpio_fwd_set;
+	chip->set_multiple_rv = gpio_fwd_set_multiple_locked;
+	chip->to_irq = gpio_fwd_to_irq;
+	chip->base = -1;
+	chip->ngpio = ngpios;
+
+	return fwd;
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -518,14 +548,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 						struct gpio_desc *descs[],
 						unsigned long features)
 {
-	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
 	struct gpio_chip *chip;
 	unsigned int i;
 	int error;
 
-	fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)),
-			   GFP_KERNEL);
+	fwd = devm_gpiochip_fwd_alloc(dev, ngpios);
 	if (!fwd)
 		return ERR_PTR(-ENOMEM);
 
@@ -549,19 +577,6 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 			chip->set_config = gpio_fwd_set_config;
 	}
 
-	chip->label = label;
-	chip->parent = dev;
-	chip->owner = THIS_MODULE;
-	chip->get_direction = gpio_fwd_get_direction;
-	chip->direction_input = gpio_fwd_direction_input;
-	chip->direction_output = gpio_fwd_direction_output;
-	chip->get = gpio_fwd_get;
-	chip->get_multiple = gpio_fwd_get_multiple_locked;
-	chip->set_rv = gpio_fwd_set;
-	chip->set_multiple_rv = gpio_fwd_set_multiple_locked;
-	chip->to_irq = gpio_fwd_to_irq;
-	chip->base = -1;
-	chip->ngpio = ngpios;
 	fwd->descs = descs;
 
 	if (chip->can_sleep)

-- 
2.39.5

Re: [PATCH v5 04/12] gpio: aggregator: move GPIO forwarder allocation in a dedicated function

[Geert Uytterhoeven]

Hi Thomas,

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Move the GPIO forwarder allocation and static initialization in a dedicated
> function.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -498,6 +498,36 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
>  }
>  #endif /* !CONFIG_OF_GPIO */
>
> +static struct gpiochip_fwd *
> +devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
> +{
> +       const char *label = dev_name(dev);
> +       struct gpiochip_fwd *fwd;
> +       struct gpio_chip *chip;
> +
> +       fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)), GFP_KERNEL);
> +       if (!fwd)
> +               return ERR_PTR(-ENOMEM);

The caller expects NULL for failures.
So either this return value has to be changed, or the caller needs
to be updated, too.

> +
> +       chip = &fwd->chip;
> +
> +       chip->label = label;

Since you will have to respin this patch anyway, you may want to
drop the label variable, and use dev_name(dev) directly.

> +       chip->parent = dev;
> +       chip->owner = THIS_MODULE;
> +       chip->get_direction = gpio_fwd_get_direction;
> +       chip->direction_input = gpio_fwd_direction_input;
> +       chip->direction_output = gpio_fwd_direction_output;
> +       chip->get = gpio_fwd_get;
> +       chip->get_multiple = gpio_fwd_get_multiple_locked;
> +       chip->set_rv = gpio_fwd_set;
> +       chip->set_multiple_rv = gpio_fwd_set_multiple_locked;
> +       chip->to_irq = gpio_fwd_to_irq;
> +       chip->base = -1;
> +       chip->ngpio = ngpios;
> +
> +       return fwd;
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -518,14 +548,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>                                                 struct gpio_desc *descs[],
>                                                 unsigned long features)
>  {
> -       const char *label = dev_name(dev);
>         struct gpiochip_fwd *fwd;
>         struct gpio_chip *chip;
>         unsigned int i;
>         int error;
>
> -       fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)),
> -                          GFP_KERNEL);
> +       fwd = devm_gpiochip_fwd_alloc(dev, ngpios);
>         if (!fwd)
>                 return ERR_PTR(-ENOMEM);
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 05/12] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Thomas Richard]

Create a dedicated function to add a GPIO desc in the forwarder. Instead of
passing an array of GPIO descs, now the GPIO descs are passed on by one to
the forwarder.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 54 +++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 4268ef94914d..7ca6f1953938 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -509,6 +509,10 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
 	if (!fwd)
 		return ERR_PTR(-ENOMEM);
 
+	fwd->descs = devm_kcalloc(dev, ngpios, sizeof(*fwd->descs), GFP_KERNEL);
+	if (!fwd->descs)
+		return ERR_PTR(-ENOMEM);
+
 	chip = &fwd->chip;
 
 	chip->label = label;
@@ -528,6 +532,36 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
 	return fwd;
 }
 
+static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
+				 struct gpio_desc *desc,
+				 unsigned int offset)
+{
+	struct gpio_chip *parent = gpiod_to_chip(desc);
+	struct gpio_chip *chip = &fwd->chip;
+
+	if (offset > chip->ngpio)
+		return -EINVAL;
+
+	/*
+	 * If any of the GPIO lines are sleeping, then the entire forwarder
+	 * will be sleeping.
+	 * If any of the chips support .set_config(), then the forwarder will
+	 * support setting configs.
+	 */
+	if (gpiod_cansleep(desc))
+		chip->can_sleep = true;
+
+	if (parent && parent->set_config)
+		chip->set_config = gpio_fwd_set_config;
+
+	fwd->descs[offset] = desc;
+
+	dev_dbg(chip->parent, "%u => gpio %d irq %d\n", offset,
+		desc_to_gpio(desc), gpiod_to_irq(desc));
+
+	return 0;
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -559,26 +593,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 
 	chip = &fwd->chip;
 
-	/*
-	 * If any of the GPIO lines are sleeping, then the entire forwarder
-	 * will be sleeping.
-	 * If any of the chips support .set_config(), then the forwarder will
-	 * support setting configs.
-	 */
 	for (i = 0; i < ngpios; i++) {
-		struct gpio_chip *parent = gpiod_to_chip(descs[i]);
-
-		dev_dbg(dev, "%u => gpio %d irq %d\n", i,
-			desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
-
-		if (gpiod_cansleep(descs[i]))
-			chip->can_sleep = true;
-		if (parent && parent->set_config)
-			chip->set_config = gpio_fwd_set_config;
+		error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
+		if (error)
+			return ERR_PTR(error);
 	}
 
-	fwd->descs = descs;
-
 	if (chip->can_sleep)
 		mutex_init(&fwd->mlock);
 	else

-- 
2.39.5

Re: [PATCH v5 05/12] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Andy Shevchenko]

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Create a dedicated function to add a GPIO desc in the forwarder. Instead of
> passing an array of GPIO descs, now the GPIO descs are passed on by one to
> the forwarder.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 05/12] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Geert Uytterhoeven]

Hi Thomas,

Thanks for your patch!

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Create a dedicated function to add a GPIO desc in the forwarder. Instead of
> passing an array of GPIO descs, now the GPIO descs are passed on by one to

one by one

> the forwarder.

Also, the passed array is no longer stored as-is, but copied.

>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -509,6 +509,10 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
>         if (!fwd)
>                 return ERR_PTR(-ENOMEM);
>
> +       fwd->descs = devm_kcalloc(dev, ngpios, sizeof(*fwd->descs), GFP_KERNEL);
> +       if (!fwd->descs)
> +               return ERR_PTR(-ENOMEM);
> +
>         chip = &fwd->chip;
>
>         chip->label = label;
> @@ -528,6 +532,36 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
>         return fwd;
>  }
>
> +static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
> +                                struct gpio_desc *desc,
> +                                unsigned int offset)
> +{
> +       struct gpio_chip *parent = gpiod_to_chip(desc);
> +       struct gpio_chip *chip = &fwd->chip;
> +
> +       if (offset > chip->ngpio)

>=

> +               return -EINVAL;
> +
> +       /*
> +        * If any of the GPIO lines are sleeping, then the entire forwarder
> +        * will be sleeping.
> +        * If any of the chips support .set_config(), then the forwarder will
> +        * support setting configs.
> +        */
> +       if (gpiod_cansleep(desc))
> +               chip->can_sleep = true;
> +
> +       if (parent && parent->set_config)
> +               chip->set_config = gpio_fwd_set_config;
> +
> +       fwd->descs[offset] = desc;
> +
> +       dev_dbg(chip->parent, "%u => gpio %d irq %d\n", offset,
> +               desc_to_gpio(desc), gpiod_to_irq(desc));
> +
> +       return 0;
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -559,26 +593,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
>         chip = &fwd->chip;
>
> -       /*
> -        * If any of the GPIO lines are sleeping, then the entire forwarder
> -        * will be sleeping.
> -        * If any of the chips support .set_config(), then the forwarder will
> -        * support setting configs.
> -        */
>         for (i = 0; i < ngpios; i++) {
> -               struct gpio_chip *parent = gpiod_to_chip(descs[i]);
> -
> -               dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> -                       desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
> -
> -               if (gpiod_cansleep(descs[i]))
> -                       chip->can_sleep = true;
> -               if (parent && parent->set_config)
> -                       chip->set_config = gpio_fwd_set_config;
> +               error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
> +               if (error)
> +                       return ERR_PTR(error);
>         }
>
> -       fwd->descs = descs;

So the passed array is no longer stored, and thus the caller
(gpio_aggregator_probe()) can free it after the call.

> -
>         if (chip->can_sleep)
>                 mutex_init(&fwd->mlock);
>         else
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 05/12] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Geert Uytterhoeven]

On Fri, 9 May 2025 at 10:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> > Create a dedicated function to add a GPIO desc in the forwarder. Instead of
> > passing an array of GPIO descs, now the GPIO descs are passed on by one to
>
> one by one
>
> > the forwarder.
>
> Also, the passed array is no longer stored as-is, but copied.
>
> >
> > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

> >  /**
> >   * gpiochip_fwd_create() - Create a new GPIO forwarder
> >   * @dev: Parent device pointer

      * @ngpios: Number of GPIOs in the forwarder.
      * @descs: Array containing the GPIO descriptors to forward to.
      *         This array must contain @ngpios entries, and must not
be deallocated
      *         before the forwarder has been destroyed again.

This needs an update, as the array is copied.

> > @@ -559,26 +593,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >
> >         chip = &fwd->chip;
> >
> > -       /*
> > -        * If any of the GPIO lines are sleeping, then the entire forwarder
> > -        * will be sleeping.
> > -        * If any of the chips support .set_config(), then the forwarder will
> > -        * support setting configs.
> > -        */
> >         for (i = 0; i < ngpios; i++) {
> > -               struct gpio_chip *parent = gpiod_to_chip(descs[i]);
> > -
> > -               dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> > -                       desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
> > -
> > -               if (gpiod_cansleep(descs[i]))
> > -                       chip->can_sleep = true;
> > -               if (parent && parent->set_config)
> > -                       chip->set_config = gpio_fwd_set_config;
> > +               error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
> > +               if (error)
> > +                       return ERR_PTR(error);
> >         }
> >
> > -       fwd->descs = descs;
>
> So the passed array is no longer stored, and thus the caller
> (gpio_aggregator_probe()) can free it after the call.
>
> > -
> >         if (chip->can_sleep)
> >                 mutex_init(&fwd->mlock);
> >         else

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 06/12] gpio: aggregator: refactor the forwarder registration part

[Thomas Richard]

Add a new function gpiochip_fwd_register(), which finalizes the
initialization of the forwarder and registers the corresponding gpiochip.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 7ca6f1953938..569c7e8ea4c2 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -562,6 +562,18 @@ static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
 	return 0;
 }
 
+static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+{
+	struct gpio_chip *chip = &fwd->chip;
+
+	if (chip->can_sleep)
+		mutex_init(&fwd->mlock);
+	else
+		spin_lock_init(&fwd->slock);
+
+	return devm_gpiochip_add_data(chip->parent, chip, fwd);
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -599,18 +611,13 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 			return ERR_PTR(error);
 	}
 
-	if (chip->can_sleep)
-		mutex_init(&fwd->mlock);
-	else
-		spin_lock_init(&fwd->slock);
-
 	if (features & FWD_FEATURE_DELAY) {
 		error = gpiochip_fwd_setup_delay_line(dev, chip, fwd);
 		if (error)
 			return ERR_PTR(error);
 	}
 
-	error = devm_gpiochip_add_data(dev, chip, fwd);
+	error = gpiochip_fwd_register(fwd);
 	if (error)
 		return ERR_PTR(error);
 

-- 
2.39.5

Re: [PATCH v5 06/12] gpio: aggregator: refactor the forwarder registration part

[Geert Uytterhoeven]

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Add a new function gpiochip_fwd_register(), which finalizes the
> initialization of the forwarder and registers the corresponding gpiochip.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 07/12] gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters

[Thomas Richard]

Remove useless parameters of gpiochip_fwd_setup_delay_line().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 569c7e8ea4c2..45d713e7a702 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -476,10 +476,11 @@ static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
 	return line;
 }
 
-static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip,
-					 struct gpiochip_fwd *fwd)
+static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 {
-	fwd->delay_timings = devm_kcalloc(dev, chip->ngpio,
+	struct gpio_chip *chip = &fwd->chip;
+
+	fwd->delay_timings = devm_kcalloc(chip->parent, chip->ngpio,
 					  sizeof(*fwd->delay_timings),
 					  GFP_KERNEL);
 	if (!fwd->delay_timings)
@@ -491,8 +492,7 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
 	return 0;
 }
 #else
-static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip,
-					 struct gpiochip_fwd *fwd)
+static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 {
 	return 0;
 }
@@ -595,7 +595,6 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 						unsigned long features)
 {
 	struct gpiochip_fwd *fwd;
-	struct gpio_chip *chip;
 	unsigned int i;
 	int error;
 
@@ -603,8 +602,6 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	if (!fwd)
 		return ERR_PTR(-ENOMEM);
 
-	chip = &fwd->chip;
-
 	for (i = 0; i < ngpios; i++) {
 		error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
 		if (error)
@@ -612,7 +609,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	}
 
 	if (features & FWD_FEATURE_DELAY) {
-		error = gpiochip_fwd_setup_delay_line(dev, chip, fwd);
+		error = gpiochip_fwd_setup_delay_line(fwd);
 		if (error)
 			return ERR_PTR(error);
 	}

-- 
2.39.5

Re: [PATCH v5 07/12] gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters

[Geert Uytterhoeven]

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Remove useless parameters of gpiochip_fwd_setup_delay_line().
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

Export all symbols and create header file for the GPIO forwarder library.
It will be used in the next changes.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 190 +++++++++++++++++++++++++++++++++--------
 include/linux/gpio/forwarder.h |  42 +++++++++
 2 files changed, 197 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 45d713e7a702..79fd44c2ceac 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -25,6 +25,7 @@
 
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/forwarder.h>
 #include <linux/gpio/machine.h>
 
 #define AGGREGATOR_MAX_GPIOS 512
@@ -275,35 +276,81 @@ struct gpiochip_fwd {
 
 #define fwd_tmp_size(ngpios)	(BITS_TO_LONGS((ngpios)) + (ngpios))
 
-static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
+/**
+ * gpio_fwd_get_gpiochip - Get the GPIO chip for the GPIO forwarder
+ * @fwd: GPIO forwarder
+ *
+ * Returns: The GPIO chip for the GPIO forwarder
+ */
+struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
+{
+	return &fwd->chip;
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
+
+/**
+ * gpio_fwd_get_direction - Return the current direction of a GPIO forwarder line
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ *
+ * Returns: 0 for output, 1 for input, or an error code in case of error.
+ */
+int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return gpiod_get_direction(fwd->descs[offset]);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_direction, "GPIO_FORWARDER");
 
-static int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset)
+/**
+ * gpio_fwd_direction_input - Set a GPIO forwarder line direction to input
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return gpiod_direction_input(fwd->descs[offset]);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_direction_input, "GPIO_FORWARDER");
 
-static int gpio_fwd_direction_output(struct gpio_chip *chip,
-				     unsigned int offset, int value)
+/**
+ * gpio_fwd_direction_output - Set a GPIO forwarder line direction to output
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ * @value: value to set
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_direction_output(struct gpio_chip *chip, unsigned int offset,
+			      int value)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return gpiod_direction_output(fwd->descs[offset], value);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_direction_output, "GPIO_FORWARDER");
 
-static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
+/**
+ * gpio_fwd_get - Return a GPIO forwarder line's value
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ *
+ * Returns: The GPIO's logical value, i.e. taking the ACTIVE_LOW status into
+ * account, or negative errno on failure.
+ */
+int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset])
 			       : gpiod_get_value(fwd->descs[offset]);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_get, "GPIO_FORWARDER");
 
 static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				 unsigned long *bits)
@@ -331,8 +378,18 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 	return 0;
 }
 
-static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
-					unsigned long *mask, unsigned long *bits)
+/**
+ * gpio_fwd_get_multiple_locked - Get values for multiple GPIO forwarder lines
+ * @chip: GPIO chip in the forwarder
+ * @mask: bit mask array; one bit per line; BITS_PER_LONG bits per word defines
+ *        which lines are to be read
+ * @bits: bit value array; one bit per line; BITS_PER_LONG bits per word will
+ *        contains the read values for the lines specified by mask
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_get_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	unsigned long flags;
@@ -350,6 +407,7 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
 
 	return error;
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_multiple_locked, "GPIO_FORWARDER");
 
 static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int value)
 {
@@ -372,7 +430,15 @@ static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int valu
 		udelay(delay_us);
 }
 
-static int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
+/**
+ * gpio_fwd_set - Assign value to a GPIO forwarder line.
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ * @value: value to set
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	int ret;
@@ -389,6 +455,7 @@ static int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 
 	return ret;
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_set, "GPIO_FORWARDER");
 
 static int gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				 unsigned long *bits)
@@ -410,8 +477,18 @@ static int gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 	return ret;
 }
 
-static int gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
-					unsigned long *mask, unsigned long *bits)
+/**
+ * gpio_fwd_set_multiple_locked - Assign values to multiple GPIO forwarder lines
+ * @chip: GPIO chip in the forwarder
+ * @mask: bit mask array; one bit per output; BITS_PER_LONG bits per word
+ *        defines which outputs are to be changed
+ * @bits: bit value array; one bit per output; BITS_PER_LONG bits per word
+ *        defines the values the outputs specified by mask are to be set to
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_set_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	unsigned long flags;
@@ -429,21 +506,41 @@ static int gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
 
 	return ret;
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_set_multiple_locked, "GPIO_FORWARDER");
 
-static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
-			       unsigned long config)
+/**
+ * gpio_fwd_set_config - Set @config for a GPIO forwarder line
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ * @config: Same packed config format as generic pinconf
+ *
+ * Returns: 0 on success, %-ENOTSUPP if the controller doesn't support setting
+ * the configuration.
+ */
+int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
+			unsigned long config)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return gpiod_set_config(fwd->descs[offset], config);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_set_config, "GPIO_FORWARDER");
 
-static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
+/**
+ * gpio_fwd_to_irq - Return the IRQ corresponding to a GPIO forwarder line
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line
+ *
+ * Returns: The IRQ corresponding to the passed line, or an error code in case
+ * of error.
+ */
+int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
 	return gpiod_to_irq(fwd->descs[offset]);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_to_irq, "GPIO_FORWARDER");
 
 /*
  * The GPIO delay provides a way to configure platform specific delays
@@ -454,9 +551,9 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
 #define FWD_FEATURE_DELAY		BIT(0)
 
 #ifdef CONFIG_OF_GPIO
-static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
-				       const struct of_phandle_args *gpiospec,
-				       u32 *flags)
+static int gpio_fwd_delay_of_xlate(struct gpio_chip *chip,
+				   const struct of_phandle_args *gpiospec,
+				   u32 *flags)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	struct gpiochip_fwd_timing *timings;
@@ -476,7 +573,7 @@ static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
 	return line;
 }
 
-static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
+static int gpio_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 
@@ -486,20 +583,27 @@ static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 	if (!fwd->delay_timings)
 		return -ENOMEM;
 
-	chip->of_xlate = gpiochip_fwd_delay_of_xlate;
+	chip->of_xlate = gpio_fwd_delay_of_xlate;
 	chip->of_gpio_n_cells = 3;
 
 	return 0;
 }
 #else
-static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
+static int gpio_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 {
 	return 0;
 }
 #endif	/* !CONFIG_OF_GPIO */
 
-static struct gpiochip_fwd *
-devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
+/**
+ * devm_gpio_fwd_alloc - Allocate and initialize a new GPIO forwarder
+ * @dev: Parent device pointer
+ * @ngpios: Number of GPIOs in the forwarder
+ *
+ * Returns: An opaque object pointer, or an ERR_PTR()-encoded negative error
+ * code on failure.
+ */
+struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev, unsigned int ngpios)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -531,10 +635,18 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
 
 	return fwd;
 }
+EXPORT_SYMBOL_NS_GPL(devm_gpio_fwd_alloc, "GPIO_FORWARDER");
 
-static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
-				 struct gpio_desc *desc,
-				 unsigned int offset)
+/**
+ * gpio_fwd_gpio_add - Add a GPIO in the forwader
+ * @fwd: GPIO forwarder
+ * @desc: GPIO decriptor to register
+ * @offset: offset for the GPIO in the forwarder
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
+		      unsigned int offset)
 {
 	struct gpio_chip *parent = gpiod_to_chip(desc);
 	struct gpio_chip *chip = &fwd->chip;
@@ -561,8 +673,15 @@ static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_add, "GPIO_FORWARDER");
 
-static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+/**
+ * gpio_fwd_register - Register a GPIO forwarder
+ * @fwd: GPIO forwarder
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 
@@ -573,9 +692,10 @@ static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 
 	return devm_gpiochip_add_data(chip->parent, chip, fwd);
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_register, "GPIO_FORWARDER");
 
 /**
- * gpiochip_fwd_create() - Create a new GPIO forwarder
+ * gpio_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
  * @ngpios: Number of GPIOs in the forwarder.
  * @descs: Array containing the GPIO descriptors to forward to.
@@ -589,32 +709,32 @@ static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
  * Return: An opaque object pointer, or an ERR_PTR()-encoded negative error
  *         code on failure.
  */
-static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
-						unsigned int ngpios,
-						struct gpio_desc *descs[],
-						unsigned long features)
+static struct gpiochip_fwd *gpio_fwd_create(struct device *dev,
+					    unsigned int ngpios,
+					    struct gpio_desc *descs[],
+					    unsigned long features)
 {
 	struct gpiochip_fwd *fwd;
 	unsigned int i;
 	int error;
 
-	fwd = devm_gpiochip_fwd_alloc(dev, ngpios);
+	fwd = devm_gpio_fwd_alloc(dev, ngpios);
 	if (!fwd)
 		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < ngpios; i++) {
-		error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
+		error = gpio_fwd_gpio_add(fwd, descs[i], i);
 		if (error)
 			return ERR_PTR(error);
 	}
 
 	if (features & FWD_FEATURE_DELAY) {
-		error = gpiochip_fwd_setup_delay_line(fwd);
+		error = gpio_fwd_setup_delay_line(fwd);
 		if (error)
 			return ERR_PTR(error);
 	}
 
-	error = gpiochip_fwd_register(fwd);
+	error = gpio_fwd_register(fwd);
 	if (error)
 		return ERR_PTR(error);
 
@@ -649,7 +769,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
 	}
 
 	features = (uintptr_t)device_get_match_data(dev);
-	fwd = gpiochip_fwd_create(dev, n, descs, features);
+	fwd = gpio_fwd_create(dev, n, descs, features);
 	if (IS_ERR(fwd))
 		return PTR_ERR(fwd);
 
diff --git a/include/linux/gpio/forwarder.h b/include/linux/gpio/forwarder.h
new file mode 100644
index 000000000000..b17ad2c8e031
--- /dev/null
+++ b/include/linux/gpio/forwarder.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_GPIO_FORWARDER_H
+#define __LINUX_GPIO_FORWARDER_H
+
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+
+struct gpiochip_fwd;
+
+struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
+
+int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
+
+int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
+
+int gpio_fwd_direction_output(struct gpio_chip *chip, unsigned int offset,
+			      int value);
+
+int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset);
+
+int gpio_fwd_get_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits);
+
+int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value);
+
+int gpio_fwd_set_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
+				 unsigned long *bits);
+
+int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
+			unsigned long config);
+
+int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
+
+struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
+					 unsigned int ngpios);
+
+int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
+		      struct gpio_desc *desc, unsigned int offset);
+
+int gpio_fwd_register(struct gpiochip_fwd *fwd);
+
+#endif

-- 
2.39.5

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Andy Shevchenko]

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Export all symbols and create header file for the GPIO forwarder library.
> It will be used in the next changes.

...

> +/**
> + * gpio_fwd_to_irq - Return the IRQ corresponding to a GPIO forwarder line
> + * @chip: GPIO chip in the forwarder
> + * @offset: the offset of the line
> + *
> + * Returns: The IRQ corresponding to the passed line, or an error code in case

"The Linux IRQ..." since it's a logical number and 0 can't be ever returned.

> + * of error.
> + */

...

> +/**
> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder

forwarder

> + * @fwd: GPIO forwarder
> + * @desc: GPIO decriptor to register

descriptor

> + * @offset: offset for the GPIO in the forwarder
> + *
> + * Returns: 0 on success, or negative errno on failure.
> + */

Please, spellcheck all of the comments.

...

> +#ifndef __LINUX_GPIO_FORWARDER_H
> +#define __LINUX_GPIO_FORWARDER_H

> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>

These are not used (at least as of this patch).

+ struct gpio_chip;

> +struct gpiochip_fwd;

> +struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
> +
> +int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
> +
> +int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
> +
> +int gpio_fwd_direction_output(struct gpio_chip *chip, unsigned int offset,
> +                             int value);
> +
> +int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset);
> +
> +int gpio_fwd_get_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits);
> +
> +int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value);
> +
> +int gpio_fwd_set_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits);
> +
> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> +                       unsigned long config);
> +
> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> +
> +struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
> +                                        unsigned int ngpios);
> +
> +int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
> +                     struct gpio_desc *desc, unsigned int offset);
> +
> +int gpio_fwd_register(struct gpiochip_fwd *fwd);
> +
> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

On 5/7/25 08:29, Andy Shevchenko wrote:
>> +/**
>> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder
> 
> forwarder

Sorry I do not see the typo :)

> 
>> + * @fwd: GPIO forwarder
>> + * @desc: GPIO decriptor to register
> 
> descriptor
> 
>> + * @offset: offset for the GPIO in the forwarder
>> + *
>> + * Returns: 0 on success, or negative errno on failure.
>> + */
> 
> Please, spellcheck all of the comments.

Ditto

> 
> ...
> 
>> +#ifndef __LINUX_GPIO_FORWARDER_H
>> +#define __LINUX_GPIO_FORWARDER_H
> 
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/gpio/driver.h>
> 
> These are not used (at least as of this patch).

Indeed they are only needed in the pinctrl driver.

> 
> + struct gpio_chip;

And also struct gpio_desc;

Regards,

Thomas

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Andy Shevchenko]

On Wed, May 7, 2025 at 5:53 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 5/7/25 08:29, Andy Shevchenko wrote:
> >> +/**
> >> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder
> >
> > forwarder
>
> Sorry I do not see the typo :)

Your original piece of the code. Please look better.

-static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
-                                struct gpio_desc *desc,
-                                unsigned int offset)
+/**
+ * gpio_fwd_gpio_add - Add a GPIO in the forwader


> >> + * @fwd: GPIO forwarder
> >> + * @desc: GPIO decriptor to register
> >
> > descriptor

Ditto.

> >> + * @offset: offset for the GPIO in the forwarder
> >> + *
> >> + * Returns: 0 on success, or negative errno on failure.
> >> + */
> >
> > Please, spellcheck all of the comments.
>
> Ditto

See above and spell check. Thank you.

...

> > + struct gpio_chip;
>
> And also struct gpio_desc;

Yes. :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Andy Shevchenko]

On Wed, May 7, 2025 at 6:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, May 7, 2025 at 5:53 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
> > On 5/7/25 08:29, Andy Shevchenko wrote:

...

> > >> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder
> > >
> > > forwarder
> >
> > Sorry I do not see the typo :)
>
> Your original piece of the code. Please look better.

Ah, it was probably me mistakenly fixing the original text :-) It has
a typo there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

On 5/7/25 17:23, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 6:21 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, May 7, 2025 at 5:53 PM Thomas Richard
>> <thomas.richard@bootlin.com> wrote:
>>> On 5/7/25 08:29, Andy Shevchenko wrote:
> 
> ...
> 
>>>>> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder
>>>>
>>>> forwarder
>>>
>>> Sorry I do not see the typo :)
>>
>> Your original piece of the code. Please look better.
> 
> Ah, it was probably me mistakenly fixing the original text :-) It has
> a typo there.
> 

Oh I get it.
Yes you fixed the original text.
But I checked in my code and I missed it :)

Thomas

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Geert Uytterhoeven]

Hi Andy,

On Wed, 7 May 2025 at 17:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, May 7, 2025 at 6:21 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, May 7, 2025 at 5:53 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:
> > > On 5/7/25 08:29, Andy Shevchenko wrote:
>
> ...
>
> > > >> + * gpio_fwd_gpio_add - Add a GPIO in the forwarder
> > > >
> > > > forwarder
> > >
> > > Sorry I do not see the typo :)
> >
> > Your original piece of the code. Please look better.
>
> Ah, it was probably me mistakenly fixing the original text :-) It has
> a typo there.

Gmail suggests correcting typos in your emails while you type them,
even in quoted parts, and you may have inadvertently accepted the
suggestion.  I've been bitten by that, too :-(

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Geert Uytterhoeven]

Hi Thomas,

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Export all symbols and create header file for the GPIO forwarder library.
> It will be used in the next changes.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch!

> --- /dev/null
> +++ b/include/linux/gpio/forwarder.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GPIO_FORWARDER_H
> +#define __LINUX_GPIO_FORWARDER_H
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +
> +struct gpiochip_fwd;
> +
> +struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
> +
> +int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
> +

Please drop empty lines between functions that belong together
logically.

> +int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
> +
> +int gpio_fwd_direction_output(struct gpio_chip *chip, unsigned int offset,
> +                             int value);
> +
> +int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset);
> +
> +int gpio_fwd_get_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits);
> +
> +int gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value);
> +
> +int gpio_fwd_set_multiple_locked(struct gpio_chip *chip, unsigned long *mask,
> +                                unsigned long *bits);
> +
> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> +                       unsigned long config);
> +
> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);

I would expect all of these to take gpiochip_fwd pointers instead of
gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
that does not correspond to a gpiochip_fwd object, causing a crash?

> +
> +struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
> +                                        unsigned int ngpios);
> +
> +int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
> +                     struct gpio_desc *desc, unsigned int offset);
> +
> +int gpio_fwd_register(struct gpiochip_fwd *fwd);

Please move these three to the top, as these functions are what the
user needs first, and drop the empty lines between them.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

Hi Geert

Thanks a lot for the review.

On 5/9/25 11:07, Geert Uytterhoeven wrote:
> Hi Thomas,
> 
> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
>> Export all symbols and create header file for the GPIO forwarder library.
>> It will be used in the next changes.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

...

>> +
>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
>> +                       unsigned long config);
>> +
>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> 
> I would expect all of these to take gpiochip_fwd pointers instead of
> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
> that does not correspond to a gpiochip_fwd object, causing a crash?

Indeed nothing prevents from passing gpio_chip pointer which does not
correspond to a gpiochip_fwd object.
And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
(for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.

I can keep GPIO operations as is, and create exported wrappers which
take a gpiochip_fwd pointer as parameter, for example:

int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
			      unsigned long *mask,
			      unsigned long *bits)
{
	struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);

	return gpio_fwd_get_multiple_locked(chip, mask, bits);
}
EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");

So exported functions are gpiochip_fwd_*().

Regards,

Thomas
-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Andy Shevchenko]

On Mon, May 12, 2025 at 04:08:35PM +0200, Thomas Richard wrote:
> On 5/9/25 11:07, Geert Uytterhoeven wrote:
> > On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:

...

> >> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> >> +                       unsigned long config);
> >> +
> >> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> > 
> > I would expect all of these to take gpiochip_fwd pointers instead of
> > gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
> > that does not correspond to a gpiochip_fwd object, causing a crash?
> 
> Indeed nothing prevents from passing gpio_chip pointer which does not
> correspond to a gpiochip_fwd object.
> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
> 
> I can keep GPIO operations as is, and create exported wrappers which
> take a gpiochip_fwd pointer as parameter, for example:
> 
> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
> 			      unsigned long *mask,
> 			      unsigned long *bits)
> {
> 	struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
> 
> 	return gpio_fwd_get_multiple_locked(chip, mask, bits);
> }
> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
> 
> So exported functions are gpiochip_fwd_*().

Sounds good for me. Let's wait for Geert's opinoin on this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

On 5/12/25 16:11, Andy Shevchenko wrote:
> On Mon, May 12, 2025 at 04:08:35PM +0200, Thomas Richard wrote:
>> On 5/9/25 11:07, Geert Uytterhoeven wrote:
>>> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> 
> ...
> 
>>>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
>>>> +                       unsigned long config);
>>>> +
>>>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
>>>
>>> I would expect all of these to take gpiochip_fwd pointers instead of
>>> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
>>> that does not correspond to a gpiochip_fwd object, causing a crash?
>>
>> Indeed nothing prevents from passing gpio_chip pointer which does not
>> correspond to a gpiochip_fwd object.
>> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
>> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
>>
>> I can keep GPIO operations as is, and create exported wrappers which
>> take a gpiochip_fwd pointer as parameter, for example:
>>
>> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
>> 			      unsigned long *mask,
>> 			      unsigned long *bits)
>> {
>> 	struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
>>
>> 	return gpio_fwd_get_multiple_locked(chip, mask, bits);
>> }
>> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
>>
>> So exported functions are gpiochip_fwd_*().
> 
> Sounds good for me. Let's wait for Geert's opinoin on this.

Regarding Geert's comment for patch 9/12, an other proposal for naming:
As mentioned above, exported functions gpiochip_fwd_*() take a
gpiochip_fwd as parameter.

But for all functions corresponding to a GPIO operation add the gpio
word, and for functions to add/remove GPIO descriptor add the desc word:

devm_gpiochip_fwd_alloc()
gpiochip_fwd_register()

gpiochip_fwd_desc_add()
gpiochip_fwd_desc_free()

gpiochip_fwd_get_gpiochip()
gpiochip_fwd_get_data()

gpiochip_fwd_gpio_request()
gpiochip_fwd_gpio_get_direction()
gpiochip_fwd_gpio_direction_input()
gpiochip_fwd_gpio_direction_output()
gpiochip_fwd_gpio_get()
gpiochip_fwd_gpio_set()
gpiochip_fwd_gpio_set_multiple()
gpiochip_fwd_gpio_get_multiple()
gpiochip_fwd_gpio_set_config()
gpiochip_fwd_gpio_to_irq()

Regards,

Thomas

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Geert Uytterhoeven]

Hi Thomas,

On Mon, 12 May 2025 at 16:30, Thomas Richard <thomas.richard@bootlin.com> wrote:
> On 5/12/25 16:11, Andy Shevchenko wrote:
> > On Mon, May 12, 2025 at 04:08:35PM +0200, Thomas Richard wrote:
> >> On 5/9/25 11:07, Geert Uytterhoeven wrote:
> >>> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> >
> > ...
> >
> >>>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> >>>> +                       unsigned long config);
> >>>> +
> >>>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> >>>
> >>> I would expect all of these to take gpiochip_fwd pointers instead of
> >>> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
> >>> that does not correspond to a gpiochip_fwd object, causing a crash?
> >>
> >> Indeed nothing prevents from passing gpio_chip pointer which does not
> >> correspond to a gpiochip_fwd object.
> >> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
> >> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
> >>
> >> I can keep GPIO operations as is, and create exported wrappers which
> >> take a gpiochip_fwd pointer as parameter, for example:
> >>
> >> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
> >>                            unsigned long *mask,
> >>                            unsigned long *bits)
> >> {
> >>      struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
> >>
> >>      return gpio_fwd_get_multiple_locked(chip, mask, bits);
> >> }
> >> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
> >>
> >> So exported functions are gpiochip_fwd_*().
> >
> > Sounds good for me. Let's wait for Geert's opinoin on this.
>
> Regarding Geert's comment for patch 9/12, an other proposal for naming:
> As mentioned above, exported functions gpiochip_fwd_*() take a
> gpiochip_fwd as parameter.
>
> But for all functions corresponding to a GPIO operation add the gpio
> word, and for functions to add/remove GPIO descriptor add the desc word:
>
> devm_gpiochip_fwd_alloc()
> gpiochip_fwd_register()
>
> gpiochip_fwd_desc_add()
> gpiochip_fwd_desc_free()
>
> gpiochip_fwd_get_gpiochip()
> gpiochip_fwd_get_data()
>
> gpiochip_fwd_gpio_request()
> gpiochip_fwd_gpio_get_direction()
> gpiochip_fwd_gpio_direction_input()
> gpiochip_fwd_gpio_direction_output()
> gpiochip_fwd_gpio_get()
> gpiochip_fwd_gpio_set()
> gpiochip_fwd_gpio_set_multiple()
> gpiochip_fwd_gpio_get_multiple()
> gpiochip_fwd_gpio_set_config()
> gpiochip_fwd_gpio_to_irq()

Sounds good to me (from a quick glance).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Geert Uytterhoeven]

Hi Thomas,

On Mon, 12 May 2025 at 16:08, Thomas Richard <thomas.richard@bootlin.com> wrote:
> On 5/9/25 11:07, Geert Uytterhoeven wrote:
> > On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> >> Export all symbols and create header file for the GPIO forwarder library.
> >> It will be used in the next changes.
> >>
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>
> ...
>
> >> +
> >> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> >> +                       unsigned long config);
> >> +
> >> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> >
> > I would expect all of these to take gpiochip_fwd pointers instead of
> > gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
> > that does not correspond to a gpiochip_fwd object, causing a crash?
>
> Indeed nothing prevents from passing gpio_chip pointer which does not
> correspond to a gpiochip_fwd object.
> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
>
> I can keep GPIO operations as is, and create exported wrappers which
> take a gpiochip_fwd pointer as parameter, for example:
>
> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
>                               unsigned long *mask,
>                               unsigned long *bits)
> {
>         struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
>
>         return gpio_fwd_get_multiple_locked(chip, mask, bits);
> }
> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
>
> So exported functions are gpiochip_fwd_*().

That sounds fine to me.

BTW, do you need to use these functions as gpio_chip callbacks?
If that is the case, they do no need to take struct gpio_chip pointers.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

On 5/12/25 16:39, Geert Uytterhoeven wrote:
> Hi Thomas,
> 
> On Mon, 12 May 2025 at 16:08, Thomas Richard <thomas.richard@bootlin.com> wrote:
>> On 5/9/25 11:07, Geert Uytterhoeven wrote:
>>> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>>> Export all symbols and create header file for the GPIO forwarder library.
>>>> It will be used in the next changes.
>>>>
>>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>>
>> ...
>>
>>>> +
>>>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
>>>> +                       unsigned long config);
>>>> +
>>>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
>>>
>>> I would expect all of these to take gpiochip_fwd pointers instead of
>>> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
>>> that does not correspond to a gpiochip_fwd object, causing a crash?
>>
>> Indeed nothing prevents from passing gpio_chip pointer which does not
>> correspond to a gpiochip_fwd object.
>> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
>> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
>>
>> I can keep GPIO operations as is, and create exported wrappers which
>> take a gpiochip_fwd pointer as parameter, for example:
>>
>> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
>>                               unsigned long *mask,
>>                               unsigned long *bits)
>> {
>>         struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
>>
>>         return gpio_fwd_get_multiple_locked(chip, mask, bits);
>> }
>> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
>>
>> So exported functions are gpiochip_fwd_*().
> 
> That sounds fine to me.
> 
> BTW, do you need to use these functions as gpio_chip callbacks?
> If that is the case, they do no need to take struct gpio_chip pointers.
> 
I'm not sure to understand the question, or the idea behind the question.

Regards,

Thomas

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Geert Uytterhoeven]

Hi Thomas,

On Mon, 12 May 2025 at 17:01, Thomas Richard <thomas.richard@bootlin.com> wrote:
> On 5/12/25 16:39, Geert Uytterhoeven wrote:
> > On Mon, 12 May 2025 at 16:08, Thomas Richard <thomas.richard@bootlin.com> wrote:
> >> On 5/9/25 11:07, Geert Uytterhoeven wrote:
> >>> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> >>>> Export all symbols and create header file for the GPIO forwarder library.
> >>>> It will be used in the next changes.
> >>>>
> >>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >>
> >> ...
> >>
> >>>> +
> >>>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
> >>>> +                       unsigned long config);
> >>>> +
> >>>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
> >>>
> >>> I would expect all of these to take gpiochip_fwd pointers instead of
> >>> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
> >>> that does not correspond to a gpiochip_fwd object, causing a crash?
> >>
> >> Indeed nothing prevents from passing gpio_chip pointer which does not
> >> correspond to a gpiochip_fwd object.
> >> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
> >> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
> >>
> >> I can keep GPIO operations as is, and create exported wrappers which
> >> take a gpiochip_fwd pointer as parameter, for example:
> >>
> >> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
> >>                               unsigned long *mask,
> >>                               unsigned long *bits)
> >> {
> >>         struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
> >>
> >>         return gpio_fwd_get_multiple_locked(chip, mask, bits);
> >> }
> >> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
> >>
> >> So exported functions are gpiochip_fwd_*().
> >
> > That sounds fine to me.
> >
> > BTW, do you need to use these functions as gpio_chip callbacks?
> > If that is the case, they do no need to take struct gpio_chip pointers.
> >
> I'm not sure to understand the question, or the idea behind the question.

Do users of the forwarder library want to use these functions directly
as callbacks in their own gpiochip?
E.g. do they want to use:

    chip->get_multiple_rv = gpiochip_fwd_get_multiple;

I hope my question is more clear now.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 08/12] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

On 5/12/25 17:14, Geert Uytterhoeven wrote:
> Hi Thomas,
> 
> On Mon, 12 May 2025 at 17:01, Thomas Richard <thomas.richard@bootlin.com> wrote:
>> On 5/12/25 16:39, Geert Uytterhoeven wrote:
>>> On Mon, 12 May 2025 at 16:08, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>>> On 5/9/25 11:07, Geert Uytterhoeven wrote:
>>>>> On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>>>>> Export all symbols and create header file for the GPIO forwarder library.
>>>>>> It will be used in the next changes.
>>>>>>
>>>>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>>>>
>>>> ...
>>>>
>>>>>> +
>>>>>> +int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
>>>>>> +                       unsigned long config);
>>>>>> +
>>>>>> +int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset);
>>>>>
>>>>> I would expect all of these to take gpiochip_fwd pointers instead of
>>>>> gpio_chip pointers.  What prevents you from passing a gpio_chip pointer
>>>>> that does not correspond to a gpiochip_fwd object, causing a crash?
>>>>
>>>> Indeed nothing prevents from passing gpio_chip pointer which does not
>>>> correspond to a gpiochip_fwd object.
>>>> And it is also a bit weird to pass a gpiochip_fwd pointer in some cases
>>>> (for example gpio_fwd_gpio_add()) and a gpio_chip in other cases.
>>>>
>>>> I can keep GPIO operations as is, and create exported wrappers which
>>>> take a gpiochip_fwd pointer as parameter, for example:
>>>>
>>>> int gpiochip_fwd_get_multiple(struct gpiochip_fwd *fwd,
>>>>                               unsigned long *mask,
>>>>                               unsigned long *bits)
>>>> {
>>>>         struct gpio_chip *gc = gpiochip_fwd_get_gpiochip(fwd);
>>>>
>>>>         return gpio_fwd_get_multiple_locked(chip, mask, bits);
>>>> }
>>>> EXPORT_SYMBOL_NS_GPL(gpiochip_fwd_get_multiple, "GPIO_FORWARDER");
>>>>
>>>> So exported functions are gpiochip_fwd_*().
>>>
>>> That sounds fine to me.
>>>
>>> BTW, do you need to use these functions as gpio_chip callbacks?
>>> If that is the case, they do no need to take struct gpio_chip pointers.
>>>
>> I'm not sure to understand the question, or the idea behind the question.
> 
> Do users of the forwarder library want to use these functions directly
> as callbacks in their own gpiochip?
> E.g. do they want to use:
> 
>     chip->get_multiple_rv = gpiochip_fwd_get_multiple;
> 
> I hope my question is more clear now.

Oh ok I understand now.
The answer is no (gpiochip_fwd_get_multiple() is already by default the
get_multiple_rv operation of the forwarder).

My use case (patch 12/12) is:
I have a pinctrl driver (for a FPGA) which registers a gpiochip_fwd. The
driver has to drive in tandem its configuration and SoC GPIOs (which are
added in the gpiochip_fwd).
During the probe, the driver will change gpiochip operation to use its
own operation.

gc = gpiochip_fwd_get_gpiochip(fwd)
gc->direction_input = my_direction_input;

This function does some custom things and them call
gpiochip_fwd_gpio_direction_input().

my_direction_input()
{
	do_something()
	gpiochip_fwd_gpio_direction_input()
}

It allows you to add custom action before/after default operation.

Regards,

Thomas

[PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Thomas Richard]

Add request() callback to check if the GPIO descriptor was well registered
in the gpiochip_fwd before using it. This is done to handle the case where
GPIO descriptor is added at runtime in the forwarder.

If at least one GPIO descriptor was not added before the forwarder
registration, we assume the forwarder can sleep as if a GPIO is added at
runtime it may sleep.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 56 +++++++++++++++++++++++++++++++++++++-----
 include/linux/gpio/forwarder.h |  4 +++
 2 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 79fd44c2ceac..234f2f55c306 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -268,6 +268,7 @@ struct gpiochip_fwd {
 		spinlock_t slock;	/* protects tmp[] if !can_sleep */
 	};
 	struct gpiochip_fwd_timing *delay_timings;
+	unsigned long *valid_mask;
 	unsigned long tmp[];		/* values and descs for multiple ops */
 };
 
@@ -288,6 +289,21 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
 
+/**
+ * gpio_fwd_request - Request a line of the GPIO forwarder
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line to request
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return test_bit(offset, fwd->valid_mask) ? 0 : -ENODEV;
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_request, "GPIO_FORWARDER");
+
 /**
  * gpio_fwd_get_direction - Return the current direction of a GPIO forwarder line
  * @chip: GPIO chip in the forwarder
@@ -299,6 +315,13 @@ int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
+	/*
+	 * get_direction() is called during gpiochip registration, return input
+	 * direction if there is no descriptor for the line.
+	 */
+	if (!test_bit(offset, fwd->valid_mask))
+		return GPIO_LINE_DIRECTION_IN;
+
 	return gpiod_get_direction(fwd->descs[offset]);
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_direction, "GPIO_FORWARDER");
@@ -617,11 +640,16 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev, unsigned int ngpios
 	if (!fwd->descs)
 		return ERR_PTR(-ENOMEM);
 
+	fwd->valid_mask = devm_bitmap_zalloc(dev, ngpios, GFP_KERNEL);
+	if (!fwd->valid_mask)
+		return ERR_PTR(-ENOMEM);
+
 	chip = &fwd->chip;
 
 	chip->label = label;
 	chip->parent = dev;
 	chip->owner = THIS_MODULE;
+	chip->request = gpio_fwd_request;
 	chip->get_direction = gpio_fwd_get_direction;
 	chip->direction_input = gpio_fwd_direction_input;
 	chip->direction_output = gpio_fwd_direction_output;
@@ -648,24 +676,21 @@ EXPORT_SYMBOL_NS_GPL(devm_gpio_fwd_alloc, "GPIO_FORWARDER");
 int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 		      unsigned int offset)
 {
-	struct gpio_chip *parent = gpiod_to_chip(desc);
 	struct gpio_chip *chip = &fwd->chip;
 
 	if (offset > chip->ngpio)
 		return -EINVAL;
 
+	if (test_and_set_bit(offset, fwd->valid_mask))
+		return -EEXIST;
+
 	/*
 	 * If any of the GPIO lines are sleeping, then the entire forwarder
 	 * will be sleeping.
-	 * If any of the chips support .set_config(), then the forwarder will
-	 * support setting configs.
 	 */
 	if (gpiod_cansleep(desc))
 		chip->can_sleep = true;
 
-	if (parent && parent->set_config)
-		chip->set_config = gpio_fwd_set_config;
-
 	fwd->descs[offset] = desc;
 
 	dev_dbg(chip->parent, "%u => gpio %d irq %d\n", offset,
@@ -675,6 +700,18 @@ int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_add, "GPIO_FORWARDER");
 
+/**
+ * gpio_fwd_gpio_free - Remove a GPIO from the forwarder
+ * @fwd: GPIO forwarder
+ * @offset: offset of GPIO to remove
+ */
+void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset)
+{
+	if (test_and_clear_bit(offset, fwd->valid_mask))
+		gpiod_put(fwd->descs[offset]);
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_free, "GPIO_FORWARDER");
+
 /**
  * gpio_fwd_register - Register a GPIO forwarder
  * @fwd: GPIO forwarder
@@ -685,6 +722,13 @@ int gpio_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 
+	/*
+	 * Some gpio_desc were not registered. They will be registered at runtime
+	 * but we have to suppose they can sleep.
+	 */
+	if (!bitmap_full(fwd->valid_mask, chip->ngpio))
+		chip->can_sleep = true;
+
 	if (chip->can_sleep)
 		mutex_init(&fwd->mlock);
 	else
diff --git a/include/linux/gpio/forwarder.h b/include/linux/gpio/forwarder.h
index b17ad2c8e031..f799b0377efd 100644
--- a/include/linux/gpio/forwarder.h
+++ b/include/linux/gpio/forwarder.h
@@ -9,6 +9,8 @@ struct gpiochip_fwd;
 
 struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
 
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
+
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
 
 int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
@@ -37,6 +39,8 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
 int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
 		      struct gpio_desc *desc, unsigned int offset);
 
+void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset);
+
 int gpio_fwd_register(struct gpiochip_fwd *fwd);
 
 #endif

-- 
2.39.5

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Add request() callback to check if the GPIO descriptor was well registered
> in the gpiochip_fwd before using it. This is done to handle the case where
> GPIO descriptor is added at runtime in the forwarder.
>
> If at least one GPIO descriptor was not added before the forwarder
> registration, we assume the forwarder can sleep as if a GPIO is added at
> runtime it may sleep.

...

>  {
>         struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>
> +       /*
> +        * get_direction() is called during gpiochip registration, return input
> +        * direction if there is no descriptor for the line.
> +        */
> +       if (!test_bit(offset, fwd->valid_mask))
> +               return GPIO_LINE_DIRECTION_IN;

Can you remind me why we choose a valid return for invalid line? From
a pure code perspective this should return an error.

>         return gpiod_get_direction(fwd->descs[offset]);
>  }

...

> +       /*
> +        * Some gpio_desc were not registered. They will be registered at runtime
> +        * but we have to suppose they can sleep.

suppose --> assume ?

> +        */
> +       if (!bitmap_full(fwd->valid_mask, chip->ngpio))
> +               chip->can_sleep = true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Thomas Richard]

Hi Andy,

Thanks again for the review.

On 5/7/25 08:34, Andy Shevchenko wrote:
> On Tue, May 6, 2025 at 6:21 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> Add request() callback to check if the GPIO descriptor was well registered
>> in the gpiochip_fwd before using it. This is done to handle the case where
>> GPIO descriptor is added at runtime in the forwarder.
>>
>> If at least one GPIO descriptor was not added before the forwarder
>> registration, we assume the forwarder can sleep as if a GPIO is added at
>> runtime it may sleep.
> 
> ...
> 
>>  {
>>         struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>>
>> +       /*
>> +        * get_direction() is called during gpiochip registration, return input
>> +        * direction if there is no descriptor for the line.
>> +        */
>> +       if (!test_bit(offset, fwd->valid_mask))
>> +               return GPIO_LINE_DIRECTION_IN;
> 
> Can you remind me why we choose a valid return for invalid line? From
> a pure code perspective this should return an error.

I reproduced gpiolib behavior. During gpiochip registration, we get the
direction of all lines. In the case the line is not valid, it is marked
as input if direction_input operation exists, otherwise it is marked as
output. [1]

But in fact we could return an error and the core will mark the line as
input. Maybe ENODEV ?

[1]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L1105-L1123

Regards,

Thomas

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Wed, May 7, 2025 at 1:10 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 5/7/25 08:34, Andy Shevchenko wrote:
> > On Tue, May 6, 2025 at 6:21 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:

...

> >> +       /*
> >> +        * get_direction() is called during gpiochip registration, return input
> >> +        * direction if there is no descriptor for the line.
> >> +        */
> >> +       if (!test_bit(offset, fwd->valid_mask))
> >> +               return GPIO_LINE_DIRECTION_IN;
> >
> > Can you remind me why we choose a valid return for invalid line? From
> > a pure code perspective this should return an error.
>
> I reproduced gpiolib behavior. During gpiochip registration, we get the
> direction of all lines. In the case the line is not valid, it is marked
> as input if direction_input operation exists, otherwise it is marked as
> output. [1]
>
> But in fact we could return an error and the core will mark the line as
> input. Maybe ENODEV ?

I am fine with this error code, but do we have similar cases already
in the kernel? Do they use the same or different error code(s)?

> [1]
> https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L1105-L1123

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Thomas Richard]

On 5/7/25 15:24, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 1:10 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>> On 5/7/25 08:34, Andy Shevchenko wrote:
>>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard
>>> <thomas.richard@bootlin.com> wrote:
> 
> ...
> 
>>>> +       /*
>>>> +        * get_direction() is called during gpiochip registration, return input
>>>> +        * direction if there is no descriptor for the line.
>>>> +        */
>>>> +       if (!test_bit(offset, fwd->valid_mask))
>>>> +               return GPIO_LINE_DIRECTION_IN;
>>>
>>> Can you remind me why we choose a valid return for invalid line? From
>>> a pure code perspective this should return an error.
>>
>> I reproduced gpiolib behavior. During gpiochip registration, we get the
>> direction of all lines. In the case the line is not valid, it is marked
>> as input if direction_input operation exists, otherwise it is marked as
>> output. [1]
>>
>> But in fact we could return an error and the core will mark the line as
>> input. Maybe ENODEV ?
> 
> I am fine with this error code, but do we have similar cases already
> in the kernel? Do they use the same or different error code(s)?

I dumped all get_direction() operations in drivers/gpio and
drivers/pinctrl and returned values are:
- GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense).
- -EINVAL (for example [1]).
- -EBADE in gpiochip_get_direction() [2].
- regmap_read() return code.

But from my point of view -EINVAL and -EBADE do not match our case.

[1]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpio-cros-ec.c#L70
[2]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L359

Regards,

Thomas

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Wed, May 7, 2025 at 4:54 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 5/7/25 15:24, Andy Shevchenko wrote:
> > On Wed, May 7, 2025 at 1:10 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:
> >> On 5/7/25 08:34, Andy Shevchenko wrote:
> >>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard
> >>> <thomas.richard@bootlin.com> wrote:

...

> >>>> +       /*
> >>>> +        * get_direction() is called during gpiochip registration, return input
> >>>> +        * direction if there is no descriptor for the line.
> >>>> +        */
> >>>> +       if (!test_bit(offset, fwd->valid_mask))
> >>>> +               return GPIO_LINE_DIRECTION_IN;
> >>>
> >>> Can you remind me why we choose a valid return for invalid line? From
> >>> a pure code perspective this should return an error.
> >>
> >> I reproduced gpiolib behavior. During gpiochip registration, we get the
> >> direction of all lines. In the case the line is not valid, it is marked
> >> as input if direction_input operation exists, otherwise it is marked as
> >> output. [1]
> >>
> >> But in fact we could return an error and the core will mark the line as
> >> input. Maybe ENODEV ?
> >
> > I am fine with this error code, but do we have similar cases already
> > in the kernel? Do they use the same or different error code(s)?
>
> I dumped all get_direction() operations in drivers/gpio and
> drivers/pinctrl and returned values are:
> - GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense).
> - -EINVAL (for example [1]).
> - -EBADE in gpiochip_get_direction() [2].
> - regmap_read() return code.
>
> But from my point of view -EINVAL and -EBADE do not match our case.

Hmm... I believe we need a GPIO maintainer to have a look at this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Bartosz Golaszewski]

On Wed, May 7, 2025 at 5:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, May 7, 2025 at 4:54 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
> > On 5/7/25 15:24, Andy Shevchenko wrote:
> > > On Wed, May 7, 2025 at 1:10 PM Thomas Richard
> > > <thomas.richard@bootlin.com> wrote:
> > >> On 5/7/25 08:34, Andy Shevchenko wrote:
> > >>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard
> > >>> <thomas.richard@bootlin.com> wrote:
>
> ...
>
> > >>>> +       /*
> > >>>> +        * get_direction() is called during gpiochip registration, return input
> > >>>> +        * direction if there is no descriptor for the line.
> > >>>> +        */
> > >>>> +       if (!test_bit(offset, fwd->valid_mask))
> > >>>> +               return GPIO_LINE_DIRECTION_IN;
> > >>>
> > >>> Can you remind me why we choose a valid return for invalid line? From
> > >>> a pure code perspective this should return an error.
> > >>
> > >> I reproduced gpiolib behavior. During gpiochip registration, we get the
> > >> direction of all lines. In the case the line is not valid, it is marked
> > >> as input if direction_input operation exists, otherwise it is marked as
> > >> output. [1]
> > >>
> > >> But in fact we could return an error and the core will mark the line as
> > >> input. Maybe ENODEV ?
> > >
> > > I am fine with this error code, but do we have similar cases already
> > > in the kernel? Do they use the same or different error code(s)?
> >
> > I dumped all get_direction() operations in drivers/gpio and
> > drivers/pinctrl and returned values are:
> > - GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense).
> > - -EINVAL (for example [1]).
> > - -EBADE in gpiochip_get_direction() [2].
> > - regmap_read() return code.
> >
> > But from my point of view -EINVAL and -EBADE do not match our case.
>
> Hmm... I believe we need a GPIO maintainer to have a look at this.
>

I went with -EBADE in GPIO core to indicate that the underlying driver
borked and returned an invalid value. I'm not sure if this is the
right one here. I'm not against using -ENODEV.

Bart

Re: [PATCH v5 09/12] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Geert Uytterhoeven]

Hi Thomas,

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Add request() callback to check if the GPIO descriptor was well registered
> in the gpiochip_fwd before using it. This is done to handle the case where
> GPIO descriptor is added at runtime in the forwarder.
>
> If at least one GPIO descriptor was not added before the forwarder
> registration, we assume the forwarder can sleep as if a GPIO is added at
> runtime it may sleep.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c

> @@ -288,6 +289,21 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
>  }
>  EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
>
> +/**
> + * gpio_fwd_request - Request a line of the GPIO forwarder
> + * @chip: GPIO chip in the forwarder
> + * @offset: the offset of the line to request
> + *
> + * Returns: 0 on success, or negative errno on failure.
> + */
> +int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)

This function should take a gpiochip_fwd pointer instead.

> +{
> +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +       return test_bit(offset, fwd->valid_mask) ? 0 : -ENODEV;
> +}

Not related to this patch, but just a random note:

     * struct gpio_chip - abstract a GPIO controller
     [...[
     * @request: optional hook for chip-specific activation, such as
     *      enabling module power and clock; may sleep; must return 0 on success
     *      or negative error number on failure
     * @free: optional hook for chip-specific deactivation, such as
     *      disabling module power and clock; may sleep

Currently all GPIOs are requested when the aggregator is created,
which means they are always powered and clocked, irrespective of a
user of the aggregator has requested them or not.
I don't see how this can be improve, except by adding suspend/resume
callbacks to gpio_chip, which could be called from the aggegator's
.free() and .request() callbacks?

> +EXPORT_SYMBOL_NS_GPL(gpio_fwd_request, "GPIO_FORWARDER");
> +
>  /**
>   * gpio_fwd_get_direction - Return the current direction of a GPIO forwarder line
>   * @chip: GPIO chip in the forwarder

> @@ -675,6 +700,18 @@ int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
>  }
>  EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_add, "GPIO_FORWARDER");
>
> +/**
> + * gpio_fwd_gpio_free - Remove a GPIO from the forwarder
> + * @fwd: GPIO forwarder
> + * @offset: offset of GPIO to remove
> + */
> +void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset)
> +{
> +       if (test_and_clear_bit(offset, fwd->valid_mask))
> +               gpiod_put(fwd->descs[offset]);
> +}
> +EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_free, "GPIO_FORWARDER");

So this is _not_ the inverse of gpio_fwd_request()
(it has an extra "_gpio" in the name).

Naming is indeed tricky.
I was also wondering about gpio_fwd_get(), which does not get the
forwarder, but gets a GPIO of the forwarder...

> +
>  /**
>   * gpio_fwd_register - Register a GPIO forwarder
>   * @fwd: GPIO forwarder

> --- a/include/linux/gpio/forwarder.h
> +++ b/include/linux/gpio/forwarder.h
> @@ -9,6 +9,8 @@ struct gpiochip_fwd;
>
>  struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
>
> +int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
> +
>  int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
>
>  int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
> @@ -37,6 +39,8 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
>  int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
>                       struct gpio_desc *desc, unsigned int offset);
>
> +void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset);
> +
>  int gpio_fwd_register(struct gpiochip_fwd *fwd);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 10/12] gpio: aggregator: add possibility to attach data to the forwarder

[Thomas Richard]

Add a data pointer to store private data in the forwarder.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 20 ++++++++++++++++++--
 include/linux/gpio/forwarder.h |  4 +++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 234f2f55c306..102205793015 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -268,6 +268,7 @@ struct gpiochip_fwd {
 		spinlock_t slock;	/* protects tmp[] if !can_sleep */
 	};
 	struct gpiochip_fwd_timing *delay_timings;
+	void *data;
 	unsigned long *valid_mask;
 	unsigned long tmp[];		/* values and descs for multiple ops */
 };
@@ -289,6 +290,18 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
 
+/**
+ * gpio_fwd_get_data - Get data for the GPIO forwarder
+ * @fwd: GPIO forwarder
+ *
+ * Returns: The data for the GPIO forwarder
+ */
+void *gpio_fwd_get_data(struct gpiochip_fwd *fwd)
+{
+	return fwd->data;
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_data, "GPIO_FORWARDER");
+
 /**
  * gpio_fwd_request - Request a line of the GPIO forwarder
  * @chip: GPIO chip in the forwarder
@@ -715,10 +728,11 @@ EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_free, "GPIO_FORWARDER");
 /**
  * gpio_fwd_register - Register a GPIO forwarder
  * @fwd: GPIO forwarder
+ * @data: driver-private data associated with this forwarder
  *
  * Returns: 0 on success, or negative errno on failure.
  */
-int gpio_fwd_register(struct gpiochip_fwd *fwd)
+int gpio_fwd_register(struct gpiochip_fwd *fwd, void *data)
 {
 	struct gpio_chip *chip = &fwd->chip;
 
@@ -734,6 +748,8 @@ int gpio_fwd_register(struct gpiochip_fwd *fwd)
 	else
 		spin_lock_init(&fwd->slock);
 
+	fwd->data = data;
+
 	return devm_gpiochip_add_data(chip->parent, chip, fwd);
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_register, "GPIO_FORWARDER");
@@ -778,7 +794,7 @@ static struct gpiochip_fwd *gpio_fwd_create(struct device *dev,
 			return ERR_PTR(error);
 	}
 
-	error = gpio_fwd_register(fwd);
+	error = gpio_fwd_register(fwd, NULL);
 	if (error)
 		return ERR_PTR(error);
 
diff --git a/include/linux/gpio/forwarder.h b/include/linux/gpio/forwarder.h
index f799b0377efd..dc9b0f13413d 100644
--- a/include/linux/gpio/forwarder.h
+++ b/include/linux/gpio/forwarder.h
@@ -9,6 +9,8 @@ struct gpiochip_fwd;
 
 struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
 
+void *gpio_fwd_get_data(struct gpiochip_fwd *fwd);
+
 int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
 
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
@@ -41,6 +43,6 @@ int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
 
 void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset);
 
-int gpio_fwd_register(struct gpiochip_fwd *fwd);
+int gpio_fwd_register(struct gpiochip_fwd *fwd, void *data);
 
 #endif

-- 
2.39.5

Re: [PATCH v5 10/12] gpio: aggregator: add possibility to attach data to the forwarder

[Geert Uytterhoeven]

Hi Thomas,

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Add a data pointer to store private data in the forwarder.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c

> @@ -289,6 +290,18 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
>  }
>  EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
>
> +/**
> + * gpio_fwd_get_data - Get data for the GPIO forwarder
> + * @fwd: GPIO forwarder
> + *
> + * Returns: The data for the GPIO forwarder

driver-private data

> + */
> +void *gpio_fwd_get_data(struct gpiochip_fwd *fwd)
> +{
> +       return fwd->data;
> +}
> +EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_data, "GPIO_FORWARDER");
> +
>  /**
>   * gpio_fwd_request - Request a line of the GPIO forwarder
>   * @chip: GPIO chip in the forwarder

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 11/12] lib/string_choices: Add str_input_output() helper

[Thomas Richard]

Add str_input_output() helper to return 'input' or 'output' string literal.
Also add the inversed variant str_output_input().

Suggested-by: Andy Shevchenko <andy@kernel.org>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 include/linux/string_choices.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h
index f3ba4f52ff26..a27c87c954ae 100644
--- a/include/linux/string_choices.h
+++ b/include/linux/string_choices.h
@@ -41,6 +41,12 @@ static inline const char *str_high_low(bool v)
 }
 #define str_low_high(v)		str_high_low(!(v))
 
+static inline const char *str_input_output(bool v)
+{
+	return v ? "input" : "output";
+}
+#define str_output_input(v)	str_input_output(!(v))
+
 static inline const char *str_on_off(bool v)
 {
 	return v ? "on" : "off";

-- 
2.39.5

[PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

This enables the pin control support of the onboard FPGA on AAEON UP
boards.

This FPGA acts as a level shifter between the Intel SoC pins and the pin
header, and also as a mux or switch.

+---------+          +--------------+             +---+
          |          |              |             |   |
          | PWM0     |       \      |             | H |
          |----------|------  \-----|-------------| E |
          | I2C0_SDA |              |             | A |
Intel SoC |----------|------\       |             | D |
          | GPIO0    |       \------|-------------| E |
          |----------|------        |             | R |
          |          |     FPGA     |             |   |
----------+          +--------------+             +---+

For most of the pins, the FPGA opens/closes a switch to enable/disable
the access to the SoC pin from a pin header.
Each switch, has a direction flag that is set depending the status of the
SoC pin.

For some other pins, the FPGA acts as a mux, and routes one pin (or the
other one) to the header.

The driver also provides a GPIO chip. It requests SoC pins in GPIO mode,
and drives them in tandem with FPGA pins (switch/mux direction).

This commit adds support only for UP Squared board.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/Kconfig           |   19 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1068 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1088 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 464cc9aca157..b19eff31146b 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -590,6 +590,25 @@ config PINCTRL_TH1520
 	  This driver is needed for RISC-V development boards like
 	  the BeagleV Ahead and the LicheePi 4A.
 
+config PINCTRL_UPBOARD
+	tristate "AAeon UP board FPGA pin controller"
+	depends on MFD_UPBOARD_FPGA
+	select PINMUX
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GPIOLIB
+	select GPIO_AGGREGATOR
+	help
+	  Pin controller for the FPGA GPIO lines on UP boards. Due to the
+	  hardware layout, the driver controls the FPGA pins in tandem with
+	  their corresponding Intel SoC GPIOs.
+
+	  Currently supported:
+	  - UP Squared
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pinctrl-upboard.
+
 config PINCTRL_ZYNQ
 	bool "Pinctrl driver for Xilinx Zynq"
 	depends on ARCH_ZYNQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ac27e88677d1..f7246aead7b5 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_TPS6594)	+= pinctrl-tps6594.o
 obj-$(CONFIG_PINCTRL_TH1520)	+= pinctrl-th1520.o
+obj-$(CONFIG_PINCTRL_UPBOARD)	+= pinctrl-upboard.o
 obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 
diff --git a/drivers/pinctrl/pinctrl-upboard.c b/drivers/pinctrl/pinctrl-upboard.c
new file mode 100644
index 000000000000..097a8dbbd472
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,1068 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * UP board pin control driver.
+ *
+ * Copyright (C) 2025 Bootlin
+ *
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/gpio/forwarder.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+#include <linux/stddef.h>
+#include <linux/string_choices.h>
+#include <linux/types.h>
+
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+enum upboard_board_id {
+	UPBOARD_APL01,
+};
+
+enum upboard_pin_mode {
+	UPBOARD_PIN_MODE_FUNCTION,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_DISABLED,
+};
+
+struct upboard_pin {
+	struct regmap_field *funcbit;
+	struct regmap_field *enbit;
+	struct regmap_field *dirbit;
+};
+
+struct upboard_pingroup {
+	struct pingroup grp;
+	enum upboard_pin_mode mode;
+	const enum upboard_pin_mode *modes;
+};
+
+struct upboard_pinctrl_data {
+	const struct upboard_pingroup *groups;
+	size_t ngroups;
+	const struct pinfunction *funcs;
+	size_t nfuncs;
+	const unsigned int *pin_header;
+	size_t ngpio;
+};
+
+struct upboard_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+	const struct upboard_pinctrl_data *pctrl_data;
+	struct gpio_pin_range pin_range;
+	struct upboard_pin *pins;
+	const struct pinctrl_map *maps;
+	size_t nmaps;
+};
+
+enum upboard_func0_fpgabit {
+	UPBOARD_FUNC_I2C0_EN = 8,
+	UPBOARD_FUNC_I2C1_EN = 9,
+	UPBOARD_FUNC_CEC0_EN = 12,
+	UPBOARD_FUNC_ADC0_EN = 14,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_I2C0_EN, UPBOARD_FUNC_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_I2C1_EN, UPBOARD_FUNC_I2C1_EN);
+
+static const struct reg_field upboard_adc0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_ADC0_EN, UPBOARD_FUNC_ADC0_EN);
+
+#define UPBOARD_UP_BIT_TO_PIN(bit) UPBOARD_UP_BIT_##bit
+
+#define UPBOARD_UP_PIN_NAME(id)					\
+	{							\
+		.number = UPBOARD_UP_BIT_##id,			\
+		.name = #id,					\
+	}
+
+#define UPBOARD_UP_PIN_MUX(bit, data)				\
+	{							\
+		.number = UPBOARD_UP_BIT_##bit,			\
+		.name = "PINMUX_"#bit,				\
+		.drv_data = (void *)(data),			\
+	}
+
+#define UPBOARD_UP_PIN_FUNC(id, data)				\
+	{							\
+		.number = UPBOARD_UP_BIT_##id,			\
+		.name = #id,					\
+		.drv_data = (void *)(data),			\
+	}
+
+enum upboard_up_fpgabit {
+	UPBOARD_UP_BIT_I2C1_SDA,
+	UPBOARD_UP_BIT_I2C1_SCL,
+	UPBOARD_UP_BIT_ADC0,
+	UPBOARD_UP_BIT_UART1_RTS,
+	UPBOARD_UP_BIT_GPIO27,
+	UPBOARD_UP_BIT_GPIO22,
+	UPBOARD_UP_BIT_SPI_MOSI,
+	UPBOARD_UP_BIT_SPI_MISO,
+	UPBOARD_UP_BIT_SPI_CLK,
+	UPBOARD_UP_BIT_I2C0_SDA,
+	UPBOARD_UP_BIT_GPIO5,
+	UPBOARD_UP_BIT_GPIO6,
+	UPBOARD_UP_BIT_PWM1,
+	UPBOARD_UP_BIT_I2S_FRM,
+	UPBOARD_UP_BIT_GPIO26,
+	UPBOARD_UP_BIT_UART1_TX,
+	UPBOARD_UP_BIT_UART1_RX,
+	UPBOARD_UP_BIT_I2S_CLK,
+	UPBOARD_UP_BIT_GPIO23,
+	UPBOARD_UP_BIT_GPIO24,
+	UPBOARD_UP_BIT_GPIO25,
+	UPBOARD_UP_BIT_SPI_CS0,
+	UPBOARD_UP_BIT_SPI_CS1,
+	UPBOARD_UP_BIT_I2C0_SCL,
+	UPBOARD_UP_BIT_PWM0,
+	UPBOARD_UP_BIT_UART1_CTS,
+	UPBOARD_UP_BIT_I2S_DIN,
+	UPBOARD_UP_BIT_I2S_DOUT,
+};
+
+static const struct pinctrl_pin_desc upboard_up_pins[] = {
+	UPBOARD_UP_PIN_FUNC(I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(ADC0, &upboard_adc0_reg),
+	UPBOARD_UP_PIN_NAME(UART1_RTS),
+	UPBOARD_UP_PIN_NAME(GPIO27),
+	UPBOARD_UP_PIN_NAME(GPIO22),
+	UPBOARD_UP_PIN_NAME(SPI_MOSI),
+	UPBOARD_UP_PIN_NAME(SPI_MISO),
+	UPBOARD_UP_PIN_NAME(SPI_CLK),
+	UPBOARD_UP_PIN_FUNC(I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(GPIO5),
+	UPBOARD_UP_PIN_NAME(GPIO6),
+	UPBOARD_UP_PIN_NAME(PWM1),
+	UPBOARD_UP_PIN_NAME(I2S_FRM),
+	UPBOARD_UP_PIN_NAME(GPIO26),
+	UPBOARD_UP_PIN_NAME(UART1_TX),
+	UPBOARD_UP_PIN_NAME(UART1_RX),
+	UPBOARD_UP_PIN_NAME(I2S_CLK),
+	UPBOARD_UP_PIN_NAME(GPIO23),
+	UPBOARD_UP_PIN_NAME(GPIO24),
+	UPBOARD_UP_PIN_NAME(GPIO25),
+	UPBOARD_UP_PIN_NAME(SPI_CS0),
+	UPBOARD_UP_PIN_NAME(SPI_CS1),
+	UPBOARD_UP_PIN_FUNC(I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(PWM0),
+	UPBOARD_UP_PIN_NAME(UART1_CTS),
+	UPBOARD_UP_PIN_NAME(I2S_DIN),
+	UPBOARD_UP_PIN_NAME(I2S_DOUT),
+};
+
+static const unsigned int upboard_up_pin_header[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SDA),
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SDA),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP_BIT_TO_PIN(ADC0),
+	UPBOARD_UP_BIT_TO_PIN(GPIO5),
+	UPBOARD_UP_BIT_TO_PIN(GPIO6),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS1),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS0),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MISO),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MOSI),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CLK),
+	UPBOARD_UP_BIT_TO_PIN(PWM0),
+	UPBOARD_UP_BIT_TO_PIN(PWM1),
+	UPBOARD_UP_BIT_TO_PIN(UART1_TX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_CTS),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP_BIT_TO_PIN(I2S_CLK),
+	UPBOARD_UP_BIT_TO_PIN(I2S_FRM),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DIN),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DOUT),
+	UPBOARD_UP_BIT_TO_PIN(GPIO22),
+	UPBOARD_UP_BIT_TO_PIN(GPIO23),
+	UPBOARD_UP_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP_BIT_TO_PIN(GPIO25),
+	UPBOARD_UP_BIT_TO_PIN(GPIO26),
+	UPBOARD_UP_BIT_TO_PIN(GPIO27),
+};
+
+static const unsigned int upboard_up_uart1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(UART1_TX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP_BIT_TO_PIN(UART1_CTS),
+};
+
+static const enum upboard_pin_mode upboard_up_uart1_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_uart1_modes) == ARRAY_SIZE(upboard_up_uart1_pins));
+
+static const unsigned int upboard_up_i2c0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SDA),
+};
+
+static const unsigned int upboard_up_i2c1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SDA),
+};
+
+static const unsigned int upboard_up_spi2_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(SPI_MOSI),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MISO),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CLK),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS0),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS1),
+};
+
+static const enum upboard_pin_mode upboard_up_spi2_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_spi2_modes) == ARRAY_SIZE(upboard_up_spi2_pins));
+
+static const unsigned int upboard_up_i2s0_pins[]  = {
+	UPBOARD_UP_BIT_TO_PIN(I2S_FRM),
+	UPBOARD_UP_BIT_TO_PIN(I2S_CLK),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DIN),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DOUT),
+};
+
+static const enum upboard_pin_mode upboard_up_i2s0_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_i2s0_pins) == ARRAY_SIZE(upboard_up_i2s0_modes));
+
+static const unsigned int upboard_up_pwm0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(PWM0),
+};
+
+static const unsigned int upboard_up_pwm1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(PWM1),
+};
+
+static const unsigned int upboard_up_adc0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(ADC0),
+};
+
+#define UPBOARD_PINGROUP(n, p, m)								\
+{												\
+	.grp = PINCTRL_PINGROUP(n, p, ARRAY_SIZE(p)),						\
+	.mode = __builtin_choose_expr(								\
+			__builtin_types_compatible_p(typeof(m), const enum upboard_pin_mode *),	\
+			0, m),									\
+	.modes = __builtin_choose_expr(								\
+			__builtin_types_compatible_p(typeof(m), const enum upboard_pin_mode *), \
+			m, NULL),								\
+}
+
+static const struct upboard_pingroup upboard_up_pin_groups[] = {
+	UPBOARD_PINGROUP("uart1_grp", upboard_up_uart1_pins, &upboard_up_uart1_modes[0]),
+	UPBOARD_PINGROUP("i2c0_grp", upboard_up_i2c0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("i2c1_grp", upboard_up_i2c1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("spi2_grp", upboard_up_spi2_pins, &upboard_up_spi2_modes[0]),
+	UPBOARD_PINGROUP("i2s0_grp", upboard_up_i2s0_pins, &upboard_up_i2s0_modes[0]),
+	UPBOARD_PINGROUP("pwm0_grp", upboard_up_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("pwm1_grp", upboard_up_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("adc0_grp", upboard_up_adc0_pins, UPBOARD_PIN_MODE_GPIO_IN),
+};
+
+static const char * const upboard_up_uart1_groups[] = { "uart1_grp" };
+static const char * const upboard_up_i2c0_groups[]  = { "i2c0_grp" };
+static const char * const upboard_up_i2c1_groups[]  = { "i2c1_grp" };
+static const char * const upboard_up_spi2_groups[]  = { "spi2_grp" };
+static const char * const upboard_up_i2s0_groups[]  = { "i2s0_grp" };
+static const char * const upboard_up_pwm0_groups[]  = { "pwm0_grp" };
+static const char * const upboard_up_pwm1_groups[]  = { "pwm1_grp" };
+static const char * const upboard_up_adc0_groups[]  = { "adc0_grp" };
+
+#define UPBOARD_FUNCTION(func, groups)	PINCTRL_PINFUNCTION(func, groups, ARRAY_SIZE(groups))
+
+static const struct pinfunction upboard_up_pin_functions[] = {
+	UPBOARD_FUNCTION("uart1", upboard_up_uart1_groups),
+	UPBOARD_FUNCTION("i2c0", upboard_up_i2c0_groups),
+	UPBOARD_FUNCTION("i2c1", upboard_up_i2c1_groups),
+	UPBOARD_FUNCTION("spi2", upboard_up_spi2_groups),
+	UPBOARD_FUNCTION("i2s0", upboard_up_i2s0_groups),
+	UPBOARD_FUNCTION("pwm0", upboard_up_pwm0_groups),
+	UPBOARD_FUNCTION("pwm1", upboard_up_pwm1_groups),
+	UPBOARD_FUNCTION("adc0", upboard_up_adc0_groups),
+};
+
+static const struct upboard_pinctrl_data upboard_up_pinctrl_data = {
+	.groups = &upboard_up_pin_groups[0],
+	.ngroups = ARRAY_SIZE(upboard_up_pin_groups),
+	.funcs = &upboard_up_pin_functions[0],
+	.nfuncs = ARRAY_SIZE(upboard_up_pin_functions),
+	.pin_header = &upboard_up_pin_header[0],
+	.ngpio = ARRAY_SIZE(upboard_up_pin_header),
+};
+
+#define UPBOARD_UP2_BIT_TO_PIN(bit) UPBOARD_UP2_BIT_##bit
+
+#define UPBOARD_UP2_PIN_NAME(id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##id,				\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_MUX(bit, data)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##bit,			\
+		.name = "PINMUX_"#bit,					\
+		.drv_data = (void *)(data),				\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(id, data)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##id,				\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_fpgabit {
+	UPBOARD_UP2_BIT_UART1_TXD,
+	UPBOARD_UP2_BIT_UART1_RXD,
+	UPBOARD_UP2_BIT_UART1_RTS,
+	UPBOARD_UP2_BIT_UART1_CTS,
+	UPBOARD_UP2_BIT_GPIO3_ADC0,
+	UPBOARD_UP2_BIT_GPIO5_ADC2,
+	UPBOARD_UP2_BIT_GPIO6_ADC3,
+	UPBOARD_UP2_BIT_GPIO11,
+	UPBOARD_UP2_BIT_EXHAT_LVDS1n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS1p,
+	UPBOARD_UP2_BIT_SPI2_TXD,
+	UPBOARD_UP2_BIT_SPI2_RXD,
+	UPBOARD_UP2_BIT_SPI2_FS1,
+	UPBOARD_UP2_BIT_SPI2_FS0,
+	UPBOARD_UP2_BIT_SPI2_CLK,
+	UPBOARD_UP2_BIT_SPI1_TXD,
+	UPBOARD_UP2_BIT_SPI1_RXD,
+	UPBOARD_UP2_BIT_SPI1_FS1,
+	UPBOARD_UP2_BIT_SPI1_FS0,
+	UPBOARD_UP2_BIT_SPI1_CLK,
+	UPBOARD_UP2_BIT_I2C0_SCL,
+	UPBOARD_UP2_BIT_I2C0_SDA,
+	UPBOARD_UP2_BIT_I2C1_SCL,
+	UPBOARD_UP2_BIT_I2C1_SDA,
+	UPBOARD_UP2_BIT_PWM1,
+	UPBOARD_UP2_BIT_PWM0,
+	UPBOARD_UP2_BIT_EXHAT_LVDS0n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS0p,
+	UPBOARD_UP2_BIT_GPIO24,
+	UPBOARD_UP2_BIT_GPIO10,
+	UPBOARD_UP2_BIT_GPIO2,
+	UPBOARD_UP2_BIT_GPIO1,
+	UPBOARD_UP2_BIT_EXHAT_LVDS3n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS3p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS4n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS4p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS5n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS5p,
+	UPBOARD_UP2_BIT_I2S_SDO,
+	UPBOARD_UP2_BIT_I2S_SDI,
+	UPBOARD_UP2_BIT_I2S_WS_SYNC,
+	UPBOARD_UP2_BIT_I2S_BCLK,
+	UPBOARD_UP2_BIT_EXHAT_LVDS6n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS6p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS7n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS7p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS2n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS2p,
+};
+
+static const struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(GPIO3_ADC0),
+	UPBOARD_UP2_PIN_NAME(GPIO5_ADC2),
+	UPBOARD_UP2_PIN_NAME(GPIO6_ADC3),
+	UPBOARD_UP2_PIN_NAME(GPIO11),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(SPI1_CLK),
+	UPBOARD_UP2_PIN_MUX(I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_MUX(I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(PWM1),
+	UPBOARD_UP2_PIN_NAME(PWM0),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_MUX(GPIO24, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO10, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO2, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO1, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS2p),
+};
+
+static const unsigned int upboard_up2_pin_header[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO10),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO1),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO2),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO3_ADC0),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO11),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(PWM0),
+	UPBOARD_UP2_BIT_TO_PIN(PWM1),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_CTS),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_BCLK),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_WS_SYNC),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDI),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDO),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO6_ADC3),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO5_ADC2),
+};
+
+static const unsigned int upboard_up2_uart1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(UART1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_CTS),
+};
+
+static const enum upboard_pin_mode upboard_up2_uart1_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_uart1_modes) == ARRAY_SIZE(upboard_up2_uart1_pins));
+
+static const unsigned int upboard_up2_i2c0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(I2C0_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO10),
+};
+
+static const unsigned int upboard_up2_i2c1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(I2C1_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO2),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO1),
+};
+
+static const unsigned int upboard_up2_spi1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_CLK),
+};
+
+static const unsigned int upboard_up2_spi2_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_CLK),
+};
+
+static const enum upboard_pin_mode upboard_up2_spi_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_spi_modes) == ARRAY_SIZE(upboard_up2_spi1_pins));
+
+static_assert(ARRAY_SIZE(upboard_up2_spi_modes) == ARRAY_SIZE(upboard_up2_spi2_pins));
+
+static const unsigned int upboard_up2_i2s0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2S_BCLK),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_WS_SYNC),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDI),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDO),
+};
+
+static const enum upboard_pin_mode upboard_up2_i2s0_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_i2s0_modes) == ARRAY_SIZE(upboard_up2_i2s0_pins));
+
+static const unsigned int upboard_up2_pwm0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(PWM0),
+};
+
+static const unsigned int upboard_up2_pwm1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(PWM1),
+};
+
+static const unsigned int upboard_up2_adc0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO3_ADC0),
+};
+
+static const unsigned int upboard_up2_adc2_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO5_ADC2),
+};
+
+static const unsigned int upboard_up2_adc3_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO6_ADC3),
+};
+
+static const struct upboard_pingroup upboard_up2_pin_groups[] = {
+	UPBOARD_PINGROUP("uart1_grp", upboard_up2_uart1_pins, &upboard_up2_uart1_modes[0]),
+	UPBOARD_PINGROUP("i2c0_grp", upboard_up2_i2c0_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP("i2c1_grp", upboard_up2_i2c1_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP("spi1_grp", upboard_up2_spi1_pins, &upboard_up2_spi_modes[0]),
+	UPBOARD_PINGROUP("spi2_grp", upboard_up2_spi2_pins, &upboard_up2_spi_modes[0]),
+	UPBOARD_PINGROUP("i2s0_grp", upboard_up2_i2s0_pins, &upboard_up2_i2s0_modes[0]),
+	UPBOARD_PINGROUP("pwm0_grp", upboard_up2_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("pwm1_grp", upboard_up2_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP("adc0_grp", upboard_up2_adc0_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP("adc2_grp", upboard_up2_adc2_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP("adc3_grp", upboard_up2_adc3_pins, UPBOARD_PIN_MODE_GPIO_IN),
+};
+
+static const char * const upboard_up2_uart1_groups[] = { "uart1_grp" };
+static const char * const upboard_up2_i2c0_groups[]  = { "i2c0_grp" };
+static const char * const upboard_up2_i2c1_groups[]  = { "i2c1_grp" };
+static const char * const upboard_up2_spi1_groups[]  = { "spi1_grp" };
+static const char * const upboard_up2_spi2_groups[]  = { "spi2_grp" };
+static const char * const upboard_up2_i2s0_groups[]  = { "i2s0_grp" };
+static const char * const upboard_up2_pwm0_groups[]  = { "pwm0_grp" };
+static const char * const upboard_up2_pwm1_groups[]  = { "pwm1_grp" };
+static const char * const upboard_up2_adc0_groups[]  = { "adc0_grp" };
+static const char * const upboard_up2_adc2_groups[]  = { "adc2_grp" };
+static const char * const upboard_up2_adc3_groups[]  = { "adc3_grp" };
+
+static const struct pinfunction upboard_up2_pin_functions[] = {
+	UPBOARD_FUNCTION("uart1", upboard_up2_uart1_groups),
+	UPBOARD_FUNCTION("i2c0", upboard_up2_i2c0_groups),
+	UPBOARD_FUNCTION("i2c1", upboard_up2_i2c1_groups),
+	UPBOARD_FUNCTION("spi1", upboard_up2_spi1_groups),
+	UPBOARD_FUNCTION("spi2", upboard_up2_spi2_groups),
+	UPBOARD_FUNCTION("i2s0", upboard_up2_i2s0_groups),
+	UPBOARD_FUNCTION("pwm0", upboard_up2_pwm0_groups),
+	UPBOARD_FUNCTION("pwm1", upboard_up2_pwm1_groups),
+	UPBOARD_FUNCTION("adc0", upboard_up2_adc0_groups),
+	UPBOARD_FUNCTION("adc2", upboard_up2_adc2_groups),
+	UPBOARD_FUNCTION("adc3", upboard_up2_adc3_groups),
+};
+
+static const struct upboard_pinctrl_data upboard_up2_pinctrl_data = {
+	.groups = &upboard_up2_pin_groups[0],
+	.ngroups = ARRAY_SIZE(upboard_up2_pin_groups),
+	.funcs = &upboard_up2_pin_functions[0],
+	.nfuncs = ARRAY_SIZE(upboard_up2_pin_functions),
+	.pin_header = &upboard_up2_pin_header[0],
+	.ngpio = ARRAY_SIZE(upboard_up2_pin_header),
+};
+
+static int upboard_pinctrl_set_function(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+	int ret;
+
+	if (!p->funcbit)
+		return -EPERM;
+
+	ret = regmap_field_write(p->enbit, 0);
+	if (ret)
+		return ret;
+
+	return regmap_field_write(p->funcbit, 1);
+}
+
+static int upboard_pinctrl_gpio_commit_enable(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+	int ret;
+
+	if (p->funcbit) {
+		ret = regmap_field_write(p->funcbit, 0);
+		if (ret)
+			return ret;
+	}
+
+	return regmap_field_write(p->enbit, 1);
+}
+
+static int upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
+					       struct pinctrl_gpio_range *range,
+					       unsigned int offset)
+{
+	return upboard_pinctrl_gpio_commit_enable(pctldev, offset);
+}
+
+static void upboard_pinctrl_gpio_commit_disable(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+
+	regmap_field_write(p->enbit, 0);
+};
+
+static void upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range, unsigned int offset)
+{
+	return upboard_pinctrl_gpio_commit_disable(pctldev, offset);
+}
+
+static int upboard_pinctrl_gpio_commit_direction(struct pinctrl_dev *pctldev, unsigned int offset,
+						 bool input)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+
+	return regmap_field_write(p->dirbit, input);
+}
+
+static int upboard_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range,
+					      unsigned int offset, bool input)
+{
+	return upboard_pinctrl_gpio_commit_direction(pctldev, offset, input);
+}
+
+static int upboard_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
+				   unsigned int group_selector)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct upboard_pinctrl_data *pctrl_data = pctrl->pctrl_data;
+	const struct upboard_pingroup *upgroups = pctrl_data->groups;
+	struct group_desc *grp;
+	unsigned int mode, i;
+	int ret;
+
+	grp = pinctrl_generic_get_group(pctldev, group_selector);
+	if (!grp)
+		return -EINVAL;
+
+	for (i = 0; i < grp->grp.npins; i++) {
+		mode = upgroups[group_selector].mode ?: upgroups[group_selector].modes[i];
+		if (mode == UPBOARD_PIN_MODE_FUNCTION) {
+			ret = upboard_pinctrl_set_function(pctldev, grp->grp.pins[i]);
+			if (ret)
+				return ret;
+
+			continue;
+		}
+
+		ret = upboard_pinctrl_gpio_commit_enable(pctldev, grp->grp.pins[i]);
+		if (ret)
+			return ret;
+
+		ret = upboard_pinctrl_gpio_commit_direction(pctldev, grp->grp.pins[i],
+							    mode == UPBOARD_PIN_MODE_GPIO_IN);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = upboard_pinctrl_set_mux,
+	.gpio_request_enable = upboard_pinctrl_gpio_request_enable,
+	.gpio_disable_free = upboard_pinctrl_gpio_disable_free,
+	.gpio_set_direction = upboard_pinctrl_gpio_set_direction,
+};
+
+static int upboard_pinctrl_pin_get_mode(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[pin];
+	unsigned int val;
+	int ret;
+
+	if (p->funcbit) {
+		ret = regmap_field_read(p->funcbit, &val);
+		if (ret)
+			return ret;
+		if (val)
+			return UPBOARD_PIN_MODE_FUNCTION;
+	}
+
+	ret = regmap_field_read(p->enbit, &val);
+	if (ret)
+		return ret;
+	if (!val)
+		return UPBOARD_PIN_MODE_DISABLED;
+
+	ret = regmap_field_read(p->dirbit, &val);
+	if (ret)
+		return ret;
+
+	return val ? UPBOARD_PIN_MODE_GPIO_IN : UPBOARD_PIN_MODE_GPIO_OUT;
+}
+
+static void upboard_pinctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+				     unsigned int offset)
+{
+	int ret;
+
+	ret = upboard_pinctrl_pin_get_mode(pctldev, offset);
+	if (ret == UPBOARD_PIN_MODE_FUNCTION)
+		seq_puts(s, "mode function ");
+	else if (ret == UPBOARD_PIN_MODE_DISABLED)
+		seq_puts(s, "HIGH-Z ");
+	else
+		seq_printf(s, "GPIO (%s) ", str_input_output(ret == UPBOARD_PIN_MODE_GPIO_IN));
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.pin_dbg_show = upboard_pinctrl_dbg_show,
+};
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(gc);
+	struct upboard_pinctrl *pctrl = gpio_fwd_get_data(fwd);
+	unsigned int pin = pctrl->pctrl_data->pin_header[offset];
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = pinctrl_gpio_request(gc, offset);
+	if (ret)
+		return ret;
+
+	desc = gpiod_get_index(pctrl->dev, "external", pin, 0);
+	if (IS_ERR(desc)) {
+		pinctrl_gpio_free(gc, offset);
+		return PTR_ERR(desc);
+	}
+
+	return gpio_fwd_gpio_add(fwd, desc, offset);
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(gc);
+
+	gpio_fwd_gpio_free(fwd, offset);
+	pinctrl_gpio_free(gc, offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(gc);
+	struct upboard_pinctrl *pctrl = gpio_fwd_get_data(fwd);
+	unsigned int pin = pctrl->pctrl_data->pin_header[offset];
+	int mode;
+
+	/* If the pin is in function mode or high-z, input direction is returned */
+	mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);
+	if (mode < 0)
+		return mode;
+
+	if (mode == UPBOARD_PIN_MODE_GPIO_OUT)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	int ret;
+
+	ret = pinctrl_gpio_direction_input(gc, offset);
+	if (ret)
+		return ret;
+
+	return gpio_fwd_direction_input(gc, offset);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	int ret;
+
+	ret = pinctrl_gpio_direction_output(gc, offset);
+	if (ret)
+		return ret;
+
+	return gpio_fwd_direction_output(gc, offset, value);
+}
+
+static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
+{
+	const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
+	size_t ngroups = pctrl->pctrl_data->ngroups;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ngroups; i++) {
+		ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
+						groups[i].grp.pins, groups[i].grp.npins, pctrl);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
+{
+	const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
+	size_t nfuncs = pctrl->pctrl_data->nfuncs;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < nfuncs ; i++) {
+		ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
+						  funcs[i].groups, funcs[i].ngroups, NULL);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinctrl_map upboard_pinctrl_mapping_apl01[] = {
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:00", "pwm0_grp", "pwm0"),
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:00", "pwm1_grp", "pwm1"),
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:00", "uart1_grp", "uart1"),
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:02", "i2c0_grp", "i2c0"),
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:02", "i2c1_grp", "i2c1"),
+	PIN_MAP_MUX_GROUP_DEFAULT("upboard-pinctrl", "INT3452:01", "ssp0_grp", "ssp0"),
+};
+
+static const struct dmi_system_id dmi_platform_info[] = {
+	{
+		/* UP Squared */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
+		},
+		.driver_data = (void *)UPBOARD_APL01,
+	},
+	{ }
+};
+
+static int upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_fpga *fpga = dev_get_drvdata(dev->parent);
+	const struct dmi_system_id *dmi_id;
+	enum upboard_board_id board_id;
+	struct pinctrl_desc *pctldesc;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	struct gpiochip_fwd *fwd;
+	struct pinctrl *pinctrl;
+	struct gpio_chip *chip;
+	unsigned int i;
+	int ret;
+
+	pctldesc = devm_kzalloc(dev, sizeof(*pctldesc), GFP_KERNEL);
+	if (!pctldesc)
+		return -ENOMEM;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	switch (fpga->fpga_data->type) {
+	case UPBOARD_UP_FPGA:
+		pctldesc->pins = upboard_up_pins;
+		pctldesc->npins = ARRAY_SIZE(upboard_up_pins);
+		pctrl->pctrl_data = &upboard_up_pinctrl_data;
+		break;
+	case UPBOARD_UP2_FPGA:
+		pctldesc->pins = upboard_up2_pins;
+		pctldesc->npins = ARRAY_SIZE(upboard_up2_pins);
+		pctrl->pctrl_data = &upboard_up2_pinctrl_data;
+		break;
+	default:
+		return dev_err_probe(dev, -ENODEV, "Unsupported device type %d\n",
+				     fpga->fpga_data->type);
+	}
+
+	dmi_id = dmi_first_match(dmi_platform_info);
+	if (!dmi_id)
+		return -ENODEV;
+
+	board_id = (enum upboard_board_id)(unsigned long)dmi_id->driver_data;
+
+	switch (board_id) {
+	case UPBOARD_APL01:
+		pctrl->maps = upboard_pinctrl_mapping_apl01;
+		pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
+		break;
+	default:
+		return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
+	}
+
+	pctldesc->name = dev_name(dev);
+	pctldesc->owner = THIS_MODULE;
+	pctldesc->pctlops = &upboard_pinctrl_ops;
+	pctldesc->pmxops = &upboard_pinmux_ops;
+
+	pctrl->dev = dev;
+
+	pins = devm_kcalloc(dev, pctldesc->npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	/* Initialize pins */
+	for (i = 0; i < pctldesc->npins; i++) {
+		const struct pinctrl_pin_desc *pin_desc = &pctldesc->pins[i];
+		unsigned int regoff = pin_desc->number / UPBOARD_REGISTER_SIZE;
+		unsigned int lsb = pin_desc->number % UPBOARD_REGISTER_SIZE;
+		struct reg_field * const fld_func = pin_desc->drv_data;
+		struct upboard_pin *pin = &pins[i];
+		struct reg_field fldconf = {};
+
+		if (fld_func) {
+			pin->funcbit = devm_regmap_field_alloc(dev, fpga->regmap, *fld_func);
+			if (IS_ERR(pin->funcbit))
+				return PTR_ERR(pin->funcbit);
+		}
+
+		fldconf.reg = UPBOARD_REG_GPIO_EN0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+		pin->enbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
+		if (IS_ERR(pin->enbit))
+			return PTR_ERR(pin->enbit);
+
+		fldconf.reg = UPBOARD_REG_GPIO_DIR0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+		pin->dirbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
+		if (IS_ERR(pin->dirbit))
+			return PTR_ERR(pin->dirbit);
+	}
+
+	pctrl->pins = pins;
+
+	ret = devm_pinctrl_register_and_init(dev, pctldesc, pctrl, &pctrl->pctldev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
+
+	ret = upboard_pinctrl_register_groups(pctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register groups\n");
+
+	ret = upboard_pinctrl_register_functions(pctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register functions\n");
+
+	ret = devm_pinctrl_register_mappings(dev, pctrl->maps, pctrl->nmaps);
+	if (ret)
+		return ret;
+
+	pinctrl = devm_pinctrl_get_select_default(dev);
+	if (IS_ERR(pinctrl))
+		return dev_err_probe(dev, PTR_ERR(pinctrl), "Failed to select pinctrl\n");
+
+	ret = pinctrl_enable(pctrl->pctldev);
+	if (ret)
+		return ret;
+
+	fwd = devm_gpio_fwd_alloc(dev, pctrl->pctrl_data->ngpio);
+	if (IS_ERR(fwd))
+		return dev_err_probe(dev, PTR_ERR(fwd), "Failed to allocate the gpiochip forwarder\n");
+
+	chip = gpio_fwd_get_gpiochip(fwd);
+	chip->request = upboard_gpio_request;
+	chip->free = upboard_gpio_free;
+	chip->get_direction = upboard_gpio_get_direction;
+	chip->direction_output = upboard_gpio_direction_output;
+	chip->direction_input = upboard_gpio_direction_input;
+
+	ret = gpio_fwd_register(fwd, pctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register the gpiochip forwarder\n");
+
+	return gpiochip_add_sparse_pin_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
+					     pctrl->pctrl_data->ngpio);
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+	.probe = upboard_pinctrl_probe,
+};
+module_platform_driver(upboard_pinctrl_driver);
+
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com");
+MODULE_DESCRIPTION("UP Board HAT pin controller driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:upboard-pinctrl");
+MODULE_IMPORT_NS("GPIO_FORWARDER");

-- 
2.39.5

Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
>
> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> header, and also as a mux or switch.
>
> +---------+          +--------------+             +---+
>           |          |              |             |   |
>           | PWM0     |       \      |             | H |
>           |----------|------  \-----|-------------| E |
>           | I2C0_SDA |              |             | A |
> Intel SoC |----------|------\       |             | D |
>           | GPIO0    |       \------|-------------| E |
>           |----------|------        |             | R |
>           |          |     FPGA     |             |   |
> ----------+          +--------------+             +---+
>
> For most of the pins, the FPGA opens/closes a switch to enable/disable
> the access to the SoC pin from a pin header.
> Each switch, has a direction flag that is set depending the status of the
> SoC pin.
>
> For some other pins, the FPGA acts as a mux, and routes one pin (or the
> other one) to the header.
>
> The driver also provides a GPIO chip. It requests SoC pins in GPIO mode,
> and drives them in tandem with FPGA pins (switch/mux direction).
>
> This commit adds support only for UP Squared board.

...

> +#define UPBOARD_UP_PIN_MUX(bit, data)                          \
> +       {                                                       \
> +               .number = UPBOARD_UP_BIT_##bit,                 \
> +               .name = "PINMUX_"#bit,                          \
> +               .drv_data = (void *)(data),                     \

Wondering why we need to cast here and there if currently we all use
constant driver data. Perhaps enable const for now and if we ever need
that to be modified by the consumer we add that.

> +       }
> +
> +#define UPBOARD_UP_PIN_FUNC(id, data)                          \
> +       {                                                       \
> +               .number = UPBOARD_UP_BIT_##id,                  \
> +               .name = #id,                                    \
> +               .drv_data = (void *)(data),                     \

Ditto.

> +       }

...

> +static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
> +{
> +       const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
> +       size_t ngroups = pctrl->pctrl_data->ngroups;
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < ngroups; i++) {
> +               ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
> +                                               groups[i].grp.pins, groups[i].grp.npins, pctrl);

> +               if (ret < 0)

Why ' < 0' ?

> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
> +{
> +       const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
> +       size_t nfuncs = pctrl->pctrl_data->nfuncs;
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < nfuncs ; i++) {
> +               ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
> +                                                 funcs[i].groups, funcs[i].ngroups, NULL);
> +               if (ret < 0)

Ditto.

> +                       return ret;
> +       }
> +
> +       return 0;
> +}

...

> +       board_id = (enum upboard_board_id)(unsigned long)dmi_id->driver_data;
> +

Unneeded blank line.

> +       switch (board_id) {
> +       case UPBOARD_APL01:
> +               pctrl->maps = upboard_pinctrl_mapping_apl01;
> +               pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
> +               break;
> +       default:
> +               return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
> +       }

And actually we can get rid of that train of castings by switching to
use the info type of the structure

(namings are just constructed without checking for the better or
existing ones, choose another if you think they fit)

struct upboard_pinctrl_map {
  ... *maps;
  ... nmaps;
};

static const struct upboard_pinctrl_map apl01 = {
  ...
};

...dmi... = {
  ...
  .data = (void *)&apl01,
  ...
};

board_map = (const ...) dmi_id->driver_data;

...

But since DMI will require castings, it's up to you as I don't like
the idea of having that driver data not to be const in the first
place.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 5/7/25 08:57, Andy Shevchenko wrote:
> 
>> +#define UPBOARD_UP_PIN_MUX(bit, data)                          \
>> +       {                                                       \
>> +               .number = UPBOARD_UP_BIT_##bit,                 \
>> +               .name = "PINMUX_"#bit,                          \
>> +               .drv_data = (void *)(data),                     \
> 
> Wondering why we need to cast here and there if currently we all use
> constant driver data. Perhaps enable const for now and if we ever need
> that to be modified by the consumer we add that.

We need to cast as drv_data is not const, so the compiler complains.
Or I can remove const.
I looked at other drivers, some do a cast, others have not const driver
data.

> 
>> +       }
>> +
>> +#define UPBOARD_UP_PIN_FUNC(id, data)                          \
>> +       {                                                       \
>> +               .number = UPBOARD_UP_BIT_##id,                  \
>> +               .name = #id,                                    \
>> +               .drv_data = (void *)(data),                     \
> 
> Ditto.
> 
>> +       }
> 
> ...
> 
>> +static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
>> +{
>> +       const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
>> +       size_t ngroups = pctrl->pctrl_data->ngroups;
>> +       unsigned int i;
>> +       int ret;
>> +
>> +       for (i = 0; i < ngroups; i++) {
>> +               ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
>> +                                               groups[i].grp.pins, groups[i].grp.npins, pctrl);
> 
>> +               if (ret < 0)
> 
> Why ' < 0' ?

pinctrl_generic_add_group() returns the group selector which can be >=
0. So all values >= 0 are valid. [1]

[1]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/pinctrl/core.c#L658-L676

> 
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
>> +{
>> +       const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
>> +       size_t nfuncs = pctrl->pctrl_data->nfuncs;
>> +       unsigned int i;
>> +       int ret;
>> +
>> +       for (i = 0; i < nfuncs ; i++) {
>> +               ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
>> +                                                 funcs[i].groups, funcs[i].ngroups, NULL);
>> +               if (ret < 0)
> 
> Ditto.
> 
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
> 
> ...
> 
>> +       board_id = (enum upboard_board_id)(unsigned long)dmi_id->driver_data;
>> +
> 
> Unneeded blank line.
> 
>> +       switch (board_id) {
>> +       case UPBOARD_APL01:
>> +               pctrl->maps = upboard_pinctrl_mapping_apl01;
>> +               pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
>> +               break;
>> +       default:
>> +               return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
>> +       }
> 
> And actually we can get rid of that train of castings by switching to
> use the info type of the structure
> 
> (namings are just constructed without checking for the better or
> existing ones, choose another if you think they fit)
> 
> struct upboard_pinctrl_map {
>   ... *maps;
>   ... nmaps;
> };
> 
> static const struct upboard_pinctrl_map apl01 = {
>   ...
> };
> 
> ...dmi... = {
>   ...
>   .data = (void *)&apl01,
>   ...
> };
> 
> board_map = (const ...) dmi_id->driver_data;
> 
> ...
> 
> But since DMI will require castings, it's up to you as I don't like
> the idea of having that driver data not to be const in the first
> place.

I definitely prefer your proposal, so we can remove the switch case and
the enum.
I will just split the definition to compute automatically nmaps like this:

static const struct pinctrl_map pinctrl_map_apl01[] = {
        ...
};

static const struct upboard_pinctrl_map upboard_pinctrl_map_apl01 = {
	.maps = &pinctrl_map_apl01[0],
	.nmaps = ARRAY_SIZE(pinctrl_map_apl01),
};

Regards,

Thomas

Re: [PATCH v5 00/12] Add pinctrl support for the AAEON UP board FPGA

[Andy Shevchenko]

On Tue, May 6, 2025 at 6:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> This is the fifth version of this series, addressing the few remaining
> issues identified by Andy.

Thanks for the updated version! We are all good now, but I have a few
nit-picks and one small thing to fix, i.e. the constifying of the
driver_data in GPIO forwarder.
(Yes, yes, spelling is another thing, but not so critical).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 00/12] Add pinctrl support for the AAEON UP board FPGA

[Linus Walleij]

Hi Thomas,

thanks for working on this!

Also thanks to Andy for excellent guidance on the series.

On Tue, May 6, 2025 at 5:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> This is the fifth version of this series, addressing the few remaining
> issues identified by Andy.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Looks good to me!

> Thomas Richard (12):
>       gpiolib: add support to register sparse pin range
>       pinctrl: remove extern specifier for functions in machine.h
>       pinctrl: core: add devm_pinctrl_register_mappings()
>       gpio: aggregator: move GPIO forwarder allocation in a dedicated function
>       gpio: aggregator: refactor the code to add GPIO desc in the forwarder
>       gpio: aggregator: refactor the forwarder registration part
>       gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters
>       gpio: aggregator: export symbols of the GPIO forwarder library
>       gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd
>       gpio: aggregator: add possibility to attach data to the forwarder
>       lib/string_choices: Add str_input_output() helper
>       pinctrl: Add pin controller driver for AAEON UP boards
>

Most heavy commits are in the GPIO subsystem, once the nitpicks
are addressed in v6, Bartosz do you want
to create an immutable branch for this and merge into your for-next
and see how it works, if all is good I can perhaps pull the same
branch into pinctrl as well.

If this is stressful I can do the same operation in pinctrl instead so
you can just pull it to GPIO from my tree.

Yours,
Linus Walleij