[PATCH] gpio: pca953x: Improve interrupt support

[PATCH] gpio: pca953x: Improve interrupt support

[Mark Tomlinson]

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(-)

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

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

[Andy Shevchenko]

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

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

[Mark Tomlinson]

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,

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

[lakabd]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[work work]

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

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

[lakabd]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[lakabd]

> 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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Tomlinson]

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.

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

[Andy Shevchenko]

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>

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

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,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>