Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.

[PaweÅ Chmiel]

On Thursday, July 19, 2018 9:54:04 PM CEST Nick Dyer wrote:
> On Wed, Jul 18, 2018 at 06:21:30PM +0200, PaweÅ Chmiel wrote:
> > On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > > On Tue, Jul 17, 2018 at 08:16:25PM +0200, PaweÅ Chmiel wrote:
> > > > This patch adds optional regulators, which can be used to power
> > > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > > This value is taken from official driver.
> > > > 
> > > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > > > 
> > > > Signed-off-by: PaweÅ Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> > > > ---
> > > > Changes from v1:
> > > >   - Enable regulators only if reset_gpio is present.
> > > >   - Switch from devm_regulator_get_optional to devm_regulator_get
> > > > ---
> > > >  drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > >  1 file changed, 44 insertions(+), 2 deletions(-)
> > > 
> > > Hi Pawel-
> > > 
> > > I see you've borrowed some of the logic from the patch I wrote a while
> > > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> > Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> > Are You going to submit it? (it has more functionalities - for example
> > suspend mode read from device tree).
> 
> Getting that work upstream has stalled for a couple of years because I
> changed jobs. I have actually started recently to dust it off again, it
> was later on in my queue but if you have the time to work on it that is
> great.
> 
> > > The correct behaviour according to Atmel should be:
> > > 
> > > * Make RESET zero
> > > * Bring up regulators
> > > * Wait for a period to settle (150 msec)
> > > * Release RESET
> > > * Wait for 100 msec whilst device gets through bootloader
> > > * Wait for CHG line assert before reading info block
> > > 
> > > I can't see the first and last steps in your patch at present.
> > About first step - reset_gpio is readed by using
> > devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> > be wrong)  that we don't need to set this gpio value again to 0 before
> > enabling regulators,
> 
> I see what you mean - that is fair enough.
> 
> > since currently only place where reset_gpio is used is in driver probe
> > (in Your patch it is used in other cases/places - for example in
> > mxt_start/stop, when we enable regulators).
> > About missing wait after releasing reset, shouldn't this be separate
> > patch (since currently driver is not doing it)? I can prepare it and
> > send with other in next version.
> 
> According to the maxtouch documentation, it isn't ready for comms until
> the firmware asserts the CHG line. I've seen a bunch of devices that get
> by without an explicit wait because the board file does the power on,
> and by the time the driver gets to probe it's a few hundred ms later
> anyway, so it doesn't matter. But if we put it all in the driver, it
> will attempt to read the info block straight after the 100 msec delay
> without waiting for CHG, and I suspect we'll end up with occasional
> probe failures. It'll depend on the maxtouch device, though: they have a
> range of different power on timings.
> 
> Which platform are you doing this for? Is it a Chromebook?
No, it's Samsung Galaxy S (i9000) phone with S5PV210 Samsung Soc.
I'm preparing v3 version with separate patch adding this wait/delay.
> 
> > Thanks for feedback
> > > 
> > > The only downside with this approach is that there are a lot of
> > > delays during driver probe, but I believe the asynchronous probe stuff
> > > that's landed since I wrote the original patch should address that.
> > > 
> > > cheers
> > > 
> > > Nick
> > > 
> > > >  	}
> > > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > >  	struct mxt_data *data = i2c_get_clientdata(client);
> > > >  
> > > >  	disable_irq(data->irq);
> > > > +	if (data->reset_gpio) {
> > > > +		regulator_disable(data->avdd_reg);
> > > > +		regulator_disable(data->vdd_reg);
> > > > +	}
> > > >  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > >  	mxt_free_input_device(data);
> > > >  	mxt_free_object_table(data);
> > > 
> > 
> > 
>