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

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

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

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

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

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

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

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

---
Thomas Richard (10):
      gpiolib: add support to register sparse pin range
      pinctrl: core: add devm_pinctrl_register_mappings()
      gpio: aggregator: move GPIO forwarder allocation in a dedicated function
      gpio: aggregator: refactor the code to add GPIO desc in the forwarder
      gpio: aggregator: refactor the forwarder registration part
      gpio: aggregator: update gpiochip_fwd_setup_delay_line() parameters
      gpio: aggregator: export symbols of the GPIO forwarder library
      gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd
      gpio: aggregator: add possibility to attach data to the forwarder
      pinctrl: Add pin controller driver for AAEON UP boards

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

Best regards,

On Wed, Apr 16, 2025 at 04:08:09PM +0200, Thomas Richard wrote:
> Add support to register for GPIO<->pin mapping using a list of non
> consecutive pins. The core already support sparse pin range (pins member
> of struct pinctrl_gpio_range), but it was not possible to register one. If
> pins is not NULL the core uses it, otherwise it assumes that a consecutive
> pin range was registered and it uses pin_base.
> 
> The function gpiochip_add_pin_range() which allocates and fill the struct
> pinctrl_gpio_range was renamed to gpiochip_add_pin_range_with_pins() and
> the pins parameter was added.
> 
> Two new functions were added, gpiochip_add_pin_range() and
> gpiochip_add_sparse_pin_range() to register a consecutive or sparse pins
> range. Both use gpiochip_add_pin_range_with_pins().

...

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

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

> +		chip_dbg(gc, "created GPIO range %d->%d ==> %s PIN %d->%d\n",
> +			 gpio_offset, gpio_offset + npins - 1,
> +			 pinctl_name,
> +			 pin_offset, pin_offset + npins - 1);
> +	else
> +		chip_dbg(gc, "created GPIO range %d->%d ==> %s %d sparse PIN range { %d, ... }",
> +			 gpio_offset, gpio_offset + npins - 1,
> +			 pinctl_name, npins, pins[0]);

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

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

...

> +static int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd,
> +				      struct gpio_desc *desc,
> +				      unsigned int offset)
> +{
> +	struct gpio_chip *parent = gpiod_to_chip(desc);
> +	struct gpio_chip *chip = &fwd->chip;
> +
> +	if (offset > chip->ngpio)

>= ?

> +		return -EINVAL;

> +	if (fwd->descs[offset])
> +		return -EEXIST;

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

> +	/*
> +	 * If any of the GPIO lines are sleeping, then the entire forwarder
> +	 * will be sleeping.
> +	 * If any of the chips support .set_config(), then the forwarder will
> +	 * support setting configs.
> +	 */
> +	if (gpiod_cansleep(desc))
> +		chip->can_sleep = true;
> +
> +	if (parent && parent->set_config)
> +		chip->set_config = gpio_fwd_set_config;
> +
> +	fwd->descs[offset] = desc;
> +
> +	dev_dbg(chip->parent, "%u => gpio %d irq %d\n", offset,
> +		desc_to_gpio(desc), gpiod_to_irq(desc));
> +
> +	return 0;
> +}

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

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

...

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

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