| Message ID | 20191127120948.22251-6-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EDT-FT5x06 improvements | expand |
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); ... } ?
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 > > >
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); ... } ?
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 > > >
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.
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.
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 >
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; }
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(-)