diff mbox series

[v2,1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply

Message ID 20210510193108.50178-1-stephan@gerhold.net
State Not Applicable
Headers show
Series [v2,1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check fail build log

Commit Message

Stephan Gerhold May 10, 2021, 7:31 p.m. UTC
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(+)

Comments

Ondřej Jirman May 10, 2021, 7:48 p.m. UTC | #1
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
>
Andy Shevchenko May 10, 2021, 8:09 p.m. UTC | #2
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.
Stephan Gerhold May 10, 2021, 8:16 p.m. UTC | #3
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
Ondřej Jirman May 10, 2021, 8:17 p.m. UTC | #4
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
> 
>
Ondřej Jirman May 10, 2021, 9:14 p.m. UTC | #5
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
Andy Shevchenko May 11, 2021, 7:21 a.m. UTC | #6
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.
Marco Felsch May 11, 2021, 7:42 a.m. UTC | #7
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
Stephan Gerhold May 11, 2021, 8:50 a.m. UTC | #8
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
diff mbox series

Patch

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.