Re: [PATCH] gpio:mcp23s08 Fixed missing interrupts

[Robert Middleton <robert.middleton@xxxxxxxxxx>]

On Mon, Feb 13, 2017 at 12:15 AM, Phil Reid <preid@xxxxxxxxxxxxxxxxx> wrote:
> G'day Rob,
>
> I've tested this and it works for my use case.
> Which has rising & falling irqs and not very fast inputs.
>
> A couple of comment below. I'm no expert on the kernel standards thou.
>
> I've also cc-dd the gpio maintainers. See MAINTAINERS file for a list people
> to copy for different subfolder etc.
>
>
> On 12/02/2017 04:02, robert.middleton@xxxxxxxxxx wrote:
>>
>> From: Robert Middleton <robert.middleton@xxxxxxxxxx>
>>
>> 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.
>
>
> Comments need to be wrapped to 75 chars.
> Also needs signed-by. "git format-patch -s" will do it for you.
>

Ah, I didn't realize that the signed-by was required(I was assuming
signed-by was
only for the actual kernel maintainers).

>
>> ---
>>  drivers/gpio/gpio-mcp23s08.c | 26 +++++++++++++++++++++-----
>>  1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
>> index 99d37b5..b7ceee3 100644
>> --- a/drivers/gpio/gpio-mcp23s08.c
>> +++ b/drivers/gpio/gpio-mcp23s08.c
>> @@ -337,7 +337,7 @@ 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_cache;
>>         unsigned int child_irq;
>>
>>         mutex_lock(&mcp->lock);
>> @@ -356,14 +356,30 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
>>         }
>>
>>         mcp->cache[MCP_INTCAP] = intcap;
>> +
>> +       /* This clears the interrupt */
>
> Only for no s18 chips. But it should matter for them.
>
>
>> +       gpio = mcp->ops->read(mcp, MCP_GPIO);
>> +       if (gpio < 0) {
>> +               mutex_unlock(&mcp->lock);
>> +               return IRQ_HANDLED;
>> +       }
>> +       gpio_cache = mcp->cache[MCP_GPIO];
>> +       mcp->cache[MCP_GPIO] = gpio;
>>         mutex_unlock(&mcp->lock);
>>
>>
>>         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
>> +                */
>> +               int bit_changed = (BIT(i) & gpio_cache) !=
>> +                       (BIT(i) & mcp->cache[MCP_GPIO]);
>> +               int defval_changed = (BIT(i) & mcp->cache[MCP_GPIO]) !=
>> +                       (BIT(i) & mcp->cache[MCP_DEFVAL]);
>> +               if ((bit_changed && (BIT(i) & mcp->irq_rise)) ||
>> +                       (bit_changed && (~BIT(i) & mcp->irq_fall)) ||
>> +                       ((BIT(i) & mcp->cache[MCP_INTCON]) &&
>
> trailing space on this line.
> I'm not sure what the formatting should be here, but most other kernel code
> I've looked
> using space to align to the open bracket of the if statement when the
> condition exceeds
> one line.
>
>> +                       defval_changed)) {
>>                         child_irq = irq_find_mapping(mcp->chip.irqdomain,
>> i);
>>                         handle_nested_irq(child_irq);
>>                 }
>>
>
> bit_changed / defval_changed could be of type bool to be clearer that it's
> meant to
> be a bool result. These should also be defined at the top of the function I
> believe.
>

I forgot that the kernel actually has a 'bool' type; I was thinking it was
C89 only.

Are variables always supposed to be at the start of a function?  I'm wondering
because since they are used only in that one block, it doesn't make sense
to me at least that you would want them as function-level variables.

> I pretty sure the logic will trigger a interrupt on both a rising and
> falling edge of the input
> even if only falling edge is requested. But I may be missing something
> obvious.
>

I think that you're correct here actually; I thought I had tested this
by setting
the 'rising'/'falling' edge in sysfs, but testing it again just now I
get changes in
userspace on both turning it on and off.  I'll look back into it today/tomorrow.

> Also what is the impact to short interrupt events. Should the INTCAP / INTF
> flags still be consider as well.
> ie. A short pulse on input which may happen before GPIO input register be
> read, but captured by INTCAP
> be consider as well. This change may break systems relying on that
> behaviour.
>

The reason that I wasn't able to compare against the INTF register was because
since only 1 bit is ever set, you don't get more than 1 pin change event.  For
example, if pin 1 changes, INTF would read 0x0001.
If both pins 1 and 2 change at the same time, INTF would read either 0x0002
or 0x0001, it wouldn't have both set.  From the testing that I did, it
looked like
INTCAP would contain the proper data, but I wasn't sure if that would always
be the case.

My reasoning for reading the GPIO input register was to clear the interrupt as
soon as possible, so that if there was a short interrupt the kernel would get
the interrupt again.  I'm not sure if that is the proper approach; I have been
told before that in the IRQ handler you should get the data and clear the
interrupt as fast as possible in order to wait for more interrupts.

> FYI: You can run scripts/checkpatch.pl against you patch to identify some of
> the problems highlighted above.
>
> not related to your changes: this function always returns IRQ_HANDLED in all
> cases.
> Other GPIO drivers return IRQ_NONE if no handler was called or in some case
> if an error occurred
> in reading the state. I'm not sure what the correct behaviour is supposed to
> be.
> Docs suggest IRQ_NONE helps the kernel detect bad irq.
> Someone more knowledgeable may be able to offer a hint on that one.
> I can do a patch if this needs changing.
>
> --
> Regards
> Phil Reid
>

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