Input: elants_i2c: Properly handle the reset GPIO when power is off

  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(-)
  

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.