Re: [PATCH] Input: elants_i2c: Properly handle the reset GPIO when power is off

[Dmitry Torokhov]

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@xxxxxxxxx> 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.

-- 
Dmitry