[v3,6/6] Input: edt-ft5x06 - improve power management operations
diff mbox series

Message ID 20200108111050.19001-7-m.felsch@pengutronix.de
State New
Headers show
Series
  • EDT-FT5x06 improvements
Related show

Commit Message

Marco Felsch Jan. 8, 2020, 11:10 a.m. UTC
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(+)

Comments

Dmitry Torokhov Jan. 10, 2020, 1:09 a.m. UTC | #1
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.
Marco Felsch Jan. 10, 2020, 7:16 a.m. UTC | #2
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
>
Marco Felsch Jan. 10, 2020, 7:18 a.m. UTC | #3
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
> >
Marco Felsch Jan. 16, 2020, 1:32 p.m. UTC | #4
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
> > > 
> 
>
Dmitry Torokhov Jan. 22, 2020, 6 a.m. UTC | #5
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.

Patch
diff mbox series

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,