diff mbox series

gpio: omap: Fix double trigger for level interrupts

Message ID 20240326145439.1293412-1-msp@baylibre.com
State New
Headers show
Series gpio: omap: Fix double trigger for level interrupts | expand

Commit Message

Markus Schneider-Pargmann March 26, 2024, 2:50 p.m. UTC
Set gpio trigger before clearing the irq status.

This patch was originally proposed by Grygorii Strashko.

Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Reported-by: Markus Mirevik <markus.mirevik@dpsolutions.se>
Closes: https://lore.kernel.org/all/20220122235959.GA10737@sol/T/
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
Hi everyone,

this patch helped me on the beagleboneblack to remove the mentioned
double trigger of level interrupts. This diff was proposed by Grygorii
in the thread linked in the commit message. I am not sure why this never
made it into the kernel, that's why I sending this patch. I did not
create the diff just made a patch out of it, I don't care about being
the author but I would be happy if this would get merged or some other
solution to the problem.

Thanks!

Best
Markus

 drivers/gpio/gpio-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Dan Carpenter March 26, 2024, 3:29 p.m. UTC | #1
In the bug report email thread Markus Mirevik said "The interrupt
associated with the GPIO module still fires twice" so while this patch
is an improvement, it might not be a complete solution?

> This patch was originally proposed by Grygorii Strashko.

The way to give authorship credit is you make the first line of your
patch:

From: Grygorii Strashko <grygorii.strashko@ti.com>

When the patch is applied then git will assign authorship credit but
remove that line from the git log.

On Tue, Mar 26, 2024 at 03:50:14PM +0100, Markus Schneider-Pargmann wrote:
> Set gpio trigger before clearing the irq status.
> 

This commit message needs some work.  When you're reviewing on email,
it's kind of common to read the commit message without reading the
subject.  See how the patch looks like on lore:

https://lore.kernel.org/linux-gpio/20240326145439.1293412-1-msp@baylibre.com/T/#u

The subject is up on the first line, but it's mixed in with the headers
so it's easy to skip.  Go ahead a restate the subject but in different
words.

But also copy and paste more of the problem from the bug report.  To me
if I were a user the important bit is that the bug ends up hogging the
CPU.

"The problem is that the interrupt handler was is run twice for each
frame.  It hogs a lot of CPU time.  Fix this by setting the GPIO trigger
before clearing the IRQ status."

regards,
dan carpenter
Markus Schneider-Pargmann March 26, 2024, 4:13 p.m. UTC | #2
Hi Dan,

On Tue, Mar 26, 2024 at 06:29:45PM +0300, Dan Carpenter wrote:
> In the bug report email thread Markus Mirevik said "The interrupt
> associated with the GPIO module still fires twice" so while this patch
> is an improvement, it might not be a complete solution?

Yes, that is possible. I actually only had the level interrupt
double trigger bug which is fixed by this change.

> 
> > This patch was originally proposed by Grygorii Strashko.
> 
> The way to give authorship credit is you make the first line of your
> patch:
> 
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> When the patch is applied then git will assign authorship credit but
> remove that line from the git log.

Thanks, yes I am aware, though I didn't want to add that author tag
without someone agreeing with that. As it wasn't a formal patch, there
was no From: already present or a SoB by Grygorii. Also there was no
reaction on a ping from me on the original thread. That's why I opted
for the comment in the commit, the explanation and as I said in the
letter part I don't care about author, just that it gets upstream.

> 
> On Tue, Mar 26, 2024 at 03:50:14PM +0100, Markus Schneider-Pargmann wrote:
> > Set gpio trigger before clearing the irq status.
> > 
> 
> This commit message needs some work.  When you're reviewing on email,
> it's kind of common to read the commit message without reading the
> subject.  See how the patch looks like on lore:
> 
> https://lore.kernel.org/linux-gpio/20240326145439.1293412-1-msp@baylibre.com/T/#u
> 
> The subject is up on the first line, but it's mixed in with the headers
> so it's easy to skip.  Go ahead a restate the subject but in different
> words.
> 
> But also copy and paste more of the problem from the bug report.  To me
> if I were a user the important bit is that the bug ends up hogging the
> CPU.
> 
> "The problem is that the interrupt handler was is run twice for each
> frame.  It hogs a lot of CPU time.  Fix this by setting the GPIO trigger
> before clearing the IRQ status."

Yes, thanks, I will work on that commit message.

Best
Markus
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 76d5d87e9681..74b8fe2995e1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -696,6 +696,9 @@  static void omap_gpio_unmask_irq(struct irq_data *d)
 	raw_spin_lock_irqsave(&bank->lock, flags);
 	omap_set_gpio_irqenable(bank, offset, 1);
 
+	if (trigger)
+		omap_set_gpio_triggering(bank, offset, trigger);
+
 	/*
 	 * For level-triggered GPIOs, clearing must be done after the source
 	 * is cleared, thus after the handler has run. OMAP4 needs this done
@@ -705,9 +708,6 @@  static void omap_gpio_unmask_irq(struct irq_data *d)
 	    trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
 		omap_clear_gpio_irqstatus(bank, offset);
 
-	if (trigger)
-		omap_set_gpio_triggering(bank, offset, trigger);
-
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 }