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]/
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
- Do not change the regulator_enable logic during power-up.
Fei Shao (2):
dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend"
property
HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend"
property
.../bindings/input/goodix,gt7375p.yaml | 9 +++++++++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
--
2.40.1.495.gc816e09b53d-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.
To address that, we add this property to skip reset during suspend.
If it's set, the driver will stop asserting the reset GPIO during
power-down. Refer to the comments in the driver for details.
Signed-off-by: Fei Shao <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
---
(no changes since v2)
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
.../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..1edad1da1196 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,15 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ goodix,no-reset-during-suspend:
+ description:
+ Set this to true to enforce the driver to not assert the reset GPIO
+ during suspend.
+ Due to potential touchscreen hardware flaw, back-powering could happen in
+ suspend if the power supply is on and with active-low reset GPIO asserted.
+ This property is used to avoid the back-powering issue.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.1.495.gc816e09b53d-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 new
"goodix,no-reset-during-suspend" property in the driver:
- When set to true, the driver does not assert the reset GPIO during
power-down.
Instead, the GPIO will be asserted during power-up to ensure the
touchscreen always has a clean start and consistent behavior after
resuming.
This is for designs with a dedicated always-on regulator.
- When set to false or unset, the driver uses the original control flow
and asserts GPIO and disables regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Do not change the regulator_enable logic during power-up.
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..3ed365b50432 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 no_reset_during_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
+ if (ihid_goodix->no_reset_during_suspend) {
+ /*
+ * We assert reset GPIO here (instead of during power-down) to
+ * ensure the device will have a clean state after powering up,
+ * just like the normal scenarios will have.
+ */
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ }
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
@@ -60,7 +69,9 @@ 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);
- gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ if (!ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
}
@@ -91,6 +102,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->no_reset_during_suspend =
+ of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.1.495.gc816e09b53d-goog
Hi,
On Wed, Apr 26, 2023 at 7:44 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 new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disables regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
>
> ---
>
> Changes in v3:
> - In power-down, only skip the GPIO but not the regulator calls if the
> flag is set
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
You already carried over my Reviewed-by tag, which is fine. ...but
just sending a quick confirmation that v3 looks good to me. Thanks!
-Doug
Hi Fei,
On Wed, Apr 26, 2023 at 10:44:21PM +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.
>
> To address that, we add this property to skip reset during suspend.
> If it's set, the driver will stop asserting the reset GPIO during
> power-down. Refer to the comments in the driver for details.
>
> Signed-off-by: Fei Shao <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> Reviewed-by: Matthias Brugger <[email protected]>
> ---
Reviewed-by: Jeff LaBundy <[email protected]>
>
> (no changes since v2)
>
> Changes in v2:
> - Use a more accurate property name and with "goodix," prefix.
>
> .../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index ce18d7dadae2..1edad1da1196 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -43,6 +43,15 @@ properties:
> itself as long as it allows the main board to make signals compatible
> with what the touchscreen is expecting for its IO rails.
>
> + goodix,no-reset-during-suspend:
> + description:
> + Set this to true to enforce the driver to not assert the reset GPIO
> + during suspend.
> + Due to potential touchscreen hardware flaw, back-powering could happen in
> + suspend if the power supply is on and with active-low reset GPIO asserted.
> + This property is used to avoid the back-powering issue.
> + type: boolean
> +
> required:
> - compatible
> - reg
> --
> 2.40.1.495.gc816e09b53d-goog
>
Many thanks to you and Doug for the informative discussion.
Kind regards,
Jeff LaBundy
Hi Fei,
On Wed, Apr 26, 2023 at 10:44:22PM +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 new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disables regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
Great work; one tiny nit below. If you do not agree with it or have found
precedent that suggests it is OK, feel free to ignore it. Either way:
Reviewed-by: Jeff LaBundy <[email protected]>
>
> ---
>
> Changes in v3:
> - In power-down, only skip the GPIO but not the regulator calls if the
> flag is set
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..3ed365b50432 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 no_reset_during_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> + if (ihid_goodix->no_reset_during_suspend) {
> + /*
> + * We assert reset GPIO here (instead of during power-down) to
> + * ensure the device will have a clean state after powering up,
> + * just like the normal scenarios will have.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + }
I don't think curly braces are technically required here, as there is only
one statement within the conditional despite the comments making it appear
otherwise. Maybe this would work too:
/* ... */
if (...)
gpiod_set_value_cansleep(...);
Again however, I do not feel strongly about this.
> ret = regulator_enable(ihid_goodix->vdd);
> if (ret)
> return ret;
> @@ -60,7 +69,9 @@ 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);
>
> - gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + if (!ihid_goodix->no_reset_during_suspend)
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> +
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
> }
> @@ -91,6 +102,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->no_reset_during_suspend =
> + of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
> +
> ihid_goodix->timings = device_get_match_data(&client->dev);
>
> return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> --
> 2.40.1.495.gc816e09b53d-goog
>
Kind regards,
Jeff LaBundy
Hi,
On Thu, Apr 27, 2023 at 9:08 AM Jeff LaBundy <[email protected]> wrote:
>
> Hi Fei,
>
> On Wed, Apr 26, 2023 at 10:44:22PM +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 new
> > "goodix,no-reset-during-suspend" property in the driver:
> > - When set to true, the driver does not assert the reset GPIO during
> > power-down.
> > Instead, the GPIO will be asserted during power-up to ensure the
> > touchscreen always has a clean start and consistent behavior after
> > resuming.
> > This is for designs with a dedicated always-on regulator.
> > - When set to false or unset, the driver uses the original control flow
> > and asserts GPIO and disables regulators normally.
> > This is for the two-regulator and shared-regulator designs.
> >
> > Signed-off-by: Fei Shao <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
>
> Great work; one tiny nit below. If you do not agree with it or have found
> precedent that suggests it is OK, feel free to ignore it. Either way:
>
> Reviewed-by: Jeff LaBundy <[email protected]>
>
> >
> > ---
> >
> > Changes in v3:
> > - In power-down, only skip the GPIO but not the regulator calls if the
> > flag is set
> >
> > Changes in v2:
> > - Do not change the regulator_enable logic during power-up.
> >
> > drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > index 0060e3dcd775..3ed365b50432 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 no_reset_during_suspend;
> > const struct goodix_i2c_hid_timing_data *timings;
> > };
> >
> > @@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> > int ret;
> >
> > + if (ihid_goodix->no_reset_during_suspend) {
> > + /*
> > + * We assert reset GPIO here (instead of during power-down) to
> > + * ensure the device will have a clean state after powering up,
> > + * just like the normal scenarios will have.
> > + */
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + }
>
> I don't think curly braces are technically required here, as there is only
> one statement within the conditional despite the comments making it appear
> otherwise. Maybe this would work too:
>
> /* ... */
> if (...)
> gpiod_set_value_cansleep(...);
>
> Again however, I do not feel strongly about this.
Oops, I just carried it over from v1 and didn't think that much at the
moment, but indeed I should have cleaned that up.
V4 is on the way, thanks for pointing that out. :)
Regards,
Fei
>
> > ret = regulator_enable(ihid_goodix->vdd);
> > if (ret)
> > return ret;
> > @@ -60,7 +69,9 @@ 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);
> >
> > - gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + if (!ihid_goodix->no_reset_during_suspend)
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > +
> > regulator_disable(ihid_goodix->vddio);
> > regulator_disable(ihid_goodix->vdd);
> > }
> > @@ -91,6 +102,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->no_reset_during_suspend =
> > + of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
> > +
> > ihid_goodix->timings = device_get_match_data(&client->dev);
> >
> > return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> > --
> > 2.40.1.495.gc816e09b53d-goog
> >
>
> Kind regards,
> Jeff LaBundy