[RFC] pinctrl: sunxi: convert to GPIO_GENERIC

Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
register frame. So far we were instantiating one GPIO chip per pinctrl
device, which covers multiple banks of up to 32 GPIO pins per bank. The
GPIO numbers were set to match the absolute pin numbers, even across the
typically two instances of the pinctrl device.

Convert the GPIO part of the sunxi pinctrl over to use the gpio_generic
framework. This alone allows to remove some sunxi specific code, which
is replaced with the existing generic code. This will become even more
useful with the upcoming A733 support, which adds set and clear
registers for the output.
As a side effect this also changes the GPIO device and number
allocation: Each bank is now represented by its own gpio_chip, with only
as many pins as there are actually implemented. The numbering is left up
to the kernel (.base = -1).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/pinctrl/sunxi/Kconfig         |   1 +
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 245 ++++++++++++--------------
 drivers/pinctrl/sunxi/pinctrl-sunxi.h |  11 +-
 3 files changed, 124 insertions(+), 133 deletions(-)
  

在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
> Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> register frame. So far we were instantiating one GPIO chip per
> pinctrl
> device, which covers multiple banks of up to 32 GPIO pins per bank.
> The
> GPIO numbers were set to match the absolute pin numbers, even across
> the
> typically two instances of the pinctrl device.
> 
> Convert the GPIO part of the sunxi pinctrl over to use the
> gpio_generic
> framework. This alone allows to remove some sunxi specific code,
> which
> is replaced with the existing generic code. This will become even
> more
> useful with the upcoming A733 support, which adds set and clear
> registers for the output.
> As a side effect this also changes the GPIO device and number
> allocation: Each bank is now represented by its own gpio_chip, with
> only
> as many pins as there are actually implemented. The numbering is left
> up

Ah, is this a userspace API break?

Sincerely,
Icenowy

> to the kernel (.base = -1).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/pinctrl/sunxi/Kconfig         |   1 +
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 245 ++++++++++++------------
> --
>  drivers/pinctrl/sunxi/pinctrl-sunxi.h |  11 +-
>  3 files changed, 124 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/Kconfig
> b/drivers/pinctrl/sunxi/Kconfig
> index dc62eba96348e..5905810dbf398 100644
> --- a/drivers/pinctrl/sunxi/Kconfig
> +++ b/drivers/pinctrl/sunxi/Kconfig
> @@ -4,6 +4,7 @@ if ARCH_SUNXI
>  config PINCTRL_SUNXI
>  	bool
>  	select PINMUX
> +	select GPIO_GENERIC
>  	select GENERIC_PINCONF
>  	select GPIOLIB
>  
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 48434292a39b5..4235f9feff00d 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -13,6 +13,7 @@
>  #include <linux/clk.h>
>  #include <linux/export.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/generic.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/irqchip/chained_irq.h>
> @@ -86,17 +87,6 @@ static void sunxi_mux_reg(const struct
> sunxi_pinctrl *pctl,
>  	*mask  = (BIT(MUX_FIELD_WIDTH) - 1) << *shift;
>  }
>  
> -static void sunxi_data_reg(const struct sunxi_pinctrl *pctl,
> -			   u32 pin, u32 *reg, u32 *shift, u32 *mask)
> -{
> -	u32 offset = pin % PINS_PER_BANK * DATA_FIELD_WIDTH;
> -
> -	*reg   = sunxi_bank_offset(pctl, pin) + DATA_REGS_OFFSET +
> -		 offset / BITS_PER_TYPE(u32) * sizeof(u32);
> -	*shift = offset % BITS_PER_TYPE(u32);
> -	*mask  = (BIT(DATA_FIELD_WIDTH) - 1) << *shift;
> -}
> -
>  static void sunxi_dlevel_reg(const struct sunxi_pinctrl *pctl,
>  			     u32 pin, u32 *reg, u32 *shift, u32
> *mask)
>  {
> @@ -930,99 +920,22 @@ static const struct pinmux_ops sunxi_pmx_ops =
> {
>  	.strict			= true,
>  };
>  
> -static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip
> *chip,
> -					unsigned offset)
> -{
> -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> -
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> -					    chip->base + offset,
> true);
> -}
> -
> -static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned
> offset)
> -{
> -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> -	bool set_mux = pctl->desc->irq_read_needs_mux &&
> -		gpiochip_line_is_irq(chip, offset);
> -	u32 pin = offset + chip->base;
> -	u32 reg, shift, mask, val;
> -
> -	sunxi_data_reg(pctl, offset, &reg, &shift, &mask);
> -
> -	if (set_mux)
> -		sunxi_pmx_set(pctl->pctl_dev, pin,
> SUN4I_FUNC_INPUT);
> -
> -	val = (readl(pctl->membase + reg) & mask) >> shift;
> -
> -	if (set_mux)
> -		sunxi_pmx_set(pctl->pctl_dev, pin, SUN4I_FUNC_IRQ);
> -
> -	return val;
> -}
> -
> -static int sunxi_pinctrl_gpio_set(struct gpio_chip *chip, unsigned
> int offset,
> -				  int value)
> -{
> -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> -	u32 reg, shift, mask, val;
> -	unsigned long flags;
> -
> -	sunxi_data_reg(pctl, offset, &reg, &shift, &mask);
> -
> -	raw_spin_lock_irqsave(&pctl->lock, flags);
> -
> -	val = readl(pctl->membase + reg);
> -
> -	if (value)
> -		val |= mask;
> -	else
> -		val &= ~mask;
> -
> -	writel(val, pctl->membase + reg);
> -
> -	raw_spin_unlock_irqrestore(&pctl->lock, flags);
> -
> -	return 0;
> -}
> -
> -static int sunxi_pinctrl_gpio_direction_output(struct gpio_chip
> *chip,
> -					unsigned offset, int value)
> -{
> -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> -
> -	sunxi_pinctrl_gpio_set(chip, offset, value);
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> -					    chip->base + offset,
> false);
> -}
> -
> -static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,
> -				const struct of_phandle_args
> *gpiospec,
> -				u32 *flags)
> -{
> -	int pin, base;
> -
> -	base = PINS_PER_BANK * gpiospec->args[0];
> -	pin = base + gpiospec->args[1];
> -
> -	if (pin > gc->ngpio)
> -		return -EINVAL;
> -
> -	if (flags)
> -		*flags = gpiospec->args[2];
> -
> -	return pin;
> -}
> -
>  static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip,
> unsigned offset)
>  {
>  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
>  	struct sunxi_desc_function *desc;
> -	unsigned pinnum = pctl->desc->pin_base + offset;
> -	unsigned irqnum;
> +	unsigned int pinnum, irqnum, i;
>  
>  	if (offset >= chip->ngpio)
>  		return -ENXIO;
>  
> +	for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++)
> +		if (pctl->banks[i].chip.gc.base == chip->base)
> +			break;
> +	if (i == SUNXI_PINCTRL_MAX_BANKS)
> +		return -EINVAL;
> +	pinnum = pctl->desc->pin_base + i * PINS_PER_BANK + offset;
> +
>  	desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum,
> "irq");
>  	if (!desc)
>  		return -EINVAL;
> @@ -1039,18 +952,19 @@ static int
> sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>  {
>  	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>  	struct sunxi_desc_function *func;
> -	int ret;
> +	int pinnum = pctl->irq_array[d->hwirq], ret;
> +	int bank = (pinnum - pctl->desc->pin_base) / PINS_PER_BANK;
>  
> -	func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
> -					pctl->irq_array[d->hwirq],
> "irq");
> +	func = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum,
> "irq");
>  	if (!func)
>  		return -EINVAL;
>  
> -	ret = gpiochip_lock_as_irq(pctl->chip,
> -			pctl->irq_array[d->hwirq] - pctl->desc-
> >pin_base);
> +	ret = gpiochip_lock_as_irq(&pctl->banks[bank].chip.gc,
> +				   d->hwirq % IRQ_PER_BANK);
>  	if (ret) {
>  		dev_err(pctl->dev, "unable to lock HW IRQ %lu for
> IRQ\n",
>  			irqd_to_hwirq(d));
> +
>  		return ret;
>  	}
>  
> @@ -1063,9 +977,10 @@ static int
> sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>  static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
>  {
>  	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +	int pinnum = pctl->irq_array[d->hwirq] - pctl->desc-
> >pin_base;
> +	struct gpio_chip *gc = &pctl->banks[pinnum /
> PINS_PER_BANK].chip.gc;
>  
> -	gpiochip_unlock_as_irq(pctl->chip,
> -			      pctl->irq_array[d->hwirq] - pctl-
> >desc->pin_base);
> +	gpiochip_unlock_as_irq(gc, pinnum);
>  }
>  
>  static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned
> int type)
> @@ -1493,6 +1408,84 @@ static int sunxi_pinctrl_setup_debounce(struct
> sunxi_pinctrl *pctl,
>  	return 0;
>  }
>  
> +static bool sunxi_of_node_instance_match(struct gpio_chip *chip,
> unsigned int i)
> +{
> +	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	if (i >= SUNXI_PINCTRL_MAX_BANKS)
> +		return false;
> +
> +	return (chip->base == pctl->banks[i].chip.gc.base);
> +}
> +
> +static int sunxi_num_pins_of_bank(struct sunxi_pinctrl *pctl, int
> bank)
> +{
> +	int max = -1, i;
> +
> +	for (i = 0; i < pctl->desc->npins; i++) {
> +		int pinnum = pctl->desc->pins[i].pin.number - pctl-
> >desc->pin_base;
> +
> +		if (pinnum / PINS_PER_BANK < bank)
> +			continue;
> +		if (pinnum / PINS_PER_BANK > bank)
> +			break;
> +		if (pinnum % PINS_PER_BANK > max)
> +			max = pinnum % PINS_PER_BANK;
> +	}
> +
> +	return max + 1;
> +}
> +
> +static int sunxi_gpio_add_bank(struct sunxi_pinctrl *pctl, int
> index)
> +{
> +	char bank_name = 'A' + index + pctl->desc->pin_base /
> PINS_PER_BANK;
> +	struct sunxi_gpio_bank *bank = &pctl->banks[index];
> +	struct gpio_generic_chip_config config;
> +	struct gpio_chip *gc = &bank->chip.gc;
> +	int ngpio, ret;
> +
> +	ngpio = sunxi_num_pins_of_bank(pctl, index);
> +	bank->pctl = pctl;
> +	bank->base = pctl->membase + index * pctl->bank_mem_size;
> +	if (!ngpio) {
> +		gc->owner = THIS_MODULE;
> +		gc->ngpio = 0;
> +		gc->base = -1;
> +		gc->of_gpio_n_cells = 3;
> +
> +		return 0;
> +	}
> +
> +	config = (struct gpio_generic_chip_config) {
> +		.dev = pctl->dev,
> +		.sz = 4,
> +		.dat = bank->base + DATA_REGS_OFFSET,
> +		.set = bank->base + DATA_REGS_OFFSET,
> +		.clr = NULL,
> +		.flags = GPIO_GENERIC_READ_OUTPUT_REG_SET |
> +			 GPIO_GENERIC_PINCTRL_BACKEND,
> +	};
> +
> +	ret = gpio_generic_chip_init(&bank->chip, &config);
> +	if (ret)
> +		return dev_err_probe(pctl->dev, ret,
> +				     "failed to init generic gpio
> chip\n");
> +
> +	gc->owner		= THIS_MODULE;
> +	gc->label		= devm_kasprintf(pctl->dev,
> GFP_KERNEL,
> +						 "%s-P%c", gc-
> >label,
> +						 bank_name);
> +	gc->ngpio		= ngpio;
> +	gc->base		= -1;
> +	gc->of_gpio_n_cells	= 3;
> +	gc->of_node_instance_match = sunxi_of_node_instance_match;
> +	gc->set_config		= gpiochip_generic_config;
> +	gc->to_irq		= sunxi_pinctrl_gpio_to_irq;
> +	gc->can_sleep		= false;
> +
> +	return gpiochip_add_data(gc, pctl);
> +}
> +
>  int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>  				  const struct sunxi_pinctrl_desc
> *desc,
>  				  unsigned long flags)
> @@ -1503,6 +1496,7 @@ int sunxi_pinctrl_init_with_flags(struct
> platform_device *pdev,
>  	struct sunxi_pinctrl *pctl;
>  	struct pinmux_ops *pmxops;
>  	int i, ret, last_pin, pin_idx;
> +	int gpio_banks;
>  	struct clk *clk;
>  
>  	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> @@ -1590,38 +1584,23 @@ int sunxi_pinctrl_init_with_flags(struct
> platform_device *pdev,
>  		return PTR_ERR(pctl->pctl_dev);
>  	}
>  
> -	pctl->chip = devm_kzalloc(&pdev->dev, sizeof(*pctl->chip),
> GFP_KERNEL);
> -	if (!pctl->chip)
> -		return -ENOMEM;
> -
> -	last_pin = pctl->desc->pins[pctl->desc->npins -
> 1].pin.number;
> -	pctl->chip->owner = THIS_MODULE;
> -	pctl->chip->request = gpiochip_generic_request;
> -	pctl->chip->free = gpiochip_generic_free;
> -	pctl->chip->set_config = gpiochip_generic_config;
> -	pctl->chip->direction_input =
> sunxi_pinctrl_gpio_direction_input;
> -	pctl->chip->direction_output =
> sunxi_pinctrl_gpio_direction_output;
> -	pctl->chip->get = sunxi_pinctrl_gpio_get;
> -	pctl->chip->set = sunxi_pinctrl_gpio_set;
> -	pctl->chip->of_xlate = sunxi_pinctrl_gpio_of_xlate;
> -	pctl->chip->to_irq = sunxi_pinctrl_gpio_to_irq;
> -	pctl->chip->of_gpio_n_cells = 3;
> -	pctl->chip->can_sleep = false;
> -	pctl->chip->ngpio = round_up(last_pin, PINS_PER_BANK) -
> -			    pctl->desc->pin_base;
> -	pctl->chip->label = dev_name(&pdev->dev);
> -	pctl->chip->parent = &pdev->dev;
> -	pctl->chip->base = pctl->desc->pin_base;
> -
> -	ret = gpiochip_add_data(pctl->chip, pctl);
> -	if (ret)
> -		return ret;
> +	last_pin = pctl->desc->pins[pctl->desc->npins -
> 1].pin.number -
> +		   pctl->desc->pin_base;
> +	for (gpio_banks = 0;
> +	     gpio_banks <= last_pin / PINS_PER_BANK;
> +	     gpio_banks++) {
> +		ret = sunxi_gpio_add_bank(pctl, gpio_banks);
> +		if (ret)
> +			goto gpiochip_error;
> +	}
>  
>  	for (i = 0; i < pctl->desc->npins; i++) {
>  		const struct sunxi_desc_pin *pin = pctl->desc->pins
> + i;
> +		int bank = (pin->pin.number - pctl->desc->pin_base)
> / PINS_PER_BANK;
> +		struct gpio_chip *gc = &pctl->banks[bank].chip.gc;
>  
> -		ret = gpiochip_add_pin_range(pctl->chip,
> dev_name(&pdev->dev),
> -					     pin->pin.number - pctl-
> >desc->pin_base,
> +		ret = gpiochip_add_pin_range(gc, dev_name(&pdev-
> >dev),
> +					     pin->pin.number %
> PINS_PER_BANK,
>  					     pin->pin.number, 1);
>  		if (ret)
>  			goto gpiochip_error;
> @@ -1690,6 +1669,8 @@ int sunxi_pinctrl_init_with_flags(struct
> platform_device *pdev,
>  	return 0;
>  
>  gpiochip_error:
> -	gpiochip_remove(pctl->chip);
> +	while (--gpio_banks >= 0)
> +		gpiochip_remove(&pctl->banks[gpio_banks].chip.gc);
> +
>  	return ret;
>  }
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> index ad26e4de16a85..085131caa02fe 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> @@ -14,6 +14,7 @@
>  #define __PINCTRL_SUNXI_H
>  
>  #include <linux/kernel.h>
> +#include <linux/gpio/generic.h>
>  #include <linux/spinlock.h>
>  
>  #define PA_BASE	0
> @@ -159,9 +160,17 @@ struct sunxi_pinctrl_regulator {
>  	refcount_t		refcount;
>  };
>  
> +struct sunxi_pinctrl;
> +
> +struct sunxi_gpio_bank {
> +	struct gpio_generic_chip chip;
> +	struct sunxi_pinctrl *pctl;
> +	void __iomem *base;
> +};
> +
>  struct sunxi_pinctrl {
>  	void __iomem			*membase;
> -	struct gpio_chip		*chip;
> +	struct
> sunxi_gpio_bank		banks[SUNXI_PINCTRL_MAX_BANKS];
>  	const struct sunxi_pinctrl_desc	*desc;
>  	struct device			*dev;
>  	struct sunxi_pinctrl_regulator	regulators[11];
  

On Sat, Mar 14, 2026 at 1:14 PM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
> > Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> > register frame. So far we were instantiating one GPIO chip per
> > pinctrl
> > device, which covers multiple banks of up to 32 GPIO pins per bank.
> > The
> > GPIO numbers were set to match the absolute pin numbers, even across
> > the
> > typically two instances of the pinctrl device.
> >
> > Convert the GPIO part of the sunxi pinctrl over to use the
> > gpio_generic
> > framework. This alone allows to remove some sunxi specific code,
> > which
> > is replaced with the existing generic code. This will become even
> > more
> > useful with the upcoming A733 support, which adds set and clear
> > registers for the output.
> > As a side effect this also changes the GPIO device and number
> > allocation: Each bank is now represented by its own gpio_chip, with
> > only
> > as many pins as there are actually implemented. The numbering is left
> > up
>
> Ah, is this a userspace API break?

Unfortunately, yes. This means the easily computable numbers that one can
use with the (deprecated) sysfs interface is gone, and also the pins are
now split amongst multiple gpiochip instances.

However if someone wanted the old "one gpiochip for one PIO instance with
evenly spaced banks" scheme, I suppose we could put together something
with the GPIO aggregator driver? It won't have same base pin number though.


ChenYu
  

Dne sobota, 14. marec 2026 ob 09:38:11 Srednjeevropski standardni čas je Chen-Yu Tsai napisal(a):
> On Sat, Mar 14, 2026 at 1:14 PM Icenowy Zheng <uwu@icenowy.me> wrote:
> >
> > 在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
> > > Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> > > register frame. So far we were instantiating one GPIO chip per
> > > pinctrl
> > > device, which covers multiple banks of up to 32 GPIO pins per bank.
> > > The
> > > GPIO numbers were set to match the absolute pin numbers, even across
> > > the
> > > typically two instances of the pinctrl device.
> > >
> > > Convert the GPIO part of the sunxi pinctrl over to use the
> > > gpio_generic
> > > framework. This alone allows to remove some sunxi specific code,
> > > which
> > > is replaced with the existing generic code. This will become even
> > > more
> > > useful with the upcoming A733 support, which adds set and clear
> > > registers for the output.
> > > As a side effect this also changes the GPIO device and number
> > > allocation: Each bank is now represented by its own gpio_chip, with
> > > only
> > > as many pins as there are actually implemented. The numbering is left
> > > up
> >
> > Ah, is this a userspace API break?
> 
> Unfortunately, yes. This means the easily computable numbers that one can
> use with the (deprecated) sysfs interface is gone, and also the pins are
> now split amongst multiple gpiochip instances.

I don't mind this at all for new SoCs, e.g. A733, but not really for already
supported SoCs.

> 
> However if someone wanted the old "one gpiochip for one PIO instance with
> evenly spaced banks" scheme, I suppose we could put together something
> with the GPIO aggregator driver? It won't have same base pin number though.

IIUC, this can be instantiated only via sysfs or configfs?

Best regards,
Jernej
  

On Fri, Mar 13, 2026 at 8:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> register frame. So far we were instantiating one GPIO chip per pinctrl
> device, which covers multiple banks of up to 32 GPIO pins per bank. The
> GPIO numbers were set to match the absolute pin numbers, even across the
> typically two instances of the pinctrl device.
>
> Convert the GPIO part of the sunxi pinctrl over to use the gpio_generic
> framework. This alone allows to remove some sunxi specific code, which
> is replaced with the existing generic code. This will become even more
> useful with the upcoming A733 support, which adds set and clear
> registers for the output.
> As a side effect this also changes the GPIO device and number
> allocation: Each bank is now represented by its own gpio_chip, with only
> as many pins as there are actually implemented. The numbering is left up
> to the kernel (.base = -1).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/pinctrl/sunxi/Kconfig         |   1 +
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 245 ++++++++++++--------------
>  drivers/pinctrl/sunxi/pinctrl-sunxi.h |  11 +-
>  3 files changed, 124 insertions(+), 133 deletions(-)
>
> diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
> index dc62eba96348e..5905810dbf398 100644
> --- a/drivers/pinctrl/sunxi/Kconfig
> +++ b/drivers/pinctrl/sunxi/Kconfig
> @@ -4,6 +4,7 @@ if ARCH_SUNXI
>  config PINCTRL_SUNXI
>         bool
>         select PINMUX
> +       select GPIO_GENERIC
>         select GENERIC_PINCONF
>         select GPIOLIB
>
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 48434292a39b5..4235f9feff00d 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c

[...]

>  static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>         struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
>         struct sunxi_desc_function *desc;
> -       unsigned pinnum = pctl->desc->pin_base + offset;
> -       unsigned irqnum;
> +       unsigned int pinnum, irqnum, i;
>
>         if (offset >= chip->ngpio)
>                 return -ENXIO;
>
> +       for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++)
> +               if (pctl->banks[i].chip.gc.base == chip->base)

Can't you simply compare the instance?

    if (&pctl->bankd[i].chip.gc == chip)

> +                       break;
> +       if (i == SUNXI_PINCTRL_MAX_BANKS)
> +               return -EINVAL;
> +       pinnum = pctl->desc->pin_base + i * PINS_PER_BANK + offset;
> +
>         desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum, "irq");
>         if (!desc)
>                 return -EINVAL;
> @@ -1039,18 +952,19 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>  {
>         struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>         struct sunxi_desc_function *func;
> -       int ret;
> +       int pinnum = pctl->irq_array[d->hwirq], ret;
> +       int bank = (pinnum - pctl->desc->pin_base) / PINS_PER_BANK;
>
> -       func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
> -                                       pctl->irq_array[d->hwirq], "irq");
> +       func = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum, "irq");
>         if (!func)
>                 return -EINVAL;
>
> -       ret = gpiochip_lock_as_irq(pctl->chip,
> -                       pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
> +       ret = gpiochip_lock_as_irq(&pctl->banks[bank].chip.gc,
> +                                  d->hwirq % IRQ_PER_BANK);
>         if (ret) {
>                 dev_err(pctl->dev, "unable to lock HW IRQ %lu for IRQ\n",
>                         irqd_to_hwirq(d));
> +
>                 return ret;
>         }
>
> @@ -1063,9 +977,10 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>  static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
>  {
>         struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +       int pinnum = pctl->irq_array[d->hwirq] - pctl->desc->pin_base;
> +       struct gpio_chip *gc = &pctl->banks[pinnum / PINS_PER_BANK].chip.gc;
>
> -       gpiochip_unlock_as_irq(pctl->chip,
> -                             pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
> +       gpiochip_unlock_as_irq(gc, pinnum);
>  }
>
>  static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type)
> @@ -1493,6 +1408,84 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl,
>         return 0;
>  }
>
> +static bool sunxi_of_node_instance_match(struct gpio_chip *chip, unsigned int i)
> +{
> +       struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +       if (i >= SUNXI_PINCTRL_MAX_BANKS)
> +               return false;
> +
> +       return (chip->base == pctl->banks[i].chip.gc.base);

Same here.

> +}
> +
> +static int sunxi_num_pins_of_bank(struct sunxi_pinctrl *pctl, int bank)

IMHO num_pins_in_bank would be better.

And I think having a comment above saying this returns the *actual* number
of valid pins would help.

Or just call it sunxi_num_valid_pins_in_bank?

> +{
> +       int max = -1, i;
> +
> +       for (i = 0; i < pctl->desc->npins; i++) {
> +               int pinnum = pctl->desc->pins[i].pin.number - pctl->desc->pin_base;
> +
> +               if (pinnum / PINS_PER_BANK < bank)
> +                       continue;
> +               if (pinnum / PINS_PER_BANK > bank)
> +                       break;
> +               if (pinnum % PINS_PER_BANK > max)
> +                       max = pinnum % PINS_PER_BANK;

This doesn't work for existing sun5i platforms, which have pins non-existent
on some variants, so we end up with holes in each bank.

Instead we have to actually calculate the number of valid pins.

> +       }
> +
> +       return max + 1;
> +}
> +
> +static int sunxi_gpio_add_bank(struct sunxi_pinctrl *pctl, int index)
> +{
> +       char bank_name = 'A' + index + pctl->desc->pin_base / PINS_PER_BANK;
> +       struct sunxi_gpio_bank *bank = &pctl->banks[index];
> +       struct gpio_generic_chip_config config;
> +       struct gpio_chip *gc = &bank->chip.gc;
> +       int ngpio, ret;
> +
> +       ngpio = sunxi_num_pins_of_bank(pctl, index);
> +       bank->pctl = pctl;
> +       bank->base = pctl->membase + index * pctl->bank_mem_size;
> +       if (!ngpio) {
> +               gc->owner = THIS_MODULE;
> +               gc->ngpio = 0;
> +               gc->base = -1;
> +               gc->of_gpio_n_cells = 3;
> +
> +               return 0;
> +       }
> +
> +       config = (struct gpio_generic_chip_config) {
> +               .dev = pctl->dev,
> +               .sz = 4,
> +               .dat = bank->base + DATA_REGS_OFFSET,
> +               .set = bank->base + DATA_REGS_OFFSET,
> +               .clr = NULL,
> +               .flags = GPIO_GENERIC_READ_OUTPUT_REG_SET |
> +                        GPIO_GENERIC_PINCTRL_BACKEND,
> +       };
> +
> +       ret = gpio_generic_chip_init(&bank->chip, &config);
> +       if (ret)
> +               return dev_err_probe(pctl->dev, ret,
> +                                    "failed to init generic gpio chip\n");

Generic GPIO assumes that the GPIO pin range starts from 0, and is contiguous.
This breaks down with the sun5i and sun6i families. For example, on the A31s,
there is no PC16 ~ PC23, nor PH0 ~ PH8, just to show a few.

> +       gc->owner               = THIS_MODULE;
> +       gc->label               = devm_kasprintf(pctl->dev, GFP_KERNEL,
> +                                                "%s-P%c", gc->label,
> +                                                bank_name);
> +       gc->ngpio               = ngpio;

Also set gc->offset?

> +       gc->base                = -1;
> +       gc->of_gpio_n_cells     = 3;
> +       gc->of_node_instance_match = sunxi_of_node_instance_match;
> +       gc->set_config          = gpiochip_generic_config;
> +       gc->to_irq              = sunxi_pinctrl_gpio_to_irq;
> +       gc->can_sleep           = false;
> +
> +       return gpiochip_add_data(gc, pctl);

Can we switch to devm_gpiochip_add_data() instead? It simplifies the
teardown as well.

> +}
> +
>  int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>                                   const struct sunxi_pinctrl_desc *desc,
>                                   unsigned long flags)
> @@ -1503,6 +1496,7 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>         struct sunxi_pinctrl *pctl;
>         struct pinmux_ops *pmxops;
>         int i, ret, last_pin, pin_idx;
> +       int gpio_banks;
>         struct clk *clk;
>
>         pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> @@ -1590,38 +1584,23 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>                 return PTR_ERR(pctl->pctl_dev);
>         }
>
> -       pctl->chip = devm_kzalloc(&pdev->dev, sizeof(*pctl->chip), GFP_KERNEL);
> -       if (!pctl->chip)
> -               return -ENOMEM;
> -
> -       last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number;
> -       pctl->chip->owner = THIS_MODULE;
> -       pctl->chip->request = gpiochip_generic_request;
> -       pctl->chip->free = gpiochip_generic_free;
> -       pctl->chip->set_config = gpiochip_generic_config;
> -       pctl->chip->direction_input = sunxi_pinctrl_gpio_direction_input;
> -       pctl->chip->direction_output = sunxi_pinctrl_gpio_direction_output;
> -       pctl->chip->get = sunxi_pinctrl_gpio_get;
> -       pctl->chip->set = sunxi_pinctrl_gpio_set;
> -       pctl->chip->of_xlate = sunxi_pinctrl_gpio_of_xlate;
> -       pctl->chip->to_irq = sunxi_pinctrl_gpio_to_irq;
> -       pctl->chip->of_gpio_n_cells = 3;
> -       pctl->chip->can_sleep = false;
> -       pctl->chip->ngpio = round_up(last_pin, PINS_PER_BANK) -
> -                           pctl->desc->pin_base;
> -       pctl->chip->label = dev_name(&pdev->dev);
> -       pctl->chip->parent = &pdev->dev;
> -       pctl->chip->base = pctl->desc->pin_base;
> -
> -       ret = gpiochip_add_data(pctl->chip, pctl);
> -       if (ret)
> -               return ret;
> +       last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number -
> +                  pctl->desc->pin_base;
> +       for (gpio_banks = 0;

If you switch to devm_gpiochip_add_data() above, you won't need
gpiochip_remove() below, and you can declare |gpio_banks| inline here in
the for statement.

> +            gpio_banks <= last_pin / PINS_PER_BANK;
> +            gpio_banks++) {
> +               ret = sunxi_gpio_add_bank(pctl, gpio_banks);
> +               if (ret)
> +                       goto gpiochip_error;
> +       }
>
>         for (i = 0; i < pctl->desc->npins; i++) {
>                 const struct sunxi_desc_pin *pin = pctl->desc->pins + i;
> +               int bank = (pin->pin.number - pctl->desc->pin_base) / PINS_PER_BANK;
> +               struct gpio_chip *gc = &pctl->banks[bank].chip.gc;
>
> -               ret = gpiochip_add_pin_range(pctl->chip, dev_name(&pdev->dev),
> -                                            pin->pin.number - pctl->desc->pin_base,
> +               ret = gpiochip_add_pin_range(gc, dev_name(&pdev->dev),
> +                                            pin->pin.number % PINS_PER_BANK,
>                                              pin->pin.number, 1);
>                 if (ret)
>                         goto gpiochip_error;
> @@ -1690,6 +1669,8 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>         return 0;
>
>  gpiochip_error:
> -       gpiochip_remove(pctl->chip);
> +       while (--gpio_banks >= 0)
> +               gpiochip_remove(&pctl->banks[gpio_banks].chip.gc);
> +
>         return ret;
>  }
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> index ad26e4de16a85..085131caa02fe 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> @@ -14,6 +14,7 @@
>  #define __PINCTRL_SUNXI_H
>
>  #include <linux/kernel.h>
> +#include <linux/gpio/generic.h>

gpio comes before kernel?

And maybe we should try to stop including the massive kernel.h header.


Thanks
ChenYu

>  #include <linux/spinlock.h>
>
>  #define PA_BASE        0
> @@ -159,9 +160,17 @@ struct sunxi_pinctrl_regulator {
>         refcount_t              refcount;
>  };
>
> +struct sunxi_pinctrl;
> +
> +struct sunxi_gpio_bank {
> +       struct gpio_generic_chip chip;
> +       struct sunxi_pinctrl *pctl;
> +       void __iomem *base;
> +};
> +
>  struct sunxi_pinctrl {
>         void __iomem                    *membase;
> -       struct gpio_chip                *chip;
> +       struct sunxi_gpio_bank          banks[SUNXI_PINCTRL_MAX_BANKS];
>         const struct sunxi_pinctrl_desc *desc;
>         struct device                   *dev;
>         struct sunxi_pinctrl_regulator  regulators[11];
> --
> 2.46.4
>
>
  

On Sat, 14 Mar 2026 13:14:25 +0800
Icenowy Zheng <uwu@icenowy.me> wrote:

> 在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
> > Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> > register frame. So far we were instantiating one GPIO chip per
> > pinctrl
> > device, which covers multiple banks of up to 32 GPIO pins per bank.
> > The
> > GPIO numbers were set to match the absolute pin numbers, even across
> > the
> > typically two instances of the pinctrl device.
> > 
> > Convert the GPIO part of the sunxi pinctrl over to use the
> > gpio_generic
> > framework. This alone allows to remove some sunxi specific code,
> > which
> > is replaced with the existing generic code. This will become even
> > more
> > useful with the upcoming A733 support, which adds set and clear
> > registers for the output.
> > As a side effect this also changes the GPIO device and number
> > allocation: Each bank is now represented by its own gpio_chip, with
> > only
> > as many pins as there are actually implemented. The numbering is left
> > up  
> 
> Ah, is this a userspace API break?

Was that ever a guaranteed user space API? Or just something
that everyone relied on because it was always the same (until it
wasn't)? Similar to /dev/mmcblk0 being the SD card?
And ignoring the ill-fated old-style sysfs interface for now, how does
this work with libgpiod? Would it still use the absolute pin numbers?

I mean looking at that warning about the forced GPIO numbering we
get, using base = -1 seems to be the recommended way?

Cheers,
Andre

> Sincerely,
> Icenowy
> 
> > to the kernel (.base = -1).
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/pinctrl/sunxi/Kconfig         |   1 +
> >  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 245 ++++++++++++------------
> > --
> >  drivers/pinctrl/sunxi/pinctrl-sunxi.h |  11 +-
> >  3 files changed, 124 insertions(+), 133 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/sunxi/Kconfig
> > b/drivers/pinctrl/sunxi/Kconfig
> > index dc62eba96348e..5905810dbf398 100644
> > --- a/drivers/pinctrl/sunxi/Kconfig
> > +++ b/drivers/pinctrl/sunxi/Kconfig
> > @@ -4,6 +4,7 @@ if ARCH_SUNXI
> >  config PINCTRL_SUNXI
> >  	bool
> >  	select PINMUX
> > +	select GPIO_GENERIC
> >  	select GENERIC_PINCONF
> >  	select GPIOLIB
> >  
> > diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > index 48434292a39b5..4235f9feff00d 100644
> > --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/export.h>
> >  #include <linux/gpio/driver.h>
> > +#include <linux/gpio/generic.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/irqchip/chained_irq.h>
> > @@ -86,17 +87,6 @@ static void sunxi_mux_reg(const struct
> > sunxi_pinctrl *pctl,
> >  	*mask  = (BIT(MUX_FIELD_WIDTH) - 1) << *shift;
> >  }
> >  
> > -static void sunxi_data_reg(const struct sunxi_pinctrl *pctl,
> > -			   u32 pin, u32 *reg, u32 *shift, u32 *mask)
> > -{
> > -	u32 offset = pin % PINS_PER_BANK * DATA_FIELD_WIDTH;
> > -
> > -	*reg   = sunxi_bank_offset(pctl, pin) + DATA_REGS_OFFSET +
> > -		 offset / BITS_PER_TYPE(u32) * sizeof(u32);
> > -	*shift = offset % BITS_PER_TYPE(u32);
> > -	*mask  = (BIT(DATA_FIELD_WIDTH) - 1) << *shift;
> > -}
> > -
> >  static void sunxi_dlevel_reg(const struct sunxi_pinctrl *pctl,
> >  			     u32 pin, u32 *reg, u32 *shift, u32
> > *mask)
> >  {
> > @@ -930,99 +920,22 @@ static const struct pinmux_ops sunxi_pmx_ops =
> > {
> >  	.strict			= true,
> >  };
> >  
> > -static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip
> > *chip,
> > -					unsigned offset)
> > -{
> > -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> > -
> > -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> > -					    chip->base + offset,
> > true);
> > -}
> > -
> > -static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned
> > offset)
> > -{
> > -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> > -	bool set_mux = pctl->desc->irq_read_needs_mux &&
> > -		gpiochip_line_is_irq(chip, offset);
> > -	u32 pin = offset + chip->base;
> > -	u32 reg, shift, mask, val;
> > -
> > -	sunxi_data_reg(pctl, offset, &reg, &shift, &mask);
> > -
> > -	if (set_mux)
> > -		sunxi_pmx_set(pctl->pctl_dev, pin,
> > SUN4I_FUNC_INPUT);
> > -
> > -	val = (readl(pctl->membase + reg) & mask) >> shift;
> > -
> > -	if (set_mux)
> > -		sunxi_pmx_set(pctl->pctl_dev, pin, SUN4I_FUNC_IRQ);
> > -
> > -	return val;
> > -}
> > -
> > -static int sunxi_pinctrl_gpio_set(struct gpio_chip *chip, unsigned
> > int offset,
> > -				  int value)
> > -{
> > -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> > -	u32 reg, shift, mask, val;
> > -	unsigned long flags;
> > -
> > -	sunxi_data_reg(pctl, offset, &reg, &shift, &mask);
> > -
> > -	raw_spin_lock_irqsave(&pctl->lock, flags);
> > -
> > -	val = readl(pctl->membase + reg);
> > -
> > -	if (value)
> > -		val |= mask;
> > -	else
> > -		val &= ~mask;
> > -
> > -	writel(val, pctl->membase + reg);
> > -
> > -	raw_spin_unlock_irqrestore(&pctl->lock, flags);
> > -
> > -	return 0;
> > -}
> > -
> > -static int sunxi_pinctrl_gpio_direction_output(struct gpio_chip
> > *chip,
> > -					unsigned offset, int value)
> > -{
> > -	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> > -
> > -	sunxi_pinctrl_gpio_set(chip, offset, value);
> > -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> > -					    chip->base + offset,
> > false);
> > -}
> > -
> > -static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,
> > -				const struct of_phandle_args
> > *gpiospec,
> > -				u32 *flags)
> > -{
> > -	int pin, base;
> > -
> > -	base = PINS_PER_BANK * gpiospec->args[0];
> > -	pin = base + gpiospec->args[1];
> > -
> > -	if (pin > gc->ngpio)
> > -		return -EINVAL;
> > -
> > -	if (flags)
> > -		*flags = gpiospec->args[2];
> > -
> > -	return pin;
> > -}
> > -
> >  static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip,
> > unsigned offset)
> >  {
> >  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> >  	struct sunxi_desc_function *desc;
> > -	unsigned pinnum = pctl->desc->pin_base + offset;
> > -	unsigned irqnum;
> > +	unsigned int pinnum, irqnum, i;
> >  
> >  	if (offset >= chip->ngpio)
> >  		return -ENXIO;
> >  
> > +	for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++)
> > +		if (pctl->banks[i].chip.gc.base == chip->base)
> > +			break;
> > +	if (i == SUNXI_PINCTRL_MAX_BANKS)
> > +		return -EINVAL;
> > +	pinnum = pctl->desc->pin_base + i * PINS_PER_BANK + offset;
> > +
> >  	desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum,
> > "irq");
> >  	if (!desc)
> >  		return -EINVAL;
> > @@ -1039,18 +952,19 @@ static int
> > sunxi_pinctrl_irq_request_resources(struct irq_data *d)
> >  {
> >  	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> >  	struct sunxi_desc_function *func;
> > -	int ret;
> > +	int pinnum = pctl->irq_array[d->hwirq], ret;
> > +	int bank = (pinnum - pctl->desc->pin_base) / PINS_PER_BANK;
> >  
> > -	func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
> > -					pctl->irq_array[d->hwirq],
> > "irq");
> > +	func = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum,
> > "irq");
> >  	if (!func)
> >  		return -EINVAL;
> >  
> > -	ret = gpiochip_lock_as_irq(pctl->chip,
> > -			pctl->irq_array[d->hwirq] - pctl->desc-  
> > >pin_base);  
> > +	ret = gpiochip_lock_as_irq(&pctl->banks[bank].chip.gc,
> > +				   d->hwirq % IRQ_PER_BANK);
> >  	if (ret) {
> >  		dev_err(pctl->dev, "unable to lock HW IRQ %lu for
> > IRQ\n",
> >  			irqd_to_hwirq(d));
> > +
> >  		return ret;
> >  	}
> >  
> > @@ -1063,9 +977,10 @@ static int
> > sunxi_pinctrl_irq_request_resources(struct irq_data *d)
> >  static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
> >  {
> >  	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> > +	int pinnum = pctl->irq_array[d->hwirq] - pctl->desc-  
> > >pin_base;  
> > +	struct gpio_chip *gc = &pctl->banks[pinnum /
> > PINS_PER_BANK].chip.gc;
> >  
> > -	gpiochip_unlock_as_irq(pctl->chip,
> > -			      pctl->irq_array[d->hwirq] - pctl-  
> > >desc->pin_base);  
> > +	gpiochip_unlock_as_irq(gc, pinnum);
> >  }
> >  
> >  static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned
> > int type)
> > @@ -1493,6 +1408,84 @@ static int sunxi_pinctrl_setup_debounce(struct
> > sunxi_pinctrl *pctl,
> >  	return 0;
> >  }
> >  
> > +static bool sunxi_of_node_instance_match(struct gpio_chip *chip,
> > unsigned int i)
> > +{
> > +	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	if (i >= SUNXI_PINCTRL_MAX_BANKS)
> > +		return false;
> > +
> > +	return (chip->base == pctl->banks[i].chip.gc.base);
> > +}
> > +
> > +static int sunxi_num_pins_of_bank(struct sunxi_pinctrl *pctl, int
> > bank)
> > +{
> > +	int max = -1, i;
> > +
> > +	for (i = 0; i < pctl->desc->npins; i++) {
> > +		int pinnum = pctl->desc->pins[i].pin.number - pctl-  
> > >desc->pin_base;  
> > +
> > +		if (pinnum / PINS_PER_BANK < bank)
> > +			continue;
> > +		if (pinnum / PINS_PER_BANK > bank)
> > +			break;
> > +		if (pinnum % PINS_PER_BANK > max)
> > +			max = pinnum % PINS_PER_BANK;
> > +	}
> > +
> > +	return max + 1;
> > +}
> > +
> > +static int sunxi_gpio_add_bank(struct sunxi_pinctrl *pctl, int
> > index)
> > +{
> > +	char bank_name = 'A' + index + pctl->desc->pin_base /
> > PINS_PER_BANK;
> > +	struct sunxi_gpio_bank *bank = &pctl->banks[index];
> > +	struct gpio_generic_chip_config config;
> > +	struct gpio_chip *gc = &bank->chip.gc;
> > +	int ngpio, ret;
> > +
> > +	ngpio = sunxi_num_pins_of_bank(pctl, index);
> > +	bank->pctl = pctl;
> > +	bank->base = pctl->membase + index * pctl->bank_mem_size;
> > +	if (!ngpio) {
> > +		gc->owner = THIS_MODULE;
> > +		gc->ngpio = 0;
> > +		gc->base = -1;
> > +		gc->of_gpio_n_cells = 3;
> > +
> > +		return 0;
> > +	}
> > +
> > +	config = (struct gpio_generic_chip_config) {
> > +		.dev = pctl->dev,
> > +		.sz = 4,
> > +		.dat = bank->base + DATA_REGS_OFFSET,
> > +		.set = bank->base + DATA_REGS_OFFSET,
> > +		.clr = NULL,
> > +		.flags = GPIO_GENERIC_READ_OUTPUT_REG_SET |
> > +			 GPIO_GENERIC_PINCTRL_BACKEND,
> > +	};
> > +
> > +	ret = gpio_generic_chip_init(&bank->chip, &config);
> > +	if (ret)
> > +		return dev_err_probe(pctl->dev, ret,
> > +				     "failed to init generic gpio
> > chip\n");
> > +
> > +	gc->owner		= THIS_MODULE;
> > +	gc->label		= devm_kasprintf(pctl->dev,
> > GFP_KERNEL,
> > +						 "%s-P%c", gc-  
> > >label,  
> > +						 bank_name);
> > +	gc->ngpio		= ngpio;
> > +	gc->base		= -1;
> > +	gc->of_gpio_n_cells	= 3;
> > +	gc->of_node_instance_match = sunxi_of_node_instance_match;
> > +	gc->set_config		= gpiochip_generic_config;
> > +	gc->to_irq		= sunxi_pinctrl_gpio_to_irq;
> > +	gc->can_sleep		= false;
> > +
> > +	return gpiochip_add_data(gc, pctl);
> > +}
> > +
> >  int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
> >  				  const struct sunxi_pinctrl_desc
> > *desc,
> >  				  unsigned long flags)
> > @@ -1503,6 +1496,7 @@ int sunxi_pinctrl_init_with_flags(struct
> > platform_device *pdev,
> >  	struct sunxi_pinctrl *pctl;
> >  	struct pinmux_ops *pmxops;
> >  	int i, ret, last_pin, pin_idx;
> > +	int gpio_banks;
> >  	struct clk *clk;
> >  
> >  	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> > @@ -1590,38 +1584,23 @@ int sunxi_pinctrl_init_with_flags(struct
> > platform_device *pdev,
> >  		return PTR_ERR(pctl->pctl_dev);
> >  	}
> >  
> > -	pctl->chip = devm_kzalloc(&pdev->dev, sizeof(*pctl->chip),
> > GFP_KERNEL);
> > -	if (!pctl->chip)
> > -		return -ENOMEM;
> > -
> > -	last_pin = pctl->desc->pins[pctl->desc->npins -
> > 1].pin.number;
> > -	pctl->chip->owner = THIS_MODULE;
> > -	pctl->chip->request = gpiochip_generic_request;
> > -	pctl->chip->free = gpiochip_generic_free;
> > -	pctl->chip->set_config = gpiochip_generic_config;
> > -	pctl->chip->direction_input =
> > sunxi_pinctrl_gpio_direction_input;
> > -	pctl->chip->direction_output =
> > sunxi_pinctrl_gpio_direction_output;
> > -	pctl->chip->get = sunxi_pinctrl_gpio_get;
> > -	pctl->chip->set = sunxi_pinctrl_gpio_set;
> > -	pctl->chip->of_xlate = sunxi_pinctrl_gpio_of_xlate;
> > -	pctl->chip->to_irq = sunxi_pinctrl_gpio_to_irq;
> > -	pctl->chip->of_gpio_n_cells = 3;
> > -	pctl->chip->can_sleep = false;
> > -	pctl->chip->ngpio = round_up(last_pin, PINS_PER_BANK) -
> > -			    pctl->desc->pin_base;
> > -	pctl->chip->label = dev_name(&pdev->dev);
> > -	pctl->chip->parent = &pdev->dev;
> > -	pctl->chip->base = pctl->desc->pin_base;
> > -
> > -	ret = gpiochip_add_data(pctl->chip, pctl);
> > -	if (ret)
> > -		return ret;
> > +	last_pin = pctl->desc->pins[pctl->desc->npins -
> > 1].pin.number -
> > +		   pctl->desc->pin_base;
> > +	for (gpio_banks = 0;
> > +	     gpio_banks <= last_pin / PINS_PER_BANK;
> > +	     gpio_banks++) {
> > +		ret = sunxi_gpio_add_bank(pctl, gpio_banks);
> > +		if (ret)
> > +			goto gpiochip_error;
> > +	}
> >  
> >  	for (i = 0; i < pctl->desc->npins; i++) {
> >  		const struct sunxi_desc_pin *pin = pctl->desc->pins
> > + i;
> > +		int bank = (pin->pin.number - pctl->desc->pin_base)
> > / PINS_PER_BANK;
> > +		struct gpio_chip *gc = &pctl->banks[bank].chip.gc;
> >  
> > -		ret = gpiochip_add_pin_range(pctl->chip,
> > dev_name(&pdev->dev),
> > -					     pin->pin.number - pctl-  
> > >desc->pin_base,  
> > +		ret = gpiochip_add_pin_range(gc, dev_name(&pdev-  
> > >dev),  
> > +					     pin->pin.number %
> > PINS_PER_BANK,
> >  					     pin->pin.number, 1);
> >  		if (ret)
> >  			goto gpiochip_error;
> > @@ -1690,6 +1669,8 @@ int sunxi_pinctrl_init_with_flags(struct
> > platform_device *pdev,
> >  	return 0;
> >  
> >  gpiochip_error:
> > -	gpiochip_remove(pctl->chip);
> > +	while (--gpio_banks >= 0)
> > +		gpiochip_remove(&pctl->banks[gpio_banks].chip.gc);
> > +
> >  	return ret;
> >  }
> > diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> > b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> > index ad26e4de16a85..085131caa02fe 100644
> > --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> > +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
> > @@ -14,6 +14,7 @@
> >  #define __PINCTRL_SUNXI_H
> >  
> >  #include <linux/kernel.h>
> > +#include <linux/gpio/generic.h>
> >  #include <linux/spinlock.h>
> >  
> >  #define PA_BASE	0
> > @@ -159,9 +160,17 @@ struct sunxi_pinctrl_regulator {
> >  	refcount_t		refcount;
> >  };
> >  
> > +struct sunxi_pinctrl;
> > +
> > +struct sunxi_gpio_bank {
> > +	struct gpio_generic_chip chip;
> > +	struct sunxi_pinctrl *pctl;
> > +	void __iomem *base;
> > +};
> > +
> >  struct sunxi_pinctrl {
> >  	void __iomem			*membase;
> > -	struct gpio_chip		*chip;
> > +	struct
> > sunxi_gpio_bank		banks[SUNXI_PINCTRL_MAX_BANKS];
> >  	const struct sunxi_pinctrl_desc	*desc;
> >  	struct device			*dev;
> >  	struct sunxi_pinctrl_regulator	regulators[11];  
>
  

On Sun, 15 Mar 2026 01:42:05 +0100, Andre Przywara
<andre.przywara@arm.com> said:
> On Sat, 14 Mar 2026 13:14:25 +0800
> Icenowy Zheng <uwu@icenowy.me> wrote:
>
>> 在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
>> > Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
>> > register frame. So far we were instantiating one GPIO chip per
>> > pinctrl
>> > device, which covers multiple banks of up to 32 GPIO pins per bank.
>> > The
>> > GPIO numbers were set to match the absolute pin numbers, even across
>> > the
>> > typically two instances of the pinctrl device.
>> >
>> > Convert the GPIO part of the sunxi pinctrl over to use the
>> > gpio_generic
>> > framework. This alone allows to remove some sunxi specific code,
>> > which
>> > is replaced with the existing generic code. This will become even
>> > more
>> > useful with the upcoming A733 support, which adds set and clear
>> > registers for the output.
>> > As a side effect this also changes the GPIO device and number
>> > allocation: Each bank is now represented by its own gpio_chip, with
>> > only
>> > as many pins as there are actually implemented. The numbering is left
>> > up
>>
>> Ah, is this a userspace API break?
>
> Was that ever a guaranteed user space API? Or just something
> that everyone relied on because it was always the same (until it
> wasn't)? Similar to /dev/mmcblk0 being the SD card?
> And ignoring the ill-fated old-style sysfs interface for now, how does
> this work with libgpiod? Would it still use the absolute pin numbers?
>
> I mean looking at that warning about the forced GPIO numbering we
> get, using base = -1 seems to be the recommended way?
>

My view is this: for libgpiod, there are no absolute GPIO numbers and the
ordering of GPIO chip character devices is not guaranteed.

For sysfs: userspace may rightfully expect the global numbering to stay the
same. I don't like it but this is the kernel policy.

Here however, there's another thing: the fact that a single GPIO chips is now
split into several. I don't mind it but if someone complains, it would have
to be reverted.

Bartosz
  

On Fri, Mar 13, 2026 at 1:08 AM Andre Przywara <andre.przywara@arm.com> wrote:

> Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
> register frame. So far we were instantiating one GPIO chip per pinctrl
> device, which covers multiple banks of up to 32 GPIO pins per bank. The
> GPIO numbers were set to match the absolute pin numbers, even across the
> typically two instances of the pinctrl device.
>
> Convert the GPIO part of the sunxi pinctrl over to use the gpio_generic
> framework. This alone allows to remove some sunxi specific code, which
> is replaced with the existing generic code. This will become even more
> useful with the upcoming A733 support, which adds set and clear
> registers for the output.
> As a side effect this also changes the GPIO device and number
> allocation: Each bank is now represented by its own gpio_chip, with only
> as many pins as there are actually implemented. The numbering is left up
> to the kernel (.base = -1).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

You could add that this makes the driver benefit of the
.get/set_multiple() accelerated implementations of the generic
GPIO MMIO driver as well.

> +static int sunxi_gpio_add_bank(struct sunxi_pinctrl *pctl, int index)
> +{
> +       char bank_name = 'A' + index + pctl->desc->pin_base / PINS_PER_BANK;
> +       struct sunxi_gpio_bank *bank = &pctl->banks[index];
> +       struct gpio_generic_chip_config config;
> +       struct gpio_chip *gc = &bank->chip.gc;
> +       int ngpio, ret;
> +
> +       ngpio = sunxi_num_pins_of_bank(pctl, index);
> +       bank->pctl = pctl;
> +       bank->base = pctl->membase + index * pctl->bank_mem_size;
> +       if (!ngpio) {
> +               gc->owner = THIS_MODULE;
> +               gc->ngpio = 0;
> +               gc->base = -1;
> +               gc->of_gpio_n_cells = 3;
> +
> +               return 0;
> +       }

Add some comment about what is going on here, I suspect you
are flagging the gpio_chip as unused, you could just not assign
anything I think?

> +       config = (struct gpio_generic_chip_config) {
> +               .dev = pctl->dev,
> +               .sz = 4,
> +               .dat = bank->base + DATA_REGS_OFFSET,
> +               .set = bank->base + DATA_REGS_OFFSET,
> +               .clr = NULL,
> +               .flags = GPIO_GENERIC_READ_OUTPUT_REG_SET |
> +                        GPIO_GENERIC_PINCTRL_BACKEND,
> +       };
> +
> +       ret = gpio_generic_chip_init(&bank->chip, &config);
> +       if (ret)
> +               return dev_err_probe(pctl->dev, ret,
> +                                    "failed to init generic gpio chip\n");
> +
> +       gc->owner               = THIS_MODULE;
> +       gc->label               = devm_kasprintf(pctl->dev, GFP_KERNEL,
> +                                                "%s-P%c", gc->label,
> +                                                bank_name);
> +       gc->ngpio               = ngpio;
> +       gc->base                = -1;
> +       gc->of_gpio_n_cells     = 3;

Nice use of the three-cell GPIO OF parser!

Overall this is a nice change and makes the kernel a
better place. I would apply a non-RFC version as soon
as we can agree on the path forward.

Yours,
Linus Walleij
  

On Sun, Mar 15, 2026 at 1:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> On Sat, 14 Mar 2026 13:14:25 +0800
> Icenowy Zheng <uwu@icenowy.me> wrote:

> > Ah, is this a userspace API break?
>
> Was that ever a guaranteed user space API? Or just something
> that everyone relied on because it was always the same (until it
> wasn't)? Similar to /dev/mmcblk0 being the SD card?

As Torvalds has pointed out, if it has users (that complain) it's an ABI.

However if a tree falls in the forest and no-one is there to hear it
fall, it is a philosophical question whether it makes a sound or not.
https://en.wikipedia.org/wiki/If_a_tree_falls_in_a_forest_and_no_one_is_around_to_hear_it,_does_it_make_a_sound%3F

The ABI has been broken since the first introduction of dynamic
GPIO devices, because anything specifying -1 as base will
get some random semi-predictable global GPIO numbers.

> And ignoring the ill-fated old-style sysfs interface for now, how does
> this work with libgpiod? Would it still use the absolute pin numbers?

No.

There is also a new sysfs that enables you to export chip-local
lines, and this is what you get with CONFIG_GPIO_SYSFS.
The old ABI is available behind CONFIG_GPIO_SYSFS_LEGACY
and if the users don't enable this, then they don't have a
problem with this patch.

> I mean looking at that warning about the forced GPIO numbering we
> get, using base = -1 seems to be the recommended way?

It is.

Unless there are users that love the old ABI (for Sunxi, I *doubt it*)
I think the patch should be applied.

Yours,
Linus Walleij
  

Hi Bartosz, Linus,

so I wanted to come back to this, posting a A733 v2, with this patch 
included, and was wondering what the consensus on this matter would be?

...

On 3/16/26 09:57, Bartosz Golaszewski wrote:
> On Sun, 15 Mar 2026 01:42:05 +0100, Andre Przywara
> <andre.przywara@arm.com> said:
>> On Sat, 14 Mar 2026 13:14:25 +0800
>> Icenowy Zheng <uwu@icenowy.me> wrote:
>>
>>> 在 2026-03-13五的 01:06 +0100，Andre Przywara写道：
>>>> Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
>>>> register frame. So far we were instantiating one GPIO chip per
>>>> pinctrl
>>>> device, which covers multiple banks of up to 32 GPIO pins per bank.
>>>> The
>>>> GPIO numbers were set to match the absolute pin numbers, even across
>>>> the
>>>> typically two instances of the pinctrl device.
>>>>
>>>> Convert the GPIO part of the sunxi pinctrl over to use the
>>>> gpio_generic
>>>> framework. This alone allows to remove some sunxi specific code,
>>>> which
>>>> is replaced with the existing generic code. This will become even
>>>> more
>>>> useful with the upcoming A733 support, which adds set and clear
>>>> registers for the output.
>>>> As a side effect this also changes the GPIO device and number
>>>> allocation: Each bank is now represented by its own gpio_chip, with
>>>> only
>>>> as many pins as there are actually implemented. The numbering is left
>>>> up
>>>
>>> Ah, is this a userspace API break?

So to summarise: yes, the user visible number in /sys/class/gpio would 
change. For a start, there would be up to a dozen gpiochip's now, 
instead of the just two before. But at the moment they are somewhat 
inconsistently named: the current labels are just <addr>.pinctrl, you 
would need to know that 300b000 is the primary controller. This is of 
course SoC specific, and DOES differ between generations of Allwinner 
chips. With this patch we get the bank number appended to the label, so 
it becomes something like: 300b000.pinctrl-PD, which is grep-able even.

As for users being affected by this change: that's hard to say, there 
seem to be a lot of silent users there (not being visible in mainline 
discussions), also a lot of people using these boards in a very embedded 
way. My hunch is that still quite some people use the sysfs interface, 
because it's so convenient to use from the shell, and many compile their 
own kernels. At the same time I am sure those people could adapt to any 
new scheme.

>> Was that ever a guaranteed user space API? Or just something
>> that everyone relied on because it was always the same (until it
>> wasn't)? Similar to /dev/mmcblk0 being the SD card?


>> And ignoring the ill-fated old-style sysfs interface for now, how does
>> this work with libgpiod? Would it still use the absolute pin numbers?
>>
>> I mean looking at that warning about the forced GPIO numbering we
>> get, using base = -1 seems to be the recommended way?
>>
> 
> My view is this: for libgpiod, there are no absolute GPIO numbers and the
> ordering of GPIO chip character devices is not guaranteed.
> 
> For sysfs: userspace may rightfully expect the global numbering to stay the
> same. I don't like it but this is the kernel policy.

Does it matter anyhow that this interface is deprecated, and even hidden 
behind CONFIG_EXPERT?

> Here however, there's another thing: the fact that a single GPIO chips is now
> split into several. I don't mind it but if someone complains, it would have
> to be reverted.

Who would care, and are there other effects outside of the sysfs 
numbering? I mean we always had two GPIO chips before (because of two 
instances of pinctrl IP in most Allwinner SoCs), one with one or two 
banks, one with up to 11. Now it's at least consistently one bank per 
chip, without gaps.

So personally I am leaning towards doing the change: most users can 
typically adapt - or they don't care because they don't update their 
kernels (beyond stable revisions). And to address the naming 
ambiguities, we could push for more gpio-line-names. That would make 
finding a GPIO much safer and easier, and works either way.

But if this patch's ABI breakage is a showstopper, I would need to know, 
so that I can rebase the A733 pinctrl patches without this one being the 
first patch in the series.

Cheers,
Andre
  

On Fri, Jul 3, 2026 at 3:53 PM Andre Przywara <andre.przywara@arm.com> wrote:

> Hi Bartosz, Linus,
>
> so I wanted to come back to this, posting a A733 v2, with this patch
> included, and was wondering what the consensus on this matter would be?
(...)
> So personally I am leaning towards doing the change: most users can
> typically adapt - or they don't care because they don't update their
> kernels (beyond stable revisions). And to address the naming
> ambiguities, we could push for more gpio-line-names. That would make
> finding a GPIO much safer and easier, and works either way.

I agree.

> But if this patch's ABI breakage is a showstopper, I would need to know,
> so that I can rebase the A733 pinctrl patches without this one being the
> first patch in the series.

I am *FOR* this change.

Wikipedia has some policy like "avoid copyright paranoia", and
some days I feel we need an "avoid ABI paranoia" policy.
Rough consensus and running code.

Yours,
Linus Walleij
  

On Fri, 3 Jul 2026 23:25:16 +0200, Linus Walleij <linusw@kernel.org> said:
> On Fri, Jul 3, 2026 at 3:53 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
>> Hi Bartosz, Linus,
>>
>> so I wanted to come back to this, posting a A733 v2, with this patch
>> included, and was wondering what the consensus on this matter would be?
> (...)
>> So personally I am leaning towards doing the change: most users can
>> typically adapt - or they don't care because they don't update their
>> kernels (beyond stable revisions). And to address the naming
>> ambiguities, we could push for more gpio-line-names. That would make
>> finding a GPIO much safer and easier, and works either way.
>
> I agree.
>
>> But if this patch's ABI breakage is a showstopper, I would need to know,
>> so that I can rebase the A733 pinctrl patches without this one being the
>> first patch in the series.
>
> I am *FOR* this change.
>
> Wikipedia has some policy like "avoid copyright paranoia", and
> some days I feel we need an "avoid ABI paranoia" policy.
> Rough consensus and running code.
>

Agreed. Let's do this.

Bart
  

Hi Chen-Yu,

many thanks for the thorough review, finally found some time to reply:

On 3/14/26 10:59, Chen-Yu Tsai wrote:
> On Fri, Mar 13, 2026 at 8:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Allwinner SoCs combine pinmuxing and GPIO control in one device/MMIO
>> register frame. So far we were instantiating one GPIO chip per pinctrl
>> device, which covers multiple banks of up to 32 GPIO pins per bank. The
>> GPIO numbers were set to match the absolute pin numbers, even across the
>> typically two instances of the pinctrl device.
>>
>> Convert the GPIO part of the sunxi pinctrl over to use the gpio_generic
>> framework. This alone allows to remove some sunxi specific code, which
>> is replaced with the existing generic code. This will become even more
>> useful with the upcoming A733 support, which adds set and clear
>> registers for the output.
>> As a side effect this also changes the GPIO device and number
>> allocation: Each bank is now represented by its own gpio_chip, with only
>> as many pins as there are actually implemented. The numbering is left up
>> to the kernel (.base = -1).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/pinctrl/sunxi/Kconfig         |   1 +
>>   drivers/pinctrl/sunxi/pinctrl-sunxi.c | 245 ++++++++++++--------------
>>   drivers/pinctrl/sunxi/pinctrl-sunxi.h |  11 +-
>>   3 files changed, 124 insertions(+), 133 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
>> index dc62eba96348e..5905810dbf398 100644
>> --- a/drivers/pinctrl/sunxi/Kconfig
>> +++ b/drivers/pinctrl/sunxi/Kconfig
>> @@ -4,6 +4,7 @@ if ARCH_SUNXI
>>   config PINCTRL_SUNXI
>>          bool
>>          select PINMUX
>> +       select GPIO_GENERIC
>>          select GENERIC_PINCONF
>>          select GPIOLIB
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index 48434292a39b5..4235f9feff00d 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> 
> [...]
> 
>>   static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>   {
>>          struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
>>          struct sunxi_desc_function *desc;
>> -       unsigned pinnum = pctl->desc->pin_base + offset;
>> -       unsigned irqnum;
>> +       unsigned int pinnum, irqnum, i;
>>
>>          if (offset >= chip->ngpio)
>>                  return -ENXIO;
>>
>> +       for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++)
>> +               if (pctl->banks[i].chip.gc.base == chip->base)
> 
> Can't you simply compare the instance?
> 
>      if (&pctl->bankd[i].chip.gc == chip)

Yes, I could, by I think comparing pointers is somewhat frowned upon, 
and since base is considered a unique identifiers within all GPIO chips, 
I found it cleaner to compare the base.
Is comparing pointers commonly accepts in the kernel? Theoretically 
there could be multiple VAs pointing to the same object.

> 
>> +                       break;
>> +       if (i == SUNXI_PINCTRL_MAX_BANKS)
>> +               return -EINVAL;
>> +       pinnum = pctl->desc->pin_base + i * PINS_PER_BANK + offset;
>> +
>>          desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum, "irq");
>>          if (!desc)
>>                  return -EINVAL;
>> @@ -1039,18 +952,19 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>>   {
>>          struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>>          struct sunxi_desc_function *func;
>> -       int ret;
>> +       int pinnum = pctl->irq_array[d->hwirq], ret;
>> +       int bank = (pinnum - pctl->desc->pin_base) / PINS_PER_BANK;
>>
>> -       func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
>> -                                       pctl->irq_array[d->hwirq], "irq");
>> +       func = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum, "irq");
>>          if (!func)
>>                  return -EINVAL;
>>
>> -       ret = gpiochip_lock_as_irq(pctl->chip,
>> -                       pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
>> +       ret = gpiochip_lock_as_irq(&pctl->banks[bank].chip.gc,
>> +                                  d->hwirq % IRQ_PER_BANK);
>>          if (ret) {
>>                  dev_err(pctl->dev, "unable to lock HW IRQ %lu for IRQ\n",
>>                          irqd_to_hwirq(d));
>> +
>>                  return ret;
>>          }
>>
>> @@ -1063,9 +977,10 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
>>   static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
>>   {
>>          struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>> +       int pinnum = pctl->irq_array[d->hwirq] - pctl->desc->pin_base;
>> +       struct gpio_chip *gc = &pctl->banks[pinnum / PINS_PER_BANK].chip.gc;
>>
>> -       gpiochip_unlock_as_irq(pctl->chip,
>> -                             pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
>> +       gpiochip_unlock_as_irq(gc, pinnum);
>>   }
>>
>>   static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type)
>> @@ -1493,6 +1408,84 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl,
>>          return 0;
>>   }
>>
>> +static bool sunxi_of_node_instance_match(struct gpio_chip *chip, unsigned int i)
>> +{
>> +       struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
>> +
>> +       if (i >= SUNXI_PINCTRL_MAX_BANKS)
>> +               return false;
>> +
>> +       return (chip->base == pctl->banks[i].chip.gc.base);
> 
> Same here.
> 
>> +}
>> +
>> +static int sunxi_num_pins_of_bank(struct sunxi_pinctrl *pctl, int bank)
> 
> IMHO num_pins_in_bank would be better.

Ah yes, I agree.

> 
> And I think having a comment above saying this returns the *actual* number
> of valid pins would help.

That looks indeed useful, added that.

> Or just call it sunxi_num_valid_pins_in_bank?

Mmh, maybe not, see below ...

> 
>> +{
>> +       int max = -1, i;
>> +
>> +       for (i = 0; i < pctl->desc->npins; i++) {
>> +               int pinnum = pctl->desc->pins[i].pin.number - pctl->desc->pin_base;
>> +
>> +               if (pinnum / PINS_PER_BANK < bank)
>> +                       continue;
>> +               if (pinnum / PINS_PER_BANK > bank)
>> +                       break;
>> +               if (pinnum % PINS_PER_BANK > max)
>> +                       max = pinnum % PINS_PER_BANK;
> 
> This doesn't work for existing sun5i platforms, which have pins non-existent
> on some variants, so we end up with holes in each bank.
> 
> Instead we have to actually calculate the number of valid pins.

Well, I think the function name promises too much, what we really need 
is "the maximum pin index used + 1", to allocate an array holding pins, 
indexed by their number. IIUC the GPIO code doesn't really support holes 
in each gpiochip. And it doesn't really need to, we just have 
potentially a few smaller gaps, compared to the bigger number of larger 
gaps we have today. They wouldn't be referenced at all, so I don't think 
it's a problem.
I can rename the function to include "max" somewhere.

> 
>> +       }
>> +
>> +       return max + 1;
>> +}
>> +
>> +static int sunxi_gpio_add_bank(struct sunxi_pinctrl *pctl, int index)
>> +{
>> +       char bank_name = 'A' + index + pctl->desc->pin_base / PINS_PER_BANK;
>> +       struct sunxi_gpio_bank *bank = &pctl->banks[index];
>> +       struct gpio_generic_chip_config config;
>> +       struct gpio_chip *gc = &bank->chip.gc;
>> +       int ngpio, ret;
>> +
>> +       ngpio = sunxi_num_pins_of_bank(pctl, index);
>> +       bank->pctl = pctl;
>> +       bank->base = pctl->membase + index * pctl->bank_mem_size;
>> +       if (!ngpio) {
>> +               gc->owner = THIS_MODULE;
>> +               gc->ngpio = 0;
>> +               gc->base = -1;
>> +               gc->of_gpio_n_cells = 3;
>> +
>> +               return 0;
>> +       }
>> +
>> +       config = (struct gpio_generic_chip_config) {
>> +               .dev = pctl->dev,
>> +               .sz = 4,
>> +               .dat = bank->base + DATA_REGS_OFFSET,
>> +               .set = bank->base + DATA_REGS_OFFSET,
>> +               .clr = NULL,
>> +               .flags = GPIO_GENERIC_READ_OUTPUT_REG_SET |
>> +                        GPIO_GENERIC_PINCTRL_BACKEND,
>> +       };
>> +
>> +       ret = gpio_generic_chip_init(&bank->chip, &config);
>> +       if (ret)
>> +               return dev_err_probe(pctl->dev, ret,
>> +                                    "failed to init generic gpio chip\n");
> 
> Generic GPIO assumes that the GPIO pin range starts from 0, and is contiguous.
> This breaks down with the sun5i and sun6i families. For example, on the A31s,
> there is no PC16 ~ PC23, nor PH0 ~ PH8, just to show a few.

But is this really a problem? We have much larger gaps today already, in 
the existing driver. At the end of the day those pins just wouldn't be 
referenced. Or am I missing something?

>> +       gc->owner               = THIS_MODULE;
>> +       gc->label               = devm_kasprintf(pctl->dev, GFP_KERNEL,
>> +                                                "%s-P%c", gc->label,
>> +                                                bank_name);
>> +       gc->ngpio               = ngpio;
> 
> Also set gc->offset?

Yes, I figured this myself when playing around with the gpio-line-names 
property. Fixed.

> 
>> +       gc->base                = -1;
>> +       gc->of_gpio_n_cells     = 3;
>> +       gc->of_node_instance_match = sunxi_of_node_instance_match;
>> +       gc->set_config          = gpiochip_generic_config;
>> +       gc->to_irq              = sunxi_pinctrl_gpio_to_irq;
>> +       gc->can_sleep           = false;
>> +
>> +       return gpiochip_add_data(gc, pctl);
> 
> Can we switch to devm_gpiochip_add_data() instead? It simplifies the
> teardown as well.

Sure, didn't know this existed.

> 
>> +}
>> +
>>   int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>>                                    const struct sunxi_pinctrl_desc *desc,
>>                                    unsigned long flags)
>> @@ -1503,6 +1496,7 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>>          struct sunxi_pinctrl *pctl;
>>          struct pinmux_ops *pmxops;
>>          int i, ret, last_pin, pin_idx;
>> +       int gpio_banks;
>>          struct clk *clk;
>>
>>          pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
>> @@ -1590,38 +1584,23 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>>                  return PTR_ERR(pctl->pctl_dev);
>>          }
>>
>> -       pctl->chip = devm_kzalloc(&pdev->dev, sizeof(*pctl->chip), GFP_KERNEL);
>> -       if (!pctl->chip)
>> -               return -ENOMEM;
>> -
>> -       last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number;
>> -       pctl->chip->owner = THIS_MODULE;
>> -       pctl->chip->request = gpiochip_generic_request;
>> -       pctl->chip->free = gpiochip_generic_free;
>> -       pctl->chip->set_config = gpiochip_generic_config;
>> -       pctl->chip->direction_input = sunxi_pinctrl_gpio_direction_input;
>> -       pctl->chip->direction_output = sunxi_pinctrl_gpio_direction_output;
>> -       pctl->chip->get = sunxi_pinctrl_gpio_get;
>> -       pctl->chip->set = sunxi_pinctrl_gpio_set;
>> -       pctl->chip->of_xlate = sunxi_pinctrl_gpio_of_xlate;
>> -       pctl->chip->to_irq = sunxi_pinctrl_gpio_to_irq;
>> -       pctl->chip->of_gpio_n_cells = 3;
>> -       pctl->chip->can_sleep = false;
>> -       pctl->chip->ngpio = round_up(last_pin, PINS_PER_BANK) -
>> -                           pctl->desc->pin_base;
>> -       pctl->chip->label = dev_name(&pdev->dev);
>> -       pctl->chip->parent = &pdev->dev;
>> -       pctl->chip->base = pctl->desc->pin_base;
>> -
>> -       ret = gpiochip_add_data(pctl->chip, pctl);
>> -       if (ret)
>> -               return ret;
>> +       last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number -
>> +                  pctl->desc->pin_base;
>> +       for (gpio_banks = 0;
> 
> If you switch to devm_gpiochip_add_data() above, you won't need
> gpiochip_remove() below, and you can declare |gpio_banks| inline here in
> the for statement.

Nice, will do this.


>> +            gpio_banks <= last_pin / PINS_PER_BANK;
>> +            gpio_banks++) {
>> +               ret = sunxi_gpio_add_bank(pctl, gpio_banks);
>> +               if (ret)
>> +                       goto gpiochip_error;
>> +       }
>>
>>          for (i = 0; i < pctl->desc->npins; i++) {
>>                  const struct sunxi_desc_pin *pin = pctl->desc->pins + i;
>> +               int bank = (pin->pin.number - pctl->desc->pin_base) / PINS_PER_BANK;
>> +               struct gpio_chip *gc = &pctl->banks[bank].chip.gc;
>>
>> -               ret = gpiochip_add_pin_range(pctl->chip, dev_name(&pdev->dev),
>> -                                            pin->pin.number - pctl->desc->pin_base,
>> +               ret = gpiochip_add_pin_range(gc, dev_name(&pdev->dev),
>> +                                            pin->pin.number % PINS_PER_BANK,
>>                                               pin->pin.number, 1);
>>                  if (ret)
>>                          goto gpiochip_error;
>> @@ -1690,6 +1669,8 @@ int sunxi_pinctrl_init_with_flags(struct platform_device *pdev,
>>          return 0;
>>
>>   gpiochip_error:
>> -       gpiochip_remove(pctl->chip);
>> +       while (--gpio_banks >= 0)
>> +               gpiochip_remove(&pctl->banks[gpio_banks].chip.gc);
>> +
>>          return ret;
>>   }
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
>> index ad26e4de16a85..085131caa02fe 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
>> @@ -14,6 +14,7 @@
>>   #define __PINCTRL_SUNXI_H
>>
>>   #include <linux/kernel.h>
>> +#include <linux/gpio/generic.h>
> 
> gpio comes before kernel?

Indeed.

> And maybe we should try to stop including the massive kernel.h header.

OK, will have a look what should be included instead.

Cheers,
Andre

> 
> 
> Thanks
> ChenYu
> 
>>   #include <linux/spinlock.h>
>>
>>   #define PA_BASE        0
>> @@ -159,9 +160,17 @@ struct sunxi_pinctrl_regulator {
>>          refcount_t              refcount;
>>   };
>>
>> +struct sunxi_pinctrl;
>> +
>> +struct sunxi_gpio_bank {
>> +       struct gpio_generic_chip chip;
>> +       struct sunxi_pinctrl *pctl;
>> +       void __iomem *base;
>> +};
>> +
>>   struct sunxi_pinctrl {
>>          void __iomem                    *membase;
>> -       struct gpio_chip                *chip;
>> +       struct sunxi_gpio_bank          banks[SUNXI_PINCTRL_MAX_BANKS];
>>          const struct sunxi_pinctrl_desc *desc;
>>          struct device                   *dev;
>>          struct sunxi_pinctrl_regulator  regulators[11];
>> --
>> 2.46.4
>>
>>