Re: [PATCH v6 09/13] gpio: pl061: use gpio-regmap and add Ambarella layout

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 04, 2026 at 02:38:16PM +0800, Long Zhao via B4 Relay wrote:

> Convert PL061 data and direction handling to gpio-regmap while
> keeping the existing PL061 irqchip path.

> Add an Ambarella CV75 register layout variant on the AMBA bus, without
> claiming unverified GPIO suspend/resume support for Ambarella.

This part should be in a separate patch.

So far I see at least 2 patches against gpio-regmap and 2 patches against
gpio-pl061.

...

> +struct pl061_variant_data {
> +	unsigned int data;
> +	unsigned int dir;
> +	unsigned int is;
> +	unsigned int ibe;
> +	unsigned int iev;
> +	unsigned int ie;
> +	unsigned int ris;
> +	unsigned int mis;
> +	unsigned int ic;
> +	unsigned int mask;
> +	unsigned int enable;
> +	unsigned int ngpio;
> +	bool access_32bit;
> +	bool masked_data_address;
> +	bool write_data_after_dir;
> +	bool clear_irq_on_type;
> +	bool pm_save_restore;
> +	const struct regmap_config *regmap_config;
> +};

Should not be like this. Part of it is in the respective regmap config (with
all volatile, precious, et cetera registers, and part of it comes from driver
data (in other words based on the compatible string or other ID).

...

>  struct pl061 {
>  	raw_spinlock_t		lock;
> -

Stray change.

>  	void __iomem		*base;
> -	struct gpio_chip	gc;
> +	const struct pl061_variant_data *variant;
> +	struct gpio_irq_chip	girq;
>  	int			parent_irq;

Here (in this structure) we should get a struct regmap instead of base and
gpio_chip.

>  	struct pl061_context_save_regs csave_regs;

This is not needed, see how gpio-pca953x does that with enabled regmap cache.

>  };

...

> +static u32 pl061_read(struct pl061 *pl061, unsigned int reg)
>  {
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -	unsigned long flags;
> -	unsigned char gpiodir;
> +	if (pl061->variant->access_32bit)
> +		return readl(pl061->base + reg);
>  
> -	raw_spin_lock_irqsave(&pl061->lock, flags);
> -	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
> -	gpiodir = readb(pl061->base + GPIODIR);
> -	gpiodir |= BIT(offset);
> -	writeb(gpiodir, pl061->base + GPIODIR);
> -
> -	/*
> -	 * gpio value is set again, because pl061 doesn't allow to set value of
> -	 * a gpio pin before configuring it in OUT mode.
> -	 */
> -	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
> -	raw_spin_unlock_irqrestore(&pl061->lock, flags);
> -
> -	return 0;
> +	return readb(pl061->base + reg);
>  }

> -static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
> +static void pl061_write(struct pl061 *pl061, u32 value, unsigned int reg)
>  {
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -
> -	return !!readb(pl061->base + (BIT(offset + 2)));
> +	if (pl061->variant->access_32bit)
> +		writel(value, pl061->base + reg);
> +	else
> +		writeb(value, pl061->base + reg);

This is achieved by different regmap config — one for 32-bit, one for 8-bit
access.

>  }

...

I guess it's enough for now. this needs one more round of designing this.

-- 
With Best Regards,
Andy Shevchenko