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@xxxxxxxxxx>
> > Cc: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxx/
> > ---
> > 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@xxxxxxxxxxx/
- 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@xxxxxxxxxx/
I dropped the change from my patch since Andy already mentioned
a similar thing back then.
Thanks!
Stephan
