| Message ID | 20190917155808.27818-1-m.felsch@pengutronix.de |
|---|---|
| Headers | show |
| Series | EDT-FT5x06 improvements | expand |
On Tue, Sep 17, 2019 at 05:58:03PM +0200, 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. But asm parts we usually put after linux parts. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/input/touchscreen/edt-ft5x06.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index 3cc4341bbdff..9e328a82b765 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -13,21 +13,21 @@ > * 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 <asm/unaligned.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/slab.h> > +#include <linux/uaccess.h> > > #define WORK_REGISTER_THRESHOLD 0x00 > #define WORK_REGISTER_REPORT_RATE 0x08 > -- > 2.20.1 >
On Tue, Sep 17, 2019 at 05:58:05PM +0200, 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. > > Add a firmware property to address this. The common 'wakeup-source' > property can't be used for that because the driver must be backward > compatible with old firmwares which may assume the default on > wakeup-source behaviour. So we need to go the other way by explicit > disable the wakeup-source capability. > > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/ \ > FT5x26.pdf Please, don't split URLs > int error; > char fw_version[EDT_NAME_LEN]; > + bool en_wakeup; Perhaps wakeup_en?
On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > Currently the driver do not care about the regulator which supplies the > controller. This can lead into problems since we support (deep-)sleep > because the regulator can be turned off before we send the (deep-)sleep > command to the controller. Using a own regulator improves the power > consumption during sleep even more. > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(tsdata->vdd)) { > + error = PTR_ERR(tsdata->vdd); > + if (error == -EPROBE_DEFER) > + return error; Before it worked w/o regulator. You have to make it optional. > + dev_err(&client->dev, > + "Failed to request vdd-supply, error %d\n", error); > + return error; > + } > + > + error = regulator_enable(tsdata->vdd); > + if (error) { > + dev_err(&client->dev, > + "Failed to enable vdd-supply, error %d\n", error); > + return error; > + }
On 19-09-17 19:29, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:03PM +0200, 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. > > But asm parts we usually put after linux parts. Thanks, I will change it in the v2. Regards, Marco > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/input/touchscreen/edt-ft5x06.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > > index 3cc4341bbdff..9e328a82b765 100644 > > --- a/drivers/input/touchscreen/edt-ft5x06.c > > +++ b/drivers/input/touchscreen/edt-ft5x06.c > > @@ -13,21 +13,21 @@ > > * 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 <asm/unaligned.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/slab.h> > > +#include <linux/uaccess.h> > > > > #define WORK_REGISTER_THRESHOLD 0x00 > > #define WORK_REGISTER_REPORT_RATE 0x08 > > -- > > 2.20.1 > > > > -- > With Best Regards, > Andy Shevchenko > > >
On 19-09-17 19:32, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:05PM +0200, 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. > > > > Add a firmware property to address this. The common 'wakeup-source' > > property can't be used for that because the driver must be backward > > compatible with old firmwares which may assume the default on > > wakeup-source behaviour. So we need to go the other way by explicit > > disable the wakeup-source capability. > > > > > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/ \ > > FT5x26.pdf > > Please, don't split URLs Hm.. then checkpatch complains.. If you prefer it, I can change it in the v2. > > int error; > > char fw_version[EDT_NAME_LEN]; > > > + bool en_wakeup; > > Perhaps wakeup_en? I have no strong opinion about that.. Regards, Marco > > -- > With Best Regards, > Andy Shevchenko > > >
On 19-09-17 19:35, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > Currently the driver do not care about the regulator which supplies the > > controller. This can lead into problems since we support (deep-)sleep > > because the regulator can be turned off before we send the (deep-)sleep > > command to the controller. Using a own regulator improves the power > > consumption during sleep even more. > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(tsdata->vdd)) { > > + error = PTR_ERR(tsdata->vdd); > > + if (error == -EPROBE_DEFER) > > + return error; > > Before it worked w/o regulator. You have to make it optional. devm_regulator_get will return a dummy regulator if no one is specified so it is optional. Regards, Marco > > > + dev_err(&client->dev, > > + "Failed to request vdd-supply, error %d\n", error); > > + return error; > > + } > > + > > + error = regulator_enable(tsdata->vdd); > > + if (error) { > > + dev_err(&client->dev, > > + "Failed to enable vdd-supply, error %d\n", error); > > + return error; > > + } > > -- > With Best Regards, > Andy Shevchenko > > >
On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > Currently the driver do not care about the regulator which supplies the > > controller. This can lead into problems since we support (deep-)sleep > > because the regulator can be turned off before we send the (deep-)sleep > > command to the controller. Using a own regulator improves the power > > consumption during sleep even more. > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(tsdata->vdd)) { > > + error = PTR_ERR(tsdata->vdd); > > + if (error == -EPROBE_DEFER) > > + return error; > > Before it worked w/o regulator. You have to make it optional. No, regulators are funky this way. If there isn't one declared in the device tree then a dummy is created automatically. Optional regulators are only to be used when parts of an IC can be powered up separately. Thanks.
On Tue, Sep 17, 2019 at 06:46:39PM +0200, Marco Felsch wrote: > On 19-09-17 19:32, Andy Shevchenko wrote: > > On Tue, Sep 17, 2019 at 05:58:05PM +0200, 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. > > > > > > Add a firmware property to address this. The common 'wakeup-source' > > > property can't be used for that because the driver must be backward > > > compatible with old firmwares which may assume the default on > > > wakeup-source behaviour. So we need to go the other way by explicit > > > disable the wakeup-source capability. > > > > > > > > [1] https://www.newhavendisplay.com/appnotes/datasheets/touchpanel/ \ > > > FT5x26.pdf > > > > Please, don't split URLs > > Hm.. then checkpatch complains.. If you prefer it, I can change it in > the v2. Checkpatch complains about valid things and it complains about insane things. In this case simply ignore it. Thanks.
On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > Currently the driver do not care about the regulator which supplies the > > > controller. This can lead into problems since we support (deep-)sleep > > > because the regulator can be turned off before we send the (deep-)sleep > > > command to the controller. Using a own regulator improves the power > > > consumption during sleep even more. > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > + if (IS_ERR(tsdata->vdd)) { > > > + error = PTR_ERR(tsdata->vdd); > > > + if (error == -EPROBE_DEFER) > > > + return error; > > > > Before it worked w/o regulator. You have to make it optional. > > No, regulators are funky this way. If there isn't one declared in the > device tree then a dummy is created automatically. Optional regulators > are only to be used when parts of an IC can be powered up separately. It depends if platform has full constrains or not. For the former, indeed, you are right, for the latter, we will get -ENODEV.
On Tue, Sep 17, 2019 at 08:12:46PM +0300, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > > Currently the driver do not care about the regulator which supplies the > > > > controller. This can lead into problems since we support (deep-)sleep > > > > because the regulator can be turned off before we send the (deep-)sleep > > > > command to the controller. Using a own regulator improves the power > > > > consumption during sleep even more. > > > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > > + if (IS_ERR(tsdata->vdd)) { > > > > + error = PTR_ERR(tsdata->vdd); > > > > + if (error == -EPROBE_DEFER) > > > > + return error; > > > > > > Before it worked w/o regulator. You have to make it optional. > > > > No, regulators are funky this way. If there isn't one declared in the > > device tree then a dummy is created automatically. Optional regulators > > are only to be used when parts of an IC can be powered up separately. > > It depends if platform has full constrains or not. Full constraints is the default behavior. Do we even have platforms that are not? ACPI and DT are fully-constrained, so that leaves legacy boards... But there isn't a single one in the tree that uses this driver. Thanks.
On Tue, Sep 17, 2019 at 10:21:05AM -0700, Dmitry Torokhov wrote: > On Tue, Sep 17, 2019 at 08:12:46PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > > > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > > > Currently the driver do not care about the regulator which supplies the > > > > > controller. This can lead into problems since we support (deep-)sleep > > > > > because the regulator can be turned off before we send the (deep-)sleep > > > > > command to the controller. Using a own regulator improves the power > > > > > consumption during sleep even more. > > > > > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > > > + if (IS_ERR(tsdata->vdd)) { > > > > > + error = PTR_ERR(tsdata->vdd); > > > > > + if (error == -EPROBE_DEFER) > > > > > + return error; > > > > > > > > Before it worked w/o regulator. You have to make it optional. > > > > > > No, regulators are funky this way. If there isn't one declared in the > > > device tree then a dummy is created automatically. Optional regulators > > > are only to be used when parts of an IC can be powered up separately. > > > > It depends if platform has full constrains or not. > > Full constraints is the default behavior. Do we even have platforms that > are not? ACPI and DT are fully-constrained, so that leaves legacy > boards... But there isn't a single one in the tree that uses this > driver. Fine then!
Hi, this series adds pm support for the edt-ts devices. It also reorder the include struct to be in line with the common rule. Finally it fixes the wakeup-source assumption. Testers are welcome since the EDT-FT devices reacts differently based on their firmware and I only have a evervision based device here.. Regards, Marco Marco Felsch (6): Input: edt-ft5x06 - alphabetical include reorder dt-bindings: Input: edt-ft5x06 - add disable wakeup-source documentation Input: edt-ft5x06 - add support to disable the wakeup-source Input: edt-ft5x06 - add deep sleep mechanism dt-bindings: Input: edt-ft5x06 - add vdd supply documentation Input: edt-ft5x06 - add supply voltage support .../bindings/input/touchscreen/edt-ft5x06.txt | 5 + drivers/input/touchscreen/edt-ft5x06.c | 126 +++++++++++++++--- 2 files changed, 110 insertions(+), 21 deletions(-)