Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Michael Walle <michael@walle.cc>
Cc: <UNGLinuxDriver@microchip.com>, <andy.shevchenko@gmail.com>,
	<linus.walleij@linaro.org>, <linux-gpio@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller
Date: Thu, 13 Oct 2022 16:11:49 +0200	[thread overview]
Message-ID: <20221013141149.zrcdtcfragerxdyw@soft-dev3-1> (raw)
In-Reply-To: <994b3dfd8d5d2c0c11c2bf6f299564c1@walle.cc>

The 10/13/2022 09:30, Michael Walle wrote:

Hi Michael,

> > We lose the interrupt here, as the HW will not generate another one
> > but at later point we read again the line status. And if the line is
> > active then we kick again the interrupt handler again.
> 
> Ahh, thanks for explaining. That also explains the read below.
> 
> Will you send a proper patch?

No worries. Yes, I will do that.

> 
> -michael
> 
> > 
> > > 
> > > > +
> > > >       /* Enable the interrupt now */
> > > >       gpiochip_enable_irq(chip, gpio);
> > > >       regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > > >                          bit, bit);
> > > >
> > > >       /*
> > > > -      * In case the interrupt line is still active and the interrupt
> > > > -      * controller has not seen any changes in the interrupt line, then it
> > > > -      * means that there happen another interrupt while the line was
> > > > active.
> > > > +      * In case the interrupt line is still active then it means that
> > > > +      * there happen another interrupt while the line was active.
> > > >        * So we missed that one, so we need to kick the interrupt again
> > > >        * handler.
> > > >        */
> > > > -     if (active && !ack) {
> > > > +     regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > > > +     if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > > > +           (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > > > +             active = true;
> > > 
> > > Why do you read the line state twice? What happens if the line state
> > > changes right after you've read it?
> > 
> > Here we need to read again the status because we might have clear the
> > ack of interrupt.
> > If the line becomes active right after this read, then the HW will
> > generate another interrupt as the interrupt is enabled and ack is
> > cleared.
> > 
> > > 
> > > > +
> > > > +     if (active) {
> > > >               struct ocelot_irq_work *work;
> > > >
> > > >               work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > > 
> > > So yes, maybe the trade-off that there will be two interrupts are
> > > better than this additional patch. But it should be documented
> > > somewhere, even if it's just a comment in this driver.
> > > 
> > > -michael

-- 
/Horatiu