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 <[email protected]>
(...)>>
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
