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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VtrJDVB0gQp-O82V3mgU6MFVMSGrvzh0fRa7sYAt_Pqw@mail.gmail.com>
Date:   Mon, 24 Apr 2023 11:14:55 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Jeff LaBundy <jeff@...undy.com>
Cc:     Fei Shao <fshao@...omium.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-mediatek <linux-mediatek@...ts.infradead.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        devicetree@...r.kernel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property

Hi,

On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@...undy.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