* [PATCH] gpio: sprd: Clear interrupt when setting the type as edge
@ 2020-07-29 9:34 Chunyan Zhang
2020-07-31 23:58 ` Baolin Wang
0 siblings, 1 reply; 3+ messages in thread
From: Chunyan Zhang @ 2020-07-29 9:34 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski
Cc: linux-gpio, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang,
Chunyan Zhang, Taiping Lai
From: Taiping Lai <taiping.lai@unisoc.com>
The raw interrupt status of GPIO maybe set before the interrupt is enabled,
which would trigger the interrupt event once enabled it from user side.
This is the case for edge interrupts only. Adding a clear operation when
setting interrupt type can avoid that.
Fixes: 9a3821c2bb47 ("gpio: Add GPIO driver for Spreadtrum SC9860 platform")
Signed-off-by: Taiping Lai <taiping.lai@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
drivers/gpio/gpio-sprd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
index d7314d39ab65..36ea8a3bd451 100644
--- a/drivers/gpio/gpio-sprd.c
+++ b/drivers/gpio/gpio-sprd.c
@@ -149,17 +149,20 @@ static int sprd_gpio_irq_set_type(struct irq_data *data,
sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
+ sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
irq_set_handler_locked(data, handle_edge_irq);
break;
case IRQ_TYPE_EDGE_FALLING:
sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
+ sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
irq_set_handler_locked(data, handle_edge_irq);
break;
case IRQ_TYPE_EDGE_BOTH:
sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 1);
+ sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
irq_set_handler_locked(data, handle_edge_irq);
break;
case IRQ_TYPE_LEVEL_HIGH:
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge
2020-07-29 9:34 [PATCH] gpio: sprd: Clear interrupt when setting the type as edge Chunyan Zhang
@ 2020-07-31 23:58 ` Baolin Wang
2020-08-19 11:35 ` Chunyan Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Baolin Wang @ 2020-07-31 23:58 UTC (permalink / raw)
To: Chunyan Zhang
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, LKML, Orson Zhai,
Chunyan Zhang, Taiping Lai
On Wed, Jul 29, 2020 at 5:35 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> From: Taiping Lai <taiping.lai@unisoc.com>
>
> The raw interrupt status of GPIO maybe set before the interrupt is enabled,
> which would trigger the interrupt event once enabled it from user side.
> This is the case for edge interrupts only. Adding a clear operation when
> setting interrupt type can avoid that.
>
> Fixes: 9a3821c2bb47 ("gpio: Add GPIO driver for Spreadtrum SC9860 platform")
> Signed-off-by: Taiping Lai <taiping.lai@unisoc.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
> drivers/gpio/gpio-sprd.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
> index d7314d39ab65..36ea8a3bd451 100644
> --- a/drivers/gpio/gpio-sprd.c
> +++ b/drivers/gpio/gpio-sprd.c
> @@ -149,17 +149,20 @@ static int sprd_gpio_irq_set_type(struct irq_data *data,
> sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
> sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
> + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
I think you should move this abonormal interrupt clearing operation to
sprd_gpio_request(), when users request a GPIO.
> irq_set_handler_locked(data, handle_edge_irq);
> break;
> case IRQ_TYPE_EDGE_FALLING:
> sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
> sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
> + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
> irq_set_handler_locked(data, handle_edge_irq);
> break;
> case IRQ_TYPE_EDGE_BOTH:
> sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 1);
> + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
> irq_set_handler_locked(data, handle_edge_irq);
> break;
> case IRQ_TYPE_LEVEL_HIGH:
> --
> 2.20.1
>
--
Baolin Wang
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge
2020-07-31 23:58 ` Baolin Wang
@ 2020-08-19 11:35 ` Chunyan Zhang
0 siblings, 0 replies; 3+ messages in thread
From: Chunyan Zhang @ 2020-08-19 11:35 UTC (permalink / raw)
To: Baolin Wang
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, LKML, Orson Zhai,
Chunyan Zhang, Taiping Lai
[reply behalf on Taipin]
Hi Baolin,
On Sat, 1 Aug 2020 at 07:59, Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> On Wed, Jul 29, 2020 at 5:35 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> >
> > From: Taiping Lai <taiping.lai@unisoc.com>
> >
> > The raw interrupt status of GPIO maybe set before the interrupt is enabled,
> > which would trigger the interrupt event once enabled it from user side.
> > This is the case for edge interrupts only. Adding a clear operation when
> > setting interrupt type can avoid that.
> >
> > Fixes: 9a3821c2bb47 ("gpio: Add GPIO driver for Spreadtrum SC9860 platform")
> > Signed-off-by: Taiping Lai <taiping.lai@unisoc.com>
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> > drivers/gpio/gpio-sprd.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
> > index d7314d39ab65..36ea8a3bd451 100644
> > --- a/drivers/gpio/gpio-sprd.c
> > +++ b/drivers/gpio/gpio-sprd.c
> > @@ -149,17 +149,20 @@ static int sprd_gpio_irq_set_type(struct irq_data *data,
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
> > + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
>
> I think you should move this abonormal interrupt clearing operation to
> sprd_gpio_request(), when users request a GPIO.
We have a few considerations:
1) Like described in the commit message, the problem this patch solves
is for edge interrupt only; The interrupt requested by user is
IRQ_TYPE_LEVEL_HIGH as default, so clearing interrupt when request is
useless.
2) We can set interrupt type to edge when request, and following up
with clearing it, but the problem is still there once users set the
interrupt type to level trggier.
3) We can add a clear operation after each time of setting interrupt
enable bit, but it is redundant for level trigger interrupt.
Therefore, adding a clear operation when setting interrupt type seems
the best solutions which I can think out so far.
>
> > irq_set_handler_locked(data, handle_edge_irq);
> > break;
> > case IRQ_TYPE_EDGE_FALLING:
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
> > + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
> > irq_set_handler_locked(data, handle_edge_irq);
> > break;
> > case IRQ_TYPE_EDGE_BOTH:
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
> > sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 1);
> > + sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
> > irq_set_handler_locked(data, handle_edge_irq);
> > break;
> > case IRQ_TYPE_LEVEL_HIGH:
> > --
> > 2.20.1
> >
>
>
> --
> Baolin Wang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-19 11:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 9:34 [PATCH] gpio: sprd: Clear interrupt when setting the type as edge Chunyan Zhang
2020-07-31 23:58 ` Baolin Wang
2020-08-19 11:35 ` Chunyan Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).