[PATCH] regmap: irq: Acknowledge also interrupts that are masked

[PATCH] regmap: irq: Acknowledge also interrupts that are masked

[Philipp Zabel]

In case the hardware interrupt mask register does not prevent the chip level
irq from being asserted by the corresponding interrupt status bit, stray
masked interrupts should to be acknowledged, too.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
I have seen GPI interrupts trigger on DA9063 trigger after being masked during
initialization, and once the status bits are set, the interrupt handler routine
never clears them, which keeps the chip irq line asserted forever.
---
 drivers/base/regmap/regmap-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 1643e88..0bdf43f 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -253,8 +253,6 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	 * doing a write per register.
 	 */
 	for (i = 0; i < data->chip->num_regs; i++) {
-		data->status_buf[i] &= ~data->mask_buf[i];
-
 		if (data->status_buf[i] && chip->ack_base) {
 			reg = chip->ack_base +
 				(i * map->reg_stride * data->irq_reg_stride);
@@ -263,6 +261,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
 					reg, ret);
 		}
+
+		data->status_buf[i] &= ~data->mask_buf[i];
 	}
 
 	for (i = 0; i < chip->num_irqs; i++) {
-- 
1.8.3.2

Re: [PATCH] regmap: irq: Acknowledge also interrupts that are masked

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Mon, Jul 22, 2013 at 11:24:52AM +0200, Philipp Zabel wrote:
> In case the hardware interrupt mask register does not prevent the chip level
> irq from being asserted by the corresponding interrupt status bit, stray
> masked interrupts should to be acknowledged, too.

> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> I have seen GPI interrupts trigger on DA9063 trigger after being masked during
> initialization, and once the status bits are set, the interrupt handler routine
> never clears them, which keeps the chip irq line asserted forever.

This can't be the standard behaviour since it breaks expectations as to
what happens for masked interrupts.  Though based on your description of
the problem it sounds like a quirk to ack interrupts immediately after
masking them might do the trick instead of the full on always ack
behaviour.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regmap: irq: Acknowledge also interrupts that are masked

[Philipp Zabel]

Hi Mark,

Am Montag, den 22.07.2013, 11:08 +0100 schrieb Mark Brown:
> On Mon, Jul 22, 2013 at 11:24:52AM +0200, Philipp Zabel wrote:
> > In case the hardware interrupt mask register does not prevent the chip level
> > irq from being asserted by the corresponding interrupt status bit, stray
> > masked interrupts should to be acknowledged, too.
> 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > I have seen GPI interrupts trigger on DA9063 trigger after being masked during
> > initialization, and once the status bits are set, the interrupt handler routine
> > never clears them, which keeps the chip irq line asserted forever.
> 
> This can't be the standard behaviour since it breaks expectations as to
> what happens for masked interrupts.  Though based on your description of
> the problem it sounds like a quirk to ack interrupts immediately after
> masking them might do the trick instead of the full on always ack
> behaviour.

thanks, this seems to work, too:

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 1643e88..4c99ed8 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -418,6 +418,24 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 				reg, ret);
 			goto err_alloc;
 		}
+
+		/* Ack masked but set interrupts */
+		reg = chip->status_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_read(map, reg, &d->status_buf[i]);
+		if (ret != 0) {
+			dev_err(map->dev, "Failed to read IRQ status: %d\n",
+				ret);
+			goto err_alloc;
+		}
+
+		reg = chip->ack_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_write(map, reg, d->status_buf[i] & d->mask_buf[i]);
+		if (ret != 0) {
+			dev_err(map->dev, "Failed to ack 0x%x: %d\n", reg, ret);
+			goto err_alloc;
+		}
 	}
 
 	/* Wake is disabled by default */
-- 
1.8.3.2

Can something like this be done unconditionally, or should I add a quirk
flag to regmap_irq_chip?

regards
Philipp

Re: [PATCH] regmap: irq: Acknowledge also interrupts that are masked

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

On Mon, Jul 22, 2013 at 01:11:14PM +0200, Philipp Zabel wrote:
> Am Montag, den 22.07.2013, 11:08 +0100 schrieb Mark Brown:

> > what happens for masked interrupts.  Though based on your description of
> > the problem it sounds like a quirk to ack interrupts immediately after
> > masking them might do the trick instead of the full on always ack
> > behaviour.

> thanks, this seems to work, too:

Great.  I think a quirk might still be in order in case something is
relying on the interrupts still being asserted (eg, for handover after
boot) and it needs to check if ack_base is defined (for clear on read
interrupt controllers) but otherwise that looks good for upstream if you
want to send a patch?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]