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

From: Fei Shao <fshao@chromium.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Jeff LaBundy <jeff@labundy.com>,
	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>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	devicetree@vger.kernel.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property
Date: Tue, 25 Apr 2023 16:34:16 +0800	[thread overview]
Message-ID: <CAC=S1nhFNq_kK5zJCWQiGBuJbDXiC8BstJ5BT7pi5O8hLDQSfA@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=VtrJDVB0gQp-O82V3mgU6MFVMSGrvzh0fRa7sYAt_Pqw@mail.gmail.com>

On Tue, Apr 25, 2023 at 2:15 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@labundy.com> wrote:
> >
> > Back-powering can come in two forms:
> >
> > 1. The one you've described, which is by far the most common. As you mention,
> > it is not the case here since the issue happens only when we drive the GPIO
> > low and not high.
> >
> > 2. Through a forbidden power supply sequence, an input pin of an IC with
> > multiple power supplies becomes a weak power supply itself. Grounding the
> > GPIO then sinks current into the SoC.
> >
> > This problem smelled like (2), especially since asserting the GPIO or powering
> > down the supply alleviates the symptom. Most Goodix controllers I've worked
> > with have two or more supplies, and the datasheet requires them to be enabled
> > or disabled in a specific order.
> >
> > Based on Fei's feedback, however, this IC does not seem to be one of those
> > since there is reportedly only a single rail. I guess either vdd or vddio is
> > tied to a dummy regulator? If so, then perhaps we can scratch this theory.
>
> There is one rail provided from the main board, but there can be two
> voltage levels involved depending on stuffings on the touchscreen
> itself. I talked a bit about this in commit 1d18c1f3b7d9
> ("dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply").
>
>
> > Fair enough, I was simply sketpical that this was the _right_ workaround.
> > Were this an issue of only supply A left on yet the IC datasheet requires
> > supply B to remain on if supply A is on, I would rather see this solved by
> > updating a regulator dts node, trusted FW sequence, etc.
>
> Right. That type of stuff was looked at in detail by two separate
> teams, so I don't think this is the issue.
>
>
> > Thanks for your due diligence; if this really is a silicon issue, then
> > the driver should indeed accommodate it.
> >
> > That being said, the name 'powered-in-suspend' seems a bit conflated. We
> > should not be duplicating regulator policy in this driver; the existing
> > naming also may cause confusion for other HW configurations that do leave
> > vdd and vddio on during suspend, but don't have this problem.
> >
> > Here, we actually mean to control the behavior of the reset GPIO through
> > suspend and therefore something like 'goodix,no-reset-during-suspend' is
> > closer to what I understand us to intend to do. I will add more details
> > in patch [2/2].
>
> I'd be fine with that name. ...ah, and adding the "goodix," prefix is
> a good call.
>
> -Doug

That sounds good to me, too. I'll update the name in the next patch.

Regards,
Fei