Message-ID:
 <f94ea2f9d0d241509d256d87f02c6de2OSQPR06MB725219B6ED261DBB4E8BC33D8B88A@OSQPR06MB7252.apcprd06.prod.outlook.com>
Date: Mon, 19 Jan 2026 02:39:12 +0000
From: Billy Tsai <billy_tsai@...eedtech.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Linus Walleij <linusw@...nel.org>, Bartosz Golaszewski <brgl@...nel.org>,
	Joel Stanley <joel@....id.au>, Andrew Jeffery <andrew@...econstruct.com.au>,
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Andrew Jeffery <andrew@...id.au>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, BMC-SW
	<BMC-SW@...eedtech.com>
Subject: Re: [PATCH 3/5] gpio: aspeed-sgpio: Create llops to handle hardware
 access

 
Thanks for the detailed review — your comments are very helpful.
 
> > 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.
 
Yes, the register numbers remain unchanged for ASPEED G4 in this patch.
The intent of introducing the llops abstraction is to decouple the driver logic
from the underlying register layout so that we can support SoCs with different
SGPIO register organizations in the future. The actual AST2700-specific support
will be added in a subsequent patch.
 
We did consider regmap. However, llops is intended to abstract not only register
access but also layout-specific bit mapping, which is difficult to express
cleanly with a flat regmap interface.
 
> > @@ -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?
 
You’re right — viewed together, this change is not obviously correct and makes
the refactoring harder to review.
 
While the llops interface is designed to handle bit positioning internally
(changing the semantics from passing a bitmask to passing a value), combining
this semantic change with the abstraction refactoring increases review
complexity.
 
To address this, I will respin the series and split it into:
                1.            a preparatory refactoring patch that introduces the llops helpers without
changing behavior, and
                2.            a follow-up patch that switches callers to the new value-based interface,
with a commit message explicitly explaining the semantic change.
 
> > @@ -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.
 
Agreed. I will revert the rename from this patch and handle it separately if
needed.
 
> >       /* 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.
 
The change from ARRAY_SIZE(aspeed_sgpio_banks) to gpio->chip.ngpio is required
because AST2700 does not use a fixed bank-based register layout.
 
Using ngpio removes the dependency on a static bank array and allows the IRQ
handling code to work with SoCs that have different SGPIO organizations.
I agree this change deserves a dedicated patch with a commit message explaining
the rationale, and I will split it out accordingly.
 
Thanks again for the review. I’ll send a revised version with the changes above.

Billy Tsai

Powered by blists - more mailing lists