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

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

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

Best regards
Artur