Received-SPF: pass (google.com: domain of linux-kernel+bounces-155855-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240423134611.31979-1-johan+linaro@kernel.org>
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
From: Doug Anderson <dianders@chromium.org>
Date: Tue, 23 Apr 2024 13:36:18 -0700
Message-ID: <CAD=FV=W2Y=Sr-=YkKb01XLQsbQJr2b981c9kcfvAc4_5E9XD7g@mail.gmail.com>
Subject: Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen
 power on
To: Johan Hovold <johan+linaro@kernel.org>
Cc: 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Tue, Apr 23, 2024 at 6:46=E2=80=AFAM Johan Hovold <johan+linaro@kernel.o=
rg> wrote:
>
> The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
> before sending commands after having deasserted reset during power on.
>
> This series switches the X13s devicetree to use the Elan specific
> binding so that the OS can determine the required power-on sequence and
> make sure that the controller is always detected during boot. [1]
>
> The Elan hid-i2c driver currently asserts reset unconditionally during
> suspend, which does not work on the X13s where the touch controller
> supply is shared with other peripherals that may remain powered. Holding
> the controller in reset can increase power consumption and also leaks
> current through the reset circuitry pull ups.

Can you provide more details about which devices exactly it shares
power with? I'm worried that you may be shooting yourself in the foot
to avoid shooting yourself in the arm.

Specifically, if those other peripherals that may remain powered ever
power themselves off then you'll end up back-driving the touchscreen
through the reset line, won't you? Since reset is active low then not
asserting reset drives the reset line high and, if you power it off,
it can leach power backwards through the reset line. The
"goodix,no-reset-during-suspend" property that I added earlier
specifically worked on systems where the rail was always-on so I could
guarantee that didn't happen.

From looking at your dts patch it looks like your power _is_ on an
always-on rail so you should be OK, but it should be documented that
this only works for always-on rails.

..also, from your patch description it sounds as if (maybe?) you
intend to eventually let the rail power off if the trackpad isn't a
wakeup source. If you eventually plan to do that then you definitely
need something more complex here...


> Note that the latter also affects X13s variants where the touchscreen is
> not populated as the driver also exits probe() with reset asserted.

I assume driving against an external pull is _probably_ not a huge
deal (should be a pretty small amount of power), but I agree it would
be nice to fix.

I'm a bit leery of actively driving the reset pin high (deasserting
the reset) just to match the pull. It feels like in your case it would
be better to make it an input w/ no pulls. It almost feels like
something in the pinctrl system should handle this. Something where
the pin is default "input no pull" at the board level and when the
driver exits it should go back to the pinctrl default...


I guess one last thought is: what do we do if/when someone needs the
same solution but they want multiple sources of touchscreens, assuming
we ever get the second-sourcing problem solved well. In that case the
different touchscreen drivers might have a different idea of how the
GPIO should be left when the driver exits...

-Doug