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

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>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2: None, added Rob's Acked-by
v1: https://lore.kernel.org/linux-input/20210108192337.563679-1-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.31.1


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

* [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 19:31 [PATCH v2 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Stephan Gerhold
@ 2021-05-10 19:31 ` Stephan Gerhold
  2021-05-10 19:48   ` Ondřej Jirman
  0 siblings, 1 reply; 10+ messages in thread
From: Stephan Gerhold @ 2021-05-10 19:31 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".
The datasheet specifies a delay of ~ 10us before enabling VDD/VCC
after IOVCC is enabled, so make sure to enable IOVCC first.

Cc: Ondrej Jirman <megous@megous.com>
Cc: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Sorry about the long delay, couldn't find the time to test the new changes :)

Changes in v2:
  - Respect 10us delay between enabling IOVCC and VDD/VCC line
    (suggested by Marco Felsch)

v1: https://lore.kernel.org/linux-input/20210108192337.563679-2-stephan@gerhold.net/
---
 drivers/input/touchscreen/edt-ft5x06.c | 34 ++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 2eefbc2485bc..d3271856bb5c 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -104,6 +104,7 @@ struct edt_ft5x06_ts_data {
 	u16 num_x;
 	u16 num_y;
 	struct regulator *vcc;
+	struct regulator *iovcc;
 
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *wake_gpio;
@@ -1067,6 +1068,7 @@ static void edt_ft5x06_disable_regulator(void *arg)
 	struct edt_ft5x06_ts_data *data = arg;
 
 	regulator_disable(data->vcc);
+	regulator_disable(data->iovcc);
 }
 
 static int edt_ft5x06_ts_probe(struct i2c_client *client,
@@ -1107,9 +1109,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
+	if (IS_ERR(tsdata->iovcc)) {
+		error = PTR_ERR(tsdata->iovcc);
+		if (error != -EPROBE_DEFER)
+			dev_err(&client->dev,
+				"failed to request iovcc regulator: %d\n", error);
+		return error;
+	}
+
+	error = regulator_enable(tsdata->iovcc);
+	if (error < 0) {
+		dev_err(&client->dev, "failed to enable iovcc: %d\n", error);
+		return error;
+	}
+
+	/* Delay enabling VCC for > 10us (T_ivd) after IOVCC */
+	usleep_range(10, 100);
+
 	error = regulator_enable(tsdata->vcc);
 	if (error < 0) {
 		dev_err(&client->dev, "failed to enable vcc: %d\n", error);
+		regulator_disable(tsdata->iovcc);
 		return error;
 	}
 
@@ -1289,6 +1310,9 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 	ret = regulator_disable(tsdata->vcc);
 	if (ret)
 		dev_warn(dev, "Failed to disable vcc\n");
+	ret = regulator_disable(tsdata->iovcc);
+	if (ret)
+		dev_warn(dev, "Failed to disable iovcc\n");
 
 	return 0;
 }
@@ -1319,9 +1343,19 @@ 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->iovcc);
+		if (ret) {
+			dev_err(dev, "Failed to enable iovcc\n");
+			return ret;
+		}
+
+		/* Delay enabling VCC for > 10us (T_ivd) after IOVCC */
+		usleep_range(10, 100);
+
 		ret = regulator_enable(tsdata->vcc);
 		if (ret) {
 			dev_err(dev, "Failed to enable vcc\n");
+			regulator_disable(tsdata->iovcc);
 			return ret;
 		}
 
-- 
2.31.1


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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 19:31 ` [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
@ 2021-05-10 19:48   ` Ondřej Jirman
  2021-05-10 20:09     ` Andy Shevchenko
  2021-05-10 20:16     ` Stephan Gerhold
  0 siblings, 2 replies; 10+ messages in thread
From: Ondřej Jirman @ 2021-05-10 19:48 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Marco Felsch

Hello Stephan,

On Mon, May 10, 2021 at 09:31:08PM +0200, 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".
> The datasheet specifies a delay of ~ 10us before enabling VDD/VCC
> after IOVCC is enabled, so make sure to enable IOVCC first.
> 
> Cc: Ondrej Jirman <megous@megous.com>
> Cc: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Sorry about the long delay, couldn't find the time to test the new changes :)
> 
> Changes in v2:
>   - Respect 10us delay between enabling IOVCC and VDD/VCC line
>     (suggested by Marco Felsch)
> 
> v1: https://lore.kernel.org/linux-input/20210108192337.563679-2-stephan@gerhold.net/
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 34 ++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 2eefbc2485bc..d3271856bb5c 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -104,6 +104,7 @@ struct edt_ft5x06_ts_data {
>  	u16 num_x;
>  	u16 num_y;
>  	struct regulator *vcc;
> +	struct regulator *iovcc;
>  
>  	struct gpio_desc *reset_gpio;
>  	struct gpio_desc *wake_gpio;
> @@ -1067,6 +1068,7 @@ static void edt_ft5x06_disable_regulator(void *arg)
>  	struct edt_ft5x06_ts_data *data = arg;
>  
>  	regulator_disable(data->vcc);
> +	regulator_disable(data->iovcc);
>  }
>  
>  static int edt_ft5x06_ts_probe(struct i2c_client *client,
> @@ -1107,9 +1109,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> +	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
> +	if (IS_ERR(tsdata->iovcc)) {
> +		error = PTR_ERR(tsdata->iovcc);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(&client->dev,
> +				"failed to request iovcc regulator: %d\n", error);

Please use dev_err_probe instead. If this pattern is used for vcc-supply, maybe
change that too. Maybe also consider using a bulk regulator API.

Thank you,
	o.

> +		return error;
> +	}
> +
> +	error = regulator_enable(tsdata->iovcc);
> +	if (error < 0) {
> +		dev_err(&client->dev, "failed to enable iovcc: %d\n", error);
> +		return error;
> +	}
> +
> +	/* Delay enabling VCC for > 10us (T_ivd) after IOVCC */
> +	usleep_range(10, 100);
> +
>  	error = regulator_enable(tsdata->vcc);
>  	if (error < 0) {
>  		dev_err(&client->dev, "failed to enable vcc: %d\n", error);
> +		regulator_disable(tsdata->iovcc);
>  		return error;
>  	}
>  
> @@ -1289,6 +1310,9 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
>  	ret = regulator_disable(tsdata->vcc);
>  	if (ret)
>  		dev_warn(dev, "Failed to disable vcc\n");
> +	ret = regulator_disable(tsdata->iovcc);
> +	if (ret)
> +		dev_warn(dev, "Failed to disable iovcc\n");
>  
>  	return 0;
>  }
> @@ -1319,9 +1343,19 @@ 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->iovcc);
> +		if (ret) {
> +			dev_err(dev, "Failed to enable iovcc\n");
> +			return ret;
> +		}
> +
> +		/* Delay enabling VCC for > 10us (T_ivd) after IOVCC */
> +		usleep_range(10, 100);
> +
>  		ret = regulator_enable(tsdata->vcc);
>  		if (ret) {
>  			dev_err(dev, "Failed to enable vcc\n");
> +			regulator_disable(tsdata->iovcc);
>  			return ret;
>  		}
>  
> -- 
> 2.31.1
> 

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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 19:48   ` Ondřej Jirman
@ 2021-05-10 20:09     ` Andy Shevchenko
  2021-05-10 20:17       ` Ondřej Jirman
  2021-05-10 20:16     ` Stephan Gerhold
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-05-10 20:09 UTC (permalink / raw)
  To: Ondřej Jirman
  Cc: Stephan Gerhold, Dmitry Torokhov, linux-input, Rob Herring,
	devicetree, Simon Budig, Marco Felsch

On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> On Mon, May 10, 2021 at 09:31:08PM +0200, Stephan Gerhold wrote:

> > +	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
> > +	if (IS_ERR(tsdata->iovcc)) {
> > +		error = PTR_ERR(tsdata->iovcc);
> > +		if (error != -EPROBE_DEFER)
> > +			dev_err(&client->dev,
> > +				"failed to request iovcc regulator: %d\n", error);
> 
> Please use dev_err_probe instead. If this pattern is used for vcc-supply, maybe
> change that too. Maybe also consider using a bulk regulator API.

Dmitry seems is having something against it last time I remember it was
discussed with him.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 19:48   ` Ondřej Jirman
  2021-05-10 20:09     ` Andy Shevchenko
@ 2021-05-10 20:16     ` Stephan Gerhold
  2021-05-10 21:14       ` Ondřej Jirman
  2021-05-11  7:21       ` Andy Shevchenko
  1 sibling, 2 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-05-10 20:16 UTC (permalink / raw)
  To: Ondřej Jirman
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Marco Felsch

Hi!

On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> Hello Stephan,
> 
> On Mon, May 10, 2021 at 09:31:08PM +0200, 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".
> > The datasheet specifies a delay of ~ 10us before enabling VDD/VCC
> > after IOVCC is enabled, so make sure to enable IOVCC first.
> > 
> > Cc: Ondrej Jirman <megous@megous.com>
> > Cc: Marco Felsch <m.felsch@pengutronix.de>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > Sorry about the long delay, couldn't find the time to test the new changes :)
> > 
> > Changes in v2:
> >   - Respect 10us delay between enabling IOVCC and VDD/VCC line
> >     (suggested by Marco Felsch)
> > 
> > v1: https://lore.kernel.org/linux-input/20210108192337.563679-2-stephan@gerhold.net/
> > ---
> >  drivers/input/touchscreen/edt-ft5x06.c | 34 ++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index 2eefbc2485bc..d3271856bb5c 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -104,6 +104,7 @@ struct edt_ft5x06_ts_data {
> >  	u16 num_x;
> >  	u16 num_y;
> >  	struct regulator *vcc;
> > +	struct regulator *iovcc;
> >  
> >  	struct gpio_desc *reset_gpio;
> >  	struct gpio_desc *wake_gpio;
> > @@ -1067,6 +1068,7 @@ static void edt_ft5x06_disable_regulator(void *arg)
> >  	struct edt_ft5x06_ts_data *data = arg;
> >  
> >  	regulator_disable(data->vcc);
> > +	regulator_disable(data->iovcc);
> >  }
> >  
> >  static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > @@ -1107,9 +1109,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  		return error;
> >  	}
> >  
> > +	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
> > +	if (IS_ERR(tsdata->iovcc)) {
> > +		error = PTR_ERR(tsdata->iovcc);
> > +		if (error != -EPROBE_DEFER)
> > +			dev_err(&client->dev,
> > +				"failed to request iovcc regulator: %d\n", error);
> 
> Please use dev_err_probe instead. If this pattern is used for vcc-supply, maybe
> change that too. Maybe also consider using a bulk regulator API.
>

I had both of that in v1 but reverted both of that as discussed.
I didn't make that very clear in the changelog, sorry about that. :)

The reasons were:

  - Bulk regulator API: AFAICT there is no way to use it while also
    maintaining the correct enable/disable order plus the 10us delay.
    See https://lore.kernel.org/linux-input/X%2Fwj+bxe%2FIlznCj6@gerhold.net/

  - dev_err_probe(): For some reason the patch set that converted a lot of
    input drivers (including edt-ft5x06) to dev_err_probe() was never applied:
    https://lore.kernel.org/linux-input/20200827185829.30096-12-krzk@kernel.org/
    I dropped the change from my patch since Andy already mentioned
    a similar thing back then.

Thanks!
Stephan

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

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

On Mon, May 10, 2021 at 11:09:24PM +0300, Andy Shevchenko wrote:
> On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> > On Mon, May 10, 2021 at 09:31:08PM +0200, Stephan Gerhold wrote:
> 
> > > +	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
> > > +	if (IS_ERR(tsdata->iovcc)) {
> > > +		error = PTR_ERR(tsdata->iovcc);
> > > +		if (error != -EPROBE_DEFER)
> > > +			dev_err(&client->dev,
> > > +				"failed to request iovcc regulator: %d\n", error);
> > 
> > Please use dev_err_probe instead. If this pattern is used for vcc-supply, maybe
> > change that too. Maybe also consider using a bulk regulator API.
> 
> Dmitry seems is having something against it last time I remember it was
> discussed with him.

It basically does the same thing this does, except that you can figure out
the failure later on from sysfs more easily just by looking at:

   /sys/kernel/debug/devices_deferred

And you'll see the error message there to help you figure out the dependency
that failed. What's to hate about this? :)

kind regards,
	o.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 20:16     ` Stephan Gerhold
@ 2021-05-10 21:14       ` Ondřej Jirman
  2021-05-11  7:21       ` Andy Shevchenko
  1 sibling, 0 replies; 10+ messages in thread
From: Ondřej Jirman @ 2021-05-10 21:14 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Dmitry Torokhov, linux-input, Rob Herring, devicetree,
	Simon Budig, Andy Shevchenko, Marco Felsch

On Mon, May 10, 2021 at 10:16:41PM +0200, Stephan Gerhold wrote:
> Hi!
> 
> On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> > Hello Stephan,
> > 
> > On Mon, May 10, 2021 at 09:31:08PM +0200, 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".
> > > The datasheet specifies a delay of ~ 10us before enabling VDD/VCC
> > > after IOVCC is enabled, so make sure to enable IOVCC first.
> > > 
> > > Cc: Ondrej Jirman <megous@megous.com>
> > > Cc: Marco Felsch <m.felsch@pengutronix.de>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > ---
> > > Sorry about the long delay, couldn't find the time to test the new changes :)
> > > 
> > > Changes in v2:
> > >   - Respect 10us delay between enabling IOVCC and VDD/VCC line
> > >     (suggested by Marco Felsch)
> > > 
> > > v1: https://lore.kernel.org/linux-input/20210108192337.563679-2-stephan@gerhold.net/
> > > ---
> > >  drivers/input/touchscreen/edt-ft5x06.c | 34 ++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index 2eefbc2485bc..d3271856bb5c 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -104,6 +104,7 @@ struct edt_ft5x06_ts_data {
> > >  	u16 num_x;
> > >  	u16 num_y;
> > >  	struct regulator *vcc;
> > > +	struct regulator *iovcc;
> > >  
> > >  	struct gpio_desc *reset_gpio;
> > >  	struct gpio_desc *wake_gpio;
> > > @@ -1067,6 +1068,7 @@ static void edt_ft5x06_disable_regulator(void *arg)
> > >  	struct edt_ft5x06_ts_data *data = arg;
> > >  
> > >  	regulator_disable(data->vcc);
> > > +	regulator_disable(data->iovcc);
> > >  }
> > >  
> > >  static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > > @@ -1107,9 +1109,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >  		return error;
> > >  	}
> > >  
> > > +	tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
> > > +	if (IS_ERR(tsdata->iovcc)) {
> > > +		error = PTR_ERR(tsdata->iovcc);
> > > +		if (error != -EPROBE_DEFER)
> > > +			dev_err(&client->dev,
> > > +				"failed to request iovcc regulator: %d\n", error);
> > 
> > Please use dev_err_probe instead. If this pattern is used for vcc-supply, maybe
> > change that too. Maybe also consider using a bulk regulator API.
> >
> 
> I had both of that in v1 but reverted both of that as discussed.
> I didn't make that very clear in the changelog, sorry about that. :)
> 
> The reasons were:
> 
>   - Bulk regulator API: AFAICT there is no way to use it while also
>     maintaining the correct enable/disable order plus the 10us delay.
>     See https://lore.kernel.org/linux-input/X%2Fwj+bxe%2FIlznCj6@gerhold.net/
> 
>   - dev_err_probe(): For some reason the patch set that converted a lot of
>     input drivers (including edt-ft5x06) to dev_err_probe() was never applied:
>     https://lore.kernel.org/linux-input/20200827185829.30096-12-krzk@kernel.org/
>     I dropped the change from my patch since Andy already mentioned
>     a similar thing back then.

Thanks for explanation.

regards,
	o.

> Thanks!
> Stephan

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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-10 20:16     ` Stephan Gerhold
  2021-05-10 21:14       ` Ondřej Jirman
@ 2021-05-11  7:21       ` Andy Shevchenko
  2021-05-11  7:42         ` Marco Felsch
  2021-05-11  8:50         ` Stephan Gerhold
  1 sibling, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2021-05-11  7:21 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Ondřej Jirman, Dmitry Torokhov, linux-input, Rob Herring,
	devicetree, Simon Budig, Marco Felsch

On Mon, May 10, 2021 at 10:16:41PM +0200, Stephan Gerhold wrote:
> On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:

...

> The reasons were:
> 
>   - Bulk regulator API: AFAICT there is no way to use it while also
>     maintaining the correct enable/disable order plus the 10us delay.
>     See https://lore.kernel.org/linux-input/X%2Fwj+bxe%2FIlznCj6@gerhold.net/

This by the way can be fixed on regulator level (adding some like ranges into
bulk structure with timeouts, and if 0, skip them).

>   - dev_err_probe(): For some reason the patch set that converted a lot of
>     input drivers (including edt-ft5x06) to dev_err_probe() was never applied:
>     https://lore.kernel.org/linux-input/20200827185829.30096-12-krzk@kernel.org/
>     I dropped the change from my patch since Andy already mentioned
>     a similar thing back then.

This question to Dmitry, because I don't remember any good argument why he
doesn't like it. Maybe he can refresh our memories by providing it again.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-11  7:21       ` Andy Shevchenko
@ 2021-05-11  7:42         ` Marco Felsch
  2021-05-11  8:50         ` Stephan Gerhold
  1 sibling, 0 replies; 10+ messages in thread
From: Marco Felsch @ 2021-05-11  7:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Stephan Gerhold, Ondřej Jirman, Dmitry Torokhov,
	linux-input, Rob Herring, devicetree, Simon Budig

On 21-05-11 10:21, Andy Shevchenko wrote:
> On Mon, May 10, 2021 at 10:16:41PM +0200, Stephan Gerhold wrote:
> > On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> 
> ...
> 
> > The reasons were:
> > 
> >   - Bulk regulator API: AFAICT there is no way to use it while also
> >     maintaining the correct enable/disable order plus the 10us delay.
> >     See https://lore.kernel.org/linux-input/X%2Fwj+bxe%2FIlznCj6@gerhold.net/
> 
> This by the way can be fixed on regulator level (adding some like ranges into
> bulk structure with timeouts, and if 0, skip them).

I would appreciate this :)

Regards,
  Marco

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

* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply
  2021-05-11  7:21       ` Andy Shevchenko
  2021-05-11  7:42         ` Marco Felsch
@ 2021-05-11  8:50         ` Stephan Gerhold
  1 sibling, 0 replies; 10+ messages in thread
From: Stephan Gerhold @ 2021-05-11  8:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Ondřej Jirman, Dmitry Torokhov, linux-input, Rob Herring,
	devicetree, Simon Budig, Marco Felsch

On Tue, May 11, 2021 at 10:21:27AM +0300, Andy Shevchenko wrote:
> On Mon, May 10, 2021 at 10:16:41PM +0200, Stephan Gerhold wrote:
> > On Mon, May 10, 2021 at 09:48:48PM +0200, Ondřej Jirman wrote:
> > 
> >   - Bulk regulator API: AFAICT there is no way to use it while also
> >     maintaining the correct enable/disable order plus the 10us delay.
> >     See https://lore.kernel.org/linux-input/X%2Fwj+bxe%2FIlznCj6@gerhold.net/
> 
> This by the way can be fixed on regulator level (adding some like ranges into
> bulk structure with timeouts, and if 0, skip them).
> 

At the moment the bulk regulator API seems specifically designed to
enable all the regulators at the same time (with some funky asynchronous
scheduling code). I'm not sure if there is a straightforward way to
fit in a sequential enable/disable order with potential delays.

I'm also not entirely convinced it's worth it in this case. I would say
the code in this patch (except for the dev_err_probe()) is still quite
easy to read. Encoding the enable/disable order + delays in some bulk
regulator struct might actually be more difficult to read.

Thanks,
Stephan

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

end of thread, other threads:[~2021-05-11  8:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 19:31 [PATCH v2 1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply Stephan Gerhold
2021-05-10 19:31 ` [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply Stephan Gerhold
2021-05-10 19:48   ` Ondřej Jirman
2021-05-10 20:09     ` Andy Shevchenko
2021-05-10 20:17       ` Ondřej Jirman
2021-05-10 20:16     ` Stephan Gerhold
2021-05-10 21:14       ` Ondřej Jirman
2021-05-11  7:21       ` Andy Shevchenko
2021-05-11  7:42         ` Marco Felsch
2021-05-11  8:50         ` Stephan Gerhold

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.