Re: [PATCH 3/5] gpio: aspeed-sgpio: Create llops to handle hardware access

[Andrew Lunn <andrew@lunn.ch>]

On Sat, Jan 17, 2026 at 07:17:10PM +0800, Billy Tsai wrote:
> Add low-level operations (llops) to abstract the register access for SGPIO
> registers. With this abstraction layer, the driver can separate the
> hardware and software logic, making it easier to extend the driver to
> support different hardware register layouts.

With a quick look at the code, it appears the register numbers stay
the same? Is that true?

I think you have reinvented regmap.

> @@ -318,30 +278,25 @@ static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
>  	u32 type0 = 0;
>  	u32 type1 = 0;
>  	u32 type2 = 0;
> -	u32 bit, reg;
> -	const struct aspeed_sgpio_bank *bank;
>  	irq_flow_handler_t handler;
> -	struct aspeed_sgpio *gpio;
> -	void __iomem *addr;
> -	int offset;
> -
> -	irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> +	struct aspeed_sgpio *gpio = irq_data_get_irq_chip_data(d);
> +	int offset = irqd_to_hwirq(d);
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_BOTH:
> -		type2 |= bit;
> +		type2 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_RISING:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_FALLING:
>  		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_LEVEL_LOW:
> -		type1 |= bit;
> +		type1 = 1;
>  		handler = handle_level_irq;
>  		break;

This change is not obviously correct to me. It is not about
abstracting register accesses, what you actually write to the
registers appears to of changed. Maybe you could add a refactoring
patch first which does this change, with a commit message explaining
it, and then insert the register abstraction?


> @@ -374,16 +318,14 @@ static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
>  {
>  	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
>  	struct irq_chip *ic = irq_desc_get_chip(desc);
> -	struct aspeed_sgpio *data = gpiochip_get_data(gc);
> +	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);

This rename does not belong in this patch. You want lots of small
patches, each doing one logical thing, with a good commit message, and
obviously correct. Changes like this make it a lot less obviously
correct.

>  	/* Disable IRQ and clear Interrupt status registers for all SGPIO Pins. */
> -	for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> -		bank =  &aspeed_sgpio_banks[i];
> +	for (i = 0; i < gpio->chip.ngpio; i += 2) {

Why are ARRAY_SIZE() gone? There probably is a good reason, so doing
this in a patch of its own, with a commit message explaining "Why?"
would make this easier to review.

      Andrew