| Message ID | 20200108111050.19001-1-m.felsch@pengutronix.de |
|---|---|
| Headers | show |
| Series | EDT-FT5x06 improvements | expand |
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 >
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 >
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 >
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 >
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.