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

[Stephan Gerhold <stephan@gerhold.net>]

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