RE: [PATCH v2 6/6] Input: snvs_pwrkey - only IRQ_HANDLED for our own events

[Robin Gong <yibin.gong@nxp.com>]

On 2020/02/27 André Draszik <git@andred.net> wrote:
> On Wed, 2020-02-26 at 01:15 +0000, Robin Gong wrote:
> > On 2020/02/26 André Draszik <git@andred.net> wrote:
> > > The snvs_pwrkey shares the SNVS LPSR status register with the snvs_rtc.
> > >
> > > This driver here should only return IRQ_HANDLED if the status
> > > register indicates that the event we're handling in the irq handler
> > > was genuinely intended for this driver. Otheriwse the interrupt
> > > subsystem will assume the interrupt was handled successfully even though
> it wasn't at all.
> > >
> > > Signed-off-by: André Draszik <git@andred.net>
> > > Cc: "Horia Geantă" <horia.geanta@nxp.com>
> > > Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> > > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Anson Huang <Anson.Huang@nxp.com>
> > > Cc: Robin Gong <yibin.gong@nxp.com>
> > > Cc: linux-crypto@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-input@vger.kernel.org
> > >
> > > ---
> > > v2:
> > > * no changes
> > > ---
> > >  drivers/input/keyboard/snvs_pwrkey.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/snvs_pwrkey.c
> > > b/drivers/input/keyboard/snvs_pwrkey.c
> > > index 382d2ae82c9b..980867886b34 100644
> > > --- a/drivers/input/keyboard/snvs_pwrkey.c
> > > +++ b/drivers/input/keyboard/snvs_pwrkey.c
> > > @@ -82,7 +82,9 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int
> > > irq, void
> > > *dev_id)
> > >  	clk_enable(pdata->clk);
> > >
> > >  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> > > -	if (lp_status & SNVS_LPSR_SPO) {
> > > +	lp_status &= SNVS_LPSR_SPO;
> > > +
> > > +	if (lp_status) {
> > >  		if (pdata->minor_rev == 0) {
> > >  			/*
> > >  			 * The first generation i.MX[6|7] SoCs only send an @@
> -98,14
> > > +100,14 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq,
> > > +void
> > > *dev_id)
> > >  			mod_timer(&pdata->check_timer,
> > >  			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> > >  		}
> > > -	}
> > >
> > > -	/* clear SPO status */
> > > -	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> > > +		/* clear SPO status */
> > > +		regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> > But irq storm will come in once there is other interrupt triggered as
> > unexpected, although I never met it before. Could we drop this patch now?
> Others are ok for me.
> 
> I don't have strong feelings about this patch, but this bit merely changes
> behaviour to clear SP0 if SP0 was in fact != 0 in the first place, whereas before
> SP0 was always cleared, even if it was == 0 anyway. Seems more logical in my
> eyes.
Seems we care something never happen:) I'm okay if you really want to do that.
> 
> 
> > Reviewed-by: Robin Gong <yibin.gong@nxp>
> > > +	}
> > >
> > >  	clk_disable(pdata->clk);
> > >
> > > -	return IRQ_HANDLED;
> > > +	return lp_status ? IRQ_HANDLED : IRQ_NONE;
> 
> If you're talking about this part, the rtc-snvs driver does the same in its
> interrupt handler.
> In other words, this driver here could prevent the rtc-snvs driver from seeing its
> events
But rtc driver has another interrupt number, and snvs_pwrkey driver just touch SPO bit
as below:
regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
So I don't think it could impact rtc-snvs driver. But you remind me rtc-snvs driver may clear SPO status, because lpsr is read from SNVS_LPSR which may SPO has already been set.
regmap_write(data->regmap, data->offset + SNVS_LPSR, lpsr);
I'll send a patch for fix that potential issue. Besides, if you really want to follow rtc driver please initialize lp_status with 0.  
> 
> 
> 
> Cheers,
> Andre'
> 
> 
> > >  }
> > >
> > >  static void imx_snvs_pwrkey_act(void *pdata)
> > > --
> > > 2.23.0.rc1