| Message ID | 20200108111050.19001-7-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDT-FT5x06 improvements | expand |
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.
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 >
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 > >
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 > > > > >
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.
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,
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(+)