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

[Jack Stone <jwjstone@fastmail.fm>]

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?

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