Re: [PATCHv2 2/4] tps65912: irq: add interrupt controller

[Margarita Olaya]

Hi Jack,

On Thu, May 12, 2011 at 2:56 PM, Jack Stone <jwjstone@xxxxxxxxxxx> wrote:
> Hi Margarita,
>
> I'm no irq expert so the following might be wrong.
>
> This comment seems to contradict the code - the comment states that all
> interrupts are clear on read but the irq handler explicitly writes the
> value back to the interrupt register.
>
> Did you mean the comment to say that the irq handler explicitly clears
> the interrupts it handles?

Yeap, the current comment is not correct, I will update it, for
tps65912 we need to write 1 to clear the irq.
Thanks for review.

Regards,
Margarita

>
> On 12/05/2011 19:43, Margarita Olaya wrote:
>> +++ b/drivers/mfd/tps65912-irq.c
>> +/*
>> + * This is a threaded IRQ handler so can access I2C/SPI.  Since all
>> + * interrupts are clear on read the IRQ line will be reasserted and
>> + * the physical IRQ will be handled again if another interrupt is
>> + * asserted while we run - in the normal course of events this is a
>> + * rare occurrence so we save I2C/SPI reads.  We're also assuming that
>> + * it's rare to get lots of interrupts firing simultaneously so try to
>> + * minimise I/O.
>> + */
>> +static irqreturn_t tps65912_irq(int irq, void *irq_data)
>> +{
>> +     struct tps65912 *tps65912 = irq_data;
>> +     u32 irq_sts;
>> +     u32 irq_mask;
>> +     u8 reg;
>> +     int i;
>> +
>> +
>> +     tps65912->read(tps65912, TPS65912_INT_STS, 1, &reg);
>> +     irq_sts = reg;
>> +     tps65912->read(tps65912, TPS65912_INT_STS2, 1, &reg);
>> +     irq_sts |= reg << 8;
>> +     tps65912->read(tps65912, TPS65912_INT_STS3, 1, &reg);
>> +     irq_sts |= reg << 16;
>> +     tps65912->read(tps65912, TPS65912_INT_STS4, 1, &reg);
>> +     irq_sts |= reg << 24;
>> +
>> +     tps65912->read(tps65912, TPS65912_INT_MSK, 1, &reg);
>> +     irq_mask = reg;
>> +     tps65912->read(tps65912, TPS65912_INT_MSK2, 1, &reg);
>> +     irq_mask |= reg << 8;
>> +     tps65912->read(tps65912, TPS65912_INT_MSK3, 1, &reg);
>> +     irq_mask |= reg << 16;
>> +     tps65912->read(tps65912, TPS65912_INT_MSK4, 1, &reg);
>> +     irq_mask |= reg << 24;
>> +
>> +     irq_sts &= ~irq_mask;
>> +     if (!irq_sts)
>> +             return IRQ_NONE;
>> +
>> +     for (i = 0; i < tps65912->irq_num; i++) {
>> +             if (!(irq_sts & (1 << i)))
>> +                     continue;
>> +
>> +             handle_nested_irq(tps65912->irq_base + i);
>> +     }
>> +
>> +     /* Write the STS register back to clear IRQs we handled */
>> +     reg = irq_sts & 0xFF;
>> +     irq_sts >>= 8;
>> +     if (reg)
>> +             tps65912->write(tps65912, TPS65912_INT_STS, 1, &reg);
>> +     reg = irq_sts & 0xFF;
>> +     irq_sts >>= 8;
>> +     if (reg)
>> +             tps65912->write(tps65912, TPS65912_INT_STS2, 1, &reg);
>> +     reg = irq_sts & 0xFF;
>> +     irq_sts >>= 8;
>> +     if (reg)
>> +             tps65912->write(tps65912, TPS65912_INT_STS3, 1, &reg);
>> +     reg = irq_sts & 0xFF;
>> +     if (reg)
>> +             tps65912->write(tps65912, TPS65912_INT_STS4, 1, &reg);
>> +
>> +     return IRQ_HANDLED;
>> +}
>
> Thanks,
>
> Jack
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/