| Message ID | 1458752487-32767-1-git-send-email-alexander.stein@systec-electronic.com |
|---|---|
| State | New |
| Headers | show |
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 -- 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
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
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
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;
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(-)