Received-SPF: pass (google.com: domain of linux-kernel+bounces-156576-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 24 Apr 2024 10:50:48 +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
Subject: Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen
 power on
Message-ID: <ZijH6EaqWKHWRcdK@hovoldconsulting.com>
References: <20240423134611.31979-1-johan+linaro@kernel.org>
 <CAD=FV=W2Y=Sr-=YkKb01XLQsbQJr2b981c9kcfvAc4_5E9XD7g@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=W2Y=Sr-=YkKb01XLQsbQJr2b981c9kcfvAc4_5E9XD7g@mail.gmail.com>

On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> 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...

No, that's the whole point: the hardware is designed so that the reset
line can be left deasserted by the CPU also when the supply is off.

The supply in this case is shared with the keyboard and touchpad, but
also some other devices which are not yet fully described. As you
rightly noted, the intention is to allow the supply to eventually be
disabled when none of these devices are enabled as wakeup sources.

I did not want to get in to too much details on exactly how this
particular reset circuit is designed, but basically you have a pull up
to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a
pull up to the supply voltage on the peripheral side.

With this design, the reset line can be left deasserted by the CPU
(tri-stated or driven high), but the important part is that the reset
signal that goes into the controller will be pulled to 3.3 V only when
the supply is left on and otherwise it will be connected to ground.

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

If you look at the DT patch that's essentially what I'm doing by
describing the reset pin as open-drain so that it will be configured as
an input (tristated) when reset is deasserted and only driven low when
reset is asserted.

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

The second-source problem is arguable a separate one, and as we've
discussed in the past, the current approach of describing both devices
in the devicetree only works when the devices are truly compatible in
terms of external resources (supplies, gpios, pinconfig). For anything
more complex, we need a more elaborate implementation.

In this case it should not be a problem, though, as the reset circuit
should have the same properties regardless of which controller you
connect (e.g. both nodes would have the 'no-reset-on-power-off'
property).

Johan