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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 16:38:25 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: nick@...anahar.org, robh@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, nicolas.ferre@...rochip.com,
	alexandre.belloni@...tlin.com, claudiu.beznea@...on.dev,
	linus.walleij@...aro.org, linux-input@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	Stefan Eichenberger <stefan.eichenberger@...adex.com>
Subject: Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on
 functions

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:
> > From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> > 
> > Add a separate function for power off and power on instead of calling
> > regulator_bulk_enable and regulator_bulk_disable directly.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 59 +++++++++++++++---------
> >  1 file changed, 37 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 542a31448c8f..52867ce3b9b6 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -1307,6 +1307,38 @@ static int mxt_soft_reset(struct mxt_data *data)
> >  	return 0;
> >  }
> >  
> > +static int mxt_power_on(struct mxt_data *data)
> > +{
> > +	int error;
> > +
> > +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > +				      data->regulators);
> > +	if (error) {
> > +		dev_err(&data->client->dev, "failed to enable regulators: %d\n",
> > +			error);
> > +		return error;
> > +	}
> > +
> > +	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;
> > +}
> > +
> > +static void mxt_power_off(struct mxt_data *data)
> > +{
> > +	if (data->reset_gpio)
> > +		gpiod_set_value(data->reset_gpio, 1);
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> > +}
> > +
> >  static void mxt_update_crc(struct mxt_data *data, u8 cmd, u8 value)
> >  {
> >  	/*
> > @@ -3305,25 +3337,9 @@ static int mxt_probe(struct i2c_client *client)
> >  		return error;
> >  	}
> >  
> > -	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > -				      data->regulators);
> > -	if (error) {
> > -		dev_err(&client->dev, "failed to enable regulators: %d\n",
> > -			error);
> > +	error = mxt_power_on(data);
> > +	if (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);
> > -	}
> >  
> >  	/*
> >  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
> > @@ -3361,8 +3377,8 @@ static int mxt_probe(struct i2c_client *client)
> >  	mxt_free_input_device(data);
> >  	mxt_free_object_table(data);
> >  err_disable_regulators:
> > -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > -			       data->regulators);
> > +	mxt_power_off(data);
> > +
> >  	return error;
> >  }
> >  
> > @@ -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. 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.

Regards,
Stefan