Re: [PATCH 3/5] gpio: aspeed-sgpio: Create llops to handle hardware access
From: Andrew Lunn <andrew@lunn.ch>
Date: 2026-01-17 15:46:54
Also in:
linux-aspeed, linux-devicetree, linux-gpio, lkml
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.
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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