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

[Artur Weber <aweber.kernel@gmail.com>]

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.

> 
>> +{
>> +	/*
>> +	 * 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.

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?

Best regards
Artur