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

[Lee Jones <lee@kernel.org>]

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.

> > > +{
> > > +	/*
> > > +	 * IRQ registers are clear-on-read, make sure we don't cache them
> > > +	 * so that they get read/cleared correctly
> > > +	 */
> > > +	return (reg >= BCM590XX_REG_IRQ1 && reg <= (BCM590XX_REG_IRQ1 + 15));
> > > +}
> > > +
> > >   static const struct regmap_config bcm590xx_regmap_config_pri = {
> > >   	.reg_bits	= 8,
> > >   	.val_bits	= 8,
> > >   	.max_register	= BCM590XX_MAX_REGISTER_PRI,
> > > +	.volatile_reg	= bcm590xx_volatile_pri,
> > >   	.cache_type	= REGCACHE_MAPLE,
> > >   };
> > > @@ -46,6 +59,258 @@ static const struct regmap_config bcm590xx_regmap_config_sec = {
> > >   	.cache_type	= REGCACHE_MAPLE,
> > >   };
> > > +#define BCM590XX_REGMAP_IRQ_REG(id)	REGMAP_IRQ_REG_LINE(id, 8)
> > 
> > It looks like this may benefit more than just this driver.
> > 
> > Please create a generic helper in include/linux/regmap.h.
> > 
> > Perhaps REGMAP_IRQ_REG_LINE_BYTE, or whatever the 8 represents.
> 
> I would rather avoid modifying the regmap code if possible.

Why not?

> I've seen another driver (drivers/power/supply/max77705_charger.c) use
> REGMAP_IRQ_REG_LINE(id, BITS_PER_BYTE), which is more descriptive than
> just leaving the raw number of bits. Would that be a good alternative?

Sounds fine to me.

-- 
Lee Jones [李琼斯]