[PATCH v2] input/imx_keypad: add PM support

[PATCH v2] input/imx_keypad: add PM support

[Eric Bénard]

We use keypad_data to enable wakeup from platform data

suspend/resume is tested on an i.MX27 when keypad is not
in use and when it's is in use (in which case we immediatly
get the key pressed at wakeup)

Signed-off-by: Eric Bénard <eric@eukrea.com>
Cc: maramaopercheseimorto@gmail.com
Cc: linux-input@vger.kernel.org
Cc: dmitry.torokhov@gmail.com
Cc: linux-arm-kernel@lists.infradead.org
---
v2 :
	- simplify suspend & resume functions
	- convert to pm_ops

 drivers/input/keyboard/imx_keypad.c |   40 +++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index d92c15c..9aa0005 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -364,6 +364,38 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
 	writew(0xff00, keypad->mmio_base + KPCR);
 }
 
+#ifdef CONFIG_PM
+static int mxc_kpp_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	keypad->enabled = false;
+	synchronize_irq(keypad->irq);
+	disable_irq(keypad->irq);
+	clk_disable(keypad->clk);
+
+	return 0;
+}
+
+static int mxc_kpp_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	keypad->enabled = true;
+	clk_enable(keypad->clk);
+	enable_irq(keypad->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mxc_kpp_pm_ops = {
+	.suspend	= mxc_kpp_suspend,
+	.resume		= mxc_kpp_resume,
+};
+#endif
+
 static void imx_keypad_close(struct input_dev *dev)
 {
 	struct imx_keypad *keypad = input_get_drvdata(dev);
@@ -410,7 +442,8 @@ open_err:
 
 static int __devinit imx_keypad_probe(struct platform_device *pdev)
 {
-	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
+	const struct matrix_keypad_platform_data *keypad_data = pdev->dev.platform_data;
+	const struct matrix_keymap_data *keymap_data = keypad_data->keymap_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;
@@ -525,7 +558,7 @@ static int __devinit imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, keypad);
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, keypad_data->wakeup);
 
 	return 0;
 
@@ -571,6 +604,9 @@ static struct platform_driver imx_keypad_driver = {
 	.driver		= {
 		.name	= "imx-keypad",
 		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &mxc_kpp_pm_ops,
+#endif
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= __devexit_p(imx_keypad_remove),
-- 
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] input/imx_keypad: add PM support

[Alberto Panizzo]

Hi Eric,

On gio, 2010-06-17 at 20:59 +0200, Eric Bénard wrote:
> We use keypad_data to enable wakeup from platform data
> 
> suspend/resume is tested on an i.MX27 when keypad is not
> in use and when it's is in use (in which case we immediatly
> get the key pressed at wakeup)
> 
> Signed-off-by: Eric Bénard <eric@eukrea.com>
> Cc: maramaopercheseimorto@gmail.com
> Cc: linux-input@vger.kernel.org
> Cc: dmitry.torokhov@gmail.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> v2 :
> 	- simplify suspend & resume functions
> 	- convert to pm_ops
> 
>  drivers/input/keyboard/imx_keypad.c |   40 +++++++++++++++++++++++++++++++++-
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
> index d92c15c..9aa0005 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -364,6 +364,38 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
>  	writew(0xff00, keypad->mmio_base + KPCR);
>  }
>  
> +#ifdef CONFIG_PM
> +static int mxc_kpp_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct imx_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	keypad->enabled = false;
> +	synchronize_irq(keypad->irq);
> +	disable_irq(keypad->irq);
> +	clk_disable(keypad->clk);
> +
> +	return 0;
> +}
> +
> +static int mxc_kpp_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct imx_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	keypad->enabled = true;
> +	clk_enable(keypad->clk);
> +	enable_irq(keypad->irq);
> +
> +	return 0;
> +}

Ok, so with this patch the keypad cannot wake the system?
It is scheduled another patch?


-- 
Alberto!

        Be Persistent!
                - Greg Kroah-Hartman (FOSDEM 2010)

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] input/imx_keypad: add PM support

[Eric Bénard]

Hi Alberto,

Le 18/06/2010 12:16, Alberto Panizzo a écrit :
> Ok, so with this patch the keypad cannot wake the system?
> It is scheduled another patch?
>
well, actually, the keypad wakes the system, at least on my board here.

I was quite surprised to get this working this way as I would have 
expected disable_irq + clk_disable would prevent the keypad from being a 
wake source.
Can you try on your board as this cleanup may be wrong on i.MX3 ?

Thanks,
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] input/imx_keypad: add PM support

[Alberto Panizzo]

On ven, 2010-06-18 at 16:37 +0200, Eric Bénard wrote:
> Hi Alberto,
> 
> Le 18/06/2010 12:16, Alberto Panizzo a écrit :
> > Ok, so with this patch the keypad cannot wake the system?
> > It is scheduled another patch?
> >
> well, actually, the keypad wakes the system, at least on my board here.
> 
> I was quite surprised to get this working this way as I would have 
> expected disable_irq + clk_disable would prevent the keypad from being a 
> wake source.
> Can you try on your board as this cleanup may be wrong on i.MX3 ?
> 
> Thanks,
> Eric
Well.. it is a surprise to me too. I have to look down the documentation
ad soon I will try on the mx31pdk.
I don't wont to tell you wrong things.


-- 
Alberto!

        Be Persistent!
                - Greg Kroah-Hartman (FOSDEM 2010)

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] input/imx_keypad: add PM support

[Eric Bénard]

Le 18/06/2010 19:40, Alberto Panizzo a écrit :
> Well.. it is a surprise to me too. I have to look down the documentation
> ad soon I will try on the mx31pdk.

OK, I'm waiting for your feedback on the mx31pdk.

 From my understanding of the documentation, in order to be able to 
wakeup the CPU, the interrupt of a peripheral and its clock should stay 
enabled while going to suspend.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] input/imx_keypad: add PM support

[Dmitry Torokhov]

On Thu, Jun 17, 2010 at 08:59:01PM +0200, Eric Bénard wrote:
> We use keypad_data to enable wakeup from platform data
> 
> suspend/resume is tested on an i.MX27 when keypad is not
> in use and when it's is in use (in which case we immediatly
> get the key pressed at wakeup)
> 
> Signed-off-by: Eric Bénard <eric@eukrea.com>
> Cc: maramaopercheseimorto@gmail.com
> Cc: linux-input@vger.kernel.org
> Cc: dmitry.torokhov@gmail.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> v2 :
> 	- simplify suspend & resume functions
> 	- convert to pm_ops
> 
>  drivers/input/keyboard/imx_keypad.c |   40 +++++++++++++++++++++++++++++++++-
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
> index d92c15c..9aa0005 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -364,6 +364,38 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
>  	writew(0xff00, keypad->mmio_base + KPCR);
>  }
>  
> +#ifdef CONFIG_PM
> +static int mxc_kpp_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct imx_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	keypad->enabled = false;
> +	synchronize_irq(keypad->irq);
> +	disable_irq(keypad->irq);
> +	clk_disable(keypad->clk);
> +

This is not correct. imx_keypad_probe() keypad->enabled = false, waiting for
imx_keypad_open() to be called. However if nobody opens the device and
you go through suspend/resume cycle you'll leave keypad->enabled = true.


BTW, why do you call new functions mxc_kpp_XXX  while the rest of the
driver uses imx_keypad_XXX naming?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html