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
