Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
From: Chunyan Zhang <zhang.lyra@gmail.com>
To: Baolin Wang <baolin.wang7@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-gpio@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Orson Zhai <orsonzhai@gmail.com>,
	Chunyan Zhang <chunyan.zhang@unisoc.com>,
	Taiping Lai <taiping.lai@unisoc.com>
Subject: Re: [PATCH] gpio: sprd: Clear interrupt when setting the type as edge
Date: Wed, 19 Aug 2020 19:35:06 +0800
Message-ID: <CAAfSe-vEW5CDJnae3AHwciAsinvMvuUAdv8TiCkxEjNga8U=SQ@mail.gmail.com> (raw)
In-Reply-To: <CADBw62p=aB3EZYkMm44Zx4Krww21SS9vCsMnPOq0pn2DgA8VkQ@mail.gmail.com>

[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

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29  9:34 Chunyan Zhang
2020-07-31 23:58 ` Baolin Wang
2020-08-19 11:35   ` Chunyan Zhang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAfSe-vEW5CDJnae3AHwciAsinvMvuUAdv8TiCkxEjNga8U=SQ@mail.gmail.com' \
    --to=zhang.lyra@gmail.com \
    --cc=baolin.wang7@gmail.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=chunyan.zhang@unisoc.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=orsonzhai@gmail.com \
    --cc=taiping.lai@unisoc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git