Re: [PATCH v2 07/10] pinctrl: ambarella: add Ambarella pin controller

Hi Long,

thanks for your patch!

On Thu, Aug 6, 2026 at 11:34 AM Long Zhao via B4 Relay
<devnull+longzhao.ambarella.com-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> +#define GPIO_DATA_OFFSET               0x00
> +#define GPIO_DIR_OFFSET                        0x04
> +#define GPIO_IS_OFFSET                 0x08
> +#define GPIO_IBE_OFFSET                        0x0c
> +#define GPIO_IEV_OFFSET                        0x10
> +#define GPIO_IE_OFFSET                 0x14
> +#define GPIO_AFSEL_OFFSET              0x18
> +#define GPIO_RIS_OFFSET                        0x1c
> +#define GPIO_MIS_OFFSET                        0x20
> +#define GPIO_IC_OFFSET                 0x24
> +#define GPIO_MASK_OFFSET               0x28
> +#define GPIO_ENABLE_OFFSET             0x2c

As everyone can see from a mile away this is a modified ARM PL061
primecell, see drivers/gpio/gpio-pl061.c.

I'm not very interested in having two drivers for pretty much the same
hardware in the kernel. Odds are that there will be new quirks and fixed
to one of the drivers that the other one doesn't get :/
Also the PL061 driver is pretty complex and specialized as GPIO
drivers go already. I certainly don't want to have a second copy
of that.

We need to find a way to re-use the PL061 driver with this pin
controller.

1. Modify the existing PL061 driver to use a dynamic register
  layout, i.e. make the different register locations part of some
  per-variant data:

struct vendor_data {
    u32 data_offset;
    u32 dir_offset;
...
};

Add:

struct pl061 {
    struct vendor_data *variant;
...
};

Add vendor data to the PL061 variant:

static struct vendor_data vendor_arm = {...};

static struct vendor_data vendor_ambarella = {...};

static const struct amba_id pl061_ids[] = {
        {
                .id     = 0x00041061,
                .mask   = 0x000fffff,
                . data = &vendor_arm;
        },
        {
               .id = /* make something up, see include/linux/amba/bus.h */
               .mask = ....
               .data = &vendor_ambarella;
        },


Essentially follow the pattern from drivers/spi/spi-pl022.c.

2. Contemplate if the GPIO should be a separate entity in the
 device tree or not. If not, Linux needs to spawn the new AMBA
 device with this pin control driver using e.g.
 amba_device_alloc() and amba_device_register() etc,
 see drivers/of/platform.c

3. select GPIO_PL061 in Kconfig for this pin controller...

This is not a simple solution but it is the right one.

Yours,
Linus Walleij