| Message ID | 20210510193108.50178-2-stephan@gerhold.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply | expand |
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.
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 > >
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
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
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
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 --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; }
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(+)