[PATCH v2 0/2] sun8i-a83t: Add touchscreen support on TBS A711

[PATCH v2 0/2] sun8i-a83t: Add touchscreen support on TBS A711

[Mylène Josserand]

Hello everyone,

This is a V2 of the patch series that adds touchscreen support
(FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC).
Based on last linux-next (next-20171222).

Changes since v1:
   - Remove patches 01 and 02 as Chen-Yu Tsai sent a similar patch:
   https://patchwork.kernel.org/patch/10111431/
   and it is merged on last next-20171222.
   (See commit f066f46ce5a5 "ARM: dts: sun8i: a83t: Add I2C device nodes and pinmux settings")
   - Update regulator according to Dmitry Torokhov's review: remove "optional"
   suffix while retrieving the regulator, rename it into "vcc" instead of
   "power" and add bindings documentation.
   - Update device tree according to Maxime Ripard's review: remove the
   label and rename the node.
   - Squash patch 03 with patch 05 to add I2C0 and touchscreen's node
   in one patch (see patch 02).

Patch 01: Add support for regulator in the FocalTech touchscreen driver
because A711 tablet is using a regulator to power-up the touchscreen.
Patch 02: Add i2c0 and touchscreen's node for A711 TBS tablet.

Thank you in advance for any review.
Best regards,
Myl?ne

Myl?ne Josserand (2):
  Input: edt-ft5x06 - Add support for regulator
  arm: dts: sun8i: a83t: a711: Add touchscreen node

 .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts          | 16 +++++++++++
 drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
 3 files changed, 50 insertions(+)

-- 
2.11.0

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

[Mylène Josserand]

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;
+	};
+
+	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;
 }
 
-- 
2.11.0

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

[kbuild test robot]

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

[PATCH] Input: fix semicolon.cocci warnings

[kbuild test robot]

From: Fengguang Wu <fengguang.wu@intel.com>

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


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: 5969d946e8aa ("Input: edt-ft5x06 - Add support for regulator")
CC: Myl?ne Josserand <mylene.josserand@free-electrons.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 edt-ft5x06.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1001,7 +1001,7 @@ static int edt_ft5x06_ts_probe(struct i2
 		dev_err(&client->dev, "failed to request regulator: %d\n",
 			error);
 		return error;
-	};
+	}
 
 	if (tsdata->vcc) {
 		error = regulator_enable(tsdata->vcc);

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

[Rob Herring]

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>

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

[Maxime Ripard]

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?

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180122/37728613/attachment.sig>

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Lothar Waßmann]

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

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

[Mylene Josserand]

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,

-- 
Myl?ne Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Mylene Josserand]

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

-- 
Myl?ne Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Mylene Josserand]

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,

-- 
Myl?ne Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 2/2] arm: dts: sun8i: a83t: a711: Add touchscreen node

[Mylène Josserand]

Tha A711 tablet has a FocalTech EDT-FT5x06 Polytouch touchscreen.
It is connected via I2C0. The reset line is PD5, the interrupt
line is PL7 and the VCC supply is the ldo_io0 regulator.

Signed-off-by: Myl?ne Josserand <mylene.josserand@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index a021ee6da396..7840f9aa9094 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -105,6 +105,22 @@
 	status = "okay";
 };
 
+&i2c0 {
+	clock-frequency = <400000>;
+	status = "okay";
+
+	touchscreen at 38 {
+		compatible = "edt,edt-ft5x06";
+		reg = <0x38>;
+		interrupt-parent = <&r_pio>;
+		interrupts = <0 7 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&pio 3 5 GPIO_ACTIVE_LOW>;
+		vcc-supply = <&reg_ldo_io0>;
+		touchscreen-size-x = <1024>;
+		touchscreen-size-y = <600>;
+	};
+};
+
 &mmc0 {
 	vmmc-supply = <&reg_dcdc1>;
 	pinctrl-names = "default";
-- 
2.11.0

[PATCH v2 0/2] sun8i-a83t: Add touchscreen support on TBS A711

[Mylene JOSSERAND]

Hello,

Le Thu, 28 Dec 2017 17:33:34 +0100,
Myl?ne Josserand <mylene.josserand@free-electrons.com> a ?crit :

> Hello everyone,
> 
> This is a V2 of the patch series that adds touchscreen support
> (FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC).
> Based on last linux-next (next-20171222).
> 
> Changes since v1:
>    - Remove patches 01 and 02 as Chen-Yu Tsai sent a similar patch:
>    https://patchwork.kernel.org/patch/10111431/
>    and it is merged on last next-20171222.
>    (See commit f066f46ce5a5 "ARM: dts: sun8i: a83t: Add I2C device nodes and pinmux settings")
>    - Update regulator according to Dmitry Torokhov's review: remove "optional"
>    suffix while retrieving the regulator, rename it into "vcc" instead of
>    "power" and add bindings documentation.

I notice that I forgot the second review of Dmitry about reset/wake
gpios so I will send a V3 with the modifications.

Thanks,

Myl?ne

>    - Update device tree according to Maxime Ripard's review: remove the
>    label and rename the node.
>    - Squash patch 03 with patch 05 to add I2C0 and touchscreen's node
>    in one patch (see patch 02).
> 
> Patch 01: Add support for regulator in the FocalTech touchscreen driver
> because A711 tablet is using a regulator to power-up the touchscreen.
> Patch 02: Add i2c0 and touchscreen's node for A711 TBS tablet.
> 
> Thank you in advance for any review.
> Best regards,
> Myl?ne
> 
> Myl?ne Josserand (2):
>   Input: edt-ft5x06 - Add support for regulator
>   arm: dts: sun8i: a83t: a711: Add touchscreen node
> 
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
>  arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts          | 16 +++++++++++
>  drivers/input/touchscreen/edt-ft5x06.c             | 33 ++++++++++++++++++++++
>  3 files changed, 50 insertions(+)
>