Re: [PATCH 3/7] gpio: pl061: convert register access to regmap

[Andy Shevchenko]

On Fri, Oct 09, 2026 at 06:57:56PM +0800, Long Zhao via B4 Relay wrote:

> Replace readb()/writeb() with a MMIO regmap while keeping the existing
> gpio_chip and girq irqchip. Use a raw spinlock so irq_ack() can run
> under the irq descriptor lock.

...

> +static const struct regmap_config pl061_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.reg_stride = 4,
> +	.max_register = GPIOIC,
> +	.volatile_reg = pl061_volatile_reg,

But this might be a dead code as long as there is no regcache defined.
Should we specify the cache type here?

> +	.fast_io = true,

No need, this is provided by struct regmap_bus::regmap_mmio in regmap-mmio.c.

> +	.use_raw_spinlock = true,
> +};

...

> static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,

>  {
>  	struct pl061 *pl061 = gpiochip_get_data(gc);
>  	unsigned long flags;
> -	unsigned char gpiodir;
> +	unsigned int mask = BIT(offset);
>  
>  	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);
> +	regmap_write(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> +		     !!value << offset);

regmap_assign_bits()

> +	regmap_set_bits(pl061->regmap, GPIODIR, mask);
>  
>  	/*
>  	 * 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)));
> +	regmap_write(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> +		     !!value << offset);

Ditto.

I already pointed out twice to this...

>  	raw_spin_unlock_irqrestore(&pl061->lock, flags);
>  
>  	return 0;
>  }

...

>  static int pl061_set_value(struct gpio_chip *gc, unsigned int offset, int value)
>  {
>  	struct pl061 *pl061 = gpiochip_get_data(gc);
>  
> -	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
> +	regmap_write(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> +		     !!value << offset);

Ditto.

>  	return 0;
>  }

...

> static int pl061_irq_type(struct irq_data *d, unsigned trigger)

>  	struct pl061 *pl061 = gpiochip_get_data(gc);
>  	int offset = irqd_to_hwirq(d);
>  	unsigned long flags;
> -	u8 gpiois, gpioibe, gpioiev;
> -	u8 bit = BIT(offset);
> +	unsigned int gpiois = 0, gpioibe = 0, gpioiev = 0;
> +	unsigned int bit = BIT(offset);

Since these are long lines, try to move them up to follow the reversed xmas
tree order.

...

>  static void pl061_irq_handler(struct irq_desc *desc)
>  {
>  	unsigned long pending;
> +	unsigned int mis = 0;
>  	int offset;
>  	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
>  	struct pl061 *pl061 = gpiochip_get_data(gc);

>  	chained_irq_enter(irqchip, desc);
>  
> -	pending = readb(pl061->base + GPIOMIS);
> -	if (pending) {

This is a dup check, please, add a precursor patch that just drops this
redundant 'if (pending) { }' check.

> +	regmap_read(pl061->regmap, GPIOMIS, &mis);
> +	if (mis) {
> +		pending = mis;
>  		for_each_set_bit(offset, &pending, PL061_GPIO_NR)
>  			generic_handle_domain_irq(gc->irq.domain,
>  						  offset);

-- 
With Best Regards,
Andy Shevchenko