[PATCH v4 0/3] sun8i-a83t: Add touchscreen support on TBS A711

[PATCH v4 0/3] sun8i-a83t: Add touchscreen support on TBS A711

[Mylène Josserand]

Hello everyone,

This is a V4 of the patch series that adds touchscreen support
(FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC)
and add regulator support for this touchscreen.
Based on last master of linux-input tree.

Changes since v3:
   - Created a new function to disable regulator via
   devm_add_action_or_reset (Dmitry Torokhov's review)
   - Moved regulator_enable/disable functions in device_may_wakeup
   guard (Ondřej Jirman and Dmitry Torokhov's reviews)
   - Added Rob Herring Reviewed-by on patch 01 (sorry again)
Changes since v2:
   - Removed the check if regulator is NULL (Dmitry Torokhov's review)
   - Added EPROBE_DEFER error not to print a error message (Lothar Waßmann's review)
   - Added set wake/reset on suspend/resume.
Changes since v1:
   - Removed 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")
   - Updated regulator according to Dmitry Torokhov's review: removed "optional"
   suffix while retrieving the regulator, renamed it into "vcc" instead of
   "power" and added bindings documentation.
   - Updated device tree according to Maxime Ripard's review: remove the
   label and rename the node.
   - Squashed 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 a set wake/reset values on resume and suspend.
Patch 03: Add i2c0 and touchscreen's node for A711 TBS tablet.

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

Mylène Josserand (3):
  Input: edt-ft5x06 - Add support for regulator
  Input: edt-ft5x06 - Set wake/reset values on resume/suspend
  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             | 55 ++++++++++++++++++++++
 3 files changed, 72 insertions(+)

-- 
2.11.0

[PATCH v4 1/3] 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@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
 drivers/input/touchscreen/edt-ft5x06.c             | 43 ++++++++++++++++++++++
 2 files changed, 44 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 1e18ca0d1b4e..dcde719094f7 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;
@@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
 	}
 }
 
+static void edt_ft5x06_disable_regulator(void *arg)
+{
+	struct edt_ft5x06_ts_data *data = arg;
+
+	regulator_disable(data->vcc);
+}
+
 static int edt_ft5x06_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
@@ -991,6 +1000,28 @@ 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);
+		if (error != -EPROBE_DEFER)
+			dev_err(&client->dev, "failed to request regulator: %d\n",
+				error);
+		return error;
+	}
+
+	error = regulator_enable(tsdata->vcc);
+	if (error < 0) {
+		dev_err(&client->dev, "failed to enable vcc: %d\n",
+			error);
+		return error;
+	}
+
+	error = devm_add_action_or_reset(&client->dev,
+					 edt_ft5x06_disable_regulator,
+					 tsdata);
+	if (error)
+		return error;
+
 	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
 						     "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(tsdata->reset_gpio)) {
@@ -1120,9 +1151,12 @@ 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);
+	else
+		regulator_disable(tsdata->vcc);
 
 	return 0;
 }
@@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
+	int ret;
 
 	if (device_may_wakeup(dev))
 		disable_irq_wake(client->irq);
+	else {
+		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 v4 2/3] Input: edt-ft5x06 - Set wake/reset values on resume/suspend

[Mylène Josserand]

On resume and suspend, set the value of wake and reset gpios
to be sure that we are in a know state after suspending/resuming.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index dcde719094f7..dad2f1f8bf89 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1158,6 +1158,12 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 	else
 		regulator_disable(tsdata->vcc);
 
+	if (tsdata->wake_gpio)
+		gpiod_set_value(tsdata->wake_gpio, 0);
+
+	if (tsdata->reset_gpio)
+		gpiod_set_value(tsdata->reset_gpio, 1);
+
 	return 0;
 }
 
@@ -1177,6 +1183,12 @@ static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
 		}
 	}
 
+	if (tsdata->wake_gpio)
+		gpiod_set_value(tsdata->wake_gpio, 1);
+
+	if (tsdata->reset_gpio)
+		gpiod_set_value(tsdata->reset_gpio, 0);
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH v4 3/3] 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@bootlin.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 1537ce148cc1..dc7b94a6c068 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -156,6 +156,22 @@
 	status = "okay";
 };
 
+&i2c0 {
+	clock-frequency = <400000>;
+	status = "okay";
+
+	touchscreen@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

Re: [PATCH v4 1/3] Input: edt-ft5x06 - Add support for regulator

[Dmitry Torokhov]

Hi Mylène,

On Wed, Jul 25, 2018 at 09:34:08AM +0200, Mylène Josserand wrote:
> Add the support of regulator to use it as VCC source.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
>  drivers/input/touchscreen/edt-ft5x06.c             | 43 ++++++++++++++++++++++
>  2 files changed, 44 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 1e18ca0d1b4e..dcde719094f7 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;
> @@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
>  	}
>  }
>  
> +static void edt_ft5x06_disable_regulator(void *arg)
> +{
> +	struct edt_ft5x06_ts_data *data = arg;
> +
> +	regulator_disable(data->vcc);
> +}
> +
>  static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  					 const struct i2c_device_id *id)
>  {
> @@ -991,6 +1000,28 @@ 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);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(&client->dev, "failed to request regulator: %d\n",
> +				error);
> +		return error;
> +	}
> +
> +	error = regulator_enable(tsdata->vcc);
> +	if (error < 0) {
> +		dev_err(&client->dev, "failed to enable vcc: %d\n",
> +			error);
> +		return error;
> +	}

It is better to put the chip into reset and then power up the regulatori
and take it out of the reset, rather than power up and then toggle reset
on and off.

> +
> +	error = devm_add_action_or_reset(&client->dev,
> +					 edt_ft5x06_disable_regulator,
> +					 tsdata);
> +	if (error)
> +		return error;
> +
>  	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
>  						     "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(tsdata->reset_gpio)) {
> @@ -1120,9 +1151,12 @@ 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);
> +	else
> +		regulator_disable(tsdata->vcc);
>  
>  	return 0;
>  }
> @@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
>  static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> +	int ret;
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(client->irq);
> +	else {
> +		ret = regulator_enable(tsdata->vcc);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable vcc: %d\n", ret);
> +			return ret;
> +		}
> +	}

I believe I mentioned in other review that once you powered up the
device, you need to restore its settings, include switching to factory
mode, if it was in factory mode, and restoring threshold/gain/offset
settings.

Thanks.

-- 
Dmitry

Re: [PATCH v4 2/3] Input: edt-ft5x06 - Set wake/reset values on resume/suspend

[Dmitry Torokhov]

Hi Mylène,

On Wed, Jul 25, 2018 at 09:34:09AM +0200, Mylène Josserand wrote:
> On resume and suspend, set the value of wake and reset gpios
> to be sure that we are in a know state after suspending/resuming.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index dcde719094f7..dad2f1f8bf89 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1158,6 +1158,12 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
>  	else
>  		regulator_disable(tsdata->vcc);
>  
> +	if (tsdata->wake_gpio)
> +		gpiod_set_value(tsdata->wake_gpio, 0);
> +
> +	if (tsdata->reset_gpio)
> +		gpiod_set_value(tsdata->reset_gpio, 1);

Ondřej mentioned in previous review that if you power off the controller
it will not be able to wake up the system, and you had to move call to
regulator_disable() into "else" branch of check whether the controller
is a wakeup device. Guess what happens if you unconditionally put the
device into reset state?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/3] Input: edt-ft5x06 - Add support for regulator

[Mylène Josserand]

Hello Dmitry,

Thank you again for the review.

On Wed, 25 Jul 2018 17:47:32 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Mylène,
> 
> On Wed, Jul 25, 2018 at 09:34:08AM +0200, Mylène Josserand wrote:
> > Add the support of regulator to use it as VCC source.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> >  drivers/input/touchscreen/edt-ft5x06.c             | 43 ++++++++++++++++++++++
> >  2 files changed, 44 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 1e18ca0d1b4e..dcde719094f7 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;
> > @@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
> >  	}
> >  }
> >  
> > +static void edt_ft5x06_disable_regulator(void *arg)
> > +{
> > +	struct edt_ft5x06_ts_data *data = arg;
> > +
> > +	regulator_disable(data->vcc);
> > +}
> > +
> >  static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  					 const struct i2c_device_id *id)
> >  {
> > @@ -991,6 +1000,28 @@ 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);
> > +		if (error != -EPROBE_DEFER)
> > +			dev_err(&client->dev, "failed to request regulator: %d\n",
> > +				error);
> > +		return error;
> > +	}
> > +
> > +	error = regulator_enable(tsdata->vcc);
> > +	if (error < 0) {
> > +		dev_err(&client->dev, "failed to enable vcc: %d\n",
> > +			error);
> > +		return error;
> > +	}  
> 
> It is better to put the chip into reset and then power up the regulatori
> and take it out of the reset, rather than power up and then toggle reset
> on and off.

okay, thanks, I will update it.

> 
> > +
> > +	error = devm_add_action_or_reset(&client->dev,
> > +					 edt_ft5x06_disable_regulator,
> > +					 tsdata);
> > +	if (error)
> > +		return error;
> > +
> >  	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> >  						     "reset", GPIOD_OUT_HIGH);
> >  	if (IS_ERR(tsdata->reset_gpio)) {
> > @@ -1120,9 +1151,12 @@ 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);
> > +	else
> > +		regulator_disable(tsdata->vcc);
> >  
> >  	return 0;
> >  }
> > @@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> >  static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > +	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> > +	int ret;
> >  
> >  	if (device_may_wakeup(dev))
> >  		disable_irq_wake(client->irq);
> > +	else {
> > +		ret = regulator_enable(tsdata->vcc);
> > +		if (ret < 0) {
> > +			dev_err(dev, "failed to enable vcc: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}  
> 
> I believe I mentioned in other review that once you powered up the
> device, you need to restore its settings, include switching to factory
> mode, if it was in factory mode, and restoring threshold/gain/offset
> settings.

Yes, I will update the driver with that but as I told you in a previous
mail, I can't test the suspend/resume :(

Best regards,

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/3] Input: edt-ft5x06 - Add support for regulator

[Dmitry Torokhov]

On Tue, Aug 07, 2018 at 08:59:44AM +0200, Mylène Josserand wrote:
> Hello Dmitry,
> 
> Thank you again for the review.
> 
> On Wed, 25 Jul 2018 17:47:32 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi Mylène,
> > 
> > On Wed, Jul 25, 2018 at 09:34:08AM +0200, Mylène Josserand wrote:
> > > Add the support of regulator to use it as VCC source.
> > > 
> > > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  .../bindings/input/touchscreen/edt-ft5x06.txt      |  1 +
> > >  drivers/input/touchscreen/edt-ft5x06.c             | 43 ++++++++++++++++++++++
> > >  2 files changed, 44 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 1e18ca0d1b4e..dcde719094f7 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;
> > > @@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
> > >  	}
> > >  }
> > >  
> > > +static void edt_ft5x06_disable_regulator(void *arg)
> > > +{
> > > +	struct edt_ft5x06_ts_data *data = arg;
> > > +
> > > +	regulator_disable(data->vcc);
> > > +}
> > > +
> > >  static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >  					 const struct i2c_device_id *id)
> > >  {
> > > @@ -991,6 +1000,28 @@ 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);
> > > +		if (error != -EPROBE_DEFER)
> > > +			dev_err(&client->dev, "failed to request regulator: %d\n",
> > > +				error);
> > > +		return error;
> > > +	}
> > > +
> > > +	error = regulator_enable(tsdata->vcc);
> > > +	if (error < 0) {
> > > +		dev_err(&client->dev, "failed to enable vcc: %d\n",
> > > +			error);
> > > +		return error;
> > > +	}  
> > 
> > It is better to put the chip into reset and then power up the regulatori
> > and take it out of the reset, rather than power up and then toggle reset
> > on and off.
> 
> okay, thanks, I will update it.
> 
> > 
> > > +
> > > +	error = devm_add_action_or_reset(&client->dev,
> > > +					 edt_ft5x06_disable_regulator,
> > > +					 tsdata);
> > > +	if (error)
> > > +		return error;
> > > +
> > >  	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> > >  						     "reset", GPIOD_OUT_HIGH);
> > >  	if (IS_ERR(tsdata->reset_gpio)) {
> > > @@ -1120,9 +1151,12 @@ 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);
> > > +	else
> > > +		regulator_disable(tsdata->vcc);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> > >  static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> > > +	int ret;
> > >  
> > >  	if (device_may_wakeup(dev))
> > >  		disable_irq_wake(client->irq);
> > > +	else {
> > > +		ret = regulator_enable(tsdata->vcc);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "failed to enable vcc: %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}  
> > 
> > I believe I mentioned in other review that once you powered up the
> > device, you need to restore its settings, include switching to factory
> > mode, if it was in factory mode, and restoring threshold/gain/offset
> > settings.
> 
> Yes, I will update the driver with that but as I told you in a previous
> mail, I can't test the suspend/resume :(

I wonder if Simon or Lothar or Franklin could help us with it (CCed). If
not, then we'll have to go by review only.

Thanks.

-- 
Dmitry