Received-SPF: pass (google.com: domain of linux-kernel+bounces-156760-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 24 Apr 2024 12:56:39 +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: <ZijlZw6zm4R9ULBU@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>
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=XP8aCjwE3LfgMy4oBL4xftFg5NkgUFso__54zNp_ZWiA@mail.gmail.com>

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.

First, the reset gpio will be set before probe() returns, just not
immediately when it is requested.

[ Sure, your panel follower implementation may defer the actual probe of
the touchscreen even further but I think that's a design flaw in the
current implementation. ]

Second, the device is not necessarily in the "powered off" state as the
driver leaves the power supplies in whatever state that the boot
firmware left them in.

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

We also don't need to worry about timing requirements, which can all be
handled in one place (i.e. in the power up and power down callbacks).
 
> Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow
> able to maintain continuity of this GPIO line from the BIOS state to
> the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS"
> property before because I've always wanted the ability to have GPIOs
> that could more seamlessly transition their firmware state to their
> kernel state. I don't think the API actually allows it. The fact that
> GPIO regulators don't support this seamless transition (even though it
> would be an obvious feature to add) supports my theory that the API
> doesn't currently allow it. It may be possible to make something work
> on some implementations but I think it's not guaranteed.
> 
> Specifically, the docs say:
> 
> * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set
>   later with one of the dedicated functions.
> 
> So that means that you can't read the pin without making it an input
> (which might change the state if it was previously driving a value)
> and you can't write the pin without making it an output and choosing a
> value to set it to. Basically grabbing a pin with "asis" doesn't allow
> you to do anything with it--it just claims it and doesn't let anyone
> else have it.

These properties may prevent it from being used by the regulator
framework, but GPIOD_ASIS works well in the case of a reset gpio where
we simply leave it in whatever state the firmware left it in if probe
fails before we get to powering on the device.

Johan