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

[Chunyan Zhang]

[reply behalf on Taipin]

Hi Baolin,

On Sat, 1 Aug 2020 at 07:59, Baolin Wang <baolin.wang7@xxxxxxxxx> wrote:
>
> On Wed, Jul 29, 2020 at 5:35 PM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
> >
> > From: Taiping Lai <taiping.lai@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > ---
> >  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