| Message ID | 20240417090527.15357-2-eichest@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a property to turn off the max touch controller if not used | expand |
Hi Stefan, On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote: > From: Stefan Eichenberger <stefan.eichenberger@toradex.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@toradex.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? Thanks.
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@toradex.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@toradex.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
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.
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); } static int mxt_suspend(struct device *dev)