Re: [PATCH v10 4/8] gpio: pl061: convert register access to regmap

[Andy Shevchenko]

On Thu, Oct 15, 2026 at 06:53:31PM +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.

A few nit-picks, otherwise
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

...

> static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct pl061 *pl061 = gpiochip_get_data(gc);
>  
> -	return !!readb(pl061->base + (BIT(offset + 2)));
> +	return regmap_test_bits(pl061->regmap,
> +				BIT(offset + PL061_DATA_OFFSET),
> +				BIT(offset));

There is some room on the previous line.

	return regmap_test_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
				BIT(offset));

>  }

...

>  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_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> +			   BIT(offset), value);
>  
>  	return 0;

Can we actually return the regmap_assign_bits() error code here?

>  }

...

> 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 gpioiev = 0;
> +	unsigned int gpioibe = 0;
> +	unsigned int gpiois = 0;
> +	unsigned int bit = BIT(offset);

Keep the reversed xmas tree order.

-- 
With Best Regards,
Andy Shevchenko