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.
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.
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.
Fixes: 44896beae605 ("gpio: pca953x: add PCAL9535 interrupt support for Galileo Gen2")
Signed-off-by: Mark Tomlinson <[email protected]>
---
drivers/gpio/gpio-pca953x.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 77a2812f2974..14b80cb00274 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -839,25 +839,6 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin
DECLARE_BITMAP(trigger, MAX_LINE);
int ret;
- if (chip->driver_data & PCA_PCAL) {
- /* Read the current interrupt status from the device */
- ret = pca953x_read_regs(chip, PCAL953X_INT_STAT, trigger);
- if (ret)
- return false;
-
- /* Check latched inputs and clear interrupt status */
- ret = pca953x_read_regs(chip, chip->regs->input, cur_stat);
- if (ret)
- return false;
-
- /* Apply filter for rising/falling edge selection */
- bitmap_replace(new_stat, chip->irq_trig_fall, chip->irq_trig_raise, cur_stat, gc->ngpio);
-
- bitmap_and(pending, new_stat, trigger, gc->ngpio);
-
- return !bitmap_empty(pending, gc->ngpio);
- }
-
ret = pca953x_read_regs(chip, chip->regs->input, cur_stat);
if (ret)
return false;
--
2.45.2
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 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.
> 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?
--
With Best Regards,
Andy Shevchenko
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,