[2/2] gpio/rockchip: Toggle edge trigger mode after acking

Otherwise the trigger mode might be out-of-sync when a level change
occurred in between.

Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:

> The thing is, we are currently toggling the trigger mode to make sure it
> matches the current GPIO level (e.g. level low -> rising edge mode),
> than ack it in gpio IRQ handler.

Yes this is an old trick, I don't know if I invented it again for Linux in
commit cc890cd78acd7ab03442907d354b6af34e973cb3
in 2011, surely the trick must be well known.

Back then I did it like this:

+       val = readl(U300_PIN_REG(offset, icr));
+       /* Set mode depending on state */
+       if (u300_gpio_get(&gpio->chip, offset)) {
+               /* High now, let's trigger on falling edge next then */
+               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
+                       offset);
+       } else {
+               /* Low now, let's trigger on rising edge next then */
+               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
+                       offset);
+       }

Notice that I read the current level of the raw input to decide what the next
trigger should be. The Rockchip driver does not do this, maybe that works
better?

Yours,
Linus Walleij

Hi Linus,

On Fri, Aug 26, 2022 at 10:54:08AM +0200, Linus Walleij wrote:
> On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:
> 
> > The thing is, we are currently toggling the trigger mode to make sure it
> > matches the current GPIO level (e.g. level low -> rising edge mode),
> > than ack it in gpio IRQ handler.
> 
> Yes this is an old trick, I don't know if I invented it again for Linux in
> commit cc890cd78acd7ab03442907d354b6af34e973cb3
> in 2011, surely the trick must be well known.
> 
> Back then I did it like this:
> 
> +       val = readl(U300_PIN_REG(offset, icr));
> +       /* Set mode depending on state */
> +       if (u300_gpio_get(&gpio->chip, offset)) {
> +               /* High now, let's trigger on falling edge next then */
> +               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));

This looks pretty close to the right solution, but you still have a
race: a falling edge may happen right between the get() and the
writel(), and you'll miss it. You're in a little better shape than any
of the rockchip proposals so far, because at least you do the ACK before
this step (Rockchip doesn't). But you do still have a narrow race
window.

I think you need to iterate on this, and check the status again
afterward, looping until you are sure you either got it right, or are
leaving an edge still latched.

And at that point, I think you have the solution I recommended in my
other email :)

> +               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
> +                       offset);
> +       } else {
> +               /* Low now, let's trigger on rising edge next then */
> +               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
> +               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
> +                       offset);
> +       }
> 
> Notice that I read the current level of the raw input to decide what the next
> trigger should be. The Rockchip driver does not do this, maybe that works
> better?

Actually, it does; that's what the 'ext_port' register is doing. The
question is when exactly to do this toggling -- before or after the ACK,
and with what sort of ordering to ensure you don't end up at the wrong
polarity when exiting.

Brian