Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property

From: Jeff LaBundy <jeff@labundy.com>
To: Doug Anderson <dianders@chromium.org>
Cc: devicetree@vger.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	linux-mediatek <linux-mediatek@lists.infradead.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-input@vger.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property
Date: Wed, 19 Apr 2023 10:18:38 -0500	[thread overview]
Message-ID: <ZEAGTiGyvynGA9P1@nixie71> (raw)
In-Reply-To: <CAD=FV=U_i26a8uJYmqYf6PUgmTUgmEB5L2DkVga0zDX_iDcGQg@mail.gmail.com>

Hi Doug and Fei,

Thank you for the informative discussion; I can empathize with the pain
these issues bring.

On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote:
> >
> > Hi Jeff,
> >
> > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote:
> > >
> > > Hi Fei,
> > >
> > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote:
> > > > We observed that on Chromebook device Steelix, if Goodix GT7375P
> > > > touchscreen is powered in suspend (because, for example, it connects to
> > > > an always-on regulator) and with the reset GPIO asserted, it will
> > > > introduce about 14mW power leakage.
> > > >
> > > > This property is used to indicate that the touchscreen is powered in
> > > > suspend. If it's set, the driver will stop asserting the reset GPIO in
> > > > power-down, and it will do it in power-up instead to ensure that the
> > > > state is always reset after resuming.
> > > >
> > > > Signed-off-by: Fei Shao <fshao@chromium.org>
> > > > ---
> > >
> > > This is an interesting problem; were you able to root-cause why the silicon
> > > exhibits this behavior? Simply asserting reset should not cause it to draw
> > > additional power, let alone 14 mW. This almost sounds like a back-powering
> > > problem during suspend.
> > >
> > There was a fix for this behavior before so I didn't dig into it on
> > the silicon side.
> > I can ask internally and see if we can have Goodix to confirm this is
> > a known HW erratum.
> 
> Certainly it doesn't hurt to check, but it's not really that shocking
> to me that asserting reset could cause a power draw on some hardware.
> Reset puts hardware into a default state and that's not necessarily
> low power. I guess ideally hardware would act like it's "off" when
> reset is asserted and then then init to the default state on the edge
> as reset was deasserted, but I not all hardware is designed in an
> ideal way.

While that is true in theory, I have never, ever seen that to be the case
when there is not some other underlying problem.

What I have seen, however, is that asserting reset actually causes the GPIO
to sink current from some other supply and through the IC. I loosely suspect
that if you probe the IC's rails and digital I/O during the failure condition,
you may find one of them resting at some mid-rail voltage or diode drop. It
seems you have a similar suspicion.

In that case, it may mean that some other supply in the system should actually
be kept on, or that supplies are being brought down out of order. In which
case, the solution should actually be a patch to the affected platform(s) dts
and not the mainline driver.

> 
> 
> > > If this is truly expected behavior, is it sufficient to use the always_on
> > > constraint of the relevant regulator(s) to make this decision as opposed to
> > > introducing a new property?
> > >
> > That sounds good to me. IIUC, for the existing designs, the boards
> > that would set this property would also exclusively set
> > `regulator-always-on` in their supply, so that should suffice.
> > Let me revise the patch. Thanks!
> 
> Yeah, I thought about this too and talked about it in my original
> reply. It doesn't handle the shared-rail case, but then again neither
> does ${SUBJECT} patch. ...then I guess the only argument against it is
> my argument that the regulator could be marked "always-on" in the
> device tree but still turned off by an external entity (PMIC or EC) in
> S3. In theory this should be specified by
> "regulator-state-(standby|mem|disk)", but I could believe it being
> tricky to figure out (what if a parent regulator gets turned off
> automatically but the child isn't explicit?). Specifically, if a
> regulator is always-on but somehow gets shut off in suspend then we
> _do_ want to assert reset (active low) during suspend, otherwise we'll
> have a power leak through the reset GPIO... :-P

D'oh! Sorry I missed your original reply. My concern is that either solution
is a band-aid and does not address the root cause. I would rather see a patch
that addresses what seems to be a back-powering problem so that the driver may
freely assert reset. That is just my $.02; let me know if I have misunderstood
or there are other factors that prevent that path from being viable.

> 
> ...so I guess I'll continue to assert that I don't think peeking at
> the regulator's "always-on" property is the best way to go. If
> everyone else disagrees with me then I won't stand in the way, but IMO
> the extra property like Fei's patch adds is better.
> 
> [1] https://lore.kernel.org/r/CAD=FV=V8ZN3969RrPu2-zZYoEE=LDxpi8K_E8EziiDpGOSsq1w@mail.gmail.com

Kind regards,
Jeff LaBundy