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

[Chen-Yu Tsai]

On Fri, Mar 13, 2026 at 8:08 AM Andre Przywara <andre.przywara@xxxxxxx> 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@xxxxxxx>
> ---
>  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
>
>