2020-07-29 09:35:42

by Chunyan Zhang

Subject: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge

From: Taiping Lai <[email protected]>

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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
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

2020-07-31 23:59:49

by Baolin Wang

Subject: Re: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge

On Wed, Jul 29, 2020 at 5:35 PM Chunyan Zhang <[email protected]> wrote:
>
> From: Taiping Lai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> 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

2020-08-19 11:36:38

by Chunyan Zhang

Subject: Re: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge

[reply behalf on Taipin]

Hi Baolin,

On Sat, 1 Aug 2020 at 07:59, Baolin Wang <[email protected]> wrote:
>
> On Wed, Jul 29, 2020 at 5:35 PM Chunyan Zhang <[email protected]> wrote:
> >
> > From: Taiping Lai <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > 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