Re: [PATCH] gpio: pca953x: Improve interrupt support

[lakabd <lakabd.work@gmail.com>]

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);
                }