All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply
@ 2021-01-08 19:23 Stephan Gerhold
  2021-01-08 19:23 ` [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
  2021-01-13 15:25 ` [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Rob Herring
  0 siblings, 2 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-08 19:23 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input, Rob Herring, devicetree, Simon Budig,
	Andy Shevchenko, Stephan Gerhold, Ondrej Jirman, Marco Felsch

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.

Document (optional) support for controlling the regulator for IOVCC
using "iovcc-supply".

Cc: Ondrej Jirman <megous@megous.com>
Cc: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml        | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
index bfc3a8b5e118..2e8da7470513 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
@@ -56,6 +56,7 @@ properties:
   wakeup-source: true
 
   vcc-supply: true
+  iovcc-supply: true
 
   gain:
     description: Allows setting the sensitivity in the range from 0 to 31.
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-08 19:23 [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Stephan Gerhold
@ 2021-01-08 19:23 ` Stephan Gerhold
  2021-01-11  8:36   ` Marco Felsch
  2021-01-13 15:25 ` [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Rob Herring
  1 sibling, 1 reply; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-08 19:23 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input, Rob Herring, devicetree, Simon Budig,
	Andy Shevchenko, Stephan Gerhold, Ondrej Jirman, Marco Felsch

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 <megous@megous.com>
Cc: Marco Felsch <m.felsch@pengutronix.de>
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];
 
 	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");
 
-	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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-08 19:23 ` [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
@ 2021-01-11  8:36   ` Marco Felsch
  2021-01-11  9:26     ` Stephan Gerhold
  0 siblings, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2021-01-11  8:36 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Ondrej Jirman

Hi Stephan,

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

On 21-01-08 20:23, Stephan Gerhold wrote:
> 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 <megous@megous.com>
> Cc: Marco Felsch <m.felsch@pengutronix.de>
> 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?

>  	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().

Regards,
  Marco

>  
> -	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 |

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11  8:36   ` Marco Felsch
@ 2021-01-11  9:26     ` Stephan Gerhold
  2021-01-11  9:45       ` Marco Felsch
  2021-01-11 10:22       ` Andy Shevchenko
  0 siblings, 2 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-11  9:26 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Ondrej Jirman

Hi Marco,

thanks for the review!

On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:
> Hi Stephan,
> 
> thanks for the patch :) Please see my inline comments.
> 
> On 21-01-08 20:23, Stephan Gerhold wrote:
> > 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 <megous@megous.com>
> > Cc: Marco Felsch <m.felsch@pengutronix.de>
> > 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 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?

> >  	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

> 
> >  
> > -	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 |

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11  9:26     ` Stephan Gerhold
@ 2021-01-11  9:45       ` Marco Felsch
  2021-01-11 10:10         ` Stephan Gerhold
  2021-01-11 10:22       ` Andy Shevchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2021-01-11  9:45 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Ondrej Jirman

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:
> > Hi Stephan,
> > 
> > thanks for the patch :) Please see my inline comments.
> > 
> > On 21-01-08 20:23, Stephan Gerhold wrote:
> > > 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 <megous@megous.com>
> > > Cc: Marco Felsch <m.felsch@pengutronix.de>
> > > 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?
> 
> > >  	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
> 
> > 
> > >  
> > > -	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 |

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11  9:45       ` Marco Felsch
@ 2021-01-11 10:10         ` Stephan Gerhold
  2021-01-11 15:46           ` Stephan Gerhold
  0 siblings, 1 reply; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-11 10:10 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Ondrej Jirman

On Mon, Jan 11, 2021 at 10:45:38AM +0100, Marco Felsch wrote:
> On 21-01-11 10:26, Stephan Gerhold wrote:
> > On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:
> > > On 21-01-08 20:23, Stephan Gerhold wrote:
> > > > 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
> 

Thanks! I looked at several datasheets, that's probably one of the few I
did not look at. :(

> 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.
> 

I think for two regulators like here it's still manageable to
get/enable/disable/put them separately, so I will just revert the bulk
API change in v2.

Thanks again!
Stephan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11  9:26     ` Stephan Gerhold
  2021-01-11  9:45       ` Marco Felsch
@ 2021-01-11 10:22       ` Andy Shevchenko
  2021-01-11 10:43         ` Stephan Gerhold
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-01-11 10:22 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Marco Felsch, Dmitry Torokhov, linux-input, Rob Herring,
	devicetree, Simon Budig, Ondrej Jirman

On Mon, Jan 11, 2021 at 10:26:25AM +0100, Stephan Gerhold wrote:
> On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:
> > On 21-01-08 20:23, Stephan Gerhold wrote:

...

> > > +	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.

AFAIR Dmitry has strong opinion against dev_err_probe(). But I might be
mistaken.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11 10:22       ` Andy Shevchenko
@ 2021-01-11 10:43         ` Stephan Gerhold
  0 siblings, 0 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-11 10:43 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Marco Felsch, Dmitry Torokhov, linux-input, Rob Herring,
	devicetree, Simon Budig, Ondrej Jirman

On Mon, Jan 11, 2021 at 12:22:36PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 11, 2021 at 10:26:25AM +0100, Stephan Gerhold wrote:
> > On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:
> > > On 21-01-08 20:23, Stephan Gerhold wrote:
> 
> ...
> 
> > > > +	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.
> 
> AFAIR Dmitry has strong opinion against dev_err_probe(). But I might be
> mistaken.
> 

Hmm, yeah actually it seems like there was a patch for this already:
https://lore.kernel.org/linux-input/20200827185829.30096-12-krzk@kernel.org/

I guess it's better to not include this here then...

Thanks,
Stephan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-01-11 10:10         ` Stephan Gerhold
@ 2021-01-11 15:46           ` Stephan Gerhold
  0 siblings, 0 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-11 15:46 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Ondrej Jirman

Hi,

On Mon, Jan 11, 2021 at 11:10:16AM +0100, Stephan Gerhold wrote:
> On Mon, Jan 11, 2021 at 10:45:38AM +0100, Marco Felsch wrote:
> > On 21-01-11 10:26, Stephan Gerhold wrote:
> > > On Mon, Jan 11, 2021 at 09:36:12AM +0100, Marco Felsch wrote:
> > > > On 21-01-08 20:23, Stephan Gerhold wrote:
> > > > > 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
> > 
> 
> Thanks! I looked at several datasheets, that's probably one of the few I
> did not look at. :(
> 
> > 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.
> > 
> 
> I think for two regulators like here it's still manageable to
> get/enable/disable/put them separately, so I will just revert the bulk
> API change in v2.
> 

While implementing this I noticed that the power-up sequence in probe()
does not quite seem right. The power-up sequence implemented by Marco in
edt_ft5x06_ts_resume() seems to match the datasheet(s) but in probe() we
enable VCC before doing anything with the reset line.

So before I add the IOVCC regulator I will try to refactor the code a
bit to make this consistent. :)

Thanks,
Stephan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply
  2021-01-08 19:23 [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Stephan Gerhold
  2021-01-08 19:23 ` [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
@ 2021-01-13 15:25 ` Rob Herring
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-01-13 15:25 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Ondrej Jirman, Marco Felsch, Andy Shevchenko, Dmitry Torokhov,
	Simon Budig, Rob Herring, devicetree, linux-input

On Fri, 08 Jan 2021 20:23:36 +0100, Stephan Gerhold wrote:
> 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.
> 
> Document (optional) support for controlling the regulator for IOVCC
> using "iovcc-supply".
> 
> Cc: Ondrej Jirman <megous@megous.com>
> Cc: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml        | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-01-13 15:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-08 19:23 [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Stephan Gerhold
2021-01-08 19:23 ` [PATCH 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
2021-01-11  8:36   ` Marco Felsch
2021-01-11  9:26     ` Stephan Gerhold
2021-01-11  9:45       ` Marco Felsch
2021-01-11 10:10         ` Stephan Gerhold
2021-01-11 15:46           ` Stephan Gerhold
2021-01-11 10:22       ` Andy Shevchenko
2021-01-11 10:43         ` Stephan Gerhold
2021-01-13 15:25 ` [PATCH 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.