[PATCH v2 0/5] EDT-FT5x06 improvements

[PATCH v2 0/5] EDT-FT5x06 improvements

[Marco Felsch]

Hi,

this v2 address all comments made on [1]. See the patch based changelog
for further information.

During the discussion we all agreed to use the common wakeup-src device
property but I added all users of this driver again. So they can provide
their ack-tag.

Regards,
  Marco

[1] https://patchwork.kernel.org/cover/11149039/

Marco Felsch (4):
  Input: edt-ft5x06 - alphabetical include reorder
  dt-bindings: Input: edt-ft5x06 - document wakeup-source capability
  Input: edt-ft5x06 - make wakeup-source switchable
  Input: edt-ft5x06 - improve power management operations

Philipp Zabel (1):
  Input: edt-ft5x06: work around first register access error

 .../bindings/input/touchscreen/edt-ft5x06.txt |  2 +
 drivers/input/touchscreen/edt-ft5x06.c        | 85 ++++++++++++++++---
 2 files changed, 73 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] Input: edt-ft5x06: work around first register access error

[Marco Felsch]

From: Philipp Zabel <p.zabel@pengutronix.de>

The EP0700MLP1 returns bogus data on the first register read access
(reading the threshold parameter from register 0x00):

    edt_ft5x06 2-0038: crc error: 0xfc expected, got 0x40

It ignores writes until then. This patch adds a dummy read after which
the number of sensors and parameter read/writes work correctly.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/input/touchscreen/edt-ft5x06.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index d61731c0037d..b87b1e074f62 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1050,6 +1050,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 {
 	const struct edt_i2c_chip_data *chip_data;
 	struct edt_ft5x06_ts_data *tsdata;
+	u8 buf[2] = { 0xfc, 0x00 };
 	struct input_dev *input;
 	unsigned long irq_flags;
 	int error;
@@ -1140,6 +1141,12 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	/*
+	 * Dummy read access. EP0700MLP1 returns bogus data on the first
+	 * register read access and ignores writes.
+	 */
+	edt_ft5x06_ts_readwrite(tsdata->client, 2, buf, 2, buf);
+
 	edt_ft5x06_ts_set_regs(tsdata);
 	edt_ft5x06_ts_get_defaults(&client->dev, tsdata);
 	edt_ft5x06_ts_get_parameters(tsdata);
-- 
2.20.1

[PATCH v2 2/5] Input: edt-ft5x06 - alphabetical include reorder

[Marco Felsch]

It seems that the include order is historical increased and no one takes
care of it. Fix this to align it with the common rule to be in a
alphabetical order.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---

v2:

- move asm include behind linux includes

 drivers/input/touchscreen/edt-ft5x06.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index b87b1e074f62..e1b31fd525e2 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -13,22 +13,23 @@
  *    http://www.glyn.com/Products/Displays
  */
 
-#include <linux/module.h>
-#include <linux/ratelimit.h>
-#include <linux/irq.h>
-#include <linux/interrupt.h>
-#include <linux/input.h>
-#include <linux/i2c.h>
-#include <linux/kernel.h>
-#include <linux/uaccess.h>
-#include <linux/delay.h>
 #include <linux/debugfs.h>
-#include <linux/slab.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
-#include <asm/unaligned.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/ratelimit.h>
 #include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <asm/unaligned.h>
 
 #define WORK_REGISTER_THRESHOLD		0x00
 #define WORK_REGISTER_REPORT_RATE	0x08
-- 
2.20.1

[PATCH v2 3/5] dt-bindings: Input: edt-ft5x06 - document wakeup-source capability

[Marco Felsch]

The current driver behaviour was to enable the wakeup-source everytime.
After discussion [1] we decided to change that behaviour so the device
will act as wakeup-source only if the "wakeup-source" dt-property is
present.

The patch adds the binding documentation to enable the wakeup-source
capability.

[1] https://patchwork.kernel.org/patch/11149037/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- make use of common dt-property wakeup-source
- adapt commit message

 .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt        | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 0f6950073d6f..0e57315e9cbd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -36,6 +36,8 @@ Optional properties:
  - pinctrl-0:   a phandle pointing to the pin settings for the
                 control gpios
 
+ - wakeup-source: If present the device will act as wakeup-source
+
  - threshold:   allows setting the "click"-threshold in the range
                 from 0 to 80.
 
-- 
2.20.1

[PATCH v2 4/5] Input: edt-ft5x06 - make wakeup-source switchable

[Marco Felsch]

Since day one the touch controller acts as wakeup-source. This seems to
be wrong since the device supports deep-sleep mechanism [1] which
requires a reset to leave it. Also some designs won't use the
touchscreen as wakeup-source.

According discussion [2] we decided to break backward compatibility and
go the common way by using the 'wakeup-source' device-property.

[1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
[2] https://patchwork.kernel.org/patch/11149037/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- make use of common wakeup-source property
- adapt commit message

 drivers/input/touchscreen/edt-ft5x06.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index e1b31fd525e2..8d2ec7947f0e 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -24,6 +24,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/ratelimit.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -1056,6 +1057,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 	unsigned long irq_flags;
 	int error;
 	char fw_version[EDT_NAME_LEN];
+	bool en_wakeup;
 
 	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
 
@@ -1114,6 +1116,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	en_wakeup = device_property_present(&client->dev, "wakeup-source");
+
 	if (tsdata->wake_gpio) {
 		usleep_range(5000, 6000);
 		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
@@ -1208,7 +1212,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		return error;
 
 	edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev));
-	device_init_wakeup(&client->dev, 1);
+	device_init_wakeup(&client->dev, en_wakeup);
 
 	dev_dbg(&client->dev,
 		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
-- 
2.20.1

[PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

It is possible to bring the device into a deep sleep state. To exit this
state the reset or wakeup pin must be toggeled as documented in [1].
Because of the poor documentation I used the several downstream kernels
[2] and other applications notes [3] to indentify the related registers.

Furthermore I added the support to disable the device completely. This is
the most effective power-saving mechanism. Disabling the device don't
change the suspend logic because the hibernate mode needs a hardware
reset anyway.

[1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
[2] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/touchscreen/ft5x_ts.c
    https://github.com/Pablito2020/focaltech-touch-driver/blob/master/ft5336_driver.c
[3] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x16_registers.pdf

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- adapt commit message
- don't return errors during suspend/resume
- replace dev_err() by dev_warn()
- add support to disable the regulator too

 drivers/input/touchscreen/edt-ft5x06.c | 49 ++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 8d2ec7947f0e..0bdd3440f684 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -39,6 +39,9 @@
 #define WORK_REGISTER_NUM_X		0x33
 #define WORK_REGISTER_NUM_Y		0x34
 
+#define PMOD_REGISTER_ACTIVE		0x00
+#define PMOD_REGISTER_HIBERNATE		0x03
+
 #define M09_REGISTER_THRESHOLD		0x80
 #define M09_REGISTER_GAIN		0x92
 #define M09_REGISTER_OFFSET		0x93
@@ -54,6 +57,7 @@
 
 #define WORK_REGISTER_OPMODE		0x3c
 #define FACTORY_REGISTER_OPMODE		0x01
+#define PMOD_REGISTER_OPMODE		0xa5
 
 #define TOUCH_EVENT_DOWN		0x00
 #define TOUCH_EVENT_UP			0x01
@@ -1235,9 +1239,29 @@ 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);
+	int ret;
 
-	if (device_may_wakeup(dev))
+	if (device_may_wakeup(dev)) {
 		enable_irq_wake(client->irq);
+		return 0;
+	}
+
+	/*
+	 * Hibernate mode requires reset or wake signal to recover to active
+	 * state.
+	 */
+	if (!tsdata->wake_gpio && !tsdata->reset_gpio)
+		return 0;
+
+	ret = edt_ft5x06_register_write(tsdata, PMOD_REGISTER_OPMODE,
+					PMOD_REGISTER_HIBERNATE);
+	if (ret)
+		dev_warn(dev, "Failed to set hibernate mode\n");
+
+	ret = regulator_disable(tsdata->vcc);
+	if (ret)
+		dev_warn(dev, "Failed to disable vcc\n");
 
 	return 0;
 }
@@ -1245,9 +1269,30 @@ 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);
+	int ret;
 
-	if (device_may_wakeup(dev))
+	if (device_may_wakeup(dev)) {
 		disable_irq_wake(client->irq);
+		return 0;
+	}
+
+	ret = regulator_enable(tsdata->vcc);
+	if (ret)
+		dev_warn(dev, "Failed to enable vcc\n");
+
+	/* Recover from hibernate mode if hardware supports it */
+	if (tsdata->wake_gpio) {
+		gpiod_set_value_cansleep(tsdata->wake_gpio, 0);
+		usleep_range(5000, 6000);
+		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
+		msleep(300);
+	} else if (tsdata->reset_gpio) {
+		gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
+		usleep_range(5000, 6000);
+		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
+		msleep(300);
+	}
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Andy Shevchenko]

On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:
> It is possible to bring the device into a deep sleep state. To exit this
> state the reset or wakeup pin must be toggeled as documented in [1].
> Because of the poor documentation I used the several downstream kernels
> [2] and other applications notes [3] to indentify the related registers.
> 
> Furthermore I added the support to disable the device completely. This is
> the most effective power-saving mechanism. Disabling the device don't
> change the suspend logic because the hibernate mode needs a hardware
> reset anyway.
> 
> [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> [2] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/touchscreen/ft5x_ts.c
>     https://github.com/Pablito2020/focaltech-touch-driver/blob/master/ft5336_driver.c
> [3] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x16_registers.pdf


> +	/* Recover from hibernate mode if hardware supports it */
> +	if (tsdata->wake_gpio) {
> +		gpiod_set_value_cansleep(tsdata->wake_gpio, 0);
> +		usleep_range(5000, 6000);
> +		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
> +		msleep(300);
> +	} else if (tsdata->reset_gpio) {
> +		gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
> +		usleep_range(5000, 6000);
> +		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> +		msleep(300);
> +	}

Perhaps

static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod)
{
	...
}

...resume(...)
{
	...
	if (wake_gpio)
		...toggle_gpio(wake_gpio);
	else if (reset_gpio)
		...toggle_gpio(reset_gpio);
	...
}

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

Hi Andy,

On 19-11-27 14:59, Andy Shevchenko wrote:
> On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:
> > It is possible to bring the device into a deep sleep state. To exit this
> > state the reset or wakeup pin must be toggeled as documented in [1].
> > Because of the poor documentation I used the several downstream kernels
> > [2] and other applications notes [3] to indentify the related registers.
> > 
> > Furthermore I added the support to disable the device completely. This is
> > the most effective power-saving mechanism. Disabling the device don't
> > change the suspend logic because the hibernate mode needs a hardware
> > reset anyway.
> > 
> > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> > [2] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/touchscreen/ft5x_ts.c
> >     https://github.com/Pablito2020/focaltech-touch-driver/blob/master/ft5336_driver.c
> > [3] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x16_registers.pdf
> 
> 
> > +	/* Recover from hibernate mode if hardware supports it */
> > +	if (tsdata->wake_gpio) {
> > +		gpiod_set_value_cansleep(tsdata->wake_gpio, 0);
> > +		usleep_range(5000, 6000);
> > +		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
> > +		msleep(300);
> > +	} else if (tsdata->reset_gpio) {
> > +		gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
> > +		usleep_range(5000, 6000);
> > +		gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> > +		msleep(300);
> > +	}
> 
> Perhaps
> 
> static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod)
> {
> 	...
> }
> 
> ...resume(...)
> {
> 	...
> 	if (wake_gpio)
> 		...toggle_gpio(wake_gpio);
> 	else if (reset_gpio)
> 		...toggle_gpio(reset_gpio);
> 	...
> }
> 
> ?

Thanks fpr your suggestion but we need to differentiate between reset
and wake logic level. The wake-gpio keeps asserted while the reset is
released. So the edt_ft5x06_ts_toggle_gpio() needs at least a 'is_reset'
parameter but then the simplification is gone.

Regards,
  Marco

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Andy Shevchenko]

On Wed, Nov 27, 2019 at 02:06:02PM +0100, Marco Felsch wrote:
> On 19-11-27 14:59, Andy Shevchenko wrote:
> > On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:

> > Perhaps
> > 
> > static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod)
> > {
> > 	...
> > }
> > 
> > ...resume(...)
> > {
> > 	...
> > 	if (wake_gpio)
> > 		...toggle_gpio(wake_gpio);
> > 	else if (reset_gpio)
> > 		...toggle_gpio(reset_gpio);
> > 	...
> > }
> > 
> > ?
> 
> Thanks fpr your suggestion but we need to differentiate between reset
> and wake logic level. The wake-gpio keeps asserted while the reset is
> released. So the edt_ft5x06_ts_toggle_gpio() needs at least a 'is_reset'
> parameter but then the simplification is gone.


How about this:
static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod, int value)
{
	gpiod_...(..., !value);
	...
	gpiod_...(..., value);
	...
}

...resume(...)
{
	...
	if (wake_gpio)
		...toggle_gpio(wake_gpio, 1);
	else if (reset_gpio)
		...toggle_gpio(reset_gpio, 0);
	...
}

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/5] EDT-FT5x06 improvements

[Andy Shevchenko]

On Wed, Nov 27, 2019 at 01:09:43PM +0100, Marco Felsch wrote:
> Hi,
> 
> this v2 address all comments made on [1]. See the patch based changelog
> for further information.
> 
> During the discussion we all agreed to use the common wakeup-src device
> property but I added all users of this driver again. So they can provide
> their ack-tag.

It doesn't break my case (Adafruit 2.8" TFT + cap/ts).

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Regards,
>   Marco
> 
> [1] https://patchwork.kernel.org/cover/11149039/
> 
> Marco Felsch (4):
>   Input: edt-ft5x06 - alphabetical include reorder
>   dt-bindings: Input: edt-ft5x06 - document wakeup-source capability
>   Input: edt-ft5x06 - make wakeup-source switchable
>   Input: edt-ft5x06 - improve power management operations
> 
> Philipp Zabel (1):
>   Input: edt-ft5x06: work around first register access error
> 
>  .../bindings/input/touchscreen/edt-ft5x06.txt |  2 +
>  drivers/input/touchscreen/edt-ft5x06.c        | 85 ++++++++++++++++---
>  2 files changed, 73 insertions(+), 14 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/5] EDT-FT5x06 improvements

[Andy Shevchenko]

On Wed, Nov 27, 2019 at 01:09:43PM +0100, Marco Felsch wrote:
> Hi,
> 
> this v2 address all comments made on [1]. See the patch based changelog
> for further information.
> 
> During the discussion we all agreed to use the common wakeup-src device
> property but I added all users of this driver again. So they can provide
> their ack-tag.
> 

Independently on the decision on patch 5, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Regards,
>   Marco
> 
> [1] https://patchwork.kernel.org/cover/11149039/
> 
> Marco Felsch (4):
>   Input: edt-ft5x06 - alphabetical include reorder
>   dt-bindings: Input: edt-ft5x06 - document wakeup-source capability
>   Input: edt-ft5x06 - make wakeup-source switchable
>   Input: edt-ft5x06 - improve power management operations
> 
> Philipp Zabel (1):
>   Input: edt-ft5x06: work around first register access error
> 
>  .../bindings/input/touchscreen/edt-ft5x06.txt |  2 +
>  drivers/input/touchscreen/edt-ft5x06.c        | 85 ++++++++++++++++---
>  2 files changed, 73 insertions(+), 14 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

Hi,

On 19-11-27 16:32, Andy Shevchenko wrote:
> On Wed, Nov 27, 2019 at 02:06:02PM +0100, Marco Felsch wrote:
> > On 19-11-27 14:59, Andy Shevchenko wrote:
> > > On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:
> 
> > > Perhaps
> > > 
> > > static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod)
> > > {
> > > 	...
> > > }
> > > 
> > > ...resume(...)
> > > {
> > > 	...
> > > 	if (wake_gpio)
> > > 		...toggle_gpio(wake_gpio);
> > > 	else if (reset_gpio)
> > > 		...toggle_gpio(reset_gpio);
> > > 	...
> > > }
> > > 
> > > ?
> > 
> > Thanks fpr your suggestion but we need to differentiate between reset
> > and wake logic level. The wake-gpio keeps asserted while the reset is
> > released. So the edt_ft5x06_ts_toggle_gpio() needs at least a 'is_reset'
> > parameter but then the simplification is gone.
> 
> 
> How about this:
> static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod, int value)
> {
> 	gpiod_...(..., !value);
> 	...
> 	gpiod_...(..., value);
> 	...
> }
> 
> ...resume(...)
> {
> 	...
> 	if (wake_gpio)
> 		...toggle_gpio(wake_gpio, 1);
> 	else if (reset_gpio)
> 		...toggle_gpio(reset_gpio, 0);
> 	...
> }
> 
> ?

That's possible.. Don't see the improvement yet, but I can prepare a
folllow-up patch if Dmitry wants.

Regards,
  Marco

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/5] EDT-FT5x06 improvements

[Marco Felsch]

Hi Andy,

On 19-11-27 19:14, Andy Shevchenko wrote:
> On Wed, Nov 27, 2019 at 01:09:43PM +0100, Marco Felsch wrote:
> > Hi,
> > 
> > this v2 address all comments made on [1]. See the patch based changelog
> > for further information.
> > 
> > During the discussion we all agreed to use the common wakeup-src device
> > property but I added all users of this driver again. So they can provide
> > their ack-tag.
> > 
> 
> Independently on the decision on patch 5, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for testing and the review =)

Regards,
  Marco

> > Regards,
> >   Marco
> > 
> > [1] https://patchwork.kernel.org/cover/11149039/
> > 
> > Marco Felsch (4):
> >   Input: edt-ft5x06 - alphabetical include reorder
> >   dt-bindings: Input: edt-ft5x06 - document wakeup-source capability
> >   Input: edt-ft5x06 - make wakeup-source switchable
> >   Input: edt-ft5x06 - improve power management operations
> > 
> > Philipp Zabel (1):
> >   Input: edt-ft5x06: work around first register access error
> > 
> >  .../bindings/input/touchscreen/edt-ft5x06.txt |  2 +
> >  drivers/input/touchscreen/edt-ft5x06.c        | 85 ++++++++++++++++---
> >  2 files changed, 73 insertions(+), 14 deletions(-)
> > 
> > -- 
> > 2.20.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Andy Shevchenko]

On Wed, Nov 27, 2019 at 06:25:22PM +0100, Marco Felsch wrote:
> On 19-11-27 16:32, Andy Shevchenko wrote:
> > On Wed, Nov 27, 2019 at 02:06:02PM +0100, Marco Felsch wrote:
> > > On 19-11-27 14:59, Andy Shevchenko wrote:
> > > > On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:

> > static void edt_ft5x06_ts_toggle_gpio(struct gpio_desc *gpiod, int value)
> > {
> > 	gpiod_...(..., !value);
> > 	...
> > 	gpiod_...(..., value);
> > 	...
> > }
> > 
> > ...resume(...)
> > {
> > 	...
> > 	if (wake_gpio)
> > 		...toggle_gpio(wake_gpio, 1);
> > 	else if (reset_gpio)
> > 		...toggle_gpio(reset_gpio, 0);
> > 	...
> > }
> > 
> > ?
> 
> That's possible.. Don't see the improvement yet, but I can prepare a
> folllow-up patch if Dmitry wants.

The improvement is to get rid of duplication of flow how you toggle the GPIO.
But yes, up to Dmitry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] Input: edt-ft5x06: work around first register access error

[Dmitry Torokhov]

On Wed, Nov 27, 2019 at 01:09:44PM +0100, Marco Felsch wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> The EP0700MLP1 returns bogus data on the first register read access
> (reading the threshold parameter from register 0x00):
> 
>     edt_ft5x06 2-0038: crc error: 0xfc expected, got 0x40
> 
> It ignores writes until then. This patch adds a dummy read after which
> the number of sensors and parameter read/writes work correctly.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Need your signed-off-by as well.

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index d61731c0037d..b87b1e074f62 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1050,6 +1050,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  {
>  	const struct edt_i2c_chip_data *chip_data;
>  	struct edt_ft5x06_ts_data *tsdata;
> +	u8 buf[2] = { 0xfc, 0x00 };
>  	struct input_dev *input;
>  	unsigned long irq_flags;
>  	int error;
> @@ -1140,6 +1141,12 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> +	/*
> +	 * Dummy read access. EP0700MLP1 returns bogus data on the first
> +	 * register read access and ignores writes.
> +	 */
> +	edt_ft5x06_ts_readwrite(tsdata->client, 2, buf, 2, buf);
> +
>  	edt_ft5x06_ts_set_regs(tsdata);
>  	edt_ft5x06_ts_get_defaults(&client->dev, tsdata);
>  	edt_ft5x06_ts_get_parameters(tsdata);
> -- 
> 2.20.1
> 

-- 
Dmitry

Re: [PATCH v2 4/5] Input: edt-ft5x06 - make wakeup-source switchable

[Dmitry Torokhov]

On Wed, Nov 27, 2019 at 01:09:47PM +0100, Marco Felsch wrote:
> Since day one the touch controller acts as wakeup-source. This seems to
> be wrong since the device supports deep-sleep mechanism [1] which
> requires a reset to leave it. Also some designs won't use the
> touchscreen as wakeup-source.
> 
> According discussion [2] we decided to break backward compatibility and
> go the common way by using the 'wakeup-source' device-property.
> 
> [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> [2] https://patchwork.kernel.org/patch/11149037/
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - make use of common wakeup-source property
> - adapt commit message
> 
>  drivers/input/touchscreen/edt-ft5x06.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index e1b31fd525e2..8d2ec7947f0e 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -24,6 +24,7 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/ratelimit.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> @@ -1056,6 +1057,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  	unsigned long irq_flags;
>  	int error;
>  	char fw_version[EDT_NAME_LEN];
> +	bool en_wakeup;
>  
>  	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
>  
> @@ -1114,6 +1116,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> +	en_wakeup = device_property_present(&client->dev, "wakeup-source");
> +
>  	if (tsdata->wake_gpio) {
>  		usleep_range(5000, 6000);
>  		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
> @@ -1208,7 +1212,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  		return error;
>  
>  	edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev));
> -	device_init_wakeup(&client->dev, 1);
> +	device_init_wakeup(&client->dev, en_wakeup);

I2C core already marks device as wakeup source if I2C_CLIEMT_WAKE is set
(and the flag is specified when, among other things, device has
"wakeup-source" property).

So the only thing that is needed is to remove

	device_init_wakeup(&client->dev, 1);

line.

Thanks.

-- 
Dmitry

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Dmitry Torokhov]

On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:
> It is possible to bring the device into a deep sleep state. To exit this
> state the reset or wakeup pin must be toggeled as documented in [1].
> Because of the poor documentation I used the several downstream kernels
> [2] and other applications notes [3] to indentify the related registers.
> 
> Furthermore I added the support to disable the device completely. This is
> the most effective power-saving mechanism. Disabling the device don't
> change the suspend logic because the hibernate mode needs a hardware
> reset anyway.
> 
> [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> [2] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/touchscreen/ft5x_ts.c
>     https://github.com/Pablito2020/focaltech-touch-driver/blob/master/ft5336_driver.c
> [3] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x16_registers.pdf
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - adapt commit message
> - don't return errors during suspend/resume
> - replace dev_err() by dev_warn()
> - add support to disable the regulator too
> 
>  drivers/input/touchscreen/edt-ft5x06.c | 49 ++++++++++++++++++++++++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 8d2ec7947f0e..0bdd3440f684 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -39,6 +39,9 @@
>  #define WORK_REGISTER_NUM_X		0x33
>  #define WORK_REGISTER_NUM_Y		0x34
>  
> +#define PMOD_REGISTER_ACTIVE		0x00
> +#define PMOD_REGISTER_HIBERNATE		0x03
> +
>  #define M09_REGISTER_THRESHOLD		0x80
>  #define M09_REGISTER_GAIN		0x92
>  #define M09_REGISTER_OFFSET		0x93
> @@ -54,6 +57,7 @@
>  
>  #define WORK_REGISTER_OPMODE		0x3c
>  #define FACTORY_REGISTER_OPMODE		0x01
> +#define PMOD_REGISTER_OPMODE		0xa5
>  
>  #define TOUCH_EVENT_DOWN		0x00
>  #define TOUCH_EVENT_UP			0x01
> @@ -1235,9 +1239,29 @@ 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);
> +	int ret;
>  
> -	if (device_may_wakeup(dev))
> +	if (device_may_wakeup(dev)) {
>  		enable_irq_wake(client->irq);

Can we start with preliminary patch dropping calls to enable_irq_wake()
and disable_irq_wake() as device/PM core will tale care of configuring
wake irqs properly for us, since we are now allowing I2C core to mark
the interrupt as wake IRQ.

So we need to do:

	if (device_may_wakeup(dev))
		return 0;

	<execute power down sequence>

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/5] Input: edt-ft5x06: work around first register access error

[Marco Felsch]

On 19-12-02 09:58, Dmitry Torokhov wrote:
> On Wed, Nov 27, 2019 at 01:09:44PM +0100, Marco Felsch wrote:
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > The EP0700MLP1 returns bogus data on the first register read access
> > (reading the threshold parameter from register 0x00):
> > 
> >     edt_ft5x06 2-0038: crc error: 0xfc expected, got 0x40
> > 
> > It ignores writes until then. This patch adds a dummy read after which
> > the number of sensors and parameter read/writes work correctly.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Need your signed-off-by as well.

Damn, you're right.. I fix this in the next version.

> 
> > ---
> >  drivers/input/touchscreen/edt-ft5x06.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index d61731c0037d..b87b1e074f62 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -1050,6 +1050,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  {
> >  	const struct edt_i2c_chip_data *chip_data;
> >  	struct edt_ft5x06_ts_data *tsdata;
> > +	u8 buf[2] = { 0xfc, 0x00 };
> >  	struct input_dev *input;
> >  	unsigned long irq_flags;
> >  	int error;
> > @@ -1140,6 +1141,12 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  		return error;
> >  	}
> >  
> > +	/*
> > +	 * Dummy read access. EP0700MLP1 returns bogus data on the first
> > +	 * register read access and ignores writes.
> > +	 */
> > +	edt_ft5x06_ts_readwrite(tsdata->client, 2, buf, 2, buf);
> > +
> >  	edt_ft5x06_ts_set_regs(tsdata);
> >  	edt_ft5x06_ts_get_defaults(&client->dev, tsdata);
> >  	edt_ft5x06_ts_get_parameters(tsdata);
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Dmitry
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/5] dt-bindings: Input: edt-ft5x06 - document wakeup-source capability

[Rob Herring]

On Wed, 27 Nov 2019 13:09:46 +0100, Marco Felsch wrote:
> The current driver behaviour was to enable the wakeup-source everytime.
> After discussion [1] we decided to change that behaviour so the device
> will act as wakeup-source only if the "wakeup-source" dt-property is
> present.
> 
> The patch adds the binding documentation to enable the wakeup-source
> capability.
> 
> [1] https://patchwork.kernel.org/patch/11149037/
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - make use of common dt-property wakeup-source
> - adapt commit message
> 
>  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt        | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 4/5] Input: edt-ft5x06 - make wakeup-source switchable

[Marco Felsch]

On 19-12-02 10:00, Dmitry Torokhov wrote:
> On Wed, Nov 27, 2019 at 01:09:47PM +0100, Marco Felsch wrote:
> > Since day one the touch controller acts as wakeup-source. This seems to
> > be wrong since the device supports deep-sleep mechanism [1] which
> > requires a reset to leave it. Also some designs won't use the
> > touchscreen as wakeup-source.
> > 
> > According discussion [2] we decided to break backward compatibility and
> > go the common way by using the 'wakeup-source' device-property.
> > 
> > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> > [2] https://patchwork.kernel.org/patch/11149037/
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - make use of common wakeup-source property
> > - adapt commit message
> > 
> >  drivers/input/touchscreen/edt-ft5x06.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index e1b31fd525e2..8d2ec7947f0e 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/property.h>
> >  #include <linux/ratelimit.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> > @@ -1056,6 +1057,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  	unsigned long irq_flags;
> >  	int error;
> >  	char fw_version[EDT_NAME_LEN];
> > +	bool en_wakeup;
> >  
> >  	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
> >  
> > @@ -1114,6 +1116,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  		return error;
> >  	}
> >  
> > +	en_wakeup = device_property_present(&client->dev, "wakeup-source");
> > +
> >  	if (tsdata->wake_gpio) {
> >  		usleep_range(5000, 6000);
> >  		gpiod_set_value_cansleep(tsdata->wake_gpio, 1);
> > @@ -1208,7 +1212,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  		return error;
> >  
> >  	edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev));
> > -	device_init_wakeup(&client->dev, 1);
> > +	device_init_wakeup(&client->dev, en_wakeup);
> 
> I2C core already marks device as wakeup source if I2C_CLIEMT_WAKE is set
> (and the flag is specified when, among other things, device has
> "wakeup-source" property).
> 
> So the only thing that is needed is to remove
> 
> 	device_init_wakeup(&client->dev, 1);
> 
> line.

You are right, thanks for covering that.

Regards,
  Marco 

> 
> Thanks.
> 
> -- 
> Dmitry
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 5/5] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

On 19-12-02 10:04, Dmitry Torokhov wrote:
> On Wed, Nov 27, 2019 at 01:09:48PM +0100, Marco Felsch wrote:
> > It is possible to bring the device into a deep sleep state. To exit this
> > state the reset or wakeup pin must be toggeled as documented in [1].
> > Because of the poor documentation I used the several downstream kernels
> > [2] and other applications notes [3] to indentify the related registers.
> > 
> > Furthermore I added the support to disable the device completely. This is
> > the most effective power-saving mechanism. Disabling the device don't
> > change the suspend logic because the hibernate mode needs a hardware
> > reset anyway.
> > 
> > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x26.pdf
> > [2] https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/touchscreen/ft5x_ts.c
> >     https://github.com/Pablito2020/focaltech-touch-driver/blob/master/ft5336_driver.c
> > [3] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/FT5x16_registers.pdf
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - adapt commit message
> > - don't return errors during suspend/resume
> > - replace dev_err() by dev_warn()
> > - add support to disable the regulator too
> > 
> >  drivers/input/touchscreen/edt-ft5x06.c | 49 ++++++++++++++++++++++++--
> >  1 file changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index 8d2ec7947f0e..0bdd3440f684 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -39,6 +39,9 @@
> >  #define WORK_REGISTER_NUM_X		0x33
> >  #define WORK_REGISTER_NUM_Y		0x34
> >  
> > +#define PMOD_REGISTER_ACTIVE		0x00
> > +#define PMOD_REGISTER_HIBERNATE		0x03
> > +
> >  #define M09_REGISTER_THRESHOLD		0x80
> >  #define M09_REGISTER_GAIN		0x92
> >  #define M09_REGISTER_OFFSET		0x93
> > @@ -54,6 +57,7 @@
> >  
> >  #define WORK_REGISTER_OPMODE		0x3c
> >  #define FACTORY_REGISTER_OPMODE		0x01
> > +#define PMOD_REGISTER_OPMODE		0xa5
> >  
> >  #define TOUCH_EVENT_DOWN		0x00
> >  #define TOUCH_EVENT_UP			0x01
> > @@ -1235,9 +1239,29 @@ 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);
> > +	int ret;
> >  
> > -	if (device_may_wakeup(dev))
> > +	if (device_may_wakeup(dev)) {
> >  		enable_irq_wake(client->irq);
> 
> Can we start with preliminary patch dropping calls to enable_irq_wake()
> and disable_irq_wake() as device/PM core will tale care of configuring
> wake irqs properly for us, since we are now allowing I2C core to mark
> the interrupt as wake IRQ.
> 
> So we need to do:
> 
> 	if (device_may_wakeup(dev))
> 		return 0;
> 
> 	<execute power down sequence>
> 
> Thanks.

Of course, thanks for covering that.A

Regards,
  Marco 

> 
> -- 
> Dmitry
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |