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