Message ID | 20210510193108.50178-1-stephan@gerhold.net |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [v2,1/2] dt-bindings: input: touchscreen: edt-ft5x06: add iovcc-supply | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success | |
robh/dtbs-check | fail | build log |
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 >
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.
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
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.
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/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.