[PATCH v4 0/2] input: rotary encoder: Add wake up support

[PATCH v4 0/2] input: rotary encoder: Add wake up support

[Sylvain Rochet]

Changes since v3:
  * Stick to the "wakeup-source" DT property, we are standardizing on it.
  * Improved wake-up support: This driver is always supporting wake up
    support, thus it is now calling device_set_wakeup_capable(dev, true) 
    followed by device_wakeup_enable(dev) if wake up is enabled at boot
    time. This way the wakeup sysfs tree is always available to the user.
    Only calling device_init_wakeup(dev, false) totally disable wake up
    support at boot tine with no way to enable wake up support furtherly,
    which isn't nice.

Changes since v2:
  * Using false instead of 0 on boolean
  * Splitted DT documentation in a separate patch

Changes since v1:
  * Using DT property wakeup-source instead of rotary-encoder,wakeup
  * Renamed int wakeup to bool wakeup_source
  * Patch cleanup (checkpatch.pl)

Sylvain Rochet (2):
  DT: Documentation: input: rotary encoder: Add wakeup-source property
  input: rotary encoder: Add wake up support

Sylvain Rochet (2):
  Documentation: input: rotary encoder: Add wakeup-source property
  input: rotary encoder: Add wake up support

 .../devicetree/bindings/input/rotary-encoder.txt   |  1 +
 Documentation/input/rotary-encoder.txt             |  1 +
 drivers/input/misc/rotary_encoder.c                | 39 ++++++++++++++++++++++
 include/linux/rotary_encoder.h                     |  1 +
 4 files changed, 42 insertions(+)

-- 
2.5.1

[PATCH v4 1/2] Documentation: input: rotary encoder: Add wakeup-source property

[Sylvain Rochet]

This patch adds property wakeup-source to GPIO rotary encoders.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
Cc: <devicetree@vger.kernel.org>
---
 Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
 Documentation/input/rotary-encoder.txt                     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
index 3315495..2c643a3 100644
--- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
+++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
@@ -15,6 +15,7 @@ Optional properties:
 - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
   greater than the specified steps or smaller than 0. For absolute axis only.
 - rotary-encoder,half-period: Makes the driver work on half-period mode.
+- wakeup-source: Boolean, rotary encoder can wake-up the system.
 
 See Documentation/input/rotary-encoder.txt for more information.
 
diff --git a/Documentation/input/rotary-encoder.txt b/Documentation/input/rotary-encoder.txt
index 5737e35..bddbee1 100644
--- a/Documentation/input/rotary-encoder.txt
+++ b/Documentation/input/rotary-encoder.txt
@@ -109,6 +109,7 @@ static struct rotary_encoder_platform_data my_rotary_encoder_info = {
 	.inverted_a	= 0,
 	.inverted_b	= 0,
 	.half_period	= false,
+	.wakeup_source	= false,
 };
 
 static struct platform_device rotary_encoder_device = {
-- 
2.5.1

[PATCH v4 2/2] input: rotary encoder: Add wake up support

[Sylvain Rochet]

This patch adds wake up support to GPIO rotary encoders.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 39 +++++++++++++++++++++++++++++++++++++
 include/linux/rotary_encoder.h      |  1 +
 2 files changed, 40 insertions(+)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index f27f81e..0d86dc4 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -26,6 +26,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
+#include <linux/pm.h>
 
 #define DRV_NAME "rotary-encoder"
 
@@ -180,6 +181,8 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
 					"rotary-encoder,rollover", NULL);
 	pdata->half_period = !!of_get_property(np,
 					"rotary-encoder,half-period", NULL);
+	pdata->wakeup_source = !!of_get_property(np,
+					"wakeup-source", NULL);
 
 	return pdata;
 }
@@ -280,6 +283,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		goto exit_free_irq_b;
 	}
 
+	device_set_wakeup_capable(&pdev->dev, true);
+	if (pdata->wakeup_source)
+		device_wakeup_enable(&pdev->dev);
+
 	platform_set_drvdata(pdev, encoder);
 
 	return 0;
@@ -306,6 +313,8 @@ static int rotary_encoder_remove(struct platform_device *pdev)
 	struct rotary_encoder *encoder = platform_get_drvdata(pdev);
 	const struct rotary_encoder_platform_data *pdata = encoder->pdata;
 
+	device_init_wakeup(&pdev->dev, false);
+
 	free_irq(encoder->irq_a, encoder);
 	free_irq(encoder->irq_b, encoder);
 	gpio_free(pdata->gpio_a);
@@ -320,11 +329,41 @@ static int rotary_encoder_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int rotary_encoder_suspend(struct device *dev)
+{
+	struct rotary_encoder *encoder = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev)) {
+		enable_irq_wake(encoder->irq_a);
+		enable_irq_wake(encoder->irq_b);
+	}
+
+	return 0;
+}
+
+static int rotary_encoder_resume(struct device *dev)
+{
+	struct rotary_encoder *encoder = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev)) {
+		disable_irq_wake(encoder->irq_a);
+		disable_irq_wake(encoder->irq_b);
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(rotary_encoder_pm_ops,
+		 rotary_encoder_suspend, rotary_encoder_resume);
+
 static struct platform_driver rotary_encoder_driver = {
 	.probe		= rotary_encoder_probe,
 	.remove		= rotary_encoder_remove,
 	.driver		= {
 		.name	= DRV_NAME,
+		.pm	= &rotary_encoder_pm_ops,
 		.of_match_table = of_match_ptr(rotary_encoder_of_match),
 	}
 };
diff --git a/include/linux/rotary_encoder.h b/include/linux/rotary_encoder.h
index 3f594dc..b33f2d2 100644
--- a/include/linux/rotary_encoder.h
+++ b/include/linux/rotary_encoder.h
@@ -11,6 +11,7 @@ struct rotary_encoder_platform_data {
 	bool relative_axis;
 	bool rollover;
 	bool half_period;
+	bool wakeup_source;
 };
 
 #endif /* __ROTARY_ENCODER_H__ */
-- 
2.5.1

Re: [PATCH v4 2/2] input: rotary encoder: Add wake up support

[Johan Hovold]

On Mon, Oct 12, 2015 at 09:13:32PM +0200, Sylvain Rochet wrote:
> This patch adds wake up support to GPIO rotary encoders.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Reviewed-by: Johan Hovold <johan@kernel.org>

Hmm. I have not yet reviewed the changes you did in v4.

> ---
>  drivers/input/misc/rotary_encoder.c | 39 +++++++++++++++++++++++++++++++++++++
>  include/linux/rotary_encoder.h      |  1 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index f27f81e..0d86dc4 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c

> @@ -280,6 +283,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
>  		goto exit_free_irq_b;
>  	}
>  
> +	device_set_wakeup_capable(&pdev->dev, true);

You should continue to use the platform data to determine whether the
device is capable of wakeup or not.

> +	if (pdata->wakeup_source)
> +		device_wakeup_enable(&pdev->dev);
> +

Just stick to

	device_init_wakeup(&pdev->dev, pdata->wakeup_source);

as in v3.

Johan

Re: [PATCH v4 2/2] input: rotary encoder: Add wake up support

[Sylvain Rochet]

Hi Johan,


On Tue, Oct 13, 2015 at 02:39:48PM +0200, Johan Hovold wrote:
> On Mon, Oct 12, 2015 at 09:13:32PM +0200, Sylvain Rochet wrote:
> > This patch adds wake up support to GPIO rotary encoders.
> > 
> > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> > Reviewed-by: Johan Hovold <johan@kernel.org>
> 
> Hmm. I have not yet reviewed the changes you did in v4.

Woops, sorry, I forgot to remove it while rebasing.


> > ---
> >  drivers/input/misc/rotary_encoder.c | 39 +++++++++++++++++++++++++++++++++++++
> >  include/linux/rotary_encoder.h      |  1 +
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> > index f27f81e..0d86dc4 100644
> > --- a/drivers/input/misc/rotary_encoder.c
> > +++ b/drivers/input/misc/rotary_encoder.c
> 
> > @@ -280,6 +283,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
> >  		goto exit_free_irq_b;
> >  	}
> >  
> > +	device_set_wakeup_capable(&pdev->dev, true);
> 
> You should continue to use the platform data to determine whether the
> device is capable of wakeup or not.
> 
> > +	if (pdata->wakeup_source)
> > +		device_wakeup_enable(&pdev->dev);
> > +
> 
> Just stick to
> 
> 	device_init_wakeup(&pdev->dev, pdata->wakeup_source);

There is unfortunately no or poor documentation on how "wakeup-source" 
DT property should behave. We have no clue if wake up should be enabled 
by default, which is what device_init_wakeup() is doing. If 
"wakeup-source" is just a flag to determine whether the device is 
capable of wakeup or not then it should probably not change the behavior 
and wake up should probably be off by default, thus the right way would 
be to call device_set_wakeup_capable(&pdev->dev, pdata->wakeup_source);

device_init_wakeup() is a bit confusing, its introductory comment says 
that "By default, most devices should leave wakeup disabled." but it 
actually enables it by default. In this case we are the exception "The 
exceptions are devices that everyone expects to be wakeup sources: 
keyboards, …" but it only adds more confusion, we are the exception, but 
by default we are not.

Anyway, I don't really care and I will resend with device_init_wakeup() 
because wakeup support enabled by default is what I need.

Thank you very much :-)

Cheers,
Sylvain
--
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