[PATCH 1/1] gpio: mcp23s08: Add support for level triggered interrupts

[PATCH 1/1] gpio: mcp23s08: Add support for level triggered interrupts

[Alexander Stein]

The interrupt for the corresponding pin is configured to trigger when the
pin state changes compared to a preconfigured state (Bit set in INTCON).
This state is set by setting/clearing the bit in DEFVAL.
In the interrupt handler we need also to check if the bit in INTCON is set
for level triggered interrupts.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/gpio/gpio-mcp23s08.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index c882c2b..ac22efc 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -362,7 +362,8 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 	for (i = 0; i < mcp->chip.ngpio; i++) {
 		if ((BIT(i) & mcp->cache[MCP_INTF]) &&
 		    ((BIT(i) & intcap & mcp->irq_rise) ||
-		     (mcp->irq_fall & ~intcap & BIT(i)))) {
+		     (mcp->irq_fall & ~intcap & BIT(i)) ||
+		     (BIT(i) & mcp->cache[MCP_INTCON]))) {
 			child_irq = irq_find_mapping(mcp->chip.irqdomain, i);
 			handle_nested_irq(child_irq);
 		}
@@ -408,6 +409,12 @@ static int mcp23s08_irq_set_type(struct irq_data *data, unsigned int type)
 		mcp->cache[MCP_INTCON] &= ~BIT(pos);
 		mcp->irq_rise &= ~BIT(pos);
 		mcp->irq_fall |= BIT(pos);
+	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
+		mcp->cache[MCP_INTCON] |= BIT(pos);
+		mcp->cache[MCP_DEFVAL] &= ~BIT(pos);
+	} else if (type & IRQ_TYPE_LEVEL_LOW) {
+		mcp->cache[MCP_INTCON] |= BIT(pos);
+		mcp->cache[MCP_DEFVAL] |= BIT(pos);
 	} else
 		return -EINVAL;
 
-- 
2.7.3

Re: [PATCH 1/1] gpio: mcp23s08: Add support for level triggered interrupts

[Linus Walleij]

On Wed, Mar 23, 2016 at 6:01 PM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> The interrupt for the corresponding pin is configured to trigger when the
> pin state changes compared to a preconfigured state (Bit set in INTCON).
> This state is set by setting/clearing the bit in DEFVAL.
> In the interrupt handler we need also to check if the bit in INTCON is set
> for level triggered interrupts.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Patch applied.

I'm a bit concerned that you now support both edge and level trigged
IRQs but this driver is using handle_simple_irq() in the
gpiochip_irqchip_add() call. I guess it "just works" because
the hardware will latch the edge IRQ and clear it when reading the
status register.

I guess you have tested it with both edge and level IRQs?

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: mcp23s08: Add support for level triggered interrupts

[Alexander Stein]

On Thursday 31 March 2016 10:41:24, Linus Walleij wrote:
> On Wed, Mar 23, 2016 at 6:01 PM, Alexander Stein
> 
> <alexander.stein@systec-electronic.com> wrote:
> > The interrupt for the corresponding pin is configured to trigger when the
> > pin state changes compared to a preconfigured state (Bit set in INTCON).
> > This state is set by setting/clearing the bit in DEFVAL.
> > In the interrupt handler we need also to check if the bit in INTCON is set
> > for level triggered interrupts.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> 
> Patch applied.
> 
> I'm a bit concerned that you now support both edge and level trigged
> IRQs but this driver is using handle_simple_irq() in the
> gpiochip_irqchip_add() call. I guess it "just works" because
> the hardware will latch the edge IRQ and clear it when reading the
> status register.

From the reference manual:
> The INTCAP register captures the GPIO port value at
> the time the interrupt occurred. The register is ‘read
> only’ and is updated only when an interrupt occurs. The
> register will remain unchanged until the interrupt is
> cleared via a read of INTCAP or GPIO.

So, i guess you're right. Although currently I don't know why 
handle_simple_irq would not work if this would not be the case.

> I guess you have tested it with both edge and level IRQs?

Yep, I have buttons and a PCA9555 added to MCP23S17. Buttons are gpio-keys, so 
rising and falling edge interrupts and PCA9555 uses low level interrupts.
See this excerpt from /proc/interrupts:

 79:          0          2  gpio-mcp23xxx   8 Edge      Digital In 0
 84:          0          4  gpio-mcp23xxx  13 Level     0-0024

Best regards,
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] gpio: mcp23s08: Add support for level triggered interrupts

[Linus Walleij]

On Thu, Mar 31, 2016 at 10:58 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> On Thursday 31 March 2016 10:41:24, Linus Walleij wrote:

> From the reference manual:
>> The INTCAP register captures the GPIO port value at
>> the time the interrupt occurred. The register is ‘read
>> only’ and is updated only when an interrupt occurs. The
>> register will remain unchanged until the interrupt is
>> cleared via a read of INTCAP or GPIO.
>
> So, i guess you're right. Although currently I don't know why
> handle_simple_irq would not work if this would not be the case.

The usual construction is that for edge triggered interrupts
there is an ACK register where a bit goes to "1" whenever it
triggers on an edge, and this must be taken down by an explicit
read or even write of the ACK registers.

Level triggered interrupts by contrast, are just a register
reflecting the value of the interrupt line: it only remains set
to "1" as long as the external source holds it high, and will
go low as soon as the device causing he interrupt releases
it.

Therefor level triggered interrupts often do not need any
explicit ACK at all.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html