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

[Fei Shao]

On Tue, Apr 25, 2023 at 2:15 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@xxxxxxxxxxx> 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