Re: [RFC PATCH] pinctrl: sunxi: convert to GPIO_GENERIC

[Andre Przywara <andre.przywara@arm.com>]

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