Re: [PATCH v3] mfd: bcm590xx: Add support for interrupt handling

[Lee Jones <lee@kernel.org>]

On Sat, 24 Jan 2026, Artur Weber wrote:

> On 13.11.2025 14:26, Lee Jones wrote:
> > On Wed, 29 Oct 2025, Artur Weber wrote:
> > 
> > > On 23.10.2025 15:03, Lee Jones wrote:
> > > > On Mon, 13 Oct 2025, Artur Weber wrote:
> > > > 
> > > > > The BCM590XX supports up to 128 internal interrupts, which are used by
> > > > > various parts of the chip. Add regmap_irq-based interrupt handling and
> > > > > helper functions to allow subdevice drivers to easily use the interrupts.
> > > > > 
> > > > > Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> > > > > 
> > > > > (...)>>
> > > > > diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
> > > > > index 5a8456bbd63f..fb6afe277ebf 100644
> > > > > --- a/drivers/mfd/bcm590xx.c
> > > > > +++ b/drivers/mfd/bcm590xx.c
> > > > > @@ -26,16 +26,29 @@
> > > > >    #define BCM590XX_PMUREV_ANA_MASK	0xF0
> > > > >    #define BCM590XX_PMUREV_ANA_SHIFT	4
> > > > > +#define BCM590XX_REG_IRQ1		0x20
> > > > > +#define BCM590XX_REG_IRQ1MASK		0x30
> > > > 
> > > > This isn't better.
> > > > 
> > > > And now the nomenclature is inconsistent with the one above.
> > > > 
> > > > What is a mask register?  I don't understand.
> > > 
> > > The IRQxMASK registers store the interrupt masks for each interrupt. To
> > > explain this more clearly:
> > > 
> > > The BCM590xx chips have up to 128 internal interrupts (the exact number
> > > is different between the BCM59054 and BCM59056, but both reserve the
> > > exact same amount of registers for them).
> > > 
> > > The status of each interrupt is stored in the IRQx registers
> > > (0x20-0x2f), and each bit represents a single interrupt.
> > > 
> > > The interrupt masks (that is, whether the interrupt is enabled or
> > > disabled) are stored in the IRQx_MASK registers (0x30-0x3f), and each
> > > bit represents the mask for a single interrupt, in the same order as the
> > > IRQx registers. (...would IRQMASKx be more consistent?)
> > > 
> > > Each register stores 8 bits of data, meaning the {status, mask} for 8
> > > interrupts can fit into one {status, mask} register.
> > 
> > Okay, so the "MASK" thing is just silly naming by the H/W designers?
> > 
> > STATUS or ENABLE sounds like it would be better, since a "mask" to me is
> > a software term which describes the methods for manipulating these kinds
> > of groups of bits.
> > 
> 
> As far as I can tell, "interrupt masking" is the standard term for
> enabling/disabling interrupts[1]. It's even consistent with regmap_irq setup
> code - if you scroll down to the part where this register gets used, it's
> passed to the struct "regmap_irq_chip" to the field ".mask_base":
> 
> > +static const struct regmap_irq_chip bcm59054_irq_chip = {
> > +	.name = "bcm59054-irq",
> > +	.irqs = bcm59054_regmap_irqs,
> > +	.num_irqs = BCM59054_IRQ_MAX,
> > +	.num_regs = 16,
> > +	.status_base = BCM590XX_REG_IRQ1,
> > +	.mask_base = BCM590XX_REG_IRQ1MASK,
> > +	.clear_on_unmask = true,
> > +};
> 
> I find it a bit reductive to just call it "silly naming by the H/W
> designers"...
> 
> ...but I won't waste any more of your time about it :). I'll rename it to
> IRQENABLE in the next version (though I do have mixed feelings about
> completely changing a hw register's name...)
> 
> [1] https://en.wikipedia.org/wiki/Interrupt#Masking

I know how masking works.

However, I was unaware of the nomenclature used by Regmap.

If the registers are called that in the datasheet, you should keep them.

-- 
Lee Jones [李琼斯]