[v2,1/2] Input: edt-ft5x06 - Add support for regulator

Message ID 20171228163336.28131-2-mylene.josserand@free-electrons.com
State New
Headers show
Series
  • sun8i-a83t: Add touchscreen support on TBS A711
Related show

Commit Message

Mylene Josserand Dec. 28, 2017, 4:33 p.m.
Add the support of regulator to use it as VCC source.

Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
---
 .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
 drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

kbuild test robot Dec. 30, 2017, 3:15 a.m. | #1
Hi Mylène,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[cannot apply to input/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/sun8i-a83t-Add-touchscreen-support-on-TBS-A711/20171230-091331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Rob Herring Jan. 3, 2018, 7:48 p.m. | #2
On Thu, Dec 28, 2017 at 05:33:35PM +0100, Mylène Josserand wrote:
> Add the support of regulator to use it as VCC source.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> ---
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
>  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
>  2 files changed, 34 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>
Maxime Ripard Jan. 22, 2018, 12:51 p.m. | #3
Dmitry,

On Thu, Dec 28, 2017 at 05:33:35PM +0100, Mylène Josserand wrote:
> Add the support of regulator to use it as VCC source.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>

Ping?
Dmitry Torokhov Jan. 22, 2018, 5:42 p.m. | #4
Hi Mylène,

On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
<mylene.josserand@free-electrons.com> wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> ---
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
>  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 025cf8c9324a..48e975b9c1aa 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -30,6 +30,7 @@ Required properties:
>  Optional properties:
>   - reset-gpios: GPIO specification for the RESET input
>   - wake-gpios:  GPIO specification for the WAKE input
> + - vcc-supply:  Regulator that supplies the touchscreen
>
>   - pinctrl-names: should be "default"
>   - pinctrl-0:   a phandle pointing to the pin settings for the
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e7..5ee14a25a382 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -39,6 +39,7 @@
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
>  #include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
>
>  #define WORK_REGISTER_THRESHOLD                0x00
>  #define WORK_REGISTER_REPORT_RATE      0x08
> @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
>         struct touchscreen_properties prop;
>         u16 num_x;
>         u16 num_y;
> +       struct regulator *vcc;
>
>         struct gpio_desc *reset_gpio;
>         struct gpio_desc *wake_gpio;
> @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>
>         tsdata->max_support_points = chip_data->max_support_points;
>
> +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> +       if (IS_ERR(tsdata->vcc)) {
> +               error = PTR_ERR(tsdata->vcc);
> +               dev_err(&client->dev, "failed to request regulator: %d\n",
> +                       error);
> +               return error;
> +       };

As 0-day pounted out, this semicolon is not needed.

> +
> +       if (tsdata->vcc) {

You do not need to check for non-NULL here, devm_regulator_get() wil
lnever give you a NULL. If regulator is not defined in DT/board
mappings, then dummy regulator will be provided. You can call
regulator_enable() and regulator_disable() and other regulator APIs
with dummy regulator.

> +               error = regulator_enable(tsdata->vcc);
> +               if (error < 0) {
> +                       dev_err(&client->dev, "failed to enable vcc: %d\n",
> +                               error);
> +                       return error;
> +               }
> +       }
> +
>         tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
>                                                      "reset", GPIOD_OUT_HIGH);
>         if (IS_ERR(tsdata->reset_gpio)) {
> @@ -1122,20 +1141,34 @@ 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);
>
>         if (device_may_wakeup(dev))
>                 enable_irq_wake(client->irq);
>
> +       if (tsdata->vcc)

Same here.

> +               regulator_disable(tsdata->vcc);
> +
>         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))
>                 disable_irq_wake(client->irq);
>
> +       if (tsdata->vcc) {

And here.

> +               ret = regulator_enable(tsdata->vcc);
> +               if (ret < 0) {
> +                       dev_err(dev, "failed to enable vcc: %d\n", ret);
> +                       return ret;
> +               }

Since power to the device may have been cut, I think you need to
restore the register settings to whatever it was (factory vs work
mode, threshold, gain and offset registers, etc, etc).

> +       }
> +
>         return 0;
>  }
>
> --
> 2.11.0
>

Thanks.
Lothar Waßmann Jan. 23, 2018, 8:04 a.m. | #5
Hi,

On Mon, 22 Jan 2018 09:42:08 -0800 Dmitry Torokhov wrote:
> Hi Mylène,
> 
> On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> <mylene.josserand@free-electrons.com> wrote:
> > Add the support of regulator to use it as VCC source.
> >
> > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> > ---
> >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> >  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > index 025cf8c9324a..48e975b9c1aa 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -30,6 +30,7 @@ Required properties:
> >  Optional properties:
> >   - reset-gpios: GPIO specification for the RESET input
> >   - wake-gpios:  GPIO specification for the WAKE input
> > + - vcc-supply:  Regulator that supplies the touchscreen
> >
> >   - pinctrl-names: should be "default"
> >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c53a3d7239e7..5ee14a25a382 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/input/mt.h>
> >  #include <linux/input/touchscreen.h>
> >  #include <linux/of_device.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #define WORK_REGISTER_THRESHOLD                0x00
> >  #define WORK_REGISTER_REPORT_RATE      0x08
> > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> >         struct touchscreen_properties prop;
> >         u16 num_x;
> >         u16 num_y;
> > +       struct regulator *vcc;
> >
> >         struct gpio_desc *reset_gpio;
> >         struct gpio_desc *wake_gpio;
> > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >
> >         tsdata->max_support_points = chip_data->max_support_points;
> >
> > +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> > +       if (IS_ERR(tsdata->vcc)) {
> > +               error = PTR_ERR(tsdata->vcc);
> > +               dev_err(&client->dev, "failed to request regulator: %d\n",
> > +                       error);
>
I would check for -EPROBE_DEFER here and omit the error message in this
case.


Lothar Waßmann
Mylene Josserand Jan. 23, 2018, 9:10 a.m. | #6
Hello Dimitry,

Thank you for the review!

Le Mon, 22 Jan 2018 09:42:08 -0800,
Dmitry Torokhov <dmitry.torokhov@gmail.com> a écrit :

> Hi Mylène,
> 
> On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> <mylene.josserand@free-electrons.com> wrote:
> > Add the support of regulator to use it as VCC source.
> >
> > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> > ---
> >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> >  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > index 025cf8c9324a..48e975b9c1aa 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -30,6 +30,7 @@ Required properties:
> >  Optional properties:
> >   - reset-gpios: GPIO specification for the RESET input
> >   - wake-gpios:  GPIO specification for the WAKE input
> > + - vcc-supply:  Regulator that supplies the touchscreen
> >
> >   - pinctrl-names: should be "default"
> >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c53a3d7239e7..5ee14a25a382 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/input/mt.h>
> >  #include <linux/input/touchscreen.h>
> >  #include <linux/of_device.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #define WORK_REGISTER_THRESHOLD                0x00
> >  #define WORK_REGISTER_REPORT_RATE      0x08
> > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> >         struct touchscreen_properties prop;
> >         u16 num_x;
> >         u16 num_y;
> > +       struct regulator *vcc;
> >
> >         struct gpio_desc *reset_gpio;
> >         struct gpio_desc *wake_gpio;
> > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >
> >         tsdata->max_support_points = chip_data->max_support_points;
> >
> > +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> > +       if (IS_ERR(tsdata->vcc)) {
> > +               error = PTR_ERR(tsdata->vcc);
> > +               dev_err(&client->dev, "failed to request regulator: %d\n",
> > +                       error);
> > +               return error;
> > +       };  
> 
> As 0-day pounted out, this semicolon is not needed.

Yes, thanks, I will fix that in next version.

> 
> > +
> > +       if (tsdata->vcc) {  
> 
> You do not need to check for non-NULL here, devm_regulator_get() wil
> lnever give you a NULL. If regulator is not defined in DT/board
> mappings, then dummy regulator will be provided. You can call
> regulator_enable() and regulator_disable() and other regulator APIs
> with dummy regulator.

Okay, thanks for the explanation, I will remove that.

> 
> > +               error = regulator_enable(tsdata->vcc);
> > +               if (error < 0) {
> > +                       dev_err(&client->dev, "failed to enable vcc: %d\n",
> > +                               error);
> > +                       return error;
> > +               }
> > +       }
> > +
> >         tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> >                                                      "reset", GPIOD_OUT_HIGH);
> >         if (IS_ERR(tsdata->reset_gpio)) {
> > @@ -1122,20 +1141,34 @@ 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);
> >
> >         if (device_may_wakeup(dev))
> >                 enable_irq_wake(client->irq);
> >
> > +       if (tsdata->vcc)  
> 
> Same here.

yep

> 
> > +               regulator_disable(tsdata->vcc);
> > +
> >         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))
> >                 disable_irq_wake(client->irq);
> >
> > +       if (tsdata->vcc) {  
> 
> And here.

yep

> 
> > +               ret = regulator_enable(tsdata->vcc);
> > +               if (ret < 0) {
> > +                       dev_err(dev, "failed to enable vcc: %d\n", ret);
> > +                       return ret;
> > +               }  
> 
> Since power to the device may have been cut, I think you need to
> restore the register settings to whatever it was (factory vs work
> mode, threshold, gain and offset registers, etc, etc).

Okay. Could you tell me how can I do that?

> 
> > +       }
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.11.0
> >  
> 
> Thanks.
> 

About your V2's review, you suggested to add support for wake/reset in
suspend/resume (that I forgot in this version). I wanted to add it but
with my board, I can't test suspend/resume. What should I do about
that?

If I send a V3 in next few days, do you think you will have time to
merge it for v4.16?

Thank you in advance,

Best regards,

Mylène
Mylene Josserand Jan. 23, 2018, 9:13 a.m. | #7
Hello Lothar,

Le Tue, 23 Jan 2018 09:04:14 +0100,
Lothar Waßmann <LW@KARO-electronics.de> a écrit :

> Hi,
> 
> On Mon, 22 Jan 2018 09:42:08 -0800 Dmitry Torokhov wrote:
> > Hi Mylène,
> > 
> > On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> > <mylene.josserand@free-electrons.com> wrote:  
> > > Add the support of regulator to use it as VCC source.
> > >
> > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> > > ---
> > >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> > >  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
> > >  2 files changed, 34 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > index 025cf8c9324a..48e975b9c1aa 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > @@ -30,6 +30,7 @@ Required properties:
> > >  Optional properties:
> > >   - reset-gpios: GPIO specification for the RESET input
> > >   - wake-gpios:  GPIO specification for the WAKE input
> > > + - vcc-supply:  Regulator that supplies the touchscreen
> > >
> > >   - pinctrl-names: should be "default"
> > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index c53a3d7239e7..5ee14a25a382 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -39,6 +39,7 @@
> > >  #include <linux/input/mt.h>
> > >  #include <linux/input/touchscreen.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  #define WORK_REGISTER_THRESHOLD                0x00
> > >  #define WORK_REGISTER_REPORT_RATE      0x08
> > > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> > >         struct touchscreen_properties prop;
> > >         u16 num_x;
> > >         u16 num_y;
> > > +       struct regulator *vcc;
> > >
> > >         struct gpio_desc *reset_gpio;
> > >         struct gpio_desc *wake_gpio;
> > > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >
> > >         tsdata->max_support_points = chip_data->max_support_points;
> > >
> > > +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> > > +       if (IS_ERR(tsdata->vcc)) {
> > > +               error = PTR_ERR(tsdata->vcc);
> > > +               dev_err(&client->dev, "failed to request regulator: %d\n",
> > > +                       error);  
> >  
> I would check for -EPROBE_DEFER here and omit the error message in this
> case.
> 
> 
> Lothar Waßmann

Sure, I will add this case, thank you for the review.

Best regards,
Dmitry Torokhov Jan. 23, 2018, 5:58 p.m. | #8
On Tue, Jan 23, 2018 at 10:10:55AM +0100, Mylene Josserand wrote:
> Hello Dimitry,
> 
> Thank you for the review!
> 
> Le Mon, 22 Jan 2018 09:42:08 -0800,
> Dmitry Torokhov <dmitry.torokhov@gmail.com> a écrit :
> 
> > Hi Mylène,
> > 
> > On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> > <mylene.josserand@free-electrons.com> wrote:
> > > Add the support of regulator to use it as VCC source.
> > >
> > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> > > ---
> > >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> > >  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
> > >  2 files changed, 34 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > index 025cf8c9324a..48e975b9c1aa 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > @@ -30,6 +30,7 @@ Required properties:
> > >  Optional properties:
> > >   - reset-gpios: GPIO specification for the RESET input
> > >   - wake-gpios:  GPIO specification for the WAKE input
> > > + - vcc-supply:  Regulator that supplies the touchscreen
> > >
> > >   - pinctrl-names: should be "default"
> > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index c53a3d7239e7..5ee14a25a382 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -39,6 +39,7 @@
> > >  #include <linux/input/mt.h>
> > >  #include <linux/input/touchscreen.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  #define WORK_REGISTER_THRESHOLD                0x00
> > >  #define WORK_REGISTER_REPORT_RATE      0x08
> > > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> > >         struct touchscreen_properties prop;
> > >         u16 num_x;
> > >         u16 num_y;
> > > +       struct regulator *vcc;
> > >
> > >         struct gpio_desc *reset_gpio;
> > >         struct gpio_desc *wake_gpio;
> > > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >
> > >         tsdata->max_support_points = chip_data->max_support_points;
> > >
> > > +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> > > +       if (IS_ERR(tsdata->vcc)) {
> > > +               error = PTR_ERR(tsdata->vcc);
> > > +               dev_err(&client->dev, "failed to request regulator: %d\n",
> > > +                       error);
> > > +               return error;
> > > +       };  
> > 
> > As 0-day pounted out, this semicolon is not needed.
> 
> Yes, thanks, I will fix that in next version.
> 
> > 
> > > +
> > > +       if (tsdata->vcc) {  
> > 
> > You do not need to check for non-NULL here, devm_regulator_get() wil
> > lnever give you a NULL. If regulator is not defined in DT/board
> > mappings, then dummy regulator will be provided. You can call
> > regulator_enable() and regulator_disable() and other regulator APIs
> > with dummy regulator.
> 
> Okay, thanks for the explanation, I will remove that.
> 
> > 
> > > +               error = regulator_enable(tsdata->vcc);
> > > +               if (error < 0) {
> > > +                       dev_err(&client->dev, "failed to enable vcc: %d\n",
> > > +                               error);
> > > +                       return error;
> > > +               }
> > > +       }
> > > +
> > >         tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> > >                                                      "reset", GPIOD_OUT_HIGH);
> > >         if (IS_ERR(tsdata->reset_gpio)) {
> > > @@ -1122,20 +1141,34 @@ 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);
> > >
> > >         if (device_may_wakeup(dev))
> > >                 enable_irq_wake(client->irq);
> > >
> > > +       if (tsdata->vcc)  
> > 
> > Same here.
> 
> yep
> 
> > 
> > > +               regulator_disable(tsdata->vcc);
> > > +
> > >         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))
> > >                 disable_irq_wake(client->irq);
> > >
> > > +       if (tsdata->vcc) {  
> > 
> > And here.
> 
> yep
> 
> > 
> > > +               ret = regulator_enable(tsdata->vcc);
> > > +               if (ret < 0) {
> > > +                       dev_err(dev, "failed to enable vcc: %d\n", ret);
> > > +                       return ret;
> > > +               }  
> > 
> > Since power to the device may have been cut, I think you need to
> > restore the register settings to whatever it was (factory vs work
> > mode, threshold, gain and offset registers, etc, etc).
> 
> Okay. Could you tell me how can I do that?

If you take a look at edt_ft5x06_work_mode() at the end ther eis code to
restore parameters. You want to factor it out and apply to the device
when resuming. You probably also want to see what mode (factory/normal)
the device was before suspend and restore the previous mode on resume.

> 
> > 
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >
> > > --
> > > 2.11.0
> > >  
> > 
> > Thanks.
> > 
> 
> About your V2's review, you suggested to add support for wake/reset in
> suspend/resume (that I forgot in this version). I wanted to add it but
> with my board, I can't test suspend/resume. What should I do about
> that?

See if Simon Budig <simon.budig@kernelconcepts.de> can help us here.

> 
> If I send a V3 in next few days, do you think you will have time to
> merge it for v4.16?

It all depends on the patch shape...

Thanks.
Mylene Josserand Jan. 26, 2018, 10:11 a.m. | #9
Hello Dmitry,

Le Tue, 23 Jan 2018 09:58:29 -0800,
Dmitry Torokhov <dmitry.torokhov@gmail.com> a écrit :

> On Tue, Jan 23, 2018 at 10:10:55AM +0100, Mylene Josserand wrote:
> > Hello Dimitry,
> > 
> > Thank you for the review!
> > 
> > Le Mon, 22 Jan 2018 09:42:08 -0800,
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> a écrit :
> >   
> > > Hi Mylène,
> > > 
> > > On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> > > <mylene.josserand@free-electrons.com> wrote:  
> > > > Add the support of regulator to use it as VCC source.
> > > >
> > > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> > > > ---
> > > >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> > > >  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
> > > >  2 files changed, 34 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > index 025cf8c9324a..48e975b9c1aa 100644
> > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > @@ -30,6 +30,7 @@ Required properties:
> > > >  Optional properties:
> > > >   - reset-gpios: GPIO specification for the RESET input
> > > >   - wake-gpios:  GPIO specification for the WAKE input
> > > > + - vcc-supply:  Regulator that supplies the touchscreen
> > > >
> > > >   - pinctrl-names: should be "default"
> > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > > index c53a3d7239e7..5ee14a25a382 100644
> > > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > > @@ -39,6 +39,7 @@
> > > >  #include <linux/input/mt.h>
> > > >  #include <linux/input/touchscreen.h>
> > > >  #include <linux/of_device.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >
> > > >  #define WORK_REGISTER_THRESHOLD                0x00
> > > >  #define WORK_REGISTER_REPORT_RATE      0x08
> > > > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> > > >         struct touchscreen_properties prop;
> > > >         u16 num_x;
> > > >         u16 num_y;
> > > > +       struct regulator *vcc;
> > > >
> > > >         struct gpio_desc *reset_gpio;
> > > >         struct gpio_desc *wake_gpio;
> > > > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > > >
> > > >         tsdata->max_support_points = chip_data->max_support_points;
> > > >
> > > > +       tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> > > > +       if (IS_ERR(tsdata->vcc)) {
> > > > +               error = PTR_ERR(tsdata->vcc);
> > > > +               dev_err(&client->dev, "failed to request regulator: %d\n",
> > > > +                       error);
> > > > +               return error;
> > > > +       };    
> > > 
> > > As 0-day pounted out, this semicolon is not needed.  
> > 
> > Yes, thanks, I will fix that in next version.
> >   
> > >   
> > > > +
> > > > +       if (tsdata->vcc) {    
> > > 
> > > You do not need to check for non-NULL here, devm_regulator_get() wil
> > > lnever give you a NULL. If regulator is not defined in DT/board
> > > mappings, then dummy regulator will be provided. You can call
> > > regulator_enable() and regulator_disable() and other regulator APIs
> > > with dummy regulator.  
> > 
> > Okay, thanks for the explanation, I will remove that.
> >   
> > >   
> > > > +               error = regulator_enable(tsdata->vcc);
> > > > +               if (error < 0) {
> > > > +                       dev_err(&client->dev, "failed to enable vcc: %d\n",
> > > > +                               error);
> > > > +                       return error;
> > > > +               }
> > > > +       }
> > > > +
> > > >         tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> > > >                                                      "reset", GPIOD_OUT_HIGH);
> > > >         if (IS_ERR(tsdata->reset_gpio)) {
> > > > @@ -1122,20 +1141,34 @@ 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);
> > > >
> > > >         if (device_may_wakeup(dev))
> > > >                 enable_irq_wake(client->irq);
> > > >
> > > > +       if (tsdata->vcc)    
> > > 
> > > Same here.  
> > 
> > yep
> >   
> > >   
> > > > +               regulator_disable(tsdata->vcc);
> > > > +
> > > >         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))
> > > >                 disable_irq_wake(client->irq);
> > > >
> > > > +       if (tsdata->vcc) {    
> > > 
> > > And here.  
> > 
> > yep
> >   
> > >   
> > > > +               ret = regulator_enable(tsdata->vcc);
> > > > +               if (ret < 0) {
> > > > +                       dev_err(dev, "failed to enable vcc: %d\n", ret);
> > > > +                       return ret;
> > > > +               }    
> > > 
> > > Since power to the device may have been cut, I think you need to
> > > restore the register settings to whatever it was (factory vs work
> > > mode, threshold, gain and offset registers, etc, etc).  
> > 
> > Okay. Could you tell me how can I do that?  
> 
> If you take a look at edt_ft5x06_work_mode() at the end ther eis code to
> restore parameters. You want to factor it out and apply to the device
> when resuming. You probably also want to see what mode (factory/normal)
> the device was before suspend and restore the previous mode on resume.

Okay, thank you for your help.

About the factory/normal mode to be sure that I understand correctly,
in the resume, I should use something like "edt_ft5x06_debugfs_mode_set"
to check the "tsdata->factory_mode" and call "edt_ft5x06_factory_mode"
or "edt_ft5x06_work_mode", is not it?

As the current suspend/resume code is only enabling/disabling an irq, I
would like to understand exactly the modifications you would like.

> 
> >   
> > >   
> > > > +       }
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.11.0
> > > >    
> > > 
> > > Thanks.
> > >   
> > 
> > About your V2's review, you suggested to add support for wake/reset in
> > suspend/resume (that I forgot in this version). I wanted to add it but
> > with my board, I can't test suspend/resume. What should I do about
> > that?  
> 
> See if Simon Budig <simon.budig@kernelconcepts.de> can help us here.

Thank you, I contacted Simon (added him in CC) and unfortunately, he
doesn't have a setup with working suspend/resume so he can't help me
on that.
My patches are ready (except for factory/normal mode, see above) but as
I can't test suspend/resume, I prefer not to send them without testing
it..

> 
> > 
> > If I send a V3 in next few days, do you think you will have time to
> > merge it for v4.16?  
> 
> It all depends on the patch shape...

Sure I understand.

Best regards,

Patch

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 025cf8c9324a..48e975b9c1aa 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -30,6 +30,7 @@  Required properties:
 Optional properties:
  - reset-gpios: GPIO specification for the RESET input
  - wake-gpios:  GPIO specification for the WAKE input
+ - vcc-supply:  Regulator that supplies the touchscreen
 
  - pinctrl-names: should be "default"
  - pinctrl-0:   a phandle pointing to the pin settings for the
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index c53a3d7239e7..5ee14a25a382 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -39,6 +39,7 @@ 
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
 #include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
 
 #define WORK_REGISTER_THRESHOLD		0x00
 #define WORK_REGISTER_REPORT_RATE	0x08
@@ -91,6 +92,7 @@  struct edt_ft5x06_ts_data {
 	struct touchscreen_properties prop;
 	u16 num_x;
 	u16 num_y;
+	struct regulator *vcc;
 
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *wake_gpio;
@@ -993,6 +995,23 @@  static int edt_ft5x06_ts_probe(struct i2c_client *client,
 
 	tsdata->max_support_points = chip_data->max_support_points;
 
+	tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
+	if (IS_ERR(tsdata->vcc)) {
+		error = PTR_ERR(tsdata->vcc);
+		dev_err(&client->dev, "failed to request regulator: %d\n",
+			error);
+		return error;
+	};
+
+	if (tsdata->vcc) {
+		error = regulator_enable(tsdata->vcc);
+		if (error < 0) {
+			dev_err(&client->dev, "failed to enable vcc: %d\n",
+				error);
+			return error;
+		}
+	}
+
 	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
 						     "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(tsdata->reset_gpio)) {
@@ -1122,20 +1141,34 @@  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);
 
 	if (device_may_wakeup(dev))
 		enable_irq_wake(client->irq);
 
+	if (tsdata->vcc)
+		regulator_disable(tsdata->vcc);
+
 	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))
 		disable_irq_wake(client->irq);
 
+	if (tsdata->vcc) {
+		ret = regulator_enable(tsdata->vcc);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable vcc: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }