Re: [PATCH v2 2/2] Input: edt-ft5x06 - add support for iovcc-supply

["Ondřej Jirman" <megous@megous.com>]

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