Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Oct 2022 16:11:49 +0200
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
Message-ID: <20221013141149.zrcdtcfragerxdyw@soft-dev3-1>
References: <20220909145942.844102-1-horatiu.vultur@microchip.com>
 <20220920120642.690340-1-michael@walle.cc>
 <20220920193033.bpmyt6pdob5b45id@soft-dev3-1.localhost>
 <683fc322fddebe39a93a46aefcd5e2dd@walle.cc>
 <20221007094938.qqf7exuthvz5gkdq@soft-dev3-1.localhost>
 <994b3dfd8d5d2c0c11c2bf6f299564c1@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <994b3dfd8d5d2c0c11c2bf6f299564c1@walle.cc>
Precedence: bulk

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