[PATCH v3 0/6] EDT-FT5x06 improvements

[PATCH v3 0/6] EDT-FT5x06 improvements

[Marco Felsch]

Hi,

I added all given git-tags and Dmitry's comment on my v2 [1]. Patch
"Input: edt-ft5x06 - use pm core to enable/disable the wake irq" is the
result of Dmitry's preliminary patch request.

Regards,
  Marco

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

Marco Felsch (5):
  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 - use pm core to enable/disable the wake irq
  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        | 76 +++++++++++++++----
 2 files changed, 64 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH v3 1/6] 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>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 v3 2/6] 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>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 v3 3/6] 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>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
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 v3 4/6] 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>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3:
- make use of i2c-core wakeup-source handling

v2:
- make use of common wakeup-source property
- adapt commit message
---
 drivers/input/touchscreen/edt-ft5x06.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index e1b31fd525e2..c781952c3409 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1208,7 +1208,6 @@ 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);
 
 	dev_dbg(&client->dev,
 		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
-- 
2.20.1

[PATCH v3 5/6] Input: edt-ft5x06 - use pm core to enable/disable the wake irq

[Marco Felsch]

We do not have to handle the wake-irq within the driver because the pm
core can handle this for us. The only use case for the suspend/resume
callbacks was to handle the wake-irq so we can remove the callbacks.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v3:
- new patch to drop enable/disable_irq_wake() calls
---
 drivers/input/touchscreen/edt-ft5x06.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index c781952c3409..d2587724c52a 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1227,29 +1227,6 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(dev))
-		enable_irq_wake(client->irq);
-
-	return 0;
-}
-
-static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(dev))
-		disable_irq_wake(client->irq);
-
-	return 0;
-}
-
-static SIMPLE_DEV_PM_OPS(edt_ft5x06_ts_pm_ops,
-			 edt_ft5x06_ts_suspend, edt_ft5x06_ts_resume);
-
 static const struct edt_i2c_chip_data edt_ft5x06_data = {
 	.max_support_points = 5,
 };
@@ -1288,7 +1265,6 @@ static struct i2c_driver edt_ft5x06_ts_driver = {
 	.driver = {
 		.name = "edt_ft5x06",
 		.of_match_table = edt_ft5x06_of_match,
-		.pm = &edt_ft5x06_ts_pm_ops,
 	},
 	.id_table = edt_ft5x06_ts_id,
 	.probe    = edt_ft5x06_ts_probe,
-- 
2.20.1

[PATCH v3 6/6] 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>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3:
- drop enable/disable_irq_wake()

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 | 65 ++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index d2587724c52a..4fd97758cbdd 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -38,6 +38,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
@@ -53,6 +56,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
@@ -1227,6 +1231,66 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
 	return 0;
 }
 
+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))
+		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;
+}
+
+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))
+		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;
+}
+
+static SIMPLE_DEV_PM_OPS(edt_ft5x06_ts_pm_ops,
+			 edt_ft5x06_ts_suspend, edt_ft5x06_ts_resume);
+
 static const struct edt_i2c_chip_data edt_ft5x06_data = {
 	.max_support_points = 5,
 };
@@ -1265,6 +1329,7 @@ static struct i2c_driver edt_ft5x06_ts_driver = {
 	.driver = {
 		.name = "edt_ft5x06",
 		.of_match_table = edt_ft5x06_of_match,
+		.pm = &edt_ft5x06_ts_pm_ops,
 	},
 	.id_table = edt_ft5x06_ts_id,
 	.probe    = edt_ft5x06_ts_probe,
-- 
2.20.1

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

[Dmitry Torokhov]

On Wed, Jan 08, 2020 at 12:10:45PM +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>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thank you.

> ---
>  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 v3 2/6] Input: edt-ft5x06 - alphabetical include reorder

[Dmitry Torokhov]

On Wed, Jan 08, 2020 at 12:10:46PM +0100, Marco Felsch wrote:
> 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>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thank you.

> ---
>  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
> 

-- 
Dmitry

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

[Dmitry Torokhov]

On Wed, Jan 08, 2020 at 12:10:47PM +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>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied, thank you.

> ---
> 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
> 

-- 
Dmitry

Re: [PATCH v3 4/6] Input: edt-ft5x06 - make wakeup-source switchable

[Dmitry Torokhov]

On Wed, Jan 08, 2020 at 12:10:48PM +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>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thank you.

> ---
> v3:
> - make use of i2c-core wakeup-source handling
> 
> v2:
> - make use of common wakeup-source property
> - adapt commit message
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index e1b31fd525e2..c781952c3409 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1208,7 +1208,6 @@ 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);
>  
>  	dev_dbg(&client->dev,
>  		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
> -- 
> 2.20.1
> 

-- 
Dmitry

Re: [PATCH v3 5/6] Input: edt-ft5x06 - use pm core to enable/disable the wake irq

[Dmitry Torokhov]

On Wed, Jan 08, 2020 at 12:10:49PM +0100, Marco Felsch wrote:
> We do not have to handle the wake-irq within the driver because the pm
> core can handle this for us. The only use case for the suspend/resume
> callbacks was to handle the wake-irq so we can remove the callbacks.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Applied, thank you.

> ---
> v3:
> - new patch to drop enable/disable_irq_wake() calls
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c781952c3409..d2587724c52a 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1227,29 +1227,6 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -
> -	if (device_may_wakeup(dev))
> -		enable_irq_wake(client->irq);
> -
> -	return 0;
> -}
> -
> -static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -
> -	if (device_may_wakeup(dev))
> -		disable_irq_wake(client->irq);
> -
> -	return 0;
> -}
> -
> -static SIMPLE_DEV_PM_OPS(edt_ft5x06_ts_pm_ops,
> -			 edt_ft5x06_ts_suspend, edt_ft5x06_ts_resume);
> -
>  static const struct edt_i2c_chip_data edt_ft5x06_data = {
>  	.max_support_points = 5,
>  };
> @@ -1288,7 +1265,6 @@ static struct i2c_driver edt_ft5x06_ts_driver = {
>  	.driver = {
>  		.name = "edt_ft5x06",
>  		.of_match_table = edt_ft5x06_of_match,
> -		.pm = &edt_ft5x06_ts_pm_ops,
>  	},
>  	.id_table = edt_ft5x06_ts_id,
>  	.probe    = edt_ft5x06_ts_probe,
> -- 
> 2.20.1
> 

-- 
Dmitry

Re: [PATCH v3 6/6] Input: edt-ft5x06 - improve power management operations

[Dmitry Torokhov]

Hi Marco,

On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> +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))
> +		return 0;
> +
> +	ret = regulator_enable(tsdata->vcc);
> +	if (ret)
> +		dev_warn(dev, "Failed to enable vcc\n");

I wonder if we should not return error here instead of continuing. If
device is not powered up properly we'll have hard time communicating
with it.

The same is for suspend: maybe we should abort if we can't switch off
regulator or write to the device.

Thanks.

-- 
Dmitry

Re: [PATCH v3 6/6] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

Hi Dmitry,

On 20-01-09 17:09, Dmitry Torokhov wrote:
> Hi Marco,
> 
> On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> > +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))
> > +		return 0;
> > +
> > +	ret = regulator_enable(tsdata->vcc);
> > +	if (ret)
> > +		dev_warn(dev, "Failed to enable vcc\n");
> 
> I wonder if we should not return error here instead of continuing. If
> device is not powered up properly we'll have hard time communicating
> with it.

That's a reasonable point.

> The same is for suspend: maybe we should abort if we can't switch off
> regulator or write to the device.

I have no strong opinion about that case but IMHO it's okay to go further
if we can't switch it off. Instead we should print a warning.

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 v3 6/6] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

On 20-01-10 08:16, Marco Felsch wrote:
> Hi Dmitry,
> 
> On 20-01-09 17:09, Dmitry Torokhov wrote:
> > Hi Marco,
> > 
> > On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> > > +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))
> > > +		return 0;
> > > +
> > > +	ret = regulator_enable(tsdata->vcc);
> > > +	if (ret)
> > > +		dev_warn(dev, "Failed to enable vcc\n");
> > 
> > I wonder if we should not return error here instead of continuing. If
> > device is not powered up properly we'll have hard time communicating
> > with it.
> 
> That's a reasonable point.
> 
> > The same is for suspend: maybe we should abort if we can't switch off
> > regulator or write to the device.
> 
> I have no strong opinion about that case but IMHO it's okay to go further
> if we can't switch it off. Instead we should print a warning.

I just noticed that we do that already.. So the suspend case should be
okay.

> Regards,
>   Marco
> 
> > Thanks.
> > 
> > -- 
> > Dmitry
> > 

Re: [PATCH v3 6/6] Input: edt-ft5x06 - improve power management operations

[Marco Felsch]

Hi Dmitry,

On 20-01-10 08:18, Marco Felsch wrote:
> On 20-01-10 08:16, Marco Felsch wrote:
> > Hi Dmitry,
> > 
> > On 20-01-09 17:09, Dmitry Torokhov wrote:
> > > Hi Marco,
> > > 
> > > On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> > > > +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))
> > > > +		return 0;
> > > > +
> > > > +	ret = regulator_enable(tsdata->vcc);
> > > > +	if (ret)
> > > > +		dev_warn(dev, "Failed to enable vcc\n");
> > > 
> > > I wonder if we should not return error here instead of continuing. If
> > > device is not powered up properly we'll have hard time communicating
> > > with it.
> > 
> > That's a reasonable point.
> > 
> > > The same is for suspend: maybe we should abort if we can't switch off
> > > regulator or write to the device.
> > 
> > I have no strong opinion about that case but IMHO it's okay to go further
> > if we can't switch it off. Instead we should print a warning.
> 
> I just noticed that we do that already.. So the suspend case should be
> okay.


Is it okay to check the return val for the resume case only? I want to
prepare a v4 of this patch to get this done.

Regards,
  Marco

> 
> > 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 v3 6/6] Input: edt-ft5x06 - improve power management operations

[Dmitry Torokhov]

Hi Marco,

On Thu, Jan 16, 2020 at 02:32:19PM +0100, Marco Felsch wrote:
> Hi Dmitry,
> 
> On 20-01-10 08:18, Marco Felsch wrote:
> > On 20-01-10 08:16, Marco Felsch wrote:
> > > Hi Dmitry,
> > > 
> > > On 20-01-09 17:09, Dmitry Torokhov wrote:
> > > > Hi Marco,
> > > > 
> > > > On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> > > > > +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))
> > > > > +		return 0;
> > > > > +
> > > > > +	ret = regulator_enable(tsdata->vcc);
> > > > > +	if (ret)
> > > > > +		dev_warn(dev, "Failed to enable vcc\n");
> > > > 
> > > > I wonder if we should not return error here instead of continuing. If
> > > > device is not powered up properly we'll have hard time communicating
> > > > with it.
> > > 
> > > That's a reasonable point.
> > > 
> > > > The same is for suspend: maybe we should abort if we can't switch off
> > > > regulator or write to the device.
> > > 
> > > I have no strong opinion about that case but IMHO it's okay to go further
> > > if we can't switch it off. Instead we should print a warning.
> > 
> > I just noticed that we do that already.. So the suspend case should be
> > okay.
> 
> 
> Is it okay to check the return val for the resume case only? I want to
> prepare a v4 of this patch to get this done.

OK, I now remember my issues with power management in this driver. It
supports factory mode vs operational/normal mode, and updating register
settings at runtime. If you want to cut power off at suspend, then you
need to make sure you restore the mode and register settings at resume
time, not simply revert to normal mode.

Thanks.

-- 
Dmitry