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@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 for explanation.
regards,
o.
> Thanks!
> Stephan
