[PATCH 1/2] input: egalax_ts: add system wakeup support

[PATCH 1/2] input: egalax_ts: add system wakeup support

[Anson Huang]

This patch adds wakeup function support for egalax touch
screen, if "wakeup-source" is added to device tree's egalax
touch screen node, the wakeup function will be enabled, and
egalax touch screen will be able to wakeup system from suspend.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/input/touchscreen/egalax_ts.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 80e69bb..74984ed 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c
@@ -164,6 +164,7 @@ static int egalax_firmware_version(struct i2c_client *client)
 static int egalax_ts_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
+	struct device_node *np = client->dev.of_node;
 	struct egalax_ts *ts;
 	struct input_dev *input_dev;
 	int error;
@@ -224,6 +225,9 @@ static int egalax_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	if (of_property_read_bool(np, "wakeup-source"))
+		device_init_wakeup(&client->dev, true);
+
 	return 0;
 }
 
@@ -241,6 +245,9 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	int ret;
 
+	if (device_may_wakeup(dev))
+		return enable_irq_wake(client->irq);
+
 	ret = i2c_master_send(client, suspend_cmd, MAX_I2C_DATA_LEN);
 	return ret > 0 ? 0 : ret;
 }
@@ -249,6 +256,9 @@ static int __maybe_unused egalax_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
+	if (device_may_wakeup(dev))
+		return 0;
+
 	return egalax_wake_up_device(client);
 }
 
-- 
2.7.4

[PATCH 2/2] dt-bindings: egalax-ts: add support for wakeup event action

[Anson Huang]

Add support for wakeup event action, this would allow the device
to configure whether to be a wakeup source of system suspend.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
index 92fb262..296a9cd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
@@ -6,6 +6,7 @@ Required properties:
 - interrupts: touch controller interrupt
 - wakeup-gpios: the gpio pin to be used for waking up the controller
   and also used as irq pin
+- wakeup-source: boolean, touch screen can wake-up the system
 
 Example:
 
@@ -15,4 +16,5 @@ Example:
 		interrupt-parent = <&gpio1>;
 		interrupts = <9 2>;
 		wakeup-gpios = <&gpio1 9 0>;
+		wakeup-source;
 	};
-- 
2.7.4

Re: [PATCH 2/2] dt-bindings: egalax-ts: add support for wakeup event action

[Rob Herring]

On Wed, Sep 05, 2018 at 05:26:47PM +0800, Anson Huang wrote:
> Add support for wakeup event action, this would allow the device
> to configure whether to be a wakeup source of system suspend.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> index 92fb262..296a9cd 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> @@ -6,6 +6,7 @@ Required properties:
>  - interrupts: touch controller interrupt
>  - wakeup-gpios: the gpio pin to be used for waking up the controller
>    and also used as irq pin

If the wakeup gpio is the same as the irq, then this should be removed 
since wakeup-source replaces it.

> +- wakeup-source: boolean, touch screen can wake-up the system
>  
>  Example:
>  
> @@ -15,4 +16,5 @@ Example:
>  		interrupt-parent = <&gpio1>;
>  		interrupts = <9 2>;
>  		wakeup-gpios = <&gpio1 9 0>;
> +		wakeup-source;
>  	};
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] dt-bindings: egalax-ts: add support for wakeup event action

[Dmitry Torokhov]

On Sun, Sep 16, 2018 at 05:54:33PM -0500, Rob Herring wrote:
> On Wed, Sep 05, 2018 at 05:26:47PM +0800, Anson Huang wrote:
> > Add support for wakeup event action, this would allow the device
> > to configure whether to be a wakeup source of system suspend.
> > 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > index 92fb262..296a9cd 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > @@ -6,6 +6,7 @@ Required properties:
> >  - interrupts: touch controller interrupt
> >  - wakeup-gpios: the gpio pin to be used for waking up the controller
> >    and also used as irq pin
> 
> If the wakeup gpio is the same as the irq, then this should be removed 
> since wakeup-source replaces it.

No, it does not: it is GPIO that we need to toggle to wake up the
controller, not host, and we do not have generic API to map from IRQ to
GPIO.

This is also existing binding...

> 
> > +- wakeup-source: boolean, touch screen can wake-up the system
> >  
> >  Example:
> >  
> > @@ -15,4 +16,5 @@ Example:
> >  		interrupt-parent = <&gpio1>;
> >  		interrupts = <9 2>;
> >  		wakeup-gpios = <&gpio1 9 0>;
> > +		wakeup-source;
> >  	};
> > -- 
> > 2.7.4
> > 
> 

-- 
Dmitry

Re: [PATCH 2/2] dt-bindings: egalax-ts: add support for wakeup event action

[Rob Herring]

On Mon, Sep 17, 2018 at 11:07 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sun, Sep 16, 2018 at 05:54:33PM -0500, Rob Herring wrote:
> > On Wed, Sep 05, 2018 at 05:26:47PM +0800, Anson Huang wrote:
> > > Add support for wakeup event action, this would allow the device
> > > to configure whether to be a wakeup source of system suspend.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >  Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > > index 92fb262..296a9cd 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt
> > > @@ -6,6 +6,7 @@ Required properties:
> > >  - interrupts: touch controller interrupt
> > >  - wakeup-gpios: the gpio pin to be used for waking up the controller
> > >    and also used as irq pin
> >
> > If the wakeup gpio is the same as the irq, then this should be removed
> > since wakeup-source replaces it.
>
> No, it does not: it is GPIO that we need to toggle to wake up the
> controller, not host, and we do not have generic API to map from IRQ to
> GPIO.

Ah, okay.

> This is also existing binding...

Right, I meant removed as in moved to deprecated. Sometimes we just
remove the documentation when deprecating.

Rob

Re: [PATCH 1/2] input: egalax_ts: add system wakeup support

[Andi Shyti]

Hi Anson,

On Wed, Sep 05, 2018 at 05:26:46PM +0800, Anson Huang wrote:
> This patch adds wakeup function support for egalax touch
> screen, if "wakeup-source" is added to device tree's egalax
> touch screen node, the wakeup function will be enabled, and
> egalax touch screen will be able to wakeup system from suspend.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

I think you messed up the order of the patches, this patch
whould come after the PATCH 2/2.

You first create the property and then use it. If someone checks
out right here, he would find an inconsistency between the dts
property and this code.

You can either send a version 2 with the correct order or check
if Dmitry and Rob are willing to sync for who takes first the
patch first.

Andi

Re: [PATCH 1/2] input: egalax_ts: add system wakeup support

[Dmitry Torokhov]

Hi Anson,

On Wed, Sep 05, 2018 at 05:26:46PM +0800, Anson Huang wrote:
> This patch adds wakeup function support for egalax touch
> screen, if "wakeup-source" is added to device tree's egalax
> touch screen node, the wakeup function will be enabled, and
> egalax touch screen will be able to wakeup system from suspend.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/input/touchscreen/egalax_ts.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
> index 80e69bb..74984ed 100644
> --- a/drivers/input/touchscreen/egalax_ts.c
> +++ b/drivers/input/touchscreen/egalax_ts.c
> @@ -164,6 +164,7 @@ static int egalax_firmware_version(struct i2c_client *client)
>  static int egalax_ts_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
> +	struct device_node *np = client->dev.of_node;
>  	struct egalax_ts *ts;
>  	struct input_dev *input_dev;
>  	int error;
> @@ -224,6 +225,9 @@ static int egalax_ts_probe(struct i2c_client *client,
>  	if (error)
>  		return error;
>  
> +	if (of_property_read_bool(np, "wakeup-source"))
> +		device_init_wakeup(&client->dev, true);

This is done in i2c core, there is no need to do it in the driver.

> +
>  	return 0;
>  }
>  
> @@ -241,6 +245,9 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	int ret;
>  
> +	if (device_may_wakeup(dev))
> +		return enable_irq_wake(client->irq);
> +
>  	ret = i2c_master_send(client, suspend_cmd, MAX_I2C_DATA_LEN);
>  	return ret > 0 ? 0 : ret;
>  }
> @@ -249,6 +256,9 @@ static int __maybe_unused egalax_ts_resume(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  
> +	if (device_may_wakeup(dev))
> +		return 0;

This will end up with unbalanced enable_irq_wake() from suspend.

> +
>  	return egalax_wake_up_device(client);
>  }
>  
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

RE: [PATCH 1/2] input: egalax_ts: add system wakeup support

[Anson Huang]

Hi, Dmitry

Anson Huang
Best Regards!


> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Thursday, September 6, 2018 1:27 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; Marco Antonio Franchi
> <marco.franchi@nxp.com>; Fabio Estevam <fabio.estevam@nxp.com>;
> linux-input@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/2] input: egalax_ts: add system wakeup support
> 
> Hi Anson,
> 
> On Wed, Sep 05, 2018 at 05:26:46PM +0800, Anson Huang wrote:
> > This patch adds wakeup function support for egalax touch screen, if
> > "wakeup-source" is added to device tree's egalax touch screen node,
> > the wakeup function will be enabled, and egalax touch screen will be
> > able to wakeup system from suspend.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/input/touchscreen/egalax_ts.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/egalax_ts.c
> > b/drivers/input/touchscreen/egalax_ts.c
> > index 80e69bb..74984ed 100644
> > --- a/drivers/input/touchscreen/egalax_ts.c
> > +++ b/drivers/input/touchscreen/egalax_ts.c
> > @@ -164,6 +164,7 @@ static int egalax_firmware_version(struct
> > i2c_client *client)  static int egalax_ts_probe(struct i2c_client *client,
> >  			   const struct i2c_device_id *id)  {
> > +	struct device_node *np = client->dev.of_node;
> >  	struct egalax_ts *ts;
> >  	struct input_dev *input_dev;
> >  	int error;
> > @@ -224,6 +225,9 @@ static int egalax_ts_probe(struct i2c_client *client,
> >  	if (error)
> >  		return error;
> >
> > +	if (of_property_read_bool(np, "wakeup-source"))
> > +		device_init_wakeup(&client->dev, true);
> 
> This is done in i2c core, there is no need to do it in the driver.
 
Correct, I removed it in V2 patch.


> 
> > +
> >  	return 0;
> >  }
> >
> > @@ -241,6 +245,9 @@ static int __maybe_unused
> egalax_ts_suspend(struct device *dev)
> >  	struct i2c_client *client = to_i2c_client(dev);
> >  	int ret;
> >
> > +	if (device_may_wakeup(dev))
> > +		return enable_irq_wake(client->irq);
> > +
> >  	ret = i2c_master_send(client, suspend_cmd, MAX_I2C_DATA_LEN);
> >  	return ret > 0 ? 0 : ret;
> >  }
> > @@ -249,6 +256,9 @@ static int __maybe_unused egalax_ts_resume(struct
> > device *dev)  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> >
> > +	if (device_may_wakeup(dev))
> > +		return 0;
> 
> This will end up with unbalanced enable_irq_wake() from suspend.

Ah, it is a mistake, I fix it in V2 patch, please help review it.

And for the other patch about adding "wakeup-source" into dt-binding, I think it may
be no need now, since it is already included in i2c's dt-binding, right?

Thanks.

Anson.

> 
> > +
> >  	return egalax_wake_up_device(client);  }
> >
> > --
> > 2.7.4
> >
> 
> Thanks.
> 
> --
> Dmitry