Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply

On 21-01-11 10:26, Stephan Gerhold wrote:

Hi Marco,

thanks for the review!

On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:

quoted

Hi Stephan,

thanks for the patch :) Please see my inline comments.

On 21-01-08 20:23, Stephan Gerhold wrote:

quoted

At the moment, the edt-ft5x06 driver can control a single regulator
("vcc"). However, some FocalTech touch controllers have an additional
IOVCC pin that should be supplied with the digital I/O voltage.

The I/O voltage might be provided by another regulator that should also
be kept on. Otherwise, the touchscreen can randomly stop functioning if
the regulator is turned off because no other components still require it.

Implement (optional) support for also enabling an "iovcc-supply".
IOVCC is needed whenever VCC is needed, so switch to the regulator bulk
APIs to request/enable/disable both when appropriate.

Cc: Ondrej Jirman <redacted>
Cc: Marco Felsch <redacted>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/input/touchscreen/edt-ft5x06.c | 35 ++++++++++++++------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 2eefbc2485bc..bf2e208112fe 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c

@@ -103,7 +103,7 @@ struct edt_ft5x06_ts_data {
 	struct touchscreen_properties prop;
 	u16 num_x;
 	u16 num_y;
-	struct regulator *vcc;
+	struct regulator_bulk_data regulators[2];

Is there an enabling order we must follow?

I don't know, sadly. The datasheets I was able to find do not mention
anything about this; the power-on sequence only includes the VDD line.

I've goolged a bit :)

Check this: https://focuslcds.com/content/FT5X26.pdf, page 12 of 32

There it is mentioned that we need to enable it first and add a 10us
delay till we can enable the vdd line. So unfortunately the bulk_api
can't be used as it is today. Another solution could be to extended the
bulk api to respect on/off delays.

Regards,
  Marco

I tried several suspend/resume cycles with both regulators set up and it
worked fine, which could mean that I was lucky or that the order does
not matter. :)

What do you think?

quoted

quoted

 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *wake_gpio;

@@ -1066,7 +1066,7 @@ static void edt_ft5x06_disable_regulator(void *arg)
 {
 	struct edt_ft5x06_ts_data *data = arg;
 
-	regulator_disable(data->vcc);
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 }
 
 static int edt_ft5x06_ts_probe(struct i2c_client *client,

@@ -1098,18 +1098,19 @@ 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;
-	}
+	tsdata->regulators[0].supply = "vcc";
+	tsdata->regulators[1].supply = "iovcc";
+	error = devm_regulator_bulk_get(&client->dev,
+					ARRAY_SIZE(tsdata->regulators),
+					tsdata->regulators);
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "failed to request regulators\n");

It would be nice to have a patch in front of this one which handles the
support for dev_err_probe().

OK, I can send a v2 with the dev_err_probe() change separated into an
extra patch.

Thanks!
Stephan

quoted

quoted

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

@@ -1286,9 +1287,10 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 	gpiod_set_value_cansleep(reset_gpio, 1);
 	usleep_range(1000, 2000);
 
-	ret = regulator_disable(tsdata->vcc);
+	ret = regulator_bulk_disable(ARRAY_SIZE(tsdata->regulators),
+				     tsdata->regulators);
 	if (ret)
-		dev_warn(dev, "Failed to disable vcc\n");
+		dev_warn(dev, "Failed to disable regulators\n");
 
 	return 0;
 }

@@ -1319,9 +1321,10 @@ static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
 		gpiod_set_value_cansleep(reset_gpio, 1);
 		usleep_range(5000, 6000);
 
-		ret = regulator_enable(tsdata->vcc);
+		ret = regulator_bulk_enable(ARRAY_SIZE(tsdata->regulators),
+					    tsdata->regulators);
 		if (ret) {
-			dev_err(dev, "Failed to enable vcc\n");
+			dev_err(dev, "Failed to enable regulators\n");
 			return ret;
 		}
 

-- 
2.30.0

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |