Date: Sun, 9 Jun 2024 22:13:27 +0000
From: Mark Tomlinson <Mark.Tomlinson@...iedtelesis.co.nz>
To: "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>
CC: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>, "brgl@...ev.pl"
	<brgl@...ev.pl>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>
Subject: Re: [PATCH] gpio: pca953x: Improve interrupt support

On Sat, 2024-06-08 at 12:44 +0300, Andy Shevchenko wrote:
> Thu, Jun 06, 2024 at 03:31:02PM +1200, Mark Tomlinson kirjoitti:
> > The GPIO drivers with latch interrupt support (typically types starting
> > with PCAL) have interrupt status registers to determine which
> > particular
> > inputs have caused an interrupt. Unfortunately there is no atomic
> > operation to read these registers and clear the interrupt. Clearing the
> > interrupt is done by reading the input registers.
> 
> What you are describing sounds to me like the case without latch enabled.
> Can you elaborate a bit more?

The latch is useful when an input changes state, but changes back again
before the input is read. Using the latch causes the input register to show
what caused the interrupt, rather than the current state of the pin.

The problem I have is not related to the latch as the inputs are not
changing back to their original state. I have two inputs which change state
at almost the same time. When the first input changes state, an interrupt
occurs. Prior to my patch, the interrupt status register was read, and only
this one interrupt is shown as pending. The second input changes state
between reading the interrupt status and reading the input (which clears
both interrupt sources). So I only get the one interrupt and not both.

> > The code was reading the interrupt status registers, and then reading
> > the input registers. If an input changed between these two events it
> > was
> > lost.
> 
> I don't see how. If there is a short pulse or a series of pulses between
> interrupt latching and input reading, the second+ will be lost in any
> case.
> This is HW limitation as far as I can see.

I feel you're thinking of the single input pin case. There is no issue with
a single pin pulsing as the latch will keep the value which caused the
interrupt until it is read. The interrupt status register will have the
correct value too.
> 
> > The solution in this patch is to revert to the non-latch version of
> > code, i.e. remembering the previous input status, and looking for the
> > changes. This system results in no more I2C transfers, so is no slower.
> > The latch property of the device still means interrupts will still be
> > noticed if the input changes back to its initial state.
> 
> Again, can you elaborate? Is it a real use case? If so, can you provide
> the
> chart of the pin sginalling against the time line and depict where the
> problem
> is?

Yes, this is real. Hopefully the above description explains what we're
seeing, but as a picture is worth 1000 words, here's a timeline:

        --------+
Input 1         |
                +---------------------------------------------
        ----------------------------------+
Input 2                                   |
                                          +-------------------
        --------+                                     +-------
IRQ             |                                     |
                +-------------------------------------+

Interrupt status              *
Register Read

Input Register                                        *
Read

Note that the interrupt status read only sees one event, but both are
cleared later. As these two reads are I2C bus transfers, they are more than
100µs apart, so this event occurs quite frequently in our system.


Thanks for reviewing this.
Best Regards,

Powered by blists - more mailing lists