Message-ID: <CAD=FV=UuLKgM+0UNfRZBij1EkEZs0nQHOkY3Xp9BE2bbJWcdqQ@mail.gmail.com>
Date:   Thu, 17 Nov 2022 15:48:17 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Benson Leung <bleung@...omium.org>,
        Johnny Chuang <johnny.chuang.emc@...il.com>,
        Scott Liu <scott.liu@....com.tw>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: elants_i2c: Properly handle the reset GPIO when
 power is off

Hi,

On Thu, Nov 17, 2022 at 2:13 PM Dmitry Torokhov
<dmitry.torokhov@...il.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

Powered by blists - more mailing lists