Received-SPF: pass (google.com: domain of linux-kernel+bounces-159786-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 26 Apr 2024 11:29:36 +0200
From: Johan Hovold <johan@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Johan Hovold <johan+linaro@kernel.org>, Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
Message-ID: <Zit0ADAycyHp67Ax@hovoldconsulting.com>
References: <20240423134611.31979-1-johan+linaro@kernel.org>
 <20240423134611.31979-5-johan+linaro@kernel.org>
 <CAD=FV=XP8aCjwE3LfgMy4oBL4xftFg5NkgUFso__54zNp_ZWiA@mail.gmail.com>
 <ZijlZw6zm4R9ULBU@hovoldconsulting.com>
 <CAD=FV=Vxgu==8Cv3sDydFpEdd6ws2stkZvxvajE1OAFm2BgmXw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD=FV=Vxgu==8Cv3sDydFpEdd6ws2stkZvxvajE1OAFm2BgmXw@mail.gmail.com>

On Wed, Apr 24, 2024 at 09:24:33AM -0700, Doug Anderson wrote:
> On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > >         ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> > > >         ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> > > >
> > > > -       /* Start out with reset asserted */
> > > > -       ihid_elan->reset_gpio =
> > > > -               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > > +       ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > > +                                                       GPIOD_ASIS);
> > >
> > > I'm not a huge fan of this part of the change. It feels like the GPIO
> > > state should be initialized by the probe function. Right before we
> > > call i2c_hid_core_probe() we should be in the state of "powered off"
> > > and the reset line should be in a consistent state. If
> > > "no_reset_on_power_off" then it should be de-asserted. Else it should
> > > be asserted.

> > Second, the device is not necessarily in the "powered off" state
> 
> Logically, the driver treats it as being in "powered off" state,
> though. That's why the i2c-hid core makes the call to power it on. IMO
> we should strive to make it more of a consistent state, not less of
> one.

That's not really true. The device is often in an undefined power state
and we try to make sure that the hand over is as smooth as possible to
avoid resetting displays and similar unnecessarily.

The power-on sequence is what brings the device into a defined power
state.

> > as the
> > driver leaves the power supplies in whatever state that the boot
> > firmware left them in.
> 
> I guess it depends on the regulator. ;-) For GPIO-regulators they
> aren't in whatever state the boot firmware left them in. For non-GPIO
> regulators we (usually) do preserve the state that the boot firmware
> left them in.

Even for GPIO regulators we have the "regulator-boot-on" devicetree
property which is supposed to be set if the boot firmware has left a
regulator on so that the regulator initialisation can preserve the
state.

> > Not immediately asserting reset and instead leaving it in the state that
> > the boot firmware left it in is also no different from what happens when
> > a probe function bails out before requesting the reset line.
> >
> > > I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> > > i2c_hid_core_probe() will power on and the first thing that'll happen
> > > there is that the reset line will be unconditionally asserted.
> >
> > It avoids asserting reset before we need to and thus also avoid the need
> > to deassert it on early probe failures (e.g. if one of the regulator
> > lookups fails).
> 
> I guess so, though I'm of the opinion that we should be robust against
> the state that firmware left things in. The firmware's job is to boot
> the kernel and make sure that the system is running in a safe/reliable
> way, not to optimize the power consumption of the board.

Agreed.

> If the
> firmware left the line configured as "output low" then you'd let that
> stand. If it's important for the line to be left in a certain state,
> isn't it better to make that explicit?

As I pointed out above we already do this for any error paths before
requesting the reset line. And I also don't think we need to worry too
much about power consumption in case of errors.

But there is one case I had not considered before, and that is your gpio
regulator example but where the boot-on flag does not match the actual
regulator state.

If the supply is on and reset deasserted, but the regulator-boot-on
flag is not set, then we want to make sure that reset is asserted before
disabling the supply when requesting the regulator.

> Also note: if we really end up keeping GPIOD_ASIS, which I'm still not
> convinced is the right move, the docs seem to imply that you need to
> explicitly set a direction before using it. Your current patch doesn't
> do that.

You're right. It will work in my case because of the gpiolib open-drain
implementation, but not generally.

I'll add back the reset during early probe and add error handling for
deasserting reset on machines like the X13s. On these, the touchscreen
may now be reset a couple of times in case of probe deferrals, but
device links should generally prevent that.

Johan