linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
@ 2019-04-04  1:40 Anson Huang
  2019-04-25  1:49 ` Anson Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Anson Huang @ 2019-04-04  1:40 UTC (permalink / raw)
  To: dmitry.torokhov, shawnguo, s.hauer, kernel, festevam,
	linux-input, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index cf08f4a..97500a2 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -524,11 +524,12 @@ static int imx_keypad_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int __maybe_unused imx_kbd_suspend(struct device *dev)
+static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_keypad *kbd = platform_get_drvdata(pdev);
 	struct input_dev *input_dev = kbd->input_dev;
+	unsigned short reg_val = readw(kbd->mmio_base + KPSR);
 
 	/* imx kbd can wake up system even clock is disabled */
 	mutex_lock(&input_dev->mutex);
@@ -538,13 +539,20 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
 
 	mutex_unlock(&input_dev->mutex);
 
-	if (device_may_wakeup(&pdev->dev))
+	if (device_may_wakeup(&pdev->dev)) {
+		if (reg_val & KBD_STAT_KPKD)
+			reg_val |= KBD_STAT_KRIE;
+		if (reg_val & KBD_STAT_KPKR)
+			reg_val |= KBD_STAT_KDIE;
+		writew(reg_val, kbd->mmio_base + KPSR);
+
 		enable_irq_wake(kbd->irq);
+	}
 
 	return 0;
 }
 
-static int __maybe_unused imx_kbd_resume(struct device *dev)
+static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_keypad *kbd = platform_get_drvdata(pdev);
@@ -568,7 +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
 	return ret;
 }
 
-static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend, imx_kbd_resume);
+static const struct dev_pm_ops imx_kbd_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend, imx_kbd_noirq_resume)
+};
 
 static struct platform_driver imx_keypad_driver = {
 	.driver		= {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-04-04  1:40 [RESEND] input: keyboard: imx: make sure keyboard can always wake up system Anson Huang
@ 2019-04-25  1:49 ` Anson Huang
  2019-05-09  1:41   ` Anson Huang
  2019-05-21  5:30 ` dmitry.torokhov
  2019-06-12  0:51 ` dmitry.torokhov
  2 siblings, 1 reply; 7+ messages in thread
From: Anson Huang @ 2019-04-25  1:49 UTC (permalink / raw)
  To: dmitry.torokhov, shawnguo, s.hauer, kernel, festevam,
	linux-input, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Gentle ping...

> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, April 4, 2019 9:40 AM
> To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
> 
> 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.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c
> b/drivers/input/keyboard/imx_keypad.c
> index cf08f4a..97500a2 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct
> platform_device *pdev)
>  	return 0;
>  }
> 
> -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct imx_keypad *kbd = platform_get_drvdata(pdev);
>  	struct input_dev *input_dev = kbd->input_dev;
> +	unsigned short reg_val = readw(kbd->mmio_base + KPSR);
> 
>  	/* imx kbd can wake up system even clock is disabled */
>  	mutex_lock(&input_dev->mutex);
> @@ -538,13 +539,20 @@ static int __maybe_unused
> imx_kbd_suspend(struct device *dev)
> 
>  	mutex_unlock(&input_dev->mutex);
> 
> -	if (device_may_wakeup(&pdev->dev))
> +	if (device_may_wakeup(&pdev->dev)) {
> +		if (reg_val & KBD_STAT_KPKD)
> +			reg_val |= KBD_STAT_KRIE;
> +		if (reg_val & KBD_STAT_KPKR)
> +			reg_val |= KBD_STAT_KDIE;
> +		writew(reg_val, kbd->mmio_base + KPSR);
> +
>  		enable_irq_wake(kbd->irq);
> +	}
> 
>  	return 0;
>  }
> 
> -static int __maybe_unused imx_kbd_resume(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct imx_keypad *kbd = platform_get_drvdata(pdev); @@ -568,7
> +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
>  	return ret;
>  }
> 
> -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend,
> imx_kbd_resume);
> +static const struct dev_pm_ops imx_kbd_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend,
> +imx_kbd_noirq_resume) };
> 
>  static struct platform_driver imx_keypad_driver = {
>  	.driver		= {
> --
> 2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-04-25  1:49 ` Anson Huang
@ 2019-05-09  1:41   ` Anson Huang
  0 siblings, 0 replies; 7+ messages in thread
From: Anson Huang @ 2019-05-09  1:41 UTC (permalink / raw)
  To: dmitry.torokhov, shawnguo, s.hauer, kernel, festevam,
	linux-input, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Ping...

> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, April 25, 2019 9:50 AM
> To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
> 
> Gentle ping...
> 
> > -----Original Message-----
> > From: Anson Huang
> > Sent: Thursday, April 4, 2019 9:40 AM
> > To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux- kernel@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > Subject: [RESEND] input: keyboard: imx: make sure keyboard can always
> > wake up system
> >
> > 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.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/imx_keypad.c
> > b/drivers/input/keyboard/imx_keypad.c
> > index cf08f4a..97500a2 100644
> > --- a/drivers/input/keyboard/imx_keypad.c
> > +++ b/drivers/input/keyboard/imx_keypad.c
> > @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct
> > platform_device *pdev)
> >  	return 0;
> >  }
> >
> > -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> > +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct imx_keypad *kbd = platform_get_drvdata(pdev);
> >  	struct input_dev *input_dev = kbd->input_dev;
> > +	unsigned short reg_val = readw(kbd->mmio_base + KPSR);
> >
> >  	/* imx kbd can wake up system even clock is disabled */
> >  	mutex_lock(&input_dev->mutex);
> > @@ -538,13 +539,20 @@ static int __maybe_unused
> imx_kbd_suspend(struct
> > device *dev)
> >
> >  	mutex_unlock(&input_dev->mutex);
> >
> > -	if (device_may_wakeup(&pdev->dev))
> > +	if (device_may_wakeup(&pdev->dev)) {
> > +		if (reg_val & KBD_STAT_KPKD)
> > +			reg_val |= KBD_STAT_KRIE;
> > +		if (reg_val & KBD_STAT_KPKR)
> > +			reg_val |= KBD_STAT_KDIE;
> > +		writew(reg_val, kbd->mmio_base + KPSR);
> > +
> >  		enable_irq_wake(kbd->irq);
> > +	}
> >
> >  	return 0;
> >  }
> >
> > -static int __maybe_unused imx_kbd_resume(struct device *dev)
> > +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct imx_keypad *kbd = platform_get_drvdata(pdev); @@ -568,7
> > +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device
> *dev)
> >  	return ret;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend,
> > imx_kbd_resume);
> > +static const struct dev_pm_ops imx_kbd_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend,
> > +imx_kbd_noirq_resume) };
> >
> >  static struct platform_driver imx_keypad_driver = {
> >  	.driver		= {
> > --
> > 2.7.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-04-04  1:40 [RESEND] input: keyboard: imx: make sure keyboard can always wake up system Anson Huang
  2019-04-25  1:49 ` Anson Huang
@ 2019-05-21  5:30 ` dmitry.torokhov
  2019-05-21  6:36   ` Anson Huang
  2019-06-12  0:51 ` dmitry.torokhov
  2 siblings, 1 reply; 7+ messages in thread
From: dmitry.torokhov @ 2019-05-21  5:30 UTC (permalink / raw)
  To: Anson Huang
  Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel,
	linux-input, festevam, linux-arm-kernel

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?

Thanks.

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-05-21  5:30 ` dmitry.torokhov
@ 2019-05-21  6:36   ` Anson Huang
  2019-06-10  6:44     ` Anson Huang
  0 siblings, 1 reply; 7+ messages in thread
From: Anson Huang @ 2019-05-21  6:36 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel,
	linux-input, festevam, linux-arm-kernel

Hi, Dmitry

> -----Original Message-----
> From: dmitry.torokhov@gmail.com [mailto:dmitry.torokhov@gmail.com]
> Sent: Tuesday, May 21, 2019 1:31 PM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; linux-input@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-05-21  6:36   ` Anson Huang
@ 2019-06-10  6:44     ` Anson Huang
  0 siblings, 0 replies; 7+ messages in thread
From: Anson Huang @ 2019-06-10  6:44 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel,
	linux-input, festevam, linux-arm-kernel

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@gmail.com
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; linux-input@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>
> Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
> 
> Hi, Dmitry
> 
> > -----Original Message-----
> > From: dmitry.torokhov@gmail.com [mailto:dmitry.torokhov@gmail.com]
> > Sent: Tuesday, May 21, 2019 1:31 PM
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com;
> > linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; dl-linux- imx <linux-imx@nxp.com>
> > 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
  2019-04-04  1:40 [RESEND] input: keyboard: imx: make sure keyboard can always wake up system Anson Huang
  2019-04-25  1:49 ` Anson Huang
  2019-05-21  5:30 ` dmitry.torokhov
@ 2019-06-12  0:51 ` dmitry.torokhov
  2 siblings, 0 replies; 7+ messages in thread
From: dmitry.torokhov @ 2019-06-12  0:51 UTC (permalink / raw)
  To: Anson Huang
  Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel,
	linux-input, festevam, linux-arm-kernel

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.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Applied, thank you.

> ---
>  drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
> index cf08f4a..97500a2 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct imx_keypad *kbd = platform_get_drvdata(pdev);
>  	struct input_dev *input_dev = kbd->input_dev;
> +	unsigned short reg_val = readw(kbd->mmio_base + KPSR);
>  
>  	/* imx kbd can wake up system even clock is disabled */
>  	mutex_lock(&input_dev->mutex);
> @@ -538,13 +539,20 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
>  
>  	mutex_unlock(&input_dev->mutex);
>  
> -	if (device_may_wakeup(&pdev->dev))
> +	if (device_may_wakeup(&pdev->dev)) {
> +		if (reg_val & KBD_STAT_KPKD)
> +			reg_val |= KBD_STAT_KRIE;
> +		if (reg_val & KBD_STAT_KPKR)
> +			reg_val |= KBD_STAT_KDIE;
> +		writew(reg_val, kbd->mmio_base + KPSR);
> +
>  		enable_irq_wake(kbd->irq);
> +	}
>  
>  	return 0;
>  }
>  
> -static int __maybe_unused imx_kbd_resume(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct imx_keypad *kbd = platform_get_drvdata(pdev);
> @@ -568,7 +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
>  	return ret;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend, imx_kbd_resume);
> +static const struct dev_pm_ops imx_kbd_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend, imx_kbd_noirq_resume)
> +};
>  
>  static struct platform_driver imx_keypad_driver = {
>  	.driver		= {
> -- 
> 2.7.4
> 

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-06-12  0:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-04  1:40 [RESEND] input: keyboard: imx: make sure keyboard can always wake up system Anson Huang
2019-04-25  1:49 ` Anson Huang
2019-05-09  1:41   ` Anson Huang
2019-05-21  5:30 ` dmitry.torokhov
2019-05-21  6:36   ` Anson Huang
2019-06-10  6:44     ` Anson Huang
2019-06-12  0:51 ` dmitry.torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).