[PATCH v3 00/10] Add pinctrl support for the AAEON UP board FPGA

[PATCH v3 00/10] Add pinctrl support for the AAEON UP board FPGA

[Thomas Richard]

This is the third version of this series (rebased on v6.15-rc2).

The gpiolib part has been reworked, the gpiochip_add_pin_range() was
renamed to gpiochip_add_pin_range_with_pins() and a new pins parameter was
addded. Two stubs were created to add consecutive or sparse pin range.

For the forwarder library, a namespace was added and patches were splitted
to more simpler changes.

In the pinctrl core, the function devm_pinctrl_register_mappings() was
created.

No big changes in the upboard pinctrl driver, only few fixes and
improvements.

Signed-off-by: Thomas Richard <thomas.richard@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 (10):
      gpiolib: add support to register sparse pin range
      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
      pinctrl: Add pin controller driver for AAEON UP boards

 drivers/gpio/gpio-aggregator.c    |  221 +++++---
 drivers/gpio/gpiolib.c            |   29 +-
 drivers/pinctrl/Kconfig           |   18 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/core.c            |   37 ++
 drivers/pinctrl/pinctrl-upboard.c | 1071 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h       |   51 +-
 include/linux/gpio/forwarder.h    |   63 +++
 include/linux/pinctrl/machine.h   |   10 +
 9 files changed, 1408 insertions(+), 93 deletions(-)
---
base-commit: 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
change-id: 20240930-aaeon-up-board-pinctrl-support-98fa4a030490

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

[PATCH v3 01/10] 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 support 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 fill 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().

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 e70197e39f55..6451dd4565c9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2285,11 +2285,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
  *
@@ -2301,9 +2303,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;
@@ -2321,6 +2326,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);
@@ -2330,16 +2336,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 PIN %d->%d\n",
+			 gpio_offset, gpio_offset + npins - 1,
+			 pinctl_name,
+			 pin_offset, pin_offset + npins - 1);
+	else
+		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]);
 
 	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 v3 01/10] gpiolib: add support to register sparse pin range

[Linus Walleij]

On Wed, Apr 16, 2025 at 4:08 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Add support to register for GPIO<->pin mapping using a list of non
> consecutive pins. The core already support 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 fill 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().
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

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

Yours,
Linus Walleij

Re: [PATCH v3 01/10] gpiolib: add support to register sparse pin range

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:09PM +0200, Thomas Richard wrote:
> Add support to register for GPIO<->pin mapping using a list of non
> consecutive pins. The core already support 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 fill 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().

...

> +	if (!pin_range->range.pins)

This change looks pretty nice, but can we use positive conditonal?

> +		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);
> +	else
> +		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]);

With that done,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()

[Thomas Richard]

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

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/core.c          | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/pinctrl/machine.h | 10 ++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 6dd48dd2c035..f02c45b98512 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1543,6 +1543,43 @@ void pinctrl_unregister_mappings(const struct pinctrl_map *map)
 }
 EXPORT_SYMBOL_GPL(pinctrl_unregister_mappings);
 
+static void devm_pinctrl_unregister_mappings(struct device *dev, void *res)
+{
+	pinctrl_unregister_mappings(*(const struct pinctrl_map **)res);
+}
+
+/**
+ * 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
+ */
+int devm_pinctrl_register_mappings(struct device *dev,
+				   const struct pinctrl_map *maps,
+				   unsigned int num_maps)
+{
+	const struct pinctrl_map **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_pinctrl_unregister_mappings, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = pinctrl_register_mappings(maps, num_maps);
+	if (!ret) {
+		*ptr = maps;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ret;
+}
+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 673e96df453b..2c178328c468 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -155,6 +155,9 @@ struct pinctrl_map;
 
 extern int pinctrl_register_mappings(const struct pinctrl_map *map,
 				     unsigned int num_maps);
+extern int devm_pinctrl_register_mappings(struct device *dev,
+					  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);
 #else
@@ -165,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

Re: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()

[Linus Walleij]

On Wed, Apr 16, 2025 at 4:08 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Using devm_pinctrl_register_mappings(), the core can automatically
> unregister pinctrl mappings.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

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

Yours,
Linus Walleij

Re: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Richard/gpiolib-add-support-to-register-sparse-pin-range/20250416-221852
base:   8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
patch link:    https://lore.kernel.org/r/20250416-aaeon-up-board-pinctrl-support-v3-2-f40776bd06ee%40bootlin.com
patch subject: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()
config: i386-buildonly-randconfig-001-20250417 (https://download.01.org/0day-ci/archive/20250417/202504172328.mrC81ooJ-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/20250417/202504172328.mrC81ooJ-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/202504172328.mrC81ooJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/pinctrl/renesas/pfc-r8a7791.c:12:
   In file included from drivers/pinctrl/renesas/sh_pfc.h:12:
   In file included from include/linux/pinctrl/pinconf-generic.h:16:
>> include/linux/pinctrl/machine.h:158:50: warning: declaration of 'struct device' will not be visible outside of this function [-Wvisibility]
     158 | extern int devm_pinctrl_register_mappings(struct device *dev,
         |                                                  ^
   1 warning generated.


vim +158 include/linux/pinctrl/machine.h

   155	
   156	extern int pinctrl_register_mappings(const struct pinctrl_map *map,
   157					     unsigned int num_maps);
 > 158	extern int devm_pinctrl_register_mappings(struct device *dev,
   159						  const struct pinctrl_map *map,
   160						  unsigned int num_maps);
   161	extern void pinctrl_unregister_mappings(const struct pinctrl_map *map);
   162	extern void pinctrl_provide_dummies(void);
   163	#else
   164	

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

Re: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:10PM +0200, Thomas Richard wrote:
> Using devm_pinctrl_register_mappings(), the core can automatically
> unregister pinctrl mappings.

...

> +int devm_pinctrl_register_mappings(struct device *dev,
> +				   const struct pinctrl_map *maps,
> +				   unsigned int num_maps)
> +{
> +	const struct pinctrl_map **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_pinctrl_unregister_mappings, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = pinctrl_register_mappings(maps, num_maps);
> +	if (!ret) {
> +		*ptr = maps;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return ret;

Why not devm_add_action_or_reset()?

> +}

...

>  extern int pinctrl_register_mappings(const struct pinctrl_map *map,
>  				     unsigned int num_maps);
> +extern int devm_pinctrl_register_mappings(struct device *dev,
> +					  const struct pinctrl_map *map,
> +					  unsigned int num_maps);

No extern, please. Perhaps a clean up patch to remove existing ones?

>  extern void pinctrl_unregister_mappings(const struct pinctrl_map *map);
>  extern void pinctrl_provide_dummies(void);

...

Test robot wants you to add a forward declaration

struct device;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Richard/gpiolib-add-support-to-register-sparse-pin-range/20250416-221852
base:   8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
patch link:    https://lore.kernel.org/r/20250416-aaeon-up-board-pinctrl-support-v3-2-f40776bd06ee%40bootlin.com
patch subject: [PATCH v3 02/10] pinctrl: core: add devm_pinctrl_register_mappings()
config: i386-buildonly-randconfig-003-20250417 (https://download.01.org/0day-ci/archive/20250418/202504180047.vMkMYzD4-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250418/202504180047.vMkMYzD4-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/202504180047.vMkMYzD4-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/pinctrl/pinconf-generic.h:16,
                    from drivers/pinctrl/renesas/sh_pfc.h:12,
                    from drivers/pinctrl/renesas/pfc-r8a77965.c:18:
>> include/linux/pinctrl/machine.h:158:50: warning: 'struct device' declared inside parameter list will not be visible outside of this definition or declaration
     158 | extern int devm_pinctrl_register_mappings(struct device *dev,
         |                                                  ^~~~~~


vim +158 include/linux/pinctrl/machine.h

   155	
   156	extern int pinctrl_register_mappings(const struct pinctrl_map *map,
   157					     unsigned int num_maps);
 > 158	extern int devm_pinctrl_register_mappings(struct device *dev,
   159						  const struct pinctrl_map *map,
   160						  unsigned int num_maps);
   161	extern void pinctrl_unregister_mappings(const struct pinctrl_map *map);
   162	extern void pinctrl_provide_dummies(void);
   163	#else
   164	

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

[PATCH v3 03/10] gpio: aggregator: move GPIO forwarder allocation in a dedicated function

[Thomas Richard]

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

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index d232ea865356..a5e3ae4d8813 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -498,6 +498,37 @@ 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 +549,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 +578,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 v3 03/10] gpio: aggregator: move GPIO forwarder allocation in a dedicated function

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:11PM +0200, Thomas Richard wrote:
> Move the GPIO forwarder allocation and static initialization in a dedicated
> function.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +	fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)),
> +			   GFP_KERNEL);

You can keep it one line.

> +	if (!fwd)
> +		return ERR_PTR(-ENOMEM);


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 04/10] 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 desc, now the GPIO desc are passed on by one to
the forwarder.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index a5e3ae4d8813..4d6e41e02659 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -510,6 +510,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;
@@ -529,6 +533,39 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
 	return fwd;
 }
 
+static int gpiochip_fwd_add_gpio_desc(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 (fwd->descs[offset])
+		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,
+		desc_to_gpio(desc), gpiod_to_irq(desc));
+
+	return 0;
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -560,26 +597,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_add_gpio_desc(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 v3 04/10] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:12PM +0200, Thomas Richard wrote:
> Create a dedicated function to add a GPIO desc in the forwarder. Instead of
> passing an array of GPIO desc, now the GPIO desc are passed on by one to
> the forwarder.

...

> +static int gpiochip_fwd_add_gpio_desc(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 (fwd->descs[offset])
> +		return -EEXIST;

Not sure we need this. I would rather think that something inside struct
gpiochip_fwd should track this. OTOH, I understand that you want to have
sparse lists perhaps. I;m wondering why GPIO valid mask can't be used for
this purposes?

> +	/*
> +	 * 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;
> +}

The bottom line is that I'm fine with this change without additional checks,
add them when function will be used not only in the original loop.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/10] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Thomas Richard]

Hi Andy,

Thanks a lot for the review !!

On 4/17/25 19:16, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 04:08:12PM +0200, Thomas Richard wrote:
>> Create a dedicated function to add a GPIO desc in the forwarder. Instead of
>> passing an array of GPIO desc, now the GPIO desc are passed on by one to
>> the forwarder.
> 
> ...
> 
>> +static int gpiochip_fwd_add_gpio_desc(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 (fwd->descs[offset])
>> +		return -EEXIST;
> 
> Not sure we need this. I would rather think that something inside struct
> gpiochip_fwd should track this. OTOH, I understand that you want to have
> sparse lists perhaps. I;m wondering why GPIO valid mask can't be used for
> this purposes?

The valid_mask in the gpio_chip is allocated in
gpiochip_add_data_with_key() which is too late for us.

But as you suggested, something (a valid_mask like in gpio_chip) inside
struct gpiochip_fwd should work.

> 
>> +	/*
>> +	 * 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;
>> +}
> 
> The bottom line is that I'm fine with this change without additional checks,
> add them when function will be used not only in the original loop.

Ok so for this patch I do not add checks.
Then I implement a valid_mask (in struct gpiochip_fwd) in patch 08/10
"gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd"

Best Regards,

Thomas

[PATCH v3 05/10] 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.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 4d6e41e02659..66ea3daafc03 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -566,6 +566,21 @@ static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
 	return 0;
 }
 
+static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+{
+	struct gpio_chip *chip = &fwd->chip;
+	int error;
+
+	if (chip->can_sleep)
+		mutex_init(&fwd->mlock);
+	else
+		spin_lock_init(&fwd->slock);
+
+	error = devm_gpiochip_add_data(chip->parent, chip, fwd);
+
+	return error;
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -603,18 +618,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 v3 05/10] gpio: aggregator: refactor the forwarder registration part

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:13PM +0200, Thomas Richard 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>
with one nit-pick below.

...

> +static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
> +{
> +	struct gpio_chip *chip = &fwd->chip;

> +	int error;

Not needed in this change.

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

> +	error = devm_gpiochip_add_data(chip->parent, chip, fwd);
> +
> +	return error;

	return devm_...

Make it differently when you will need this.

> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 06/10] gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters

[Thomas Richard]

Remove useless parameters of gpiochip_fwd_setup_delay_line().

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 66ea3daafc03..96b82974969d 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;
 }
@@ -602,7 +602,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;
 
@@ -610,8 +609,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_add_gpio_desc(fwd, descs[i], i);
 		if (error)
@@ -619,7 +616,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 v3 06/10] gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:14PM +0200, Thomas Richard wrote:
> Remove useless parameters of gpiochip_fwd_setup_delay_line().

Makes sense!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 07/10] gpio: aggregator: export symbols of the GPIO forwarder library

[Thomas Richard]

Export all symbols and create header file for the GPIO forwarder library.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 96b82974969d..5743f84e1310 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
@@ -254,56 +255,44 @@ static void __exit gpio_aggregator_remove_all(void)
  *  GPIO Forwarder
  */
 
-struct gpiochip_fwd_timing {
-	u32 ramp_up_us;
-	u32 ramp_down_us;
-};
-
-struct gpiochip_fwd {
-	struct gpio_chip chip;
-	struct gpio_desc **descs;
-	union {
-		struct mutex mlock;	/* protects tmp[] if can_sleep */
-		spinlock_t slock;	/* protects tmp[] if !can_sleep */
-	};
-	struct gpiochip_fwd_timing *delay_timings;
-	unsigned long tmp[];		/* values and descs for multiple ops */
-};
-
 #define fwd_tmp_values(fwd)	&(fwd)->tmp[0]
 #define fwd_tmp_descs(fwd)	(void *)&(fwd)->tmp[BITS_TO_LONGS((fwd)->chip.ngpio)]
 
 #define fwd_tmp_size(ngpios)	(BITS_TO_LONGS((ngpios)) + (ngpios))
 
-static int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
+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)
+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)
+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)
+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 +320,8 @@ 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)
+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 +339,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 +362,7 @@ 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)
+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 +379,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 +401,8 @@ 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)
+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 +420,24 @@ 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)
+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)
+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 +448,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 +470,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 +480,20 @@ 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)
+struct gpiochip_fwd *
+devm_gpio_fwd_alloc(struct device *dev, unsigned int ngpios)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -532,10 +526,10 @@ 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_add_gpio_desc(struct gpiochip_fwd *fwd,
-				      struct gpio_desc *desc,
-				      unsigned int offset)
+int gpio_fwd_add_gpio_desc(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;
@@ -565,8 +559,9 @@ static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_add_gpio_desc, "GPIO_FORWARDER");
 
-static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+int gpio_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	int error;
@@ -580,9 +575,10 @@ static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 
 	return error;
 }
+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.
@@ -596,32 +592,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_add_gpio_desc(fwd, descs[i], i);
+		error = gpio_fwd_add_gpio_desc(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);
 
@@ -656,7 +652,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..78adfe6f162f
--- /dev/null
+++ b/include/linux/gpio/forwarder.h
@@ -0,0 +1,60 @@
+/* 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>
+#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
+#include <linux/types.h>
+
+struct gpio_chip;
+struct gpio_desc;
+
+struct gpiochip_fwd_timing {
+	u32 ramp_up_us;
+	u32 ramp_down_us;
+};
+
+struct gpiochip_fwd {
+	struct gpio_chip chip;
+	struct gpio_desc **descs;
+	union {
+		struct mutex mlock;	/* protects tmp[] if can_sleep */
+		spinlock_t slock;	/* protects tmp[] if !can_sleep */
+	};
+	struct gpiochip_fwd_timing *delay_timings;
+	unsigned long tmp[];		/* values and descs for multiple ops */
+};
+
+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_add_gpio_desc(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 v3 07/10] gpio: aggregator: export symbols of the GPIO forwarder library

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:15PM +0200, Thomas Richard wrote:
> Export all symbols and create header file for the GPIO forwarder library.

...

> -struct gpiochip_fwd_timing {
> -	u32 ramp_up_us;
> -	u32 ramp_down_us;
> -};
> -
> -struct gpiochip_fwd {
> -	struct gpio_chip chip;
> -	struct gpio_desc **descs;
> -	union {
> -		struct mutex mlock;	/* protects tmp[] if can_sleep */
> -		spinlock_t slock;	/* protects tmp[] if !can_sleep */
> -	};
> -	struct gpiochip_fwd_timing *delay_timings;
> -	unsigned long tmp[];		/* values and descs for multiple ops */
> -};

I don't like this piece at all. It will repeat one of the biggest mistake done
with many (old) subsystems and core.

Looking at the last patch ahead, I can tell that this is not needed. Instead
add a couple of APIs:
- getter for the gpiochip
- getter for the specific private data

and use the error code filtering for the existed GPIO descriptor, I actually
don't understand why for the existed descriptor you return there 0 and not an
error.

TL;DR: Follow OOP and make this an opaque pointer, no need to move it to the
header. The rest looks okay on the first glance.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 08/10] 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 to use 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 | 27 +++++++++++++++++++++------
 include/linux/gpio/forwarder.h |  2 ++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 5743f84e1310..0fb890f0793d 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -260,6 +260,14 @@ static void __exit gpio_aggregator_remove_all(void)
 
 #define fwd_tmp_size(ngpios)	(BITS_TO_LONGS((ngpios)) + (ngpios))
 
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return fwd->descs[offset] ? 0 : -ENODEV;
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_request, "GPIO_FORWARDER");
+
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
@@ -513,6 +521,7 @@ devm_gpio_fwd_alloc(struct device *dev, unsigned int ngpios)
 	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;
@@ -531,7 +540,6 @@ EXPORT_SYMBOL_NS_GPL(devm_gpio_fwd_alloc, "GPIO_FORWARDER");
 int gpio_fwd_add_gpio_desc(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)
@@ -543,15 +551,10 @@ int gpio_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 	/*
 	 * 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,
@@ -564,8 +567,20 @@ EXPORT_SYMBOL_NS_GPL(gpio_fwd_add_gpio_desc, "GPIO_FORWARDER");
 int gpio_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
+	unsigned int ndescs = 0, i;
 	int error;
 
+	for (i = 0; i < chip->ngpio; i++)
+		if (fwd->descs[i])
+			ndescs++;
+
+	/*
+	 * Some gpio_desc were not registers. They will be registered at runtime
+	 * but we have to suppose they can sleep.
+	 */
+	if (ndescs != 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 78adfe6f162f..f3fba3c39bda 100644
--- a/include/linux/gpio/forwarder.h
+++ b/include/linux/gpio/forwarder.h
@@ -27,6 +27,8 @@ struct gpiochip_fwd {
 	unsigned long tmp[];		/* values and descs for multiple ops */
 };
 
+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);

-- 
2.39.5

Re: [PATCH v3 08/10] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:16PM +0200, Thomas Richard wrote:
> Add request() callback to check if the GPIO descriptor was well registered
> in the gpiochip_fwd before to use 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.

...

> +int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return fwd->descs[offset] ? 0 : -ENODEV;

ENODEV? Hmm... Why has that error code been chosen?

> +}

...

>  int gpio_fwd_register(struct gpiochip_fwd *fwd)
>  {
>  	struct gpio_chip *chip = &fwd->chip;
> +	unsigned int ndescs = 0, i;

Slightly better (from maintenance perspective) to decouple assignment as it's
not a standalone function that just counts them. So it means some code may
appear in between and in long-term someone might screw up with the initial
value for that.

>  	int error;

	ndescs = 0;

> +	for (i = 0; i < chip->ngpio; i++)
> +		if (fwd->descs[i])
> +			ndescs++;
> +
> +	/*
> +	 * Some gpio_desc were not registers. They will be registered at runtime

registered

> +	 * but we have to suppose they can sleep.
> +	 */
> +	if (ndescs != chip->ngpio)
> +		chip->can_sleep = true;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/10] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Thu, Apr 17, 2025 at 08:38:33PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 04:08:16PM +0200, Thomas Richard wrote:

...

> > +	unsigned int ndescs = 0, i;
> 
> Slightly better (from maintenance perspective) to decouple assignment as it's
> not a standalone function that just counts them. So it means some code may
> appear in between and in long-term someone might screw up with the initial
> value for that.
> 
> >  	int error;
> 
> 	ndescs = 0;
> 
> > +	for (i = 0; i < chip->ngpio; i++)
> > +		if (fwd->descs[i])
> > +			ndescs++;

Btw, note with a mask for them you can just have a bitmap_weight() call instead of all this.

	ndescs = bitmap_weight(valid_mask);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 09/10] gpio: aggregator: add possibility to attach data to the forwarder

[Thomas Richard]

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

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 0fb890f0793d..9e27ea3f78ec 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -564,7 +564,7 @@ int gpio_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 }
 EXPORT_SYMBOL_NS_GPL(gpio_fwd_add_gpio_desc, "GPIO_FORWARDER");
 
-int gpio_fwd_register(struct gpiochip_fwd *fwd)
+int gpio_fwd_register(struct gpiochip_fwd *fwd, void *data)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	unsigned int ndescs = 0, i;
@@ -586,6 +586,8 @@ int gpio_fwd_register(struct gpiochip_fwd *fwd)
 	else
 		spin_lock_init(&fwd->slock);
 
+	fwd->data = data;
+
 	error = devm_gpiochip_add_data(chip->parent, chip, fwd);
 
 	return error;
@@ -632,7 +634,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 f3fba3c39bda..304c793313db 100644
--- a/include/linux/gpio/forwarder.h
+++ b/include/linux/gpio/forwarder.h
@@ -18,6 +18,7 @@ struct gpiochip_fwd_timing {
 
 struct gpiochip_fwd {
 	struct gpio_chip chip;
+	void *data;
 	struct gpio_desc **descs;
 	union {
 		struct mutex mlock;	/* protects tmp[] if can_sleep */
@@ -57,6 +58,6 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
 int gpio_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
 			   struct gpio_desc *desc, 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 v3 09/10] gpio: aggregator: add possibility to attach data to the forwarder

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:17PM +0200, Thomas Richard wrote:
> Add a data pointer to store private data in the forwarder.

...

> -int gpio_fwd_register(struct gpiochip_fwd *fwd)
> +int gpio_fwd_register(struct gpiochip_fwd *fwd, void *data)

Since it comes from outside there is no need to have a getter at all.
No? Currently I don't understand how this change is used.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 10/10] 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 provides also a gpiochip. 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

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/Kconfig           |   18 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1071 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1090 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 464cc9aca157..58a47abebb51 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -590,6 +590,24 @@ 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 GPIO_AGGREGATOR
+	help
+	  Pin controller for the FPGA GPIO lines on UP boards. Due to the
+	  hardware layout, the driver control 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..27ac4a338458
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,1071 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * UP board pin control driver.
+ *
+ * Copyright (C) 2024 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/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 gpio_chip chip;
+	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) ", ret == UPBOARD_PIN_MODE_GPIO_IN ? "input" : "output");
+}
+
+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 = container_of(gc, struct gpiochip_fwd, chip);
+	struct upboard_pinctrl *pctrl = fwd->data;
+	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;
+
+	/* GPIO desc is already registered */
+	if (fwd->descs[offset])
+		return 0;
+
+	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_add_gpio_desc(fwd, desc, offset);
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	pinctrl_gpio_free(gc, offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
+	struct upboard_pinctrl *pctrl = fwd->data;
+	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;
+	else
+		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;
+	int ret, i;
+
+	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)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 = &fwd->chip;
+	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);
+
+	return ret;
+}
+
+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 v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards

[Linus Walleij]

On Wed, Apr 16, 2025 at 4:08 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 provides also a gpiochip. 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
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

From a pin control PoV this looks good:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:18PM +0200, Thomas Richard 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 provides also a gpiochip. 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

Missed period.

...

> +/*
> + * UP board pin control driver.
> + *
> + * Copyright (C) 2024 Bootlin

My calendar shows something different :-)

> + *
> + * 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>

+ types.h (bool and NULL from stddef.h that will be included).

...

> +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

Leave trailing comma in such cases.

> +};

...

> +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];

> +

Unneeded blank line.

> +		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 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);

> +

Unneeded blank line.

> +	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) ", ret == UPBOARD_PIN_MODE_GPIO_IN ? "input" : "output");

str_input_output() from string_choices.h ?

Ah, we still have no such... In case you have motivation, you can add a patch,
I will Ack/Review it.

> +}

...

> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
> +	struct upboard_pinctrl *pctrl = fwd->data;

Yeah, something like

	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;

> +	/* GPIO desc is already registered */
> +	if (fwd->descs[offset])
> +		return 0;

As mentioned in another reply, why 0 and even though, why can't it be simply
filtered by EEXIST from the below?

In worst scenario, you can add an API gpio_fwd_is_registered(fwd, offset).

> +	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_add_gpio_desc(fwd, desc, offset);
> +}

...

> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
> +	struct upboard_pinctrl *pctrl = fwd->data;
> +	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;

> +	else

Redundant 'else'.

> +		return GPIO_LINE_DIRECTION_IN;
> +}

...

> +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) {

Why out of a sudden pre-increment?

> +		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 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;
> +	int ret, i;

Why i is signed while other code uses unsigned loop iterators?

> +	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)dmi_id->driver_data;

This might need to have an intermediate cast to (unsigned long). Have you run
the build with Clang 19 and `make W=1`?

> +
> +	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 = &fwd->chip;
> +	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);
> +
> +	return ret;

Stray return.

> +}

...

> +static struct platform_driver upboard_pinctrl_driver = {
> +	.driver = {
> +		.name = "upboard-pinctrl",
> +	},
> +	.probe = upboard_pinctrl_probe,
> +};

> +

Unneeded blank line.

> +module_platform_driver(upboard_pinctrl_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

>> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +	struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
>> +	struct upboard_pinctrl *pctrl = fwd->data;
> 
> Yeah, something like
> 
> 	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;
> 
>> +	/* GPIO desc is already registered */
>> +	if (fwd->descs[offset])
>> +		return 0;
> 
> As mentioned in another reply, why 0 and even though, why can't it be simply
> filtered by EEXIST from the below?
> 
> In worst scenario, you can add an API gpio_fwd_is_registered(fwd, offset).

I cannot filter using EEXIST, because I have to get the GPIO desc first.
And using the retcode of gpiod_get_index() I cannot detect that I
already requested the GPIO.

As now gpiochip_fwd is an opaque pointer, I will add the
gpio_fwd_is_registered() helper.

It is due to the fact that the forwarder never releases a GPIO desc. An
other solution could be to add the possibility to remove a GPIO desc.
In upboard_gpio_free() the GPIO desc is free, and we can remove the check.

upboard_gpio_free()
{
	gpio_fwd_free_desc(fwd, offset);
	pinctrl_gpio_free(gc, offset);
}


Best Regards,

Thomas

Re: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 04:36:33PM +0200, Thomas Richard wrote:

...

> >> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> >> +{
> >> +	struct gpiochip_fwd *fwd = container_of(gc, struct gpiochip_fwd, chip);
> >> +	struct upboard_pinctrl *pctrl = fwd->data;
> > 
> > Yeah, something like
> > 
> > 	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;
> > 
> >> +	/* GPIO desc is already registered */
> >> +	if (fwd->descs[offset])
> >> +		return 0;
> > 
> > As mentioned in another reply, why 0 and even though, why can't it be simply
> > filtered by EEXIST from the below?
> > 
> > In worst scenario, you can add an API gpio_fwd_is_registered(fwd, offset).
> 
> I cannot filter using EEXIST, because I have to get the GPIO desc first.
> And using the retcode of gpiod_get_index() I cannot detect that I
> already requested the GPIO.
> 
> As now gpiochip_fwd is an opaque pointer, I will add the
> gpio_fwd_is_registered() helper.
> 
> It is due to the fact that the forwarder never releases a GPIO desc. An
> other solution could be to add the possibility to remove a GPIO desc.
> In upboard_gpio_free() the GPIO desc is free, and we can remove the check.
> 
> upboard_gpio_free()
> {
> 	gpio_fwd_free_desc(fwd, offset);
> 	pinctrl_gpio_free(gc, offset);
> }

From given options I prefer to have the _gpio_free(), i.e. the latter one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Richard/gpiolib-add-support-to-register-sparse-pin-range/20250416-221852
base:   8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
patch link:    https://lore.kernel.org/r/20250416-aaeon-up-board-pinctrl-support-v3-10-f40776bd06ee%40bootlin.com
patch subject: [PATCH v3 10/10] pinctrl: Add pin controller driver for AAEON UP boards
config: x86_64-kismet-CONFIG_GPIO_AGGREGATOR-CONFIG_PINCTRL_UPBOARD-0-0 (https://download.01.org/0day-ci/archive/20250419/202504190519.GwvdasH8-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250419/202504190519.GwvdasH8-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/202504190519.GwvdasH8-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for GPIO_AGGREGATOR when selected by PINCTRL_UPBOARD
   WARNING: unmet direct dependencies detected for GPIO_AGGREGATOR
     Depends on [n]: GPIOLIB [=n]
     Selected by [y]:
     - PINCTRL_UPBOARD [=y] && PINCTRL [=y] && MFD_UPBOARD_FPGA [=y]

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

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

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 04:08:08PM +0200, Thomas Richard wrote:
> This is the third version of this series (rebased on v6.15-rc2).
> 
> The gpiolib part has been reworked, the gpiochip_add_pin_range() was
> renamed to gpiochip_add_pin_range_with_pins() and a new pins parameter was
> addded. Two stubs were created to add consecutive or sparse pin range.
> 
> For the forwarder library, a namespace was added and patches were splitted
> to more simpler changes.
> 
> In the pinctrl core, the function devm_pinctrl_register_mappings() was
> created.
> 
> No big changes in the upboard pinctrl driver, only few fixes and
> improvements.

I reviewed it and the whole impression is very good, it seems much better in
comparison to the previous one(s).

The biggest issue I see here is the exporting the forwarder internal data type.
Most of the rest is the style related comments. Looking forward for a new
version.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 00/10] Add pinctrl support for the AAEON UP board FPGA

[Thomas Richard]

This is the third version of this series (rebased on v6.15-rc2).

The gpiolib part has been reworked, the gpiochip_add_pin_range() was
renamed to gpiochip_add_pin_range_with_pins() and a new pins parameter was
addded. Two stubs were created to add consecutive or sparse pin range.

For the forwarder library, a namespace was added and patches were splitted
to more simpler changes.

In the pinctrl core, the function devm_pinctrl_register_mappings() was
created.

No big changes in the upboard pinctrl driver, only few fixes and
improvements.

Signed-off-by: Thomas Richard <thomas.richard@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 (10):
      gpiolib: add support to register sparse pin range
      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
      pinctrl: Add pin controller driver for AAEON UP boards

 drivers/gpio/gpio-aggregator.c    |  221 +++++---
 drivers/gpio/gpiolib.c            |   29 +-
 drivers/pinctrl/Kconfig           |   18 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/core.c            |   37 ++
 drivers/pinctrl/pinctrl-upboard.c | 1071 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h       |   51 +-
 include/linux/gpio/forwarder.h    |   63 +++
 include/linux/pinctrl/machine.h   |   10 +
 9 files changed, 1408 insertions(+), 93 deletions(-)
---
base-commit: 8a834b0ac9ceb354a6e0b8cf5b363edca8221bdd
change-id: 20240930-aaeon-up-board-pinctrl-support-98fa4a030490

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

[PATCH v3 04/10] 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 desc, now the GPIO desc are passed on by one to
the forwarder.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index a5e3ae4d8813..4d6e41e02659 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -510,6 +510,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;
@@ -529,6 +533,39 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
 	return fwd;
 }
 
+static int gpiochip_fwd_add_gpio_desc(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 (fwd->descs[offset])
+		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,
+		desc_to_gpio(desc), gpiod_to_irq(desc));
+
+	return 0;
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -560,26 +597,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_add_gpio_desc(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