These changes are based on the series in [1], which modified the
i2c-hid-of-goodix driver and removed the workaround for a power leakage
issue, so the issue revisits on Mediatek MT8186 boards (Steelix).
The root cause is that the touchscreen can be powered in different ways
depending on the hardware designs, and it's not as easy to come up with
a solution that is both simple and elegant for all the known designs.
To address the issue, I ended up adding a new boolean property for the
driver so that we can control the power up/down sequence depending on
that.
Adding a new property might not be the cleanest approach for this, but
at least the intention would be easy enough to understand, and it
introduces relatively small change to the code and fully preserves the
original control flow.
I hope this is something acceptable, and I'm open to any better
approaches.
[1] https://lore.kernel.org/all/[email protected]/
Fei Shao (2):
dt-bindings: input: goodix: Add powered-in-suspend property
HID: i2c-hid: goodix: Add support for powered-in-suspend property
.../bindings/input/goodix,gt7375p.yaml | 6 +++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 ++++++++++++++++---
2 files changed, 45 insertions(+), 7 deletions(-)
--
2.40.0.634.g4ca3ef3211-goog
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 <[email protected]>
---
Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..942acb286d77 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,12 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ powered-in-suspend:
+ description:
+ This indicates that the touchscreen is powered in suspend, so the driver
+ will not assert the reset GPIO in power-down to prevent power leakage.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.0.634.g4ca3ef3211-goog
In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
reset line to true state of the regulator") introduced a change to tie
the reset line of the Goodix touchscreen to the state of the regulator
to fix a power leakage issue in suspend.
After some time, the change was deemed unnecessary and was reverted in
commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
the regulator") due to difficulties in managing regulator notifiers for
designs like Evoker, which provides a second power rail to touchscreen.
However, the revert caused a power regression on another Chromebook
device Steelix in the field, which has a dedicated always-on regulator
for touchscreen and was covered by the workaround in the first commit.
To address both cases, this patch adds the support for the
`powered-in-suspend` property in the driver that allows the driver to
determine whether the touchscreen is still powered in suspend, and
handle the reset GPIO accordingly as below:
- When set to true, the driver does not assert the reset GPIO in power
down. To ensure a clean start and the consistent behavior, it does the
assertion in power up instead.
This is for designs with a dedicated always-on regulator.
- When set to false, the driver uses the original control flow and
asserts GPIO and disable regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++----
1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..b438db8ca6f4 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct regulator *vddio;
struct gpio_desc *reset_gpio;
+ bool powered_in_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
- ret = regulator_enable(ihid_goodix->vdd);
- if (ret)
- return ret;
-
- ret = regulator_enable(ihid_goodix->vddio);
- if (ret)
- return ret;
+ /*
+ * This is to ensure that the reset GPIO will be asserted and the
+ * regulators will be enabled for all cases.
+ */
+ if (ihid_goodix->powered_in_suspend) {
+ /*
+ * This is not mandatory, but we assert reset here (instead of
+ * in power-down) to ensure that the device will have a clean
+ * state later on just like the normal scenarios would have.
+ *
+ * Also, since the regulators were not disabled in power-down,
+ * we don't need to enable them here.
+ */
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ } else {
+ /*
+ * In this case, the reset is already asserted (either in
+ * probe or power-down).
+ * All we need is to enable the regulators.
+ */
+ ret = regulator_enable(ihid_goodix->vdd);
+ if (ret)
+ return ret;
+
+ ret = regulator_enable(ihid_goodix->vddio);
+ if (ret)
+ return ret;
+ }
if (ihid_goodix->timings->post_power_delay_ms)
msleep(ihid_goodix->timings->post_power_delay_ms);
@@ -60,6 +82,13 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
+ /*
+ * Don't assert reset GPIO or disable regulators if we're keeping the
+ * device powered in suspend.
+ */
+ if (ihid_goodix->powered_in_suspend)
+ return;
+
gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
@@ -91,6 +120,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
if (IS_ERR(ihid_goodix->vddio))
return PTR_ERR(ihid_goodix->vddio);
+ ihid_goodix->powered_in_suspend =
+ of_property_read_bool(client->dev.of_node, "powered-in-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.0.634.g4ca3ef3211-goog
Hi,
On Tue, Apr 18, 2023 at 5:50 AM Fei Shao <[email protected]> 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 <[email protected]>
> ---
>
> Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index ce18d7dadae2..942acb286d77 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -43,6 +43,12 @@ properties:
> itself as long as it allows the main board to make signals compatible
> with what the touchscreen is expecting for its IO rails.
>
> + powered-in-suspend:
> + description:
> + This indicates that the touchscreen is powered in suspend, so the driver
> + will not assert the reset GPIO in power-down to prevent power leakage.
> + type: boolean
This seems OK to me. The overall idea is that if we ever turn off the
power to the touchscreen we have to assert reset (active low) before
doing so, but we don't want to hold reset if we're not actually going
to turn the power off because it causes the touchscreen to go into a
high power state. This gets complicated if the power rail is always-on
or shared with another device.
Alternatives I could think of:
1. In the OS, monitor the regulator and see when it's about to go off
and then assert reset. This is what I tried to do in previous patches
but it got too messy in Linux. It also wasn't perfect since it's
(theoretically) possible for a regulator to turn off outside of the
scope of the OS (some PMICs will auto turn off rails when the main
processor says it's off).
2. In the OS, peek to see if our regulator is marked "always on". This
doesn't handle the shared rail case, of course. Also, regulators that
are "always on" from the OS perspective could still (theoretically)
get turned off at suspend time by the PMIC.
3. Don't even hook up the reset line and just leave the touchscreen
out of reset all the time. This has the disadvantage that we can't
reset the touchscreen if it gets into a bad state, which could be even
more important if the touchscreen is on an always-on or shared rail.
Given the above, the solution in ${SUBJECT} patch seems reasonable.
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Tue, Apr 18, 2023 at 5:51 AM Fei Shao <[email protected]> wrote:
>
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the
> `powered-in-suspend` property in the driver that allows the driver to
> determine whether the touchscreen is still powered in suspend, and
> handle the reset GPIO accordingly as below:
> - When set to true, the driver does not assert the reset GPIO in power
> down. To ensure a clean start and the consistent behavior, it does the
> assertion in power up instead.
> This is for designs with a dedicated always-on regulator.
> - When set to false, the driver uses the original control flow and
> asserts GPIO and disable regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <[email protected]>
>
> ---
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 7 deletions(-)
I privately reviewed earlier versions of this patch, so it's
unsurprising that I have no comments. Assuming that the DT folks don't
have any objections to the bindings change, this LGTM.
Reviewed-by: Douglas Anderson <[email protected]>
On 18/04/2023 14:49, 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 <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
>
> Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index ce18d7dadae2..942acb286d77 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -43,6 +43,12 @@ properties:
> itself as long as it allows the main board to make signals compatible
> with what the touchscreen is expecting for its IO rails.
>
> + powered-in-suspend:
> + description:
> + This indicates that the touchscreen is powered in suspend, so the driver
> + will not assert the reset GPIO in power-down to prevent power leakage.
> + type: boolean
> +
> required:
> - compatible
> - reg
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 <[email protected]>
> ---
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.
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?
>
> Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index ce18d7dadae2..942acb286d77 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -43,6 +43,12 @@ properties:
> itself as long as it allows the main board to make signals compatible
> with what the touchscreen is expecting for its IO rails.
>
> + powered-in-suspend:
> + description:
> + This indicates that the touchscreen is powered in suspend, so the driver
> + will not assert the reset GPIO in power-down to prevent power leakage.
> + type: boolean
> +
> required:
> - compatible
> - reg
> --
> 2.40.0.634.g4ca3ef3211-goog
>
Kind regards,
Jeff LaBundy
Hi Jeff,
On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > ---
>
> 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.
> 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!
Fei
> >
> > Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> > index ce18d7dadae2..942acb286d77 100644
> > --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> > +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> > @@ -43,6 +43,12 @@ properties:
> > itself as long as it allows the main board to make signals compatible
> > with what the touchscreen is expecting for its IO rails.
> >
> > + powered-in-suspend:
> > + description:
> > + This indicates that the touchscreen is powered in suspend, so the driver
> > + will not assert the reset GPIO in power-down to prevent power leakage.
> > + type: boolean
> > +
> > required:
> > - compatible
> > - reg
> > --
> > 2.40.0.634.g4ca3ef3211-goog
> >
>
> Kind regards,
> Jeff LaBundy
Hi,
On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <[email protected]> wrote:
>
> Hi Jeff,
>
> On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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.
> > 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
...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
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 <[email protected]> wrote:
> >
> > Hi Jeff,
> >
> > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > > > ---
> > >
> > > 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
Hi,
On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <[email protected]> wrote:
>
> 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 <[email protected]> wrote:
> > >
> > > Hi Jeff,
> > >
> > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > > > > ---
> > > >
> > > > 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.
I agree that it's a bandaid, but I'm not hopeful that a better
solution will be found.
Specifically, I'd expect a current leak in the system when you turn a
supply off and then assert a GPIO high. That's when the device can
start backpowering itself from a GPIO. In this case, it's the
opposite. We're keeping the supply on and asserting the (active low)
reset GPIO to cause the higher power draw. In another design it was
confirmed that the power draw went away when we were able to turn the
regulator off (but still keep the active low reset GPIO asserted).
We've also confirmed that power is good if we keep the supply on and
_don't_ assert the reset GPIO. Both of these two experiments provide
some evidence that the system is configured properly and we're not
backpowering something.
I guess I should revise the above, though. I could believe that there
is a current leak but on the touchscreen controller board itself,
which is a black box to us. I have certainly been involved in products
in the past where the default state of the system at reset caused a
minor current leak (I remember an EE telling me that as soon as
software started running I should quickly change the direction of a
GPIO) and it wouldn't shock me if the touchscreen controller board had
a problem like this. If there is a problem like this on the
touchscreen controller board there's not much we can do to workaround
it.
Unfortunately, if the problem ends up needing a hardware change to fix
more correctly (which I suspect it does), our hands are tied a bit.
This is not prototype hardware but is final hardware.
I guess one further note is that, at least on the project I was
involved in that had a similar problem, folks in China did a bunch of
analysis on this and went as far as adding an extra regulator to the
main board schematic to "fix" it. Had the issue just been something
where we were misconfiguing GPIOs or leaving a regulator in the wrong
state then they (probably) would have identified it rather than
spinning the board.
-Doug
Hi,
On Wed, Apr 19, 2023 at 11:41 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <[email protected]> wrote:
> >
> > 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 <[email protected]> wrote:
> > > >
> > > > Hi Jeff,
> > > >
> > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > >
> > > > > 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.
> >
I reached out to our EE with your thoughts.
He said that he understands the concern, but this doesn't apply in our
schematics because there's only one supply.
Also he simulated a few scenarios that could explain the
back-powering, but none of them is possible without having the
problematic circuit/rsense layout from within the IC itself.
> > 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.
>
> I agree that it's a bandaid, but I'm not hopeful that a better
> solution will be found.
>
> Specifically, I'd expect a current leak in the system when you turn a
> supply off and then assert a GPIO high. That's when the device can
> start backpowering itself from a GPIO. In this case, it's the
> opposite. We're keeping the supply on and asserting the (active low)
> reset GPIO to cause the higher power draw. In another design it was
> confirmed that the power draw went away when we were able to turn the
> regulator off (but still keep the active low reset GPIO asserted).
> We've also confirmed that power is good if we keep the supply on and
> _don't_ assert the reset GPIO. Both of these two experiments provide
> some evidence that the system is configured properly and we're not
> backpowering something.
>
> I guess I should revise the above, though. I could believe that there
> is a current leak but on the touchscreen controller board itself,
> which is a black box to us. I have certainly been involved in products
> in the past where the default state of the system at reset caused a
> minor current leak (I remember an EE telling me that as soon as
> software started running I should quickly change the direction of a
> GPIO) and it wouldn't shock me if the touchscreen controller board had
> a problem like this. If there is a problem like this on the
> touchscreen controller board there's not much we can do to workaround
> it.
>
> Unfortunately, if the problem ends up needing a hardware change to fix
> more correctly (which I suspect it does), our hands are tied a bit.
> This is not prototype hardware but is final hardware.
>
> I guess one further note is that, at least on the project I was
> involved in that had a similar problem, folks in China did a bunch of
> analysis on this and went as far as adding an extra regulator to the
> main board schematic to "fix" it. Had the issue just been something
> where we were misconfiguing GPIOs or leaving a regulator in the wrong
> state then they (probably) would have identified it rather than
> spinning the board.
Thank you Doug for providing the details and explanation, and sorry
that I also missed your original reply...
I only considered the ideal scenarios for the always_on usage but not
the cases you brought up. The concerns make sense to me.
I'm still awaiting the response from Goodix, but +1 that if it turns
out to be a GT7375P hw issue, we're not able to do much about that
except relying on the driver workaround.
One more note I want to add is that the first attempt of the fix was
added ~2yrs ago, so it's not an one-off on a particular platform, plus
`sc7180-trogdor-homestar` and `mt8186-corsola-steelix` are two
different designs come from two different teams, but they ended up
with the same symptom.
With that said, I think we have more confidence to say it's a
component misbehavior, and we just fell into the edge case that was
not covered or considered by its design.
Regards,
Fei
>
> -Doug
Hi Doug and Fei,
Thank you for this additional information, and your continued patience
with my many questions :)
On Thu, Apr 20, 2023 at 04:13:37PM +0800, Fei Shao wrote:
> Hi,
>
> On Wed, Apr 19, 2023 at 11:41 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <[email protected]> wrote:
> > >
> > > 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 <[email protected]> wrote:
> > > > >
> > > > > Hi Jeff,
> > > > >
> > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <[email protected]> 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 <[email protected]>
> > > > > > > ---
> > > > > >
> > > > > > 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.
> > >
>
> I reached out to our EE with your thoughts.
> He said that he understands the concern, but this doesn't apply in our
> schematics because there's only one supply.
> Also he simulated a few scenarios that could explain the
> back-powering, but none of them is possible without having the
> problematic circuit/rsense layout from within the IC itself.
>
> > > 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.
> >
> > I agree that it's a bandaid, but I'm not hopeful that a better
> > solution will be found.
> >
> > Specifically, I'd expect a current leak in the system when you turn a
> > supply off and then assert a GPIO high. That's when the device can
> > start backpowering itself from a GPIO. In this case, it's the
> > opposite. We're keeping the supply on and asserting the (active low)
> > reset GPIO to cause the higher power draw. In another design it was
> > confirmed that the power draw went away when we were able to turn the
> > regulator off (but still keep the active low reset GPIO asserted).
> > We've also confirmed that power is good if we keep the supply on and
> > _don't_ assert the reset GPIO. Both of these two experiments provide
> > some evidence that the system is configured properly and we're not
> > backpowering something.
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.
> >
> > I guess I should revise the above, though. I could believe that there
> > is a current leak but on the touchscreen controller board itself,
> > which is a black box to us. I have certainly been involved in products
> > in the past where the default state of the system at reset caused a
> > minor current leak (I remember an EE telling me that as soon as
> > software started running I should quickly change the direction of a
> > GPIO) and it wouldn't shock me if the touchscreen controller board had
> > a problem like this. If there is a problem like this on the
> > touchscreen controller board there's not much we can do to workaround
> > it.
> >
> > Unfortunately, if the problem ends up needing a hardware change to fix
> > more correctly (which I suspect it does), our hands are tied a bit.
> > This is not prototype hardware but is final hardware.
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.
> >
> > I guess one further note is that, at least on the project I was
> > involved in that had a similar problem, folks in China did a bunch of
> > analysis on this and went as far as adding an extra regulator to the
> > main board schematic to "fix" it. Had the issue just been something
> > where we were misconfiguing GPIOs or leaving a regulator in the wrong
> > state then they (probably) would have identified it rather than
> > spinning the board.
>
> Thank you Doug for providing the details and explanation, and sorry
> that I also missed your original reply...
> I only considered the ideal scenarios for the always_on usage but not
> the cases you brought up. The concerns make sense to me.
>
> I'm still awaiting the response from Goodix, but +1 that if it turns
> out to be a GT7375P hw issue, we're not able to do much about that
> except relying on the driver workaround.
> One more note I want to add is that the first attempt of the fix was
> added ~2yrs ago, so it's not an one-off on a particular platform, plus
> `sc7180-trogdor-homestar` and `mt8186-corsola-steelix` are two
> different designs come from two different teams, but they ended up
> with the same symptom.
> With that said, I think we have more confidence to say it's a
> component misbehavior, and we just fell into the edge case that was
> not covered or considered by its design.
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].
>
> Regards,
> Fei
>
> >
> > -Doug
Kind regards,
Jeff LaBundy
Hi Fei,
On Tue, Apr 18, 2023 at 08:49:52PM +0800, Fei Shao wrote:
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the
> `powered-in-suspend` property in the driver that allows the driver to
> determine whether the touchscreen is still powered in suspend, and
> handle the reset GPIO accordingly as below:
> - When set to true, the driver does not assert the reset GPIO in power
> down. To ensure a clean start and the consistent behavior, it does the
> assertion in power up instead.
> This is for designs with a dedicated always-on regulator.
> - When set to false, the driver uses the original control flow and
> asserts GPIO and disable regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <[email protected]>
>
> ---
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..b438db8ca6f4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> struct regulator *vdd;
> struct regulator *vddio;
> struct gpio_desc *reset_gpio;
> + bool powered_in_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> - ret = regulator_enable(ihid_goodix->vdd);
> - if (ret)
> - return ret;
> -
> - ret = regulator_enable(ihid_goodix->vddio);
> - if (ret)
> - return ret;
> + /*
> + * This is to ensure that the reset GPIO will be asserted and the
> + * regulators will be enabled for all cases.
> + */
> + if (ihid_goodix->powered_in_suspend) {
> + /*
> + * This is not mandatory, but we assert reset here (instead of
> + * in power-down) to ensure that the device will have a clean
> + * state later on just like the normal scenarios would have.
> + *
> + * Also, since the regulators were not disabled in power-down,
> + * we don't need to enable them here.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + } else {
> + /*
> + * In this case, the reset is already asserted (either in
> + * probe or power-down).
> + * All we need is to enable the regulators.
> + */
> + ret = regulator_enable(ihid_goodix->vdd);
> + if (ret)
> + return ret;
> +
> + ret = regulator_enable(ihid_goodix->vddio);
> + if (ret)
> + return ret;
> + }
Please let me know in case I have misunderstood, but I don't see a need
to change the regulator_enable/disable() logic if this property is set.
If the regulators are truly always-on, the regulator core already knows
what to do and we should not duplicate that logic here.
Based on the alleged silicon erratum discussed in patch [1/2], it seems
we only want to control the behavior of the reset GPIO. Therefore, only
the calls to gpiod_set_value_cansleep() should be affected and the name
of the property updated to reflect what it's actually doing.
>
> if (ihid_goodix->timings->post_power_delay_ms)
> msleep(ihid_goodix->timings->post_power_delay_ms);
> @@ -60,6 +82,13 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_goodix *ihid_goodix =
> container_of(ops, struct i2c_hid_of_goodix, ops);
>
> + /*
> + * Don't assert reset GPIO or disable regulators if we're keeping the
> + * device powered in suspend.
> + */
> + if (ihid_goodix->powered_in_suspend)
> + return;
> +
> gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
> @@ -91,6 +120,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> if (IS_ERR(ihid_goodix->vddio))
> return PTR_ERR(ihid_goodix->vddio);
>
> + ihid_goodix->powered_in_suspend =
> + of_property_read_bool(client->dev.of_node, "powered-in-suspend");
> +
> ihid_goodix->timings = device_get_match_data(&client->dev);
>
> return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> --
> 2.40.0.634.g4ca3ef3211-goog
>
Kind regards,
Jeff LaBundy
Hi,
On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <[email protected]> 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
Hi,
On Sun, Apr 23, 2023 at 8:38 PM Jeff LaBundy <[email protected]> wrote:
>
> > @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> > int ret;
> >
> > - ret = regulator_enable(ihid_goodix->vdd);
> > - if (ret)
> > - return ret;
> > -
> > - ret = regulator_enable(ihid_goodix->vddio);
> > - if (ret)
> > - return ret;
> > + /*
> > + * This is to ensure that the reset GPIO will be asserted and the
> > + * regulators will be enabled for all cases.
> > + */
> > + if (ihid_goodix->powered_in_suspend) {
> > + /*
> > + * This is not mandatory, but we assert reset here (instead of
> > + * in power-down) to ensure that the device will have a clean
> > + * state later on just like the normal scenarios would have.
> > + *
> > + * Also, since the regulators were not disabled in power-down,
> > + * we don't need to enable them here.
> > + */
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + } else {
> > + /*
> > + * In this case, the reset is already asserted (either in
> > + * probe or power-down).
> > + * All we need is to enable the regulators.
> > + */
> > + ret = regulator_enable(ihid_goodix->vdd);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_enable(ihid_goodix->vddio);
> > + if (ret)
> > + return ret;
> > + }
>
> Please let me know in case I have misunderstood, but I don't see a need
> to change the regulator_enable/disable() logic if this property is set.
> If the regulators are truly always-on, the regulator core already knows
> what to do and we should not duplicate that logic here.
>
> Based on the alleged silicon erratum discussed in patch [1/2], it seems
> we only want to control the behavior of the reset GPIO. Therefore, only
> the calls to gpiod_set_value_cansleep() should be affected and the name
> of the property updated to reflect what it's actually doing.
This would be OK w/ me.
On Tue, Apr 25, 2023 at 2:15 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <[email protected]> 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
Hi,
On Tue, Apr 25, 2023 at 2:16 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sun, Apr 23, 2023 at 8:38 PM Jeff LaBundy <[email protected]> wrote:
> >
> > > @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > > container_of(ops, struct i2c_hid_of_goodix, ops);
> > > int ret;
> > >
> > > - ret = regulator_enable(ihid_goodix->vdd);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - ret = regulator_enable(ihid_goodix->vddio);
> > > - if (ret)
> > > - return ret;
> > > + /*
> > > + * This is to ensure that the reset GPIO will be asserted and the
> > > + * regulators will be enabled for all cases.
> > > + */
> > > + if (ihid_goodix->powered_in_suspend) {
> > > + /*
> > > + * This is not mandatory, but we assert reset here (instead of
> > > + * in power-down) to ensure that the device will have a clean
> > > + * state later on just like the normal scenarios would have.
> > > + *
> > > + * Also, since the regulators were not disabled in power-down,
> > > + * we don't need to enable them here.
> > > + */
> > > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > > + } else {
> > > + /*
> > > + * In this case, the reset is already asserted (either in
> > > + * probe or power-down).
> > > + * All we need is to enable the regulators.
> > > + */
> > > + ret = regulator_enable(ihid_goodix->vdd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regulator_enable(ihid_goodix->vddio);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > Please let me know in case I have misunderstood, but I don't see a need
> > to change the regulator_enable/disable() logic if this property is set.
> > If the regulators are truly always-on, the regulator core already knows
> > what to do and we should not duplicate that logic here.
Your understanding is totally right, let me restore that in the next
revision. Thanks!
Regards,
Fei
> >
> > Based on the alleged silicon erratum discussed in patch [1/2], it seems
> > we only want to control the behavior of the reset GPIO. Therefore, only
> > the calls to gpiod_set_value_cansleep() should be affected and the name
> > of the property updated to reflect what it's actually doing.
>
> This would be OK w/ me.