[PATCH 0/2] pixcir_i2c_ts: Add optional wakeup irq support

[PATCH 0/2] pixcir_i2c_ts: Add optional wakeup irq support

[Vignesh R]

On am437x-gp-evm, pixcir_i2c_tsc can wake-up system from low power
state via pinctrl and IO daisy chain mechanism. This patch series add
support for such optional wake up interrupt to be handled via recently
introduced generic wake irq handling framework.

Tested on am437x-gp-evm, with some out of tree patches to support
suspend/resume on am437x.


Vignesh R (2):
  input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup
    interrupt
  ARM: dts: AM437x-gp-evm: Add wakeup interrupt source for
    pixcir_i2c_tsc

 arch/arm/boot/dts/am437x-gp-evm.dts       |  3 +++
 drivers/input/touchscreen/pixcir_i2c_ts.c | 14 ++++++++++++++
 2 files changed, 17 insertions(+)

-- 
2.4.5

[PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Vignesh R]

On am437x-gp-evm, pixcir touchscreen can wake the system from low power
state by generating wake-up interrupt via pinctrl and IO daisy chain.
Add support for optional wakeup interrupt source by regsitering to
automated wake IRQ framework introduced by commit 4990d4fe327b ("PM /
Wakeirq: Add automated device wake IRQ handling").
This is similar in approach to commit 2a0b965cfb6e ("serial: omap: Add
support for optional wake-up")

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 8f3e243a62bf..f7c602027fbd 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -29,6 +29,8 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/pm_wakeirq.h>
 
 #define PIXCIR_MAX_SLOTS       5 /* Max fingers supported by driver */
 
@@ -38,6 +40,7 @@ struct pixcir_i2c_ts_data {
 	const struct pixcir_ts_platform_data *pdata;
 	bool running;
 	int max_fingers;	/* Max fingers supported in this instance */
+	int wakeirq;
 };
 
 struct pixcir_touch {
@@ -564,11 +567,22 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, tsdata);
 	device_init_wakeup(&client->dev, 1);
 
+	/* Register wakeirq, if available */
+	tsdata->wakeirq = of_irq_get(dev->of_node, 1);
+	if (tsdata->wakeirq) {
+		error = dev_pm_set_dedicated_wake_irq(dev,
+						      tsdata->wakeirq);
+		if (error)
+			dev_dbg(dev, "unable to get wakeirq %d\n",
+				error);
+	}
+
 	return 0;
 }
 
 static int pixcir_i2c_ts_remove(struct i2c_client *client)
 {
+	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, 0);
 
 	return 0;
-- 
2.4.5

[PATCH 2/2] ARM: dts: am437x-gp-evm: Add wakeup interrupt source for pixcir_i2c_tsc

[Vignesh R]

Pixcir_i2c_tsc driver can now wakeup the system from lower power state
via pinctrl and IO daisy chain using generic wakeirq framwework. Add
optional wakeup irq entry to allow pixcir_i2c_tsc to wake system from
low power state.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 arch/arm/boot/dts/am437x-gp-evm.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts b/arch/arm/boot/dts/am437x-gp-evm.dts
index 84aa30c3235a..04f7667c604e 100644
--- a/arch/arm/boot/dts/am437x-gp-evm.dts
+++ b/arch/arm/boot/dts/am437x-gp-evm.dts
@@ -503,6 +503,9 @@
 
 		attb-gpio = <&gpio3 22 GPIO_ACTIVE_HIGH>;
 
+		interrupts-extended = <&gpio3 22 GPIO_ACTIVE_HIGH>,
+				      <&am43xx_pinmux 0x264>;
+
 		touchscreen-size-x = <1024>;
 		touchscreen-size-y = <600>;
 	};
-- 
2.4.5

Re: [PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Dmitry Torokhov]

Hi Vignesh,

On Fri, Jul 17, 2015 at 12:10:40PM +0530, Vignesh R wrote:
> On am437x-gp-evm, pixcir touchscreen can wake the system from low power
> state by generating wake-up interrupt via pinctrl and IO daisy chain.
> Add support for optional wakeup interrupt source by regsitering to
> automated wake IRQ framework introduced by commit 4990d4fe327b ("PM /
> Wakeirq: Add automated device wake IRQ handling").
> This is similar in approach to commit 2a0b965cfb6e ("serial: omap: Add
> support for optional wake-up")
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/input/touchscreen/pixcir_i2c_ts.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index 8f3e243a62bf..f7c602027fbd 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -29,6 +29,8 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/pm_wakeirq.h>
>  
>  #define PIXCIR_MAX_SLOTS       5 /* Max fingers supported by driver */
>  
> @@ -38,6 +40,7 @@ struct pixcir_i2c_ts_data {
>  	const struct pixcir_ts_platform_data *pdata;
>  	bool running;
>  	int max_fingers;	/* Max fingers supported in this instance */
> +	int wakeirq;
>  };
>  
>  struct pixcir_touch {
> @@ -564,11 +567,22 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, tsdata);
>  	device_init_wakeup(&client->dev, 1);
>  
> +	/* Register wakeirq, if available */
> +	tsdata->wakeirq = of_irq_get(dev->of_node, 1);

Can we put this in platform data and parse in pixcir_parse_dt() please?
Also, why not of_irq_get_byname()?

> +	if (tsdata->wakeirq) {
> +		error = dev_pm_set_dedicated_wake_irq(dev,
> +						      tsdata->wakeirq);
> +		if (error)
> +			dev_dbg(dev, "unable to get wakeirq %d\n",
> +				error);
> +	}

Shouldn't his actually be:

	error = tsdata->wakeirq ?
		dev_pm_set_dedicated_wake_irq(dev, tsdata->wakeirq) :
		dev_pm_set_wake_irq(dev, client->irq);
	if (error) {
		...
	}

and then we can get rid of enable_irq_wake()/disable_irq_wake() in
pixcir_i2c_ts_suspend() and pixcir_i2c_ts_resume().

> +
>  	return 0;
>  }
>  
>  static int pixcir_i2c_ts_remove(struct i2c_client *client)
>  {
> +	dev_pm_clear_wake_irq(&client->dev);
>  	device_init_wakeup(&client->dev, 0);

I wonder if driver core should be responsible for clearing wake irq and
also for clearing wakeup flag.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Vignesh R]

Hi Dmitry,

On 7/18/2015 3:21 AM, Dmitry Torokhov wrote:
> Hi Vignesh,
> 
> On Fri, Jul 17, 2015 at 12:10:40PM +0530, Vignesh R wrote:
>> On am437x-gp-evm, pixcir touchscreen can wake the system from low power
>> state by generating wake-up interrupt via pinctrl and IO daisy chain.
>> Add support for optional wakeup interrupt source by regsitering to
>> automated wake IRQ framework introduced by commit 4990d4fe327b ("PM /
>> Wakeirq: Add automated device wake IRQ handling").
>> This is similar in approach to commit 2a0b965cfb6e ("serial: omap: Add
>> support for optional wake-up")
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/input/touchscreen/pixcir_i2c_ts.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 8f3e243a62bf..f7c602027fbd 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -29,6 +29,8 @@
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>>  #include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/pm_wakeirq.h>
>>  
>>  #define PIXCIR_MAX_SLOTS       5 /* Max fingers supported by driver */
>>  
>> @@ -38,6 +40,7 @@ struct pixcir_i2c_ts_data {
>>  	const struct pixcir_ts_platform_data *pdata;
>>  	bool running;
>>  	int max_fingers;	/* Max fingers supported in this instance */
>> +	int wakeirq;
>>  };
>>  
>>  struct pixcir_touch {
>> @@ -564,11 +567,22 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	i2c_set_clientdata(client, tsdata);
>>  	device_init_wakeup(&client->dev, 1);
>>  
>> +	/* Register wakeirq, if available */
>> +	tsdata->wakeirq = of_irq_get(dev->of_node, 1);
> 
> Can we put this in platform data and parse in pixcir_parse_dt() please?
> Also, why not of_irq_get_byname()?
>

Ok.

>> +	if (tsdata->wakeirq) {
>> +		error = dev_pm_set_dedicated_wake_irq(dev,
>> +						      tsdata->wakeirq);
>> +		if (error)
>> +			dev_dbg(dev, "unable to get wakeirq %d\n",
>> +				error);
>> +	}
> 
> Shouldn't his actually be:
> 
> 	error = tsdata->wakeirq ?
> 		dev_pm_set_dedicated_wake_irq(dev, tsdata->wakeirq) :
> 		dev_pm_set_wake_irq(dev, client->irq);
> 	if (error) {
> 		...
> 	}
> 
> and then we can get rid of enable_irq_wake()/disable_irq_wake() in
> pixcir_i2c_ts_suspend() and pixcir_i2c_ts_resume().
> 

Yes, I will do this in v2.

>> +
>>  	return 0;
>>  }
>>  
>>  static int pixcir_i2c_ts_remove(struct i2c_client *client)
>>  {
>> +	dev_pm_clear_wake_irq(&client->dev);
>>  	device_init_wakeup(&client->dev, 0);
> 
> I wonder if driver core should be responsible for clearing wake irq and
> also for clearing wakeup flag.
> 

AFAICU, wakeup flag is deleted when struct device is deleted, hence,
device_init_wakeup() call may not be required in .remove(). But,
dev_pm_clear_wake_irq() can be moved to driver core.

Regards
Vignesh

Re: [PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Tony Lindgren]

* Vignesh R <vigneshr@ti.com> [150719 21:51]:
> On 7/18/2015 3:21 AM, Dmitry Torokhov wrote:
> > 
> > I wonder if driver core should be responsible for clearing wake irq and
> > also for clearing wakeup flag.
> > 
> 
> AFAICU, wakeup flag is deleted when struct device is deleted, hence,
> device_init_wakeup() call may not be required in .remove(). But,
> dev_pm_clear_wake_irq() can be moved to driver core.

Currently the lifecycle of struct wakeup_source is not necessarily
the same as the lifecycle struct device. I believe net and usb drivers
at least allocate it dynamically.

Regards,

Tony

Re: [PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Dmitry Torokhov]

On Sun, Jul 19, 2015 at 11:05:07PM -0700, Tony Lindgren wrote:
> * Vignesh R <vigneshr@ti.com> [150719 21:51]:
> > On 7/18/2015 3:21 AM, Dmitry Torokhov wrote:
> > > 
> > > I wonder if driver core should be responsible for clearing wake irq and
> > > also for clearing wakeup flag.
> > > 
> > 
> > AFAICU, wakeup flag is deleted when struct device is deleted, hence,
> > device_init_wakeup() call may not be required in .remove(). But,
> > dev_pm_clear_wake_irq() can be moved to driver core.
> 
> Currently the lifecycle of struct wakeup_source is not necessarily
> the same as the lifecycle struct device. I believe net and usb drivers
> at least allocate it dynamically.

I am not sure if I follow. I was wondering if we should clear the wakeup
IRQ setting on the driver unbinding. It does not mean that we'd be
deleting wakeup_source, just that we'll clear wakeup irq setting from
it.

-- 
Dmitry

Re: [PATCH 1/2] input: touchscreen: pixcir_i2c_ts: Add support for optional wakeup interrupt

[Tony Lindgren]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [150719 23:36]:
> On Sun, Jul 19, 2015 at 11:05:07PM -0700, Tony Lindgren wrote:
> > * Vignesh R <vigneshr@ti.com> [150719 21:51]:
> > > On 7/18/2015 3:21 AM, Dmitry Torokhov wrote:
> > > > 
> > > > I wonder if driver core should be responsible for clearing wake irq and
> > > > also for clearing wakeup flag.
> > > > 
> > > 
> > > AFAICU, wakeup flag is deleted when struct device is deleted, hence,
> > > device_init_wakeup() call may not be required in .remove(). But,
> > > dev_pm_clear_wake_irq() can be moved to driver core.
> > 
> > Currently the lifecycle of struct wakeup_source is not necessarily
> > the same as the lifecycle struct device. I believe net and usb drivers
> > at least allocate it dynamically.
> 
> I am not sure if I follow. I was wondering if we should clear the wakeup
> IRQ setting on the driver unbinding. It does not mean that we'd be
> deleting wakeup_source, just that we'll clear wakeup irq setting from
> it.

Yes you're right we can do that. I was mostly commenting on why we
currently can't automate things further with devm.

Regards,

Tony