[PATCH v2 0/3] input: touchscreen: edt-ft5x06: make wakeup source behavior configurable

[PATCH v2 0/3] input: touchscreen: edt-ft5x06: make wakeup source behavior configurable

[Daniel Mack]

Hi,

I have a platform that features an edt-ft5x06 touch panel and that
doesn't want to wake on touch screen activity.

Here's a trivial series of patches that make the edt-ft5x06 driver only
act as wakeup source when requested through device properties or DTS.

The third patch changes the default in two DTS files that use this
driver.

I guess the first two patches should go through the input tree, while
the third can be picked by the IMX people. There are no compile-time
dependencies, so the order doesn't matter.


Thanks,
Daniel

v1 → v2:
	* simply drop device_init_wakeup() as the I2C core does handles
	  the wakeup-source property already (1/3)
	* add a delay after reset deassertion at resume time (2/3)
	* no changes in the DTS patch (3/3)


Daniel Mack (3):
  input: touchscreen: edt-ft5x06: don't make device a wakeup source by
    default
  input: touchscreen: edt-ft5x06: assert reset during suspend
  ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source

 .../bindings/input/touchscreen/edt-ft5x06.txt      |  3 +++
 arch/arm/boot/dts/imx28-tx28.dts                   |  1 +
 arch/arm/boot/dts/imx53-tx53-x03x.dts              |  1 +
 drivers/input/touchscreen/edt-ft5x06.c             | 27 +++++++++++++++++-----
 4 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.14.3


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

[PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Daniel Mack]

Allow configuring the device as wakeup source through device properties, as
not all platforms want to wake up on touch screen activity.

The I2C core automatically reads the "wakeup-source" DT property to
configure a device's wakeup capability, and board supports files can set
I2C_CLIENT_WAKE in the flags.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 3 +++
 drivers/input/touchscreen/edt-ft5x06.c                             | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 025cf8c9324a..83f792d4d88c 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -52,6 +52,8 @@ Optional properties:
  - touchscreen-inverted-y  : See touchscreen.txt
  - touchscreen-swapped-x-y : See touchscreen.txt
 
+ - wakeup-source: touchscreen acts as wakeup source
+
 Example:
 	polytouch: edt-ft5x06@38 {
 		compatible = "edt,edt-ft5406", "edt,edt-ft5x06";
@@ -62,4 +64,5 @@ Example:
 		interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
 		reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
 		wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+		wakeup-source;
 	};
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 5bf63f76ddda..e18a2f215500 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1007,7 +1007,6 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		goto err_remove_attrs;
 
 	edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev));
-	device_init_wakeup(&client->dev, 1);
 
 	dev_dbg(&client->dev,
 		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
-- 
2.14.3

[PATCH v2 2/3] input: touchscreen: edt-ft5x06: assert reset during suspend

[Daniel Mack]

If the device is not configured as wakeup source, it can be put in reset
during suspend to save some power.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/input/touchscreen/edt-ft5x06.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index e18a2f215500..145499022e1c 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -883,6 +883,20 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
 	}
 }
 
+static void edt_ft5x06_reset(struct edt_ft5x06_ts_data *tsdata, bool reset)
+{
+	if (!tsdata->reset_gpio)
+		return;
+
+	if (reset) {
+		gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
+	} else {
+		usleep_range(5000, 6000);
+		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
+		msleep(300);
+	}
+}
+
 static int edt_ft5x06_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
@@ -934,11 +948,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
 	}
 
-	if (tsdata->reset_gpio) {
-		usleep_range(5000, 6000);
-		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
-		msleep(300);
-	}
+	edt_ft5x06_reset(tsdata, false);
 
 	input = devm_input_allocate_device(&client->dev);
 	if (!input) {
@@ -1034,9 +1044,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
 static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(dev))
 		enable_irq_wake(client->irq);
+	else
+		edt_ft5x06_reset(tsdata, true);
 
 	return 0;
 }
@@ -1044,9 +1057,12 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(dev))
 		disable_irq_wake(client->irq);
+	else
+		edt_ft5x06_reset(tsdata, false);
 
 	return 0;
 }
-- 
2.14.3

[PATCH v2 3/3] ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source

[Daniel Mack]

The touchscreen driver no longer configures the device as wakeup source by
default. A "wakeup-source" property is needed.

To avoid regressions, this patch changes the DTS files for the only two
users of this driver that didn't have this property yet.

Signed-off-by: Daniel Mack <daniel@zonque.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
---
 arch/arm/boot/dts/imx28-tx28.dts      | 1 +
 arch/arm/boot/dts/imx53-tx53-x03x.dts | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts
index 0ebbc83852d0..094a39a67ec8 100644
--- a/arch/arm/boot/dts/imx28-tx28.dts
+++ b/arch/arm/boot/dts/imx28-tx28.dts
@@ -328,6 +328,7 @@
 		interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
 		reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
 		wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+		wakeup-source;
 	};
 
 	touchscreen: tsc2007@48 {
diff --git a/arch/arm/boot/dts/imx53-tx53-x03x.dts b/arch/arm/boot/dts/imx53-tx53-x03x.dts
index 0ecb43d88522..dbf0d73dc7b9 100644
--- a/arch/arm/boot/dts/imx53-tx53-x03x.dts
+++ b/arch/arm/boot/dts/imx53-tx53-x03x.dts
@@ -220,6 +220,7 @@
 		interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
 		reset-gpios = <&gpio2 22 GPIO_ACTIVE_LOW>;
 		wake-gpios = <&gpio2 21 GPIO_ACTIVE_HIGH>;
+		wakeup-source;
 	};
 
 	touchscreen: tsc2007@48 {
-- 
2.14.3

Re: [PATCH v2 3/3] ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source

[Shawn Guo]

On Thu, May 17, 2018 at 11:05:52AM +0200, Daniel Mack wrote:
> The touchscreen driver no longer configures the device as wakeup source by
> default. A "wakeup-source" property is needed.
> 
> To avoid regressions, this patch changes the DTS files for the only two
> users of this driver that didn't have this property yet.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>

Applied this one, thanks.

Re: [PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Rob Herring]

On Thu, May 17, 2018 at 11:05:50AM +0200, Daniel Mack wrote:
> Allow configuring the device as wakeup source through device properties, as
> not all platforms want to wake up on touch screen activity.
> 
> The I2C core automatically reads the "wakeup-source" DT property to
> configure a device's wakeup capability, and board supports files can set
> I2C_CLIENT_WAKE in the flags.

This will break wake-up on working systems. Looks like mostly i.MX, but 
there's one AM437x board. If that board doesn't care, then it is up to 
Shawn.

Rob

Re: [PATCH v2 3/3] ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source

[Dmitry Torokhov]

On Sun, May 20, 2018 at 09:05:30PM +0800, Shawn Guo wrote:
> On Thu, May 17, 2018 at 11:05:52AM +0200, Daniel Mack wrote:
> > The touchscreen driver no longer configures the device as wakeup source by
> > default. A "wakeup-source" property is needed.
> > 
> > To avoid regressions, this patch changes the DTS files for the only two
> > users of this driver that didn't have this property yet.
> > 
> > Signed-off-by: Daniel Mack <daniel@zonque.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> 
> Applied this one, thanks.

I think there are few more that need "wakeup-source" added:

arch/arm/boot/dts/am437x-sk-evm.dts
arch/arm/boot/dts/imx6q-var-dt6customboard.dts
arch/arm/boot/dts/imx6qdl-nit6xlite.dtsi
arch/arm/boot/dts/imx6qdl-nitrogen6_max.dtsi
arch/arm/boot/dts/imx6qdl-nitrogen6_som2.dtsi
arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/3] ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source

[Daniel Mack]

On Tuesday, May 22, 2018 08:20 PM, Dmitry Torokhov wrote:
> On Sun, May 20, 2018 at 09:05:30PM +0800, Shawn Guo wrote:
>> On Thu, May 17, 2018 at 11:05:52AM +0200, Daniel Mack wrote:
>>> The touchscreen driver no longer configures the device as wakeup source by
>>> default. A "wakeup-source" property is needed.
>>>
>>> To avoid regressions, this patch changes the DTS files for the only two
>>> users of this driver that didn't have this property yet.
>>>
>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>
>> Applied this one, thanks.
> 
> I think there are few more that need "wakeup-source" added:
> 
> arch/arm/boot/dts/am437x-sk-evm.dts
> arch/arm/boot/dts/imx6q-var-dt6customboard.dts
> arch/arm/boot/dts/imx6qdl-nit6xlite.dtsi
> arch/arm/boot/dts/imx6qdl-nitrogen6_max.dtsi
> arch/arm/boot/dts/imx6qdl-nitrogen6_som2.dtsi
> arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi

Oh, sorry for having missed them. Will send patches.


Thanks,
Daniel

Re: [PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Daniel Mack]

On Tuesday, May 22, 2018 07:54 PM, Rob Herring wrote:
> On Thu, May 17, 2018 at 11:05:50AM +0200, Daniel Mack wrote:
>> Allow configuring the device as wakeup source through device properties, as
>> not all platforms want to wake up on touch screen activity.
>>
>> The I2C core automatically reads the "wakeup-source" DT property to
>> configure a device's wakeup capability, and board supports files can set
>> I2C_CLIENT_WAKE in the flags.
> 
> This will break wake-up on working systems. Looks like mostly i.MX, but
> there's one AM437x board. If that board doesn't care, then it is up to
> Shawn.

I added the property to the dts files, but as Dmitry pointed out, I 
missed some. Sorry for that.


Thanks,
Daniel

Re: [PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Rob Herring]

On Wed, May 23, 2018 at 3:27 AM, Daniel Mack <daniel@zonque.org> wrote:
> On Tuesday, May 22, 2018 07:54 PM, Rob Herring wrote:
>>
>> On Thu, May 17, 2018 at 11:05:50AM +0200, Daniel Mack wrote:
>>>
>>> Allow configuring the device as wakeup source through device properties,
>>> as
>>> not all platforms want to wake up on touch screen activity.
>>>
>>> The I2C core automatically reads the "wakeup-source" DT property to
>>> configure a device's wakeup capability, and board supports files can set
>>> I2C_CLIENT_WAKE in the flags.
>>
>>
>> This will break wake-up on working systems. Looks like mostly i.MX, but
>> there's one AM437x board. If that board doesn't care, then it is up to
>> Shawn.
>
>
> I added the property to the dts files, but as Dmitry pointed out, I missed
> some. Sorry for that.

Just adding the property to dts files doesn't fix the compatibility
problem. If a user uses an existing dtb (before this change) with a
new kernel (after this change), then wakeup will stop working.

Rob

Re: [PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Dmitry Torokhov]

On Wed, May 23, 2018 at 09:45:05AM -0500, Rob Herring wrote:
> On Wed, May 23, 2018 at 3:27 AM, Daniel Mack <daniel@zonque.org> wrote:
> > On Tuesday, May 22, 2018 07:54 PM, Rob Herring wrote:
> >>
> >> On Thu, May 17, 2018 at 11:05:50AM +0200, Daniel Mack wrote:
> >>>
> >>> Allow configuring the device as wakeup source through device properties,
> >>> as
> >>> not all platforms want to wake up on touch screen activity.
> >>>
> >>> The I2C core automatically reads the "wakeup-source" DT property to
> >>> configure a device's wakeup capability, and board supports files can set
> >>> I2C_CLIENT_WAKE in the flags.
> >>
> >>
> >> This will break wake-up on working systems. Looks like mostly i.MX, but
> >> there's one AM437x board. If that board doesn't care, then it is up to
> >> Shawn.
> >
> >
> > I added the property to the dts files, but as Dmitry pointed out, I missed
> > some. Sorry for that.
> 
> Just adding the property to dts files doesn't fix the compatibility
> problem. If a user uses an existing dtb (before this change) with a
> new kernel (after this change), then wakeup will stop working.

Is this a practical problem though? Do we know of any products with
this touch panel that use DTS not distributed with the kernel?

-- 
Dmitry

Re: [PATCH v2 1/3] input: touchscreen: edt-ft5x06: don't make device a wakeup source by default

[Rob Herring]

On Thu, May 24, 2018 at 6:17 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, May 23, 2018 at 09:45:05AM -0500, Rob Herring wrote:
>> On Wed, May 23, 2018 at 3:27 AM, Daniel Mack <daniel@zonque.org> wrote:
>> > On Tuesday, May 22, 2018 07:54 PM, Rob Herring wrote:
>> >>
>> >> On Thu, May 17, 2018 at 11:05:50AM +0200, Daniel Mack wrote:
>> >>>
>> >>> Allow configuring the device as wakeup source through device properties,
>> >>> as
>> >>> not all platforms want to wake up on touch screen activity.
>> >>>
>> >>> The I2C core automatically reads the "wakeup-source" DT property to
>> >>> configure a device's wakeup capability, and board supports files can set
>> >>> I2C_CLIENT_WAKE in the flags.
>> >>
>> >>
>> >> This will break wake-up on working systems. Looks like mostly i.MX, but
>> >> there's one AM437x board. If that board doesn't care, then it is up to
>> >> Shawn.
>> >
>> >
>> > I added the property to the dts files, but as Dmitry pointed out, I missed
>> > some. Sorry for that.
>>
>> Just adding the property to dts files doesn't fix the compatibility
>> problem. If a user uses an existing dtb (before this change) with a
>> new kernel (after this change), then wakeup will stop working.
>
> Is this a practical problem though? Do we know of any products with
> this touch panel that use DTS not distributed with the kernel?

Distribution of dts with kernel is irrelevant. It is how the dtb's are
handled that matters. You still need to ask in tree users.

I can't have any way of knowing what DTs may exist as there is only
one upstream repository of dts files. There's not really a good
solution on this to avoid breaking users, so we'll just have to see if
anyone (besides the known users) complains.

Rob