[PATCH RFC v2 0/6] Add pinctrl support for the AAEON UP board FPGA

[PATCH RFC v2 0/6] Add pinctrl support for the AAEON UP board FPGA

[Thomas Richard]

Second version of this series. I removed MFD and LED drivers as they were
already merged. It is in RFC state as I would like some feedback before to
add support for more boards, and to do some cleanup.

After some discussions with Andy, the preferred solution is to set the
pinctrl upboard driver as a consumer of SoC pins. Each supported has its
pinctrl mappings, the right mappings are registered and enabled (board is
identified using DMI table).

The hog mappings were removed because by default the FPGA is configured
correctly (thanks to the BIOS) for its board.

As requested by Linus, the GPIO part was reworked. The gpio-aggregator
driver was refactored to create a new library (gpio-fwd) which allows other
drivers to create gpiochip forwarder. The upboard pinctrl driver registers
a gpiochip forwarder, then requests and registers SoC GPIOs at runtime, and
forwards all GPIO accesses to the right gpiochip.

Linus, I know you were not ok with the gpiochip_add_pinlist_range()
function, but as we did not understood each other I kept it, so we can
continue the discussion.

The pinctrl drivers supports FPGA for two types of boards, UP and UP2 (UP
Squared). For now I only added pinctrl mappings for the "UP Squared" board
[1] (I selected this board as the pinctrl-broxton drivers had already all
functions and groups needed). More boards will be supported later (missing
groups and funtions should be added in Intel pinctrl drivers at the same
time).

[1] https://www.aaeon.com/en/product/detail/iot-gateway-maker-boards-up-squared

Signed-off-by: Thomas Richard <thomas.richard@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 (6):
      gpiolib: add gpiochip_add_pin_range_sparse() function
      gpio: aggregator: refactor the forwarder part.
      gpio: aggregator: export symbols of the gpio-fwd 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    |  232 +++++---
 drivers/gpio/gpiolib.c            |   74 ++-
 drivers/pinctrl/Kconfig           |   15 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1094 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h       |   12 +
 include/linux/gpio/gpio-fwd.h     |   56 ++
 7 files changed, 1379 insertions(+), 105 deletions(-)
---
base-commit: 12b58398bffc23db89e715414399b0533255da51
change-id: 20240930-aaeon-up-board-pinctrl-support-98fa4a030490

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

[PATCH RFC v2 1/6] gpiolib: add gpiochip_add_pin_range_sparse() function

[Thomas Richard]

Add gpiochip_add_pin_range_sparse() function to add a range for GPIO<->pin
mapping, using a list of non consecutive pins.
Previously, it was only possible to add range of consecutive pins using
gpiochip_add_pin_range_sparse().

The struct pinctrl_gpio_range has a 'pins' member which allows to set a
list of pins (which can be non consecutive).
gpiochip_add_pin_range_sparse() is identical to gpiochip_add_pin_range(),
except it set 'pins' member instead of 'pin_base' member.

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpiolib.c      | 74 +++++++++++++++++++++++++++++++++------------
 include/linux/gpio/driver.h | 12 ++++++++
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 16d190c1d6802..5a6d97116be9f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2234,26 +2234,9 @@ 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
- * @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
- * @npins: the number of pins from the offset of each pin space (GPIO and
- *	pin controller) to accumulate in this range
- *
- * Calling this function directly from a DeviceTree-supported
- * pinctrl driver is DEPRECATED. Please see Section 2.1 of
- * Documentation/devicetree/bindings/gpio/gpio.txt on how to
- * bind pinctrl and gpio drivers via the "gpio-ranges" property.
- *
- * Returns:
- * 0 on success, or a negative errno on failure.
- */
-int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
-			   unsigned int gpio_offset, unsigned int pin_offset,
-			   unsigned int npins)
+static int __gpiochip_add_pin_range(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;
@@ -2271,6 +2254,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);
@@ -2289,8 +2273,58 @@ int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 
 	return 0;
 }
+
+/**
+ * gpiochip_add_pin_range() - 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
+ * @npins: the number of pins from the offset of each pin space (GPIO and
+ *	pin controller) to accumulate in this range
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
+			   unsigned int gpio_offset, unsigned int pin_offset,
+			   unsigned int npins)
+{
+	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset,
+					pin_offset, NULL, npins);
+}
 EXPORT_SYMBOL_GPL(gpiochip_add_pin_range);
 
+/**
+ * gpiochip_add_pin_range_sparse() - 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_list: the list of pins to accumulate in this range
+ * @npins: the number of pins to accumulate in this range
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
+				  unsigned int gpio_offset, unsigned int const *pins,
+				  unsigned int npins)
+{
+	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset, 0, pins,
+					npins);
+}
+EXPORT_SYMBOL_GPL(gpiochip_add_pin_range_sparse);
+
 /**
  * gpiochip_remove_pin_ranges() - remove all the GPIO <-> pin mappings
  * @gc: the chip to remove all the mappings for
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270a..0402f94ec6a02 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -751,6 +751,9 @@ struct gpio_pin_range {
 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_sparse(struct gpio_chip *gc, const char *pinctl_name,
+				  unsigned int gpio_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);
@@ -765,6 +768,15 @@ gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 {
 	return 0;
 }
+
+static inline int
+gpiochip_add_pin_range_sparse(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 RFC v2 1/6] gpiolib: add gpiochip_add_pin_range_sparse() function

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 04:37:59PM +0100, Thomas Richard wrote:
> Add gpiochip_add_pin_range_sparse() function to add a range for GPIO<->pin
> mapping, using a list of non consecutive pins.
> Previously, it was only possible to add range of consecutive pins using
> gpiochip_add_pin_range_sparse().
> 
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive).
> gpiochip_add_pin_range_sparse() is identical to gpiochip_add_pin_range(),
> except it set 'pins' member instead of 'pin_base' member.

...

> +static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> +				    unsigned int gpio_offset, unsigned int pin_offset,
> +				    unsigned int const *pins, unsigned int npins)

I really do not like the __ naming here.
Can we rather create a better one? E.g., gpiochip_add_pin_range_with_pins().

...

> +/**
> + * gpiochip_add_pin_range_sparse() - 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_list: the list of pins to accumulate in this range
> + * @npins: the number of pins to accumulate in this range

> + * Calling this function directly from a DeviceTree-supported
> + * pinctrl driver is DEPRECATED. Please see Section 2.1 of
> + * Documentation/devicetree/bindings/gpio/gpio.txt on how to
> + * bind pinctrl and gpio drivers via the "gpio-ranges" property.

New API can't be deprecated. You probably want to say
"NOTE, this API is not supposed to be used on DeviceTree-supported platforms."
or something like that.

Also it's not clear which function should be used to clean up this. I would
clarify that: "When tearing down the driver don't forget to remove added ranges
with help of gpiochip_remove_pin_ranges()."

> + * Returns:
> + * 0 on success, or a negative errno on failure.
> + */
> +int gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +				  unsigned int gpio_offset, unsigned int const *pins,
> +				  unsigned int npins)
> +{
> +	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset, 0, pins,
> +					npins);
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_add_pin_range_sparse);

To me the gpiochip_add_sparse_pin_range() name sounds better.

...

>  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_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +				  unsigned int gpio_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);

> +static inline int
> +gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +			      unsigned int gpio_offset, unsigned int const *pins,
> +			      unsigned int npins)
> +{
> +	return 0;
> +}

Yeah, two stubs, two almost identical doc sections, no explanations of pins in
the core function...

I would rather refactor this to just rename the current function while adding
parameter to it, but leave it is being exported, just add a description to the
new parameter into the kernel doc. Make two new out of it as static inline:rs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 1/6] gpiolib: add gpiochip_add_pin_range_sparse() function

[Thomas Richard]

On 3/17/25 17:59, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 04:37:59PM +0100, Thomas Richard wrote:
>> Add gpiochip_add_pin_range_sparse() function to add a range for GPIO<->pin
>> mapping, using a list of non consecutive pins.
>> Previously, it was only possible to add range of consecutive pins using
>> gpiochip_add_pin_range_sparse().
>>
>> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
>> list of pins (which can be non consecutive).
>> gpiochip_add_pin_range_sparse() is identical to gpiochip_add_pin_range(),
>> except it set 'pins' member instead of 'pin_base' member.
> 
> ...
> 
>> +static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
>> +				    unsigned int gpio_offset, unsigned int pin_offset,
>> +				    unsigned int const *pins, unsigned int npins)
> 
> I really do not like the __ naming here.
> Can we rather create a better one? E.g., gpiochip_add_pin_range_with_pins().
> 
> ...
> 
>> +/**
>> + * gpiochip_add_pin_range_sparse() - 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_list: the list of pins to accumulate in this range
>> + * @npins: the number of pins to accumulate in this range
> 
>> + * Calling this function directly from a DeviceTree-supported
>> + * pinctrl driver is DEPRECATED. Please see Section 2.1 of
>> + * Documentation/devicetree/bindings/gpio/gpio.txt on how to
>> + * bind pinctrl and gpio drivers via the "gpio-ranges" property.
> 
> New API can't be deprecated. You probably want to say
> "NOTE, this API is not supposed to be used on DeviceTree-supported platforms."
> or something like that.
> 
> Also it's not clear which function should be used to clean up this. I would
> clarify that: "When tearing down the driver don't forget to remove added ranges
> with help of gpiochip_remove_pin_ranges()."

gpiochip_remove_pin_ranges() is automatically called when the gpiochip
is removed [1]

[1]
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/gpio/gpiolib.c#L1189

Regards,

Thomas

[PATCH RFC v2 2/6] gpio: aggregator: refactor the forwarder part.

[Thomas Richard]

Prepare the code to create a gpio-fwd library. This library will allow to
create and register a gpiochip forwarder.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index d668ddb2e81d3..a86e76daf67ab 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -260,6 +260,7 @@ struct gpiochip_fwd_timing {
 };
 
 struct gpiochip_fwd {
+	struct device *dev;
 	struct gpio_chip chip;
 	struct gpio_desc **descs;
 	union {
@@ -466,10 +467,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(fwd->dev, chip->ngpio,
 					  sizeof(*fwd->delay_timings),
 					  GFP_KERNEL);
 	if (!fwd->delay_timings)
@@ -481,63 +483,30 @@ 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;
 }
 #endif	/* !CONFIG_OF_GPIO */
 
-/**
- * gpiochip_fwd_create() - Create a new GPIO forwarder
- * @dev: Parent device pointer
- * @ngpios: Number of GPIOs in the forwarder.
- * @descs: Array containing the GPIO descriptors to forward to.
- *         This array must contain @ngpios entries, and must not be deallocated
- *         before the forwarder has been destroyed again.
- * @features: Bitwise ORed features as defined with FWD_FEATURE_*.
- *
- * This function creates a new gpiochip, which forwards all GPIO operations to
- * the passed GPIO descriptors.
- *
- * 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 *devm_gpiochip_fwd_alloc(struct device *dev,
+						    unsigned int ngpios)
 {
 	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);
 	if (!fwd)
 		return ERR_PTR(-ENOMEM);
 
-	chip = &fwd->chip;
+	fwd->descs = devm_kcalloc(dev, ngpios, sizeof(*fwd->descs),
+				  GFP_KERNEL);
 
-	/*
-	 * 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]);
+	fwd->dev = dev;
 
-		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;
-	}
+	chip = &fwd->chip;
 
 	chip->label = label;
 	chip->parent = dev;
@@ -552,20 +521,99 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->to_irq = gpio_fwd_to_irq;
 	chip->base = -1;
 	chip->ngpio = ngpios;
-	fwd->descs = descs;
+
+	return fwd;
+}
+
+static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
+				      unsigned int offset)
+{
+	struct gpio_chip *chip = &fwd->chip;
+	struct gpio_chip *parent = gpiod_to_chip(desc);
+
+	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(fwd->dev, "%u => gpio %d irq %d\n", offset,
+		desc_to_gpio(desc), gpiod_to_irq(desc));
+
+	return 0;
+}
+
+static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+{
+	struct gpio_chip *chip = &fwd->chip;
+	struct device *dev = fwd->dev;
+	int error;
 
 	if (chip->can_sleep)
 		mutex_init(&fwd->mlock);
 	else
 		spin_lock_init(&fwd->slock);
 
+	error = devm_gpiochip_add_data(dev, chip, fwd);
+
+	return error;
+}
+
+/**
+ * gpiochip_fwd_create() - Create a new GPIO forwarder
+ * @dev: Parent device pointer
+ * @ngpios: Number of GPIOs in the forwarder.
+ * @descs: Array containing the GPIO descriptors to forward to.
+ *         This array must contain @ngpios entries, and must not be deallocated
+ *         before the forwarder has been destroyed again.
+ * @features: Bitwise ORed features as defined with FWD_FEATURE_*.
+ *
+ * This function creates a new gpiochip, which forwards all GPIO operations to
+ * the passed GPIO descriptors.
+ *
+ * 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)
+{
+	struct gpiochip_fwd *fwd;
+	unsigned int i;
+	int error;
+
+	fwd = devm_gpiochip_fwd_alloc(dev, ngpios);
+	if (!fwd)
+		return fwd;
+
+	for (i = 0; i < ngpios; i++) {
+		error = gpiochip_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(dev, chip, fwd);
+		error = gpiochip_fwd_setup_delay_line(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 RFC v2 2/6] gpio: aggregator: refactor the forwarder part.

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 04:38:00PM +0100, Thomas Richard wrote:
> Prepare the code to create a gpio-fwd library. This library will allow to
> create and register a gpiochip forwarder.

...

>  struct gpiochip_fwd {
> +	struct device *dev;
>  	struct gpio_chip chip;

Have you checked the code generation?
Also, is this new pointer the same as chip.parent?

>  	struct gpio_desc **descs;
>  	union {

>  };

...

> +static struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
> +						    unsigned int ngpios)

I would rather split as

static struct gpiochip_fwd *
devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)

...

> +	fwd->descs = devm_kcalloc(dev, ngpios, sizeof(*fwd->descs),
> +				  GFP_KERNEL);

One line.

...

> +static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
> +{
> +	struct gpio_chip *chip = &fwd->chip;
> +	struct device *dev = fwd->dev;
> +	int error;
>  
>  	if (chip->can_sleep)
>  		mutex_init(&fwd->mlock);
>  	else
>  		spin_lock_init(&fwd->slock);
>  
> +	error = devm_gpiochip_add_data(dev, chip, fwd);
> +
> +	return error;

	return devm_...

> +}

...

Overall it looks and feels like this can be split to more simpler logically
isolated changes. At least I see that folding function parameters can be a
separate patch.

-- 
With Best Regards,
Andy Shevchenko

[PATCH RFC v2 3/6] gpio: aggregator: export symbols of the gpio-fwd library

[Thomas Richard]

Export all symbols and create header file for the gpio-fwd library.

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

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index a86e76daf67ab..7d00247f5268c 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/gpio-fwd.h>
 #include <linux/gpio/machine.h>
 
 #define AGGREGATOR_MAX_GPIOS 512
@@ -254,60 +255,47 @@ 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 device *dev;
-	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_GPL(gpio_fwd_get_direction);
 
-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_GPL(gpio_fwd_direction_input);
 
-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_GPL(gpio_fwd_direction_output);
 
-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_GPL(gpio_fwd_get);
 
-static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
-				 unsigned long *bits)
+static int gpio_fwd_get_multiple_unlocked(struct gpiochip_fwd *fwd,
+					  unsigned long *mask, unsigned long *bits)
 {
 	struct gpio_desc **descs = fwd_tmp_descs(fwd);
 	unsigned long *values = fwd_tmp_values(fwd);
@@ -332,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(struct gpio_chip *chip, unsigned long *mask,
+			  unsigned long *bits)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	unsigned long flags;
@@ -341,16 +329,17 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
 
 	if (chip->can_sleep) {
 		mutex_lock(&fwd->mlock);
-		error = gpio_fwd_get_multiple(fwd, mask, bits);
+		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
 	} else {
 		spin_lock_irqsave(&fwd->slock, flags);
-		error = gpio_fwd_get_multiple(fwd, mask, bits);
+		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
 	}
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(gpio_fwd_get_multiple);
 
 static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int value)
 {
@@ -373,7 +362,7 @@ static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int valu
 		udelay(delay_us);
 }
 
-static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
+void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 
@@ -385,9 +374,11 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 	if (fwd->delay_timings)
 		gpio_fwd_delay(chip, offset, value);
 }
+EXPORT_SYMBOL_GPL(gpio_fwd_set);
 
-static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
-				  unsigned long *bits)
+static void gpio_fwd_set_multiple_unlocked(struct gpiochip_fwd *fwd,
+					   unsigned long *mask,
+					   unsigned long *bits)
 {
 	struct gpio_desc **descs = fwd_tmp_descs(fwd);
 	unsigned long *values = fwd_tmp_values(fwd);
@@ -404,37 +395,40 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 		gpiod_set_array_value(j, descs, NULL, values);
 }
 
-static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
-					 unsigned long *mask, unsigned long *bits)
+void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+			   unsigned long *bits)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
 	unsigned long flags;
 
 	if (chip->can_sleep) {
 		mutex_lock(&fwd->mlock);
-		gpio_fwd_set_multiple(fwd, mask, bits);
+		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
 	} else {
 		spin_lock_irqsave(&fwd->slock, flags);
-		gpio_fwd_set_multiple(fwd, mask, bits);
+		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
 	}
 }
+EXPORT_SYMBOL_GPL(gpio_fwd_set_multiple);
 
-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_GPL(gpio_fwd_set_config);
 
-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_GPL(gpio_fwd_to_irq);
 
 /*
  * The GPIO delay provides a way to configure platform specific delays
@@ -489,8 +483,8 @@ static int gpiochip_fwd_setup_delay_line(struct gpiochip_fwd *fwd)
 }
 #endif	/* !CONFIG_OF_GPIO */
 
-static struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
-						    unsigned int ngpios)
+struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
+					     unsigned int ngpios)
 {
 	const char *label = dev_name(dev);
 	struct gpiochip_fwd *fwd;
@@ -515,18 +509,19 @@ static struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
 	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->get_multiple = gpio_fwd_get_multiple;
 	chip->set = gpio_fwd_set;
-	chip->set_multiple = gpio_fwd_set_multiple_locked;
+	chip->set_multiple = gpio_fwd_set_multiple;
 	chip->to_irq = gpio_fwd_to_irq;
 	chip->base = -1;
 	chip->ngpio = ngpios;
 
 	return fwd;
 }
+EXPORT_SYMBOL_GPL(devm_gpiochip_fwd_alloc);
 
-static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
-				      unsigned int offset)
+int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
+			       unsigned int offset)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	struct gpio_chip *parent = gpiod_to_chip(desc);
@@ -556,8 +551,9 @@ static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gpiochip_fwd_add_gpio_desc);
 
-static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	struct device *dev = fwd->dev;
@@ -572,6 +568,7 @@ static int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(gpiochip_fwd_register);
 
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
diff --git a/include/linux/gpio/gpio-fwd.h b/include/linux/gpio/gpio-fwd.h
new file mode 100644
index 0000000000000..d705b6d7ad76a
--- /dev/null
+++ b/include/linux/gpio/gpio-fwd.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_GPIO_FWD_H
+#define __LINUX_GPIO_FWD_H
+
+struct gpiochip_fwd_timing {
+	u32 ramp_up_us;
+	u32 ramp_down_us;
+};
+
+struct gpiochip_fwd {
+	struct device *dev;
+	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(struct gpio_chip *chip, unsigned long *mask,
+			  unsigned long *bits);
+
+void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value);
+
+void gpio_fwd_set_multiple(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_gpiochip_fwd_alloc(struct device *dev,
+					     unsigned int ngpios);
+
+int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
+			       struct gpio_desc *desc,
+			       unsigned int offset);
+
+int gpiochip_fwd_register(struct gpiochip_fwd *fwd);
+
+#endif

-- 
2.39.5

Re: [PATCH RFC v2 3/6] gpio: aggregator: export symbols of the gpio-fwd library

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 04:38:01PM +0100, Thomas Richard wrote:
> Export all symbols and create header file for the gpio-fwd library.

...

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

Please, name it forwarder.h.

>  #include <linux/gpio/machine.h>

...

> +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_GPL(gpio_fwd_get_direction);

No namespace? Ditto for all exports.

> -static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				 unsigned long *bits)
> +static int gpio_fwd_get_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					  unsigned long *mask, unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -332,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(struct gpio_chip *chip, unsigned long *mask,
> +			  unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
> @@ -341,16 +329,17 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_get_multiple);

These two are nicely named. Instead of touching them, just simply add an
exported wrapper. We can optimize it latter if needed, but it reduces a lot
the churn in this patch.

...

> -static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				  unsigned long *bits)
> +static void gpio_fwd_set_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					   unsigned long *mask,
> +					   unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -404,37 +395,40 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>  		gpiod_set_array_value(j, descs, NULL, values);
>  }
>  
> -static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
> -					 unsigned long *mask, unsigned long *bits)
> +void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +			   unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_set_multiple);

Ditto.

...

> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GPIO_FWD_H
> +#define __LINUX_GPIO_FWD_H

This header uses something that is defined in other headers. Please follow IWYU
principle.

...

> +struct gpiochip_fwd_timing {
> +	u32 ramp_up_us;
> +	u32 ramp_down_us;

types.h

> +};

...

> +struct gpiochip_fwd {
> +	struct device *dev;

struct device;

// forward declaration is enough.

> +	struct gpio_chip chip;

Where is this being defined?

> +	struct gpio_desc **descs;

> +	union {
> +		struct mutex mlock;	/* protects tmp[] if can_sleep */
> +		spinlock_t slock;	/* protects tmp[] if !can_sleep */

And these?

> +	};
> +	struct gpiochip_fwd_timing *delay_timings;
> +	unsigned long tmp[];		/* values and descs for multiple ops */
> +};

-- 
With Best Regards,
Andy Shevchenko

[PATCH RFC v2 4/6] 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 | 29 ++++++++++++++++++++++-------
 include/linux/gpio/gpio-fwd.h  |  2 ++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 7d00247f5268c..b9026ff2bfdc1 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 : -ENXIO;
+}
+EXPORT_SYMBOL_GPL(gpio_fwd_request);
+
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
@@ -505,6 +513,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
 	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;
@@ -524,7 +533,6 @@ int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 			       unsigned int offset)
 {
 	struct gpio_chip *chip = &fwd->chip;
-	struct gpio_chip *parent = gpiod_to_chip(desc);
 
 	if (offset > chip->ngpio)
 		return -EINVAL;
@@ -535,15 +543,10 @@ int gpiochip_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(fwd->dev, "%u => gpio %d irq %d\n", offset,
@@ -557,7 +560,19 @@ int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	struct device *dev = fwd->dev;
-	int error;
+	int ndescs = 0;
+	int error, i;
+
+	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);
diff --git a/include/linux/gpio/gpio-fwd.h b/include/linux/gpio/gpio-fwd.h
index d705b6d7ad76a..80ec34ee282fc 100644
--- a/include/linux/gpio/gpio-fwd.h
+++ b/include/linux/gpio/gpio-fwd.h
@@ -19,6 +19,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 RFC v2 4/6] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 04:38:02PM +0100, 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.

Hmm... This should rather be reformatted each time a new descriptor is added,
no?

...

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

Why was this error code chosen?

> +}

...

>  	struct gpio_chip *chip = &fwd->chip;
>  	struct device *dev = fwd->dev;
> -	int error;
> +	int ndescs = 0;
> +	int error, i;

The new added variables are signed. Why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 4/6] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

[Thomas Richard]

On 3/17/25 18:13, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 04:38:02PM +0100, 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.
> 
> Hmm... This should rather be reformatted each time a new descriptor is added,
> no?
I used the easiest solution.

Otherwise to switch to can_sleep mode:
- gpio_fwd_add_gpio_desc() inits the mutex if a sleeping GPIO line is added.
- gpio_fwd_add_gpio_desc() locks the mutex if the spinlock is locked
(gpio_fwd_get_multiple_locked() or gpio_fwd_set_multiple_locked() was
called).
- gpio_fwd_add_gpio_desc() set can_sleep mode to true
- gpio_fwd_get_multiple_locked() or gpio_fwd_set_multiple_locked() shall
unlock the spinlock and the mutex at end.

I think it is a bit over-engineered, and I probably do not handle all
corner cases.

Regards,

Thomas

[PATCH RFC v2 5/6] 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/gpio-fwd.h  | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index b9026ff2bfdc1..3c78c47ce40ae 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -556,7 +556,7 @@ int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_fwd_add_gpio_desc);
 
-int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
+int gpiochip_fwd_register(struct gpiochip_fwd *fwd, void *data)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	struct device *dev = fwd->dev;
@@ -579,6 +579,8 @@ int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 	else
 		spin_lock_init(&fwd->slock);
 
+	fwd->data = data;
+
 	error = devm_gpiochip_add_data(dev, chip, fwd);
 
 	return error;
@@ -625,7 +627,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 			return ERR_PTR(error);
 	}
 
-	error = gpiochip_fwd_register(fwd);
+	error = gpiochip_fwd_register(fwd, NULL);
 	if (error)
 		return ERR_PTR(error);
 
diff --git a/include/linux/gpio/gpio-fwd.h b/include/linux/gpio/gpio-fwd.h
index 80ec34ee282fc..c242cc1888180 100644
--- a/include/linux/gpio/gpio-fwd.h
+++ b/include/linux/gpio/gpio-fwd.h
@@ -10,6 +10,7 @@ struct gpiochip_fwd_timing {
 struct gpiochip_fwd {
 	struct device *dev;
 	struct gpio_chip chip;
+	void *data;
 	struct gpio_desc **descs;
 	union {
 		struct mutex mlock;	/* protects tmp[] if can_sleep */
@@ -50,6 +51,6 @@ int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
 			       struct gpio_desc *desc,
 			       unsigned int offset);
 
-int gpiochip_fwd_register(struct gpiochip_fwd *fwd);
+int gpiochip_fwd_register(struct gpiochip_fwd *fwd, void *data);
 
 #endif

-- 
2.39.5

Re: [PATCH RFC v2 5/6] gpio: aggregator: add possibility to attach data to the forwarder

[Bartosz Golaszewski]

On Mon, Mar 17, 2025 at 4:38 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> 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/gpio-fwd.h  | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index b9026ff2bfdc1..3c78c47ce40ae 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -556,7 +556,7 @@ int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_fwd_add_gpio_desc);
>
> -int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
> +int gpiochip_fwd_register(struct gpiochip_fwd *fwd, void *data)
>  {
>         struct gpio_chip *chip = &fwd->chip;
>         struct device *dev = fwd->dev;
> @@ -579,6 +579,8 @@ int gpiochip_fwd_register(struct gpiochip_fwd *fwd)

I see Andy already requested some renaming but can you also use a
gpio_fwd_ prefix for these exported interfaces?

Bart

>         else
>                 spin_lock_init(&fwd->slock);
>
> +       fwd->data = data;
> +
>         error = devm_gpiochip_add_data(dev, chip, fwd);
>
>         return error;
> @@ -625,7 +627,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>                         return ERR_PTR(error);
>         }
>
> -       error = gpiochip_fwd_register(fwd);
> +       error = gpiochip_fwd_register(fwd, NULL);
>         if (error)
>                 return ERR_PTR(error);
>
> diff --git a/include/linux/gpio/gpio-fwd.h b/include/linux/gpio/gpio-fwd.h
> index 80ec34ee282fc..c242cc1888180 100644
> --- a/include/linux/gpio/gpio-fwd.h
> +++ b/include/linux/gpio/gpio-fwd.h
> @@ -10,6 +10,7 @@ struct gpiochip_fwd_timing {
>  struct gpiochip_fwd {
>         struct device *dev;
>         struct gpio_chip chip;
> +       void *data;
>         struct gpio_desc **descs;
>         union {
>                 struct mutex mlock;     /* protects tmp[] if can_sleep */
> @@ -50,6 +51,6 @@ int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
>                                struct gpio_desc *desc,
>                                unsigned int offset);
>
> -int gpiochip_fwd_register(struct gpiochip_fwd *fwd);
> +int gpiochip_fwd_register(struct gpiochip_fwd *fwd, void *data);
>
>  #endif
>
> --
> 2.39.5
>

[PATCH RFC v2 6/6] 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).

UP boards and UP Squared boards are supported.

Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
rewritten.

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

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 95a8e2b9a614a..856152363e3a6 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -576,6 +576,21 @@ 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.
+
+	  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 fba1c56624c0d..989b8d28ecac3 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -56,6 +56,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 0000000000000..9f7d0c6b707d0
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,1094 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * UP board pin control driver.
+ *
+ * FPGA provides more GPIO driving power, LEDS and pin mux function.
+ *
+ * Copyright (c) AAEON. All rights reserved.
+ * Copyright (C) 2024 Bootlin
+ *
+ * Author: Gary Wang <garywang@aaeon.com.tw>
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/gpio-fwd.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+enum upboard_board_id {
+	BOARD_UP_APL01,
+};
+
+enum upboard_pin_mode {
+	UPBOARD_PIN_MODE_FUNCTION = 1,
+	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_MODE(_grp, _pins, _mode)			\
+{									\
+	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
+	.mode = _mode,							\
+}
+
+#define UPBOARD_PINGROUP_MODES(_grp, _pins, _modes)			\
+{									\
+	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
+	.modes = _modes,							\
+}
+
+static const struct upboard_pingroup upboard_up_pin_groups[] = {
+	UPBOARD_PINGROUP_MODES("uart1_grp", upboard_up_uart1_pins, upboard_up_uart1_modes),
+	UPBOARD_PINGROUP_MODE("i2c0_grp", upboard_up_i2c0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("i2c1_grp", upboard_up_i2c1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODES("spi2_grp", upboard_up_spi2_pins, &upboard_up_spi2_modes[0]),
+	UPBOARD_PINGROUP_MODES("i2s0_grp", upboard_up_i2s0_pins, &upboard_up_i2s0_modes[0]),
+	UPBOARD_PINGROUP_MODE("pwm0_grp", upboard_up_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("pwm1_grp", upboard_up_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("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_MODES("uart1_grp", upboard_up2_uart1_pins, upboard_up2_uart1_modes),
+	UPBOARD_PINGROUP_MODE("i2c0_grp", upboard_up2_i2c0_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP_MODE("i2c1_grp", upboard_up2_i2c1_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP_MODES("spi1_grp", upboard_up2_spi1_pins, upboard_up2_spi_modes),
+	UPBOARD_PINGROUP_MODES("spi2_grp", upboard_up2_spi2_pins, upboard_up2_spi_modes),
+	UPBOARD_PINGROUP_MODES("i2s0_grp", upboard_up2_i2s0_pins, upboard_up2_i2s0_modes),
+	UPBOARD_PINGROUP_MODE("pwm0_grp", upboard_up2_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("pwm1_grp", upboard_up2_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("adc0_grp", upboard_up2_adc0_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP_MODE("adc2_grp", upboard_up2_adc2_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP_MODE("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];
+
+	if (p->funcbit) {
+		int 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;
+	int mode, ret, i;
+
+	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].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 ?
+							    true : false);
+		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 = 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 gpiochip_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;
+
+	mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);
+
+	/* If the pin is in function mode or high-z, input direction is returned */
+	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;
+	int ret, i;
+
+	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;
+	int ret, i;
+
+	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_up_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 *)BOARD_UP_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 (IS_ERR_OR_NULL(dmi_id))
+		return -ENODEV;
+
+	board_id = (enum upboard_board_id)dmi_id->driver_data;
+
+	switch (board_id) {
+	case BOARD_UP_APL01:
+		pctrl->maps = upboard_pinctrl_mapping_up_apl01;
+		pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_up_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 = pinctrl_register_mappings(pctrl->maps, pctrl->nmaps);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pinctrl mappings\n");
+
+	pinctrl = devm_pinctrl_get_select_default(dev);
+	if (IS_ERR(pinctrl)) {
+		ret = PTR_ERR(pinctrl);
+		dev_err(dev, "Failed to select pinctrl: %d\n", ret);
+		goto free_mapping;
+	}
+
+	ret = pinctrl_enable(pctrl->pctldev);
+	if (ret) {
+		dev_err(dev, "Failed to enable pinctrl: %d\n", ret);
+		goto free_mapping;
+	}
+
+	fwd = devm_gpiochip_fwd_alloc(dev, pctrl->pctrl_data->ngpio);
+	if (IS_ERR(fwd)) {
+		ret = PTR_ERR(fwd);
+		dev_err(dev, "Failed to allocate the gpiochip fowarder: %d\n", ret);
+		goto free_mapping;
+	}
+
+	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 = gpiochip_fwd_register(fwd, pctrl);
+	if (ret) {
+		dev_err(dev, "Failed to register the gpiochip forwarder: %d\n", ret);
+		goto free_mapping;
+	}
+
+	return gpiochip_add_pin_range_sparse(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
+					     pctrl->pctrl_data->ngpio);
+
+free_mapping:
+	pinctrl_unregister_mappings(pctrl->maps);
+
+	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("Gary Wang <garywang@aaeon.com.tw>");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com");
+MODULE_DESCRIPTION("UP Board HAT pin controller driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:upboard-pinctrl");

-- 
2.39.5

Re: [PATCH RFC v2 6/6] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 04:38:04PM +0100, 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).
> 
> UP boards and UP Squared boards are supported.

> Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
> rewritten.

I am not against giving the credit(s), but with "largely rewritten" it sounds
to me like a cover letter material (or comments block after '---' line below).

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

...

> +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 board
	  - UP Squared board

> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pinctrl-upboard.

...

> +/*
> + * UP board pin control driver.
> + *
> + * FPGA provides more GPIO driving power, LEDS and pin mux function.
> + *
> + * Copyright (c) AAEON. All rights reserved.
> + * Copyright (C) 2024 Bootlin

> + * Author: Gary Wang <garywang@aaeon.com.tw>

Hmm... If you want him to be the author you probably want to have his SoB
and Co-developed-by, but again, it contradicts with "largely rewritten"
(in my book it's 67%+, is it the case here?).

> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */

...

> +#include <linux/device.h>
> +#include <linux/dmi.h>

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

I'm wondering if we may guarantee the consumer.h and driver.h be provided by
forwarder.h as it sounds natural choice if one wants to have the GPIO forwarder
library.

> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>

> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>

We usually move this group of inclusions separately...

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>

...somewhere here.

> +#include "core.h"
> +#include "pinmux.h"

...

> +enum upboard_board_id {
> +	BOARD_UP_APL01,
> +};

This doesn't sound like a list of two or more... Why do you need it?

...

> +enum upboard_pin_mode {
> +	UPBOARD_PIN_MODE_FUNCTION = 1,
> +	UPBOARD_PIN_MODE_GPIO_IN,
> +	UPBOARD_PIN_MODE_GPIO_OUT,
> +	UPBOARD_PIN_MODE_DISABLED,

Why from 1? Is it by HW? In such a case you need to strictly assign _all_ of
them.

> +};

...

> +#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),			\
> +	}

Don't we have macros already in pin control headers (global and
subsystem ones)?

...

> +static_assert(ARRAY_SIZE(upboard_up_uart1_modes) ==
> +	      ARRAY_SIZE(upboard_up_uart1_pins));

One line, also include array_size.h.
Ditto for the rest of similar cases.

...

> +#define UPBOARD_PINGROUP_MODE(_grp, _pins, _mode)			\
> +{									\
> +	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
> +	.mode = _mode,							\
> +}
> +
> +#define UPBOARD_PINGROUP_MODES(_grp, _pins, _modes)			\
> +{									\
> +	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
> +	.modes = _modes,							\
> +}

See the trick in pinctrl-intel.h how to make it a single macro.

...

> +#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),				\
> +	}

As per above, use already given macros in the above.

...

> +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) {
> +		int ret = regmap_field_write(p->funcbit, 0);
> +
> +		if (ret)
> +			return ret;

		ret = regmap_field_write(p->funcbit, 0);
		if (ret)
			return ret;

> +	}
> +
> +	return regmap_field_write(p->enbit, 1);
> +}

...

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

Why i is signed?

> +	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].mode :

Use Elvis.

> +			upgroups[group_selector].modes[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 ?

> +							    true : false);

Useless ternary.

> +		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 = upboard_pinctrl_pin_get_mode(pctldev, offset);

Split it.

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

Shouldn't be this and respective assignment below under ifdeffery?
I don't remember.

...

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

+ container_of.h

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

+ err.h

> +		pinctrl_gpio_free(gc, offset);
> +		return PTR_ERR(desc);
> +	}
> +
> +	return gpiochip_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;

> +	mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);
> +
> +	/* If the pin is in function mode or high-z, input direction is returned */
> +	if (mode < 0)
> +		return mode;

Write it like this (1 LoC less):

	/* 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_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
> +{
> +	const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
> +	size_t ngroups = pctrl->pctrl_data->ngroups;
> +	int ret, i;
> +
> +	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;
> +	int ret, i;
> +
> +	for (i = 0; i < nfuncs ; ++i) {
> +		ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name, funcs[i].groups,
> +						  funcs[i].ngroups, NULL);

Logically in the similar way to the above:

		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 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 *)BOARD_UP_APL01,
> +	},
> +	{ },

No comma in the terminator entry.

> +};

...

> +	dmi_id = dmi_first_match(dmi_platform_info);
> +	if (IS_ERR_OR_NULL(dmi_id))

Can it really return error pointer?

> +		return -ENODEV;

...

> +	board_id = (enum upboard_board_id)dmi_id->driver_data;
> +
> +	switch (board_id) {
> +	case BOARD_UP_APL01:
> +		pctrl->maps = upboard_pinctrl_mapping_up_apl01;
> +		pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_up_apl01);
> +		break;

Hmm... This is strange. Seems it has only Apollo Lake in the name while
the above states that there is UP board support (which is Cherry Trail based).

> +	default:
> +		return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
> +	}

...

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

> +

Redundant blank line.

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

...

> +	ret = pinctrl_register_mappings(pctrl->maps, pctrl->nmaps);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register pinctrl mappings\n");
> +
> +	pinctrl = devm_pinctrl_get_select_default(dev);
> +	if (IS_ERR(pinctrl)) {
> +		ret = PTR_ERR(pinctrl);
> +		dev_err(dev, "Failed to select pinctrl: %d\n", ret);
> +		goto free_mapping;

One should not mix devm_*() with non-devm_*() in such a way. Here we expect
to have return dev_err_probe() as usual. What is missing is devm_ wrapper
for the above.

> +	}

...

> +free_mapping:
> +	pinctrl_unregister_mappings(pctrl->maps);

And also you don't do that when remove the module...

> +	return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 6/6] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 3/17/25 19:42, Andy Shevchenko wrote:
>> +	board_id = (enum upboard_board_id)dmi_id->driver_data;
>> +
>> +	switch (board_id) {
>> +	case BOARD_UP_APL01:
>> +		pctrl->maps = upboard_pinctrl_mapping_up_apl01;
>> +		pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_up_apl01);
>> +		break;
> 
> Hmm... This is strange. Seems it has only Apollo Lake in the name while
> the above states that there is UP board support (which is Cherry Trail based).

There is pinctrl code for UP and UP Squared. But I only added mapping
for UP Squared board because it was the easiest board to add (all needed
pinctrl groups and functions are already defined in the Intel pinctrl
driver).
I wanted to focus on the driver itself and the forwarder library, and
send later an other series to add UP board support (with the
corresponding functions/groups in the Intel pinctrl driver).

So yes UP board pinctrl part is unused for now, but everything is ready
to add support for UP board in the future.

Otherwise I can remove UP part if you prefer.

Regards,

Thomas

Re: [PATCH RFC v2 6/6] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 4/9/25 16:02, Thomas Richard wrote:
> On 3/17/25 19:42, Andy Shevchenko wrote:
>>> +	board_id = (enum upboard_board_id)dmi_id->driver_data;
>>> +
>>> +	switch (board_id) {
>>> +	case BOARD_UP_APL01:
>>> +		pctrl->maps = upboard_pinctrl_mapping_up_apl01;
>>> +		pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_up_apl01);
>>> +		break;
>>
>> Hmm... This is strange. Seems it has only Apollo Lake in the name while
>> the above states that there is UP board support (which is Cherry Trail based).
> 
> There is pinctrl code for UP and UP Squared. But I only added mapping
> for UP Squared board because it was the easiest board to add (all needed
> pinctrl groups and functions are already defined in the Intel pinctrl
> driver).
> I wanted to focus on the driver itself and the forwarder library, and
> send later an other series to add UP board support (with the
> corresponding functions/groups in the Intel pinctrl driver).
> 
> So yes UP board pinctrl part is unused for now, but everything is ready
> to add support for UP board in the future.
> 
> Otherwise I can remove UP part if you prefer.

Hi Andy,

I just realized that my wording was incorrect, so let me clarify.

The UP Board [1] (based on Cherry Trail) is not supported at all (its
FPGA is not supported by the MFD driver), and there is no plan to
support it.

The "up" code (for example upboard_up_pinctrl_data) is for most boards
of the UP family (UP 7000, UPXi12, UP Squared Pro 7000 ...) but not the
UP Board [1]. Indeed for now there is no pinctrl mapping for these boards.

The "up2" code is for the UP Squared board which uses a different FPGA.

As I explained I only added pinctrl mapping for the UP Squared board, as
it was the easiest one (no need to add groups/functions in Intel pinctrl
driver). But the plan is to add support for other boards (UP 7000,
IPXi12 ...) later.

Regarding the name of the pinctrl mapping for UP Squared board, it is
indeed a bit confusing, I will rename it.

[1]
https://www.aaeon.com/en/product/detail/up-board-computer-board-for-professional-makers

Best Regards,

Thomas