Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Stefan,

On Fri, Jun 21, 2024 at 04:38:25PM +0200, Stefan Eichenberger wrote:
> Hi Dmitry
> 
> On Thu, Jun 20, 2024 at 08:37:40AM -0700, Dmitry Torokhov wrote:
> > Hi Stefan,
> > 
> > On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote:
> > > @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
> > >  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > >  	mxt_free_input_device(data);
> > >  	mxt_free_object_table(data);
> > > -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > > -			       data->regulators);
> > > +	mxt_power_off(data);
> > 
> > This change means that on unbind we will leave with GPIO line asserted.
> > Won't this potentially cause some current leakage? Why do we need to
> > have reset asserted here?
> 
> This is correct, but I checked the datasheet of three different maxTouch
> models and all of them have the reset line low active. This means we had
> a current leakage before this patch.

No, the reset line would be either set or pulled high, which is the
normal state for the device.

> Now it is fixed because we assert
> the reset line, which sets the pin to 0. I also think it makes sense if
> we look at the power on sequence. There we first power on the controller
> before we release the reset line. Without asserting it on unbind this
> would never trigger a reset after a power on.

Please see that in mxt_probe() we do:

	/* Request the RESET line as asserted so we go into reset */
	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
						   "reset", GPIOD_OUT_HIGH);

If the reset GPIO is annotated as "active low" (as it should unless
there are inverters on the line) this will cause the line
be driven to 0, putting the chip into the reset state. Then we enable
regulators and deassert the reset GPIO with this bit of code:

	if (data->reset_gpio) {
		/* Wait a while and then de-assert the RESET GPIO line */
		msleep(MXT_RESET_GPIO_TIME);
		gpiod_set_value(data->reset_gpio, 0);
		msleep(MXT_RESET_INVALID_CHG);
	}

So the line here will be left at "1" state (logical off). It should stay
this way until we need to go through the reset sequence again.

I can see that you need the power on sequence to be executed again after
probing is done. I recommend you make it something like this:

static int mxt_power_on()
{
	int error;

	if (data->reset_gpio)
		gpiod_set_value_cansleep(data->reset_gpio, 1);


	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
				      data->regulators);
	if (error) {
		dev_err(&client->dev, "failed to enable regulators: %d\n",
			error);
		return error;
	}

	/*
	 * The device takes 40ms to come up after power-on according
	 * to the mXT224 datasheet, page 13.
	 */
	msleep(MXT_BACKUP_TIME);

	if (data->reset_gpio) {
		/* Wait a while and then de-assert the RESET GPIO line */
		msleep(MXT_RESET_GPIO_TIME);
		gpiod_set_value(data->reset_gpio, 0);
		msleep(MXT_RESET_INVALID_CHG);
	}

	return 0;
}

And then mxt_power_off() should only disable regulators, and leave the
reset line alone. This way first time around the first
"piod_set_value_cansleep(data->reset_gpio, 1)" will be effectively a
noop, but on subsequent calls it will ensure that you have the
transition inactive->active->inactive for the reset line.

I wonder if we need both MXT_BACKUP_TIME and MXT_RESET_GPIO_TIME
though...

Thanks.

-- 
Dmitry