PATCH v2

PATCH v2

[robert.middleton]

This v2 patch changes the MCP23S08 interrupt logic to ensure that if multiple
inputs are changed at the same time, the kernel will be notified of all
the pins that changed.

I added several variables to the irq handler, however this means that there
are now quite a few local variables(possibly too many, as the kernel coding 
standard says that more than 5-10 is probably too many: 
https://www.kernel.org/doc/Documentation/process/coding-style.rst).  I'm 
not sure what the best way to fix this is, or if it has to be fixed.

-Robert Middleton

[PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[robert.middleton]

From: Robert Middleton <robert.middleton@rm5248.com>

When an interrupt occurs on an MCP23S08 chip, the INTF register will only
contain one bit as causing the interrupt.  If more than two pins change at
the same time on the chip, this causes one of the pins to not be reported.
This patch fixes the logic for checking if a pin has changed, so that
multiple pins will always cause more than one change.

Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>
---
 drivers/gpio/gpio-mcp23s08.c | 66 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 99d37b5..4e03919 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -337,8 +337,10 @@ mcp23s08_direction_output(struct gpio_chip *chip, unsigned offset, int value)
 static irqreturn_t mcp23s08_irq(int irq, void *data)
 {
 	struct mcp23s08 *mcp = data;
-	int intcap, intf, i;
+	int intcap, intf, i, gpio, gpio_orig, intcap_mask;
 	unsigned int child_irq;
+	bool intf_set, intcap_changed, gpio_bit_changed,
+		defval_changed, gpio_set;
 
 	mutex_lock(&mcp->lock);
 	intf = mcp->ops->read(mcp, MCP_INTF);
@@ -356,14 +358,68 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 	}
 
 	mcp->cache[MCP_INTCAP] = intcap;
+
+	/* This clears the interrupt(configurable on S18) */
+	gpio = mcp->ops->read(mcp, MCP_GPIO);
+	if (gpio < 0) {
+		mutex_unlock(&mcp->lock);
+		return IRQ_HANDLED;
+	}
+	gpio_orig = mcp->cache[MCP_GPIO];
+	mcp->cache[MCP_GPIO] = gpio;
 	mutex_unlock(&mcp->lock);
 
+	if (mcp->cache[MCP_INTF] == 0) {
+		/* There is no interrupt pending */
+		return IRQ_HANDLED;
+	}
+
+	dev_dbg(mcp->chip.parent,
+		"intcap 0x%04X intf 0x%04X gpio_orig 0x%04X gpio 0x%04X\n",
+		intcap, intf, gpio_orig, gpio);
 
 	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)) ||
-		     (BIT(i) & mcp->cache[MCP_INTCON]))) {
+		/* We must check all of the inputs on the chip,
+		 * otherwise we may not notice a change on >=2 pins.
+		 *
+		 * On at least the mcp23s17, INTCAP is only updated
+		 * one byte at a time(INTCAPA and INTCAPB are
+		 * not written to at the same time - only on a per-bank
+		 * basis).
+		 *
+		 * INTF only contains the single bit that caused the
+		 * interrupt per-bank.  On the mcp23s17, there is
+		 * INTFA and INTFB.  If two pins are changed on the A
+		 * side at the same time, INTF will only have one bit
+		 * set.  If one pin on the A side and one pin on the B
+		 * side are changed at the same time, INTF will have
+		 * two bits set.  Thus, INTF can't be the only check
+		 * to see if the input has changed.
+		 */
+
+		intf_set = BIT(i) & mcp->cache[MCP_INTF];
+		if (i < 8 && intf_set)
+			intcap_mask = 0x00FF;
+		else if (i >= 8 && intf_set
+			intcap_mask = 0xFF00;
+		else
+			intcap_mask = 0x00;
+
+		intcap_changed = (intcap_mask &
+			(BIT(i) & mcp->cache[MCP_INTCAP])) !=
+			(intcap_mask & (BIT(i) & gpio_orig));
+		gpio_set = BIT(i) & mcp->cache[MCP_GPIO];
+		gpio_bit_changed = (BIT(i) & gpio_orig) !=
+			(BIT(i) & mcp->cache[MCP_GPIO]);
+		defval_changed = (BIT(i) & mcp->cache[MCP_INTCON]) &&
+			((BIT(i) & mcp->cache[MCP_GPIO]) !=
+			(BIT(i) & mcp->cache[MCP_DEFVAL]));
+
+		if (((gpio_bit_changed || intcap_changed) &&
+			(BIT(i) & mcp->irq_rise) && gpio_set) ||
+		    ((gpio_bit_changed || intcap_changed) &&
+			(BIT(i) & mcp->irq_fall) && !gpio_set) ||
+		    defval_changed) {
 			child_irq = irq_find_mapping(mcp->chip.irqdomain, i);
 			handle_nested_irq(child_irq);
 		}
-- 
2.1.4

Re: [PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[Linus Walleij]

On Wed, Feb 22, 2017 at 4:34 PM,  <robert.middleton@rm5248.com> wrote:

> From: Robert Middleton <robert.middleton@rm5248.com>
>
> When an interrupt occurs on an MCP23S08 chip, the INTF register will only
> contain one bit as causing the interrupt.  If more than two pins change at
> the same time on the chip, this causes one of the pins to not be reported.
> This patch fixes the logic for checking if a pin has changed, so that
> multiple pins will always cause more than one change.
>
> Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>

OMG that looks complicated. But good commit with proper comments,
so patch applied for fixes. I had to do some fuzzing patch -p1 < foo.patch
so please check the result.

Yours,
Linus Walleij

Re: [PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[Robert Middleton]

On Tue, Mar 14, 2017 at 9:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Feb 22, 2017 at 4:34 PM,  <robert.middleton@rm5248.com> wrote:
>
>> From: Robert Middleton <robert.middleton@rm5248.com>
>>
>> When an interrupt occurs on an MCP23S08 chip, the INTF register will only
>> contain one bit as causing the interrupt.  If more than two pins change at
>> the same time on the chip, this causes one of the pins to not be reported.
>> This patch fixes the logic for checking if a pin has changed, so that
>> multiple pins will always cause more than one change.
>>
>> Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>
>
> OMG that looks complicated. But good commit with proper comments,
> so patch applied for fixes. I had to do some fuzzing patch -p1 < foo.patch
> so please check the result.
>
> Yours,
> Linus Walleij

Linux,

Thanks, I will check it out once that gets pushed(I'm assuming it will
be in the linux-gpio repository at
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/)

-Robert Middleton

Re: [PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[Linus Walleij]

On Tue, Mar 14, 2017 at 6:41 PM, Robert Middleton
<robert.middleton@rm5248.com> wrote:
> On Tue, Mar 14, 2017 at 9:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, Feb 22, 2017 at 4:34 PM,  <robert.middleton@rm5248.com> wrote:
>>
>>> From: Robert Middleton <robert.middleton@rm5248.com>
>>>
>>> When an interrupt occurs on an MCP23S08 chip, the INTF register will only
>>> contain one bit as causing the interrupt.  If more than two pins change at
>>> the same time on the chip, this causes one of the pins to not be reported.
>>> This patch fixes the logic for checking if a pin has changed, so that
>>> multiple pins will always cause more than one change.
>>>
>>> Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>
>>
>> OMG that looks complicated. But good commit with proper comments,
>> so patch applied for fixes. I had to do some fuzzing patch -p1 < foo.patch
>> so please check the result.
>>
>> Yours,
>> Linus Walleij
>
> Linux,
>
> Thanks, I will check it out once that gets pushed(I'm assuming it will
> be in the linux-gpio repository at
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/)

Unfortunately it explodes in the build servers due to changes in
the driver.

Can you rebase and test on v4.11-rc1 and resend?

Yours,
Linus Walleij

Re: [PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[Robert Middleton]

On Tue, Mar 14, 2017 at 5:16 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 14, 2017 at 6:41 PM, Robert Middleton
> <robert.middleton@rm5248.com> wrote:
>> On Tue, Mar 14, 2017 at 9:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, Feb 22, 2017 at 4:34 PM,  <robert.middleton@rm5248.com> wrote:
>>>
>>>> From: Robert Middleton <robert.middleton@rm5248.com>
>>>>
>>>> When an interrupt occurs on an MCP23S08 chip, the INTF register will only
>>>> contain one bit as causing the interrupt.  If more than two pins change at
>>>> the same time on the chip, this causes one of the pins to not be reported.
>>>> This patch fixes the logic for checking if a pin has changed, so that
>>>> multiple pins will always cause more than one change.
>>>>
>>>> Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>
>>>
>>> OMG that looks complicated. But good commit with proper comments,
>>> so patch applied for fixes. I had to do some fuzzing patch -p1 < foo.patch
>>> so please check the result.
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Linux,
>>
>> Thanks, I will check it out once that gets pushed(I'm assuming it will
>> be in the linux-gpio repository at
>> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/)
>
> Unfortunately it explodes in the build servers due to changes in
> the driver.
>
> Can you rebase and test on v4.11-rc1 and resend?
>
> Yours,
> Linus Walleij

Sure, that will take me a day or two but I can get that done.

-Robert Middleton

Re: [PATCH v2] gpio:mcp23s08 Fixed missing interrupts

[Robert Middleton]

On Tue, Mar 14, 2017 at 9:56 PM, Robert Middleton
<robert.middleton@rm5248.com> wrote:
> On Tue, Mar 14, 2017 at 5:16 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Mar 14, 2017 at 6:41 PM, Robert Middleton
>> <robert.middleton@rm5248.com> wrote:
>>> On Tue, Mar 14, 2017 at 9:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Wed, Feb 22, 2017 at 4:34 PM,  <robert.middleton@rm5248.com> wrote:
>>>>
>>>>> From: Robert Middleton <robert.middleton@rm5248.com>
>>>>>
>>>>> When an interrupt occurs on an MCP23S08 chip, the INTF register will only
>>>>> contain one bit as causing the interrupt.  If more than two pins change at
>>>>> the same time on the chip, this causes one of the pins to not be reported.
>>>>> This patch fixes the logic for checking if a pin has changed, so that
>>>>> multiple pins will always cause more than one change.
>>>>>
>>>>> Signed-off-by: Robert Middleton <robert.middleton@rm5248.com>
>>>>
>>>> OMG that looks complicated. But good commit with proper comments,
>>>> so patch applied for fixes. I had to do some fuzzing patch -p1 < foo.patch
>>>> so please check the result.
>>>>
>>>> Yours,
>>>> Linus Walleij
>>>
>>> Linux,
>>>
>>> Thanks, I will check it out once that gets pushed(I'm assuming it will
>>> be in the linux-gpio repository at
>>> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/)
>>
>> Unfortunately it explodes in the build servers due to changes in
>> the driver.
>>
>> Can you rebase and test on v4.11-rc1 and resend?
>>
>> Yours,
>> Linus Walleij
>
> Sure, that will take me a day or two but I can get that done.
>
> -Robert Middleton

Patch v3 has been sent.  Looking at the v2 patch, the biggest problem was
that I had somehow forgot a ), which was causing a parse error in gcc.
I'm not quite sure how that happened.

I tested with Torvalds' 4.11-rc1 kernel, everything still worked properly.

-Robert Middleton

Re: PATCH v2

[Robert Middleton]

On Wed, Feb 22, 2017 at 10:34 AM,  <robert.middleton@rm5248.com> wrote:
>
> This v2 patch changes the MCP23S08 interrupt logic to ensure that if multiple
> inputs are changed at the same time, the kernel will be notified of all
> the pins that changed.
>
> I added several variables to the irq handler, however this means that there
> are now quite a few local variables(possibly too many, as the kernel coding
> standard says that more than 5-10 is probably too many:
> https://www.kernel.org/doc/Documentation/process/coding-style.rst).  I'm
> not sure what the best way to fix this is, or if it has to be fixed.
>
> -Robert Middleton
> --
> 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

Hi, I would like to try and get some verification here on if this patch
is a good fix or not.  I have done testing on my side, and was
wondering if there was something else that I had to do in order to have
this patched upstream.

-Robert Middleton

Re: PATCH v2

[Phil Reid]

On 10/03/2017 00:37, Robert Middleton wrote:
> On Wed, Feb 22, 2017 at 10:34 AM,  <robert.middleton@rm5248.com> wrote:
>>
>> This v2 patch changes the MCP23S08 interrupt logic to ensure that if multiple
>> inputs are changed at the same time, the kernel will be notified of all
>> the pins that changed.
>>
>> I added several variables to the irq handler, however this means that there
>> are now quite a few local variables(possibly too many, as the kernel coding
>> standard says that more than 5-10 is probably too many:
>> https://www.kernel.org/doc/Documentation/process/coding-style.rst).  I'm
>> not sure what the best way to fix this is, or if it has to be fixed.
>>
>> -Robert Middleton
>> --
>> 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
>
> Hi, I would like to try and get some verification here on if this patch
> is a good fix or not.  I have done testing on my side, and was
> wondering if there was something else that I had to do in order to have
> this patched upstream.
>
G'day Rob,

I was going to test it on my setup, just haven't got to it this week so far.
Probably Monday for me at this rate.

-- 
Regards
Phil Reid