| Message ID | 20221117123805.1.I9959ac561dd6e1e8e1ce7085e4de6167b27c574f@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a85fbd6498441694475716a4d5c65f9d3e073faf |
| Headers | show |
| Series | Input: elants_i2c: Properly handle the reset GPIO when power is off | expand |
On Thu, Nov 17, 2022 at 12:38:23PM -0800, Douglas Anderson wrote: > As can be seen in elants_i2c_power_off(), we want the reset GPIO > asserted when power is off. The reset GPIO is active low so we need > the reset line logic low when power is off to avoid leakage. > > We have a problem, though, at probe time. At probe time we haven't > powered the regulators on yet but we have: > devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW); > > While that _looks_ right, it turns out that it's not. The > GPIOD_OUT_LOW doesn't mean to init the GPIO to low. It means init the > GPIO to "not asserted". Since this is an active low GPIO that inits it > to be high. > > Let's fix this to properly init the GPIO. Now after both probe and > power off the state of the GPIO is consistent (it's "asserted" or > level low). > > Once we fix this, we can see that at power on time we no longer to > assert the reset GPIO as the first thing. The reset GPIO is _always_ > asserted before powering on. Let's fix powering on to account for > this. I kind of like that elants_i2c_power_on() is self-contained and does the full power sequence. Can we simply change devm_gpiod_get() to use GPIOD_ASIS to avoid the momentary spike in reset line state (assuming that the firmware initializes the reset line sanely because if it does not we have much longer time where we are leaking into the controller)? Thanks.
Hi, On Thu, Nov 17, 2022 at 2:13 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Thu, Nov 17, 2022 at 12:38:23PM -0800, Douglas Anderson wrote: > > As can be seen in elants_i2c_power_off(), we want the reset GPIO > > asserted when power is off. The reset GPIO is active low so we need > > the reset line logic low when power is off to avoid leakage. > > > > We have a problem, though, at probe time. At probe time we haven't > > powered the regulators on yet but we have: > > devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW); > > > > While that _looks_ right, it turns out that it's not. The > > GPIOD_OUT_LOW doesn't mean to init the GPIO to low. It means init the > > GPIO to "not asserted". Since this is an active low GPIO that inits it > > to be high. > > > > Let's fix this to properly init the GPIO. Now after both probe and > > power off the state of the GPIO is consistent (it's "asserted" or > > level low). > > > > Once we fix this, we can see that at power on time we no longer to > > assert the reset GPIO as the first thing. The reset GPIO is _always_ > > asserted before powering on. Let's fix powering on to account for > > this. > > I kind of like that elants_i2c_power_on() is self-contained and does the > full power sequence. Can we simply change devm_gpiod_get() to use > GPIOD_ASIS to avoid the momentary spike in reset line state (assuming > that the firmware initializes the reset line sanely because if it does > not we have much longer time where we are leaking into the controller)? I'm not sure I see the benefit of elants_i2c_power_on() initting the reset GPIO. In general that function _has_ to make assumptions about the state of the world before it's called. Otherwise the function should start: if (ts->did_I_inexplicably_turn_vcc33_on) { regulator_disable(ts->vcc33); ts->did_I_inexplicably_turn_vcc33_on = false; } if (ts->did_I_inexplicably_turn_vccio_on) { regulator_disable(ts->vccio); ts->did_I_inexplicably_turn_vccio_on = false; } Said another way: we already need to rely on the regulators being in a reasonable state when the function starts. Why is that different from relying on the reset GPIO being in a reasonable state? The reset GPIO needs to be sequenced together with the regulators. It should always be "asserted" (driven low) when the regulators are off and only ever deasserted (driven high) when the regulators are on. I'll also note that, as coded today (without my patch), the elants_i2c_power_on() is actively doing the _wrong_ thing in its error handling. Specifically if either of the regulators fail to turn on it will explicitly de-assert the reset again which, since it's active low, will set the GPIO to high and start leaking power / backdriving the touchscreen. We could remove this bit of error handling but then we're suddenly not undoing the things that the function did. ;-) It feels cleaner to me to just make it a requirement that the reset GPIO is always asserted (low) when the regulators are off. I guess one last note is that if you use GPIOD_ASIS you still officially need to change the output later. The docs for GPIOD_ASIS say: "The direction must be set later with one of the dedicated functions." So I guess then you'd have to set the direction in elants_i2c_power_on() ? -Doug
On Thu, Nov 17, 2022 at 03:48:17PM -0800, Doug Anderson wrote: > Hi, > > On Thu, Nov 17, 2022 at 2:13 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Thu, Nov 17, 2022 at 12:38:23PM -0800, Douglas Anderson wrote: > > > As can be seen in elants_i2c_power_off(), we want the reset GPIO > > > asserted when power is off. The reset GPIO is active low so we need > > > the reset line logic low when power is off to avoid leakage. > > > > > > We have a problem, though, at probe time. At probe time we haven't > > > powered the regulators on yet but we have: > > > devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW); > > > > > > While that _looks_ right, it turns out that it's not. The > > > GPIOD_OUT_LOW doesn't mean to init the GPIO to low. It means init the > > > GPIO to "not asserted". Since this is an active low GPIO that inits it > > > to be high. > > > > > > Let's fix this to properly init the GPIO. Now after both probe and > > > power off the state of the GPIO is consistent (it's "asserted" or > > > level low). > > > > > > Once we fix this, we can see that at power on time we no longer to > > > assert the reset GPIO as the first thing. The reset GPIO is _always_ > > > asserted before powering on. Let's fix powering on to account for > > > this. > > > > I kind of like that elants_i2c_power_on() is self-contained and does the > > full power sequence. Can we simply change devm_gpiod_get() to use > > GPIOD_ASIS to avoid the momentary spike in reset line state (assuming > > that the firmware initializes the reset line sanely because if it does > > not we have much longer time where we are leaking into the controller)? > > I'm not sure I see the benefit of elants_i2c_power_on() initting the > reset GPIO. In general that function _has_ to make assumptions about > the state of the world before it's called. Otherwise the function > should start: > > if (ts->did_I_inexplicably_turn_vcc33_on) { > regulator_disable(ts->vcc33); > ts->did_I_inexplicably_turn_vcc33_on = false; > } > > if (ts->did_I_inexplicably_turn_vccio_on) { > regulator_disable(ts->vccio); > ts->did_I_inexplicably_turn_vccio_on = false; > } > > Said another way: we already need to rely on the regulators being in a > reasonable state when the function starts. Why is that different from > relying on the reset GPIO being in a reasonable state? The reset GPIO > needs to be sequenced together with the regulators. It should always > be "asserted" (driven low) when the regulators are off and only ever > deasserted (driven high) when the regulators are on. > > I'll also note that, as coded today (without my patch), the > elants_i2c_power_on() is actively doing the _wrong_ thing in its error > handling. Specifically if either of the regulators fail to turn on it > will explicitly de-assert the reset again which, since it's active > low, will set the GPIO to high and start leaking power / backdriving > the touchscreen. We could remove this bit of error handling but then > we're suddenly not undoing the things that the function did. ;-) It > feels cleaner to me to just make it a requirement that the reset GPIO > is always asserted (low) when the regulators are off. OK, fair enough, applied. Thanks.
diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c index 879a4d984c90..e1308e179dd6 100644 --- a/drivers/input/touchscreen/elants_i2c.c +++ b/drivers/input/touchscreen/elants_i2c.c @@ -1329,14 +1329,12 @@ static int elants_i2c_power_on(struct elants_data *ts) if (IS_ERR_OR_NULL(ts->reset_gpio)) return 0; - gpiod_set_value_cansleep(ts->reset_gpio, 1); - error = regulator_enable(ts->vcc33); if (error) { dev_err(&ts->client->dev, "failed to enable vcc33 regulator: %d\n", error); - goto release_reset_gpio; + return error; } error = regulator_enable(ts->vccio); @@ -1345,7 +1343,7 @@ static int elants_i2c_power_on(struct elants_data *ts) "failed to enable vccio regulator: %d\n", error); regulator_disable(ts->vcc33); - goto release_reset_gpio; + return error; } /* @@ -1354,7 +1352,6 @@ static int elants_i2c_power_on(struct elants_data *ts) */ udelay(ELAN_POWERON_DELAY_USEC); -release_reset_gpio: gpiod_set_value_cansleep(ts->reset_gpio, 0); if (error) return error; @@ -1462,7 +1459,7 @@ static int elants_i2c_probe(struct i2c_client *client) return error; } - ts->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW); + ts->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(ts->reset_gpio)) { error = PTR_ERR(ts->reset_gpio);
As can be seen in elants_i2c_power_off(), we want the reset GPIO asserted when power is off. The reset GPIO is active low so we need the reset line logic low when power is off to avoid leakage. We have a problem, though, at probe time. At probe time we haven't powered the regulators on yet but we have: devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW); While that _looks_ right, it turns out that it's not. The GPIOD_OUT_LOW doesn't mean to init the GPIO to low. It means init the GPIO to "not asserted". Since this is an active low GPIO that inits it to be high. Let's fix this to properly init the GPIO. Now after both probe and power off the state of the GPIO is consistent (it's "asserted" or level low). Once we fix this, we can see that at power on time we no longer to assert the reset GPIO as the first thing. The reset GPIO is _always_ asserted before powering on. Let's fix powering on to account for this. Fixes: afe10358e47a ("Input: elants_i2c - wire up regulator support") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- This issue was found mostly due to code inspection. A partner was having issues with power sequencing with this touch screen and I noticed this issue in the code. Though this patch doesn't seem to fully address the problems that the partner was having (they are still debugging), in the very least it seems correct and we should probably take it. This problem has been around for a while. Presumably the touchscreen is tolerant enough to this short period of time at bootup where the reset line was leaking power into the unpowered touchscreen, though it's still not great. drivers/input/touchscreen/elants_i2c.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)