RE: [PATCH v5 6/9] EDAC/aspeed: Replace regmap with direct register access

[Ryan Chen <ryan_chen@aspeedtech.com>]

> Subject: Re: [PATCH v5 6/9] EDAC/aspeed: Replace regmap with direct register
> access
> 
> On Wed, Oct 07, 2026 at 02:13:23PM +0800, Ryan Chen wrote:
> > The driver instantiates its own regmap purely as an MMIO wrapper: it
> > has no register cache, uses custom .reg_read()/.reg_write() callbacks,
> > and is not shared as a syscon with other drivers. So it brings nothing
> > here beyond the spinlock that regmap takes around each access when
> > fast_io is set.
> >
> > Drop the regmap and access the registers directly with
> > readl()/writel() under an explicit raw spinlock, held across the whole
> > read-modify-write so the controller is unlocked once around the
> > grouped writes rather than on every register write.
> >
> > Annotate the register base with __guarded_by() so that, under
> > CONFIG_WARN_CONTEXT_ANALYSIS, the compiler checks at build time that
> > every hardware register access is performed while holding the lock.
> >
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >
> > ---
> > Changes in v2:
> > - Take the register lock with the irqsave variant in init_csrows() and in
> >   aspeed_probe(); the interrupt handler takes the same lock in hardirq
> >   context.
> > - Move the dev_dbg() of the interrupt status register out of the
> >   raw_spinlock critical section in the interrupt handler.
> > - Opt aspeed_edac.o into context analysis in drivers/edac/Makefile, so
> >   that the __guarded_by() annotation is actually checked.
> > - Note in the interrupt handler that the counter and interrupt flag
> >   fields are read-only, so writing back the read value is harmless.
> > ---
> >  drivers/edac/Makefile      |   1 +
> >  drivers/edac/aspeed_edac.c | 131
> > +++++++++++++++++----------------------------
> >  2 files changed, 51 insertions(+), 81 deletions(-)
> 
> https://lore.kernel.org/r/sashiko-outbox-162667@kernel.org
> 
Hello Boris,
Sorry, I do not replied to that one.

I'll take the second option in v6: aspeed_probe() keeps its goto-based
error paths and no longer uses scoped_guard().

v6 drops the annotation and the context analysis opt-in, and assigns
the register base directly:

-	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
-		aspeed_regs = regs;
+	aspeed_regs = regs;

While at it, v6 also switches the lock from raw_spinlock_t to
spinlock_t. That is what regmap used before this patch, and the
interrupt handler is not IRQF_NO_THREAD, so a raw lock is not needed.

Patches 7/9 and 8/9 follow the same changes.

Thanks your help.

Ryan


> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette