RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system

[Anson Huang]

Hi, Dmitry
	Any feedback for this patch?

Thanks,
Anson

> -----Original Message-----
> From: Anson Huang
> Sent: Tuesday, May 21, 2019 2:36 PM
> To: dmitry.torokhov@xxxxxxxxx
> Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; linux-input@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-
> imx <linux-imx@xxxxxxx>
> Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
> 
> Hi, Dmitry
> 
> > -----Original Message-----
> > From: dmitry.torokhov@xxxxxxxxx [mailto:dmitry.torokhov@xxxxxxxxx]
> > Sent: Tuesday, May 21, 2019 1:31 PM
> > To: Anson Huang <anson.huang@xxxxxxx>
> > Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > linux-input@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; dl-linux- imx <linux-imx@xxxxxxx>
> > Subject: Re: [RESEND] input: keyboard: imx: make sure keyboard can
> > always wake up system
> >
> > Hi Anson,
> > On Thu, Apr 04, 2019 at 01:40:16AM +0000, Anson Huang wrote:
> > > There are several scenarios that keyboard can NOT wake up system
> > > from suspend, e.g., if a keyboard is depressed between system device
> > > suspend phase and device noirq suspend phase, the keyboard ISR will
> > > be called and both keyboard depress and release interrupts will be
> > > disabled, then keyboard will no longer be able to wake up system.
> > > Another scenario would be, if a keyboard is kept depressed, and then
> > > system goes into suspend, the expected behavior would be when
> > > keyboard is released, system will be waked up, but current
> > > implementation can NOT achieve that, because both depress and
> > > release interrupts are disabled in ISR, and the event check is still in
> progress.
> > >
> > > To fix these issues, need to make sure keyboard's depress or release
> > > interrupt is enabled after noirq device suspend phase, this patch
> > > moves the suspend/resume callback to noirq suspend/resume phase,
> and
> > > enable the corresponding interrupt according to current keyboard status.
> >
> > I believe it is possible for IRQ to be disabled and still  being
> > enabled as wakeup source. What happens if you call disable_irq()
> > before disabling the clock?
> 
> Doing below does NOT fix the scenario/issue 100%, if the keypad's IRQ
> arrived during suspend phase but before disabling its IRQ in its suspend
> callback, then issue is still there, as the issue is that when system suspend,
> keypad's irq arrived during suspend and noirq suspend phase, then keypad's
> hardware interrupt detection will be disabled in the ISR handler, and the
> timer event setup by ISR handler is NOT fired,
> imx_keypad_check_for_events() is NOT executed and hardware keypad's
> depress/release interrupt is NOT re-enabled yet, so it can NOT wake up
> system anymore...
> 
> So I think the solid fix is to make sure keypad can generate IRQ (either
> depress or release) at any time during system suspend flow.
> 
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -533,6 +533,8 @@ static int __maybe_unused imx_kbd_suspend(struct
> device *dev)
>         /* imx kbd can wake up system even clock is disabled */
>         mutex_lock(&input_dev->mutex);
> 
> +       disable_irq(kbd->irq);
> +
>         if (input_dev->users)
>                 clk_disable_unprepare(kbd->clk);
> 
> 
> @@ -562,6 +569,8 @@ static int __maybe_unused imx_kbd_resume(struct
> device *dev)
>                         goto err_clk;
>         }
> 
> +       enable_irq(kbd->irq);
> +
>  err_clk:
> 
> Anson.
> 
> >
> > Thanks.
> >
> > --
> > Dmitry