Re: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property

From: Doug Anderson <dianders@chromium.org>
To: Fei Shao <fshao@chromium.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek <linux-mediatek@lists.infradead.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Jiri Kosina <jikos@kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Stephen Kitt <steve@sk2.org>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property
Date: Tue, 18 Apr 2023 07:22:14 -0700	[thread overview]
Message-ID: <CAD=FV=WVH3H9R=dnrOkiCEa-+izcSKjfdXbRDP8bc4xo_AhMnw@mail.gmail.com> (raw)
In-Reply-To: <20230418124953.3170028-3-fshao@chromium.org>

Hi,

On Tue, Apr 18, 2023 at 5:51 AM Fei Shao <fshao@chromium.org> wrote:
>
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the
> `powered-in-suspend` property in the driver that allows the driver to
> determine whether the touchscreen is still powered in suspend, and
> handle the reset GPIO accordingly as below:
> - When set to true, the driver does not assert the reset GPIO in power
>   down. To ensure a clean start and the consistent behavior, it does the
>   assertion in power up instead.
>   This is for designs with a dedicated always-on regulator.
> - When set to false, the driver uses the original control flow and
>   asserts GPIO and disable regulators normally.
>   This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
>
> ---
>
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 7 deletions(-)

I privately reviewed earlier versions of this patch, so it's
unsurprising that I have no comments. Assuming that the DT folks don't
have any objections to the bindings change, this LGTM.

Reviewed-by: Douglas Anderson <dianders@chromium.org>