| Message ID | 20240606033102.2271916-1-mark.tomlinson@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pca953x: Improve interrupt support | expand |
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,
Hi All,
I'm encountering exactly the same issue, and there is indeed a problem in the process of pca953x_irq_pending().
Mark has already explained the issue, but as apparently the discussion stopped, I've tried below to add some details to help better understand the issue. I've also a solution to propose.
The issue:
In the current implementation, when an IRQ occurs, the function pca953x_irq_pending() is called to fill the pending list of IRQs. This function will accomplish the following (for PCA_PCAL):
1- read the interrupt status register
2- read the latched inputs to clear the interrupt
3- apply filter for rising/falling edge selection on the input value
4- filter any bits that aren't related to the IRQ by applying a bitmap_and operation between: value calculated in step 3 and the value of ISR in step 1
5- return True with the pending bitmap if not empty
The problem here is that any interrupt that occurs between operation 1 and 2 will be lost even if latching is enabled !
Example:
* Interrupt occurs in pin 0 of port 0
1- Interrupt status register read (port0) = 0x10
** Interrupt occurs in pin 4 of port 0
2- input register read (port0) = 0x11 --> resets Interruptline
4- bitmap_and operation will remove the newly changed bit:0x11 & 0x10 = 0x10 and the returned pending bitmap will have only the pin0 interrupt !
The latching helps with very short interrupts to not be lost, but in the situation above it is not relevant.
Proposed solution:
In the 4th step apply bitmap_and between the filtered latched input and the bitmap of the unmasked interrupts. This will ensure the same outcome by letting only pins for which the IRQ is unmasked to pass but will not remove newly triggered interrupts.
This new unmasked interrupts bitmap can be filled in pca953x_irq_bus_sync_unlock() when an irq mask is getting set.
Also, by applying this, we can discard completely the read of the interrupt status register in step 1. Hence, the only I2C read that will be sent is the read of the Input register which minimizes the time to interrupt forwarding.
Signed-off-by: Abderrahim LAKBIR <abderrahim.lakbir@actia.fr>
--
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 272febc..886a287 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -215,6 +215,7 @@ struct pca953x_chip {
DECLARE_BITMAP(irq_stat, MAX_LINE);
DECLARE_BITMAP(irq_trig_raise, MAX_LINE);
DECLARE_BITMAP(irq_trig_fall, MAX_LINE);
+ DECLARE_BITMAP(unmasked_interrupts, MAX_LINE);
#endif
atomic_t wakeup_path;
@@ -763,6 +764,9 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
/* Enable latch on interrupt-enabled inputs */
pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
+ /* Store irq_mask for later use when checking pending IRQs */
+ bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio);
+
bitmap_complement(irq_mask, chip->irq_mask, gc->ngpio);
/* Unmask enabled interrupts */
@@ -842,11 +846,6 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin
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)
@@ -855,7 +854,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin
/* 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);
+ bitmap_and(pending, new_stat, chip->unmasked_interrupts, gc->ngpio);
return !bitmap_empty(pending, gc->ngpio);
}
On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: > > Hi All, > > I'm encountering exactly the same issue, and there is indeed a problem in the process of pca953x_irq_pending(). > > Mark has already explained the issue, but as apparently the discussion stopped, I've tried below to add some details to help better understand the issue. I've also a solution to propose. > > The issue: > In the current implementation, when an IRQ occurs, the function pca953x_irq_pending() is called to fill the pending list of IRQs. This function will accomplish the following (for PCA_PCAL): > 1- read the interrupt status register > 2- read the latched inputs to clear the interrupt > 3- apply filter for rising/falling edge selection on the input value > 4- filter any bits that aren't related to the IRQ by applying a bitmap_and operation between: value calculated in step 3 and the value of ISR in step 1 > 5- return True with the pending bitmap if not empty > > The problem here is that any interrupt that occurs between operation 1 and 2 will be lost even if latching is enabled ! This is clear. > Example: > * Interrupt occurs in pin 0 of port 0 > 1- Interrupt status register read (port0) = 0x10 > ** Interrupt occurs in pin 4 of port 0 > 2- input register read (port0) = 0x11 --> resets Interruptline > 4- bitmap_and operation will remove the newly changed bit:0x11 & 0x10 = 0x10 and the returned pending bitmap will have only the pin0 interrupt ! > > The latching helps with very short interrupts to not be lost, but in the situation above it is not relevant. > > Proposed solution: > In the 4th step apply bitmap_and between the filtered latched input and the bitmap of the unmasked interrupts. This will ensure the same outcome by letting only pins for which the IRQ is unmasked to pass but will not remove newly triggered interrupts. > This new unmasked interrupts bitmap can be filled in pca953x_irq_bus_sync_unlock() when an irq mask is getting set. > Also, by applying this, we can discard completely the read of the interrupt status register in step 1. Hence, the only I2C read that will be sent is the read of the Input register which minimizes the time to interrupt forwarding. ... > + /* Store irq_mask for later use when checking pending IRQs */ > + bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio); This solution has a flaw. Where is any code that clears this new bitmap? The code starts with 0 (obviously) and step by step it gets saturated to all-1s.
Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: > > .... > > + /* Store irq_mask for later use when checking pending IRQs */ > > + bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio); > > This solution has a flaw. Where is any code that clears this new > bitmap? The code starts with 0 (obviously) and step by step it gets > saturated to all-1s. > Yes indeed, and actually the new bitmap is not necessary at all because what we need does already exist which is chip->irq_mask (I noticed it just now!). chip->irq_mask is updated whenever a pin is masked or unmasked via pca953x_irq_mask() and pca953x_irq_unmask(). The solution should look like this: diff --git a/gpio-pca953x.c b/gpio-pca953x.c index 272febc..29e8c20 100644 --- a/gpio-pca953x.c +++ b/gpio-pca953x.c @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin 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) @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pendin /* 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); + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio); return !bitmap_empty(pending, gc->ngpio); } -- Best Regards Abderrahim LAKBIR
Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@gmail.com> a écrit : > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: > > > > .... > > > > + /* Store irq_mask for later use when checking pending IRQs */ > > > + bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio); > > > > This solution has a flaw. Where is any code that clears this new > > bitmap? The code starts with 0 (obviously) and step by step it gets > > saturated to all-1s. > > > > Yes indeed, and actually the new bitmap is not necessary at all > because what we need does already exist which is chip->irq_mask (I > noticed it just now!). > chip->irq_mask is updated whenever a pin is masked or unmasked via > pca953x_irq_mask() and pca953x_irq_unmask(). > > The solution should look like this: > > diff --git a/gpio-pca953x.c b/gpio-pca953x.c > index 272febc..29e8c20 100644 > --- a/gpio-pca953x.c > +++ b/gpio-pca953x.c > @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct > pca953x_chip *chip, unsigned long *pendin > 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) > @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct > pca953x_chip *chip, unsigned long *pendin > /* 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); > + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio); > > return !bitmap_empty(pending, gc->ngpio); > } > Hi Andy, do you have any other suggestions regarding the proposed fix ? -- Best Regards Abderrahim LAKBIR
On Tue, Jan 21, 2025 at 12:12 PM lakabd <lakabd.work@gmail.com> wrote: > Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@gmail.com> a écrit : > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko > > <andy.shevchenko@gmail.com> a écrit : > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: .... > > > > + /* Store irq_mask for later use when checking pending IRQs */ > > > > + bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio); > > > > > > This solution has a flaw. Where is any code that clears this new > > > bitmap? The code starts with 0 (obviously) and step by step it gets > > > saturated to all-1s. > > > > Yes indeed, and actually the new bitmap is not necessary at all > > because what we need does already exist which is chip->irq_mask (I > > noticed it just now!). > > chip->irq_mask is updated whenever a pin is masked or unmasked via > > pca953x_irq_mask() and pca953x_irq_unmask(). > > > > The solution should look like this: > > > > diff --git a/gpio-pca953x.c b/gpio-pca953x.c > > index 272febc..29e8c20 100644 > > --- a/gpio-pca953x.c > > +++ b/gpio-pca953x.c > > @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct > > pca953x_chip *chip, unsigned long *pendin > > 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) > > @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct > > pca953x_chip *chip, unsigned long *pendin > > /* 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); > > + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio); > > > > return !bitmap_empty(pending, gc->ngpio);> > > } > > Hi Andy, do you have any other suggestions regarding the proposed fix ? Currently I'm reading the datasheet to understand how the chip actually works. I'll come back to you soon. Nevertheless, I would like to hear from Mark if your patch fixes the issue. Preliminary I can say that it looks like we need slightly different and more complex logic there. P.S. Sorry for the delays.
On Mon, Jan 27, 2025 at 09:47:17AM +0200, Andy Shevchenko wrote: > On Tue, Jan 21, 2025 at 12:12 PM lakabd <lakabd.work@gmail.com> wrote: > > Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@gmail.com> a écrit : > > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko > > > <andy.shevchenko@gmail.com> a écrit : > > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: ... > > > > > + /* Store irq_mask for later use when checking pending IRQs */ > > > > > + bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio); > > > > > > > > This solution has a flaw. Where is any code that clears this new > > > > bitmap? The code starts with 0 (obviously) and step by step it gets > > > > saturated to all-1s. > > > > > > Yes indeed, and actually the new bitmap is not necessary at all > > > because what we need does already exist which is chip->irq_mask (I > > > noticed it just now!). > > > chip->irq_mask is updated whenever a pin is masked or unmasked via > > > pca953x_irq_mask() and pca953x_irq_unmask(). > > > > > > The solution should look like this: > > > > > > diff --git a/gpio-pca953x.c b/gpio-pca953x.c > > > index 272febc..29e8c20 100644 > > > --- a/gpio-pca953x.c > > > +++ b/gpio-pca953x.c > > > @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct > > > pca953x_chip *chip, unsigned long *pendin > > > 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) > > > @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct > > > pca953x_chip *chip, unsigned long *pendin > > > /* 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); > > > + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio); > > > > > > return !bitmap_empty(pending, gc->ngpio);> > > > > } > > > > Hi Andy, do you have any other suggestions regarding the proposed fix ? > > Currently I'm reading the datasheet to understand how the chip > actually works. I'll come back to you soon. > > Nevertheless, I would like to hear from Mark if your patch fixes the > issue. Preliminary I can say that it looks like we need slightly > different and more complex logic there. > > P.S. Sorry for the delays. Okay, I have read a lot the datasheet (PCAL9535A), and while Fig.12 shows an example of what happens in practice, neither the schematic on Fig.6 nor the description of the interrupt status register doesn't clarify this behaviour. The bottom line is that the latch helps only to keep the input data for longer and doesn't anyhow affect the input change on another pin while servicing the one that asserts the interrupt. Basically what they should have said is that the interrupt status register snapshot is made on the very first event and doesn't reflect the actual status anymore in case more input changes are coming. Hence there is no practical use of the interrupt status register. Seems to me a good candidate for errata. Does anybody inform NXP about this? Meanwhile looking into the code I'm wondering why we can't actually use just input port register data with the logic as for PCAL. Nonetheless this can be optimized later. I think Mark's patch is good enough as current fix.
On Thu, Jun 06, 2024 at 03:31:02PM +1200, Mark Tomlinson wrote: > 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. Sorry for such a delay. I think we are good to go with this and think about any optimisations later on. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> On Mon, Jan 27, 2025 at 09:47:17AM +0200, Andy Shevchenko wrote: > > On Tue, Jan 21, 2025 at 12:12 PM lakabd <lakabd.work@gmail.com> wrote: > > > Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@gmail.com> a écrit : > > > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko > > > > <andy.shevchenko@gmail.com> a écrit : > > > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> wrote: > > ... > > Okay, I have read a lot the datasheet (PCAL9535A), and while Fig.12 shows > an example of what happens in practice, neither the schematic on Fig.6 nor > the description of the interrupt status register doesn't clarify this > behaviour. The bottom line is that the latch helps only to keep the input data > for longer and doesn't anyhow affect the input change on another pin while > servicing the one that asserts the interrupt. Basically what they should have > said is that the interrupt status register snapshot is made on the very first > event and doesn't reflect the actual status anymore in case more input changes > are coming. Hence there is no practical use of the interrupt status register. Indeed, I came to the same conclusion i.e., if reading the interrupt status register doesn't reset the interrupt line it is not practical and can be considered a HW design flaw. > > Seems to me a good candidate for errata. Does anybody inform NXP about this? > > Meanwhile looking into the code I'm wondering why we can't actually use > just input port register data with the logic as for PCAL. Nonetheless this > can be optimized later. I think Mark's patch is good enough as current fix. > If we accept Mark's patch there will be no difference between PCA_PCAL and regular chips in IRQ handling. Looking at pca953x_irq_pending() the process for non-PCA_PCAL is quite slower; there is one I2C read in addition, plus multiple bitmap operations. I think that the solution I proposed at least helps in keeping the leverage for PCA_PCAL chips. -- Best Regards, Abderrahim LAKBIR
On Mon, Jan 27, 2025 at 6:45 PM lakabd <lakabd.work@gmail.com> wrote: > > On Mon, Jan 27, 2025 at 09:47:17AM +0200, Andy Shevchenko wrote: ... > > Meanwhile looking into the code I'm wondering why we can't actually use > > just input port register data with the logic as for PCAL. Nonetheless this > > can be optimized later. I think Mark's patch is good enough as current fix. > > If we accept Mark's patch there will be no difference between PCA_PCAL > and regular chips in IRQ handling. > Looking at pca953x_irq_pending() the process for non-PCA_PCAL is quite > slower; there is one I2C read in addition, plus multiple bitmap > operations. I think that the solution I proposed at least helps in > keeping the leverage for PCA_PCAL chips. As I said, we can do optimisations later on. The non-PCAL code is widely tested, so I prefer to have a slower but tested approach. On top of that bitmap operations for the chips up to 32 lines are just operations on one register, which are quite fast even on slow CPUs (like Intel Quark), in comparison to I²C transactions.
On Mon, 2025-01-27 at 09:47 +0200, Andy Shevchenko wrote: > On Tue, Jan 21, 2025 at 12:12 PM lakabd <lakabd.work@gmail.com> wrote: > > Le mar. 14 janv. 2025 à 16:44, work work <lakabd.work@gmail.com> a > > écrit : > > > Le mar. 14 janv. 2025 à 10:37, Andy Shevchenko > > > <andy.shevchenko@gmail.com> a écrit : > > > > On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@gmail.com> > > > > wrote: > > .... > > > > > > + /* Store irq_mask for later use > > > > > when checking pending IRQs */ > > > > > + bitmap_or(chip- > > > > > >unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, > > > > > gc->ngpio); > > > > > > > > This solution has a flaw. Where is any code that clears this new > > > > bitmap? The code starts with 0 (obviously) and step by step it gets > > > > saturated to all-1s. > > > > > > Yes indeed, and actually the new bitmap is not necessary at all > > > because what we need does already exist which is chip->irq_mask (I > > > noticed it just now!). > > > chip->irq_mask is updated whenever a pin is masked or unmasked via > > > pca953x_irq_mask() and pca953x_irq_unmask(). > > > > > > The solution should look like this: > > > > > > diff --git a/gpio-pca953x.c b/gpio-pca953x.c > > > index 272febc..29e8c20 100644 > > > --- a/gpio-pca953x.c > > > +++ b/gpio-pca953x.c > > > @@ -842,11 +842,6 @@ static bool pca953x_irq_pending(struct > > > pca953x_chip *chip, unsigned long *pendin > > > 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) > > > @@ -855,7 +850,7 @@ static bool pca953x_irq_pending(struct > > > pca953x_chip *chip, unsigned long *pendin > > > /* 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); > > > + bitmap_and(pending, new_stat, chip->irq_mask, gc->ngpio); > > > > > > return !bitmap_empty(pending, gc->ngpio);> > > > > } > > > > Hi Andy, do you have any other suggestions regarding the proposed fix ? > > Currently I'm reading the datasheet to understand how the chip > actually works. I'll come back to you soon. > > Nevertheless, I would like to hear from Mark if your patch fixes the > issue. Preliminary I can say that it looks like we need slightly > different and more complex logic there. This patch will not miss interrupts, but I believe will trigger false interrupts, as an input will behave more like a level triggered interrupt if other inputs are triggering interrupts on the device. I would prefer that we just remove the special PCA_PCAL block in this function, which was my simple solution.
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> On Thu, 06 Jun 2024 15:31:02 +1200, Mark Tomlinson wrote: > 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. > > [...] Queued for fixes. Thanks Andy for taking the time to review it. [1/1] gpio: pca953x: Improve interrupt support commit: d6179f6c6204f9932aed3a7a2100b4a295dfed9d Best regards,
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;
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 <mark.tomlinson@alliedtelesis.co.nz> --- drivers/gpio/gpio-pca953x.c | 19 ------------------- 1 file changed, 19 deletions(-)