Hi Jeff,
> On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
>>
>> +static irqreturn_t cs40l50_error(int irq, void *data);
>> +
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
>> + CS40L50_IRQ(VIRT2_MBOX, "Mailbox", process_mbox),
>> + CS40L50_IRQ(TEMP_ERR, "Overtemperature", error),
>> + CS40L50_IRQ(BST_UVP, "Boost undervoltage", error),
>> + CS40L50_IRQ(BST_SHORT, "Boost short", error),
>> + CS40L50_IRQ(BST_ILIMIT, "Boost current limit", error),
>> + CS40L50_IRQ(UVLO_VDDBATT, "Boost UVLO", error),
>> + CS40L50_IRQ(GLOBAL_ERROR, "Global", error),
>> +};
>> +
>> +static irqreturn_t cs40l50_error(int irq, void *data)
>> +{
>> + struct cs40l50_private *cs40l50 = data;
>> +
>> + dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
>> +
>> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
>> +}
>> +
>> +static const struct regmap_irq cs40l50_reg_irqs[] = {
>> + CS40L50_REG_IRQ(IRQ1_INT_1, AMP_SHORT),
>> + CS40L50_REG_IRQ(IRQ1_INT_2, VIRT2_MBOX),
>> + CS40L50_REG_IRQ(IRQ1_INT_8, TEMP_ERR),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_UVP),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_SHORT),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_ILIMIT),
>> + CS40L50_REG_IRQ(IRQ1_INT_10, UVLO_VDDBATT),
>> + CS40L50_REG_IRQ(IRQ1_INT_18, GLOBAL_ERROR),
>> +};
>> +
>> +static struct regmap_irq_chip cs40l50_irq_chip = {
>> + .name = "CS40L50 IRQ Controller",
>> +
>> + .status_base = CS40L50_IRQ1_INT_1,
>> + .mask_base = CS40L50_IRQ1_MASK_1,
>> + .ack_base = CS40L50_IRQ1_INT_1,
>> + .num_regs = 22,
>> +
>> + .irqs = cs40l50_reg_irqs,
>> + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
>> +
>> + .runtime_pm = true,
>> +};
>> +
>> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
>> +{
>> + struct device *dev = cs40l50->dev;
>> + int error, i, irq;
>> +
>> + error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
>> + IRQF_ONESHOT | IRQF_SHARED, 0,
>> + &cs40l50_irq_chip, &cs40l50->irq_data);
>> + if (error)
>> + return error;
>> +
>> + for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
>> + irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
>> + if (irq < 0) {
>> + dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
>> + return irq;
>> + }
>> +
>> + error = devm_request_threaded_irq(dev, irq, NULL,
>> + cs40l50_irqs[i].handler,
>> + IRQF_ONESHOT | IRQF_SHARED,
>> + cs40l50_irqs[i].name, cs40l50);
>> + if (error) {
>> + dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
>> + return error;
>> + }
>> + }
>
> This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
> on their system, it's going to be full of L50 interrupts. Normally we declare
> a single IRQ, read the status register(s) from inside its handler and then
> act accordingly.
>
> What is the motivation for having one handler per interrupt status bit? If
> multiple bits are set at once, does the register get read multiple times and
> if so, does doing so clear any pending status? Or are the status registers
> write-to-clear instead of read-to-clear?
The reason I used the regmap_irq framework is that it takes care of
the reading and clearing of the status register, and yes it handles the
situation of multiple bits getting set at once. I think I will merge the IRQ
handlers into one for the next version. The fact of /proc/interrupts filling
up with these interrupts is not great and was something I overlooked,
though I think I see instances of drivers with similar amount of interrupts
upstream.
Best,
James
