[PATCH 0/7] arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards

[PATCH 0/7] arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards

[Douglas Anderson]

Trying to figure out how to talk to the touchscreen properly on
sc7280-herobrine boards was a long and difficult process. Many
Engineering hours were spent deciding how exactly one should talk over
i2c to a peripheral. In the end, a solution has been found and this
patch series attempts to implement it in a way that will work for all
herobrine-based boards.

Validation of this code has actually been done on
sc7280-herobrine-villager. Those patches are device-tree only and are
placed first. They shouldn't be super controversial, I hope.

In order to make this work, we also need to support
sc7280-herobrine-evoker. For evoker, things are a bit tricker, though
(I think) the solution we ended up with is not terrible. See the
"Goodix" bindings patch for the full details. Unfortunately, I haven't
tested the final version of these patches on evoker hardware. Thus
those patches are at the end of the series and marked as such. It
still wouldn't hurt to land them, if people are OK with it, since
nobody in the wild has this hardware and and the evoker-specific parts
of the patch are very easy to validate.


Douglas Anderson (7):
  arm64: dts: qcom: sc7280: On QCard, regulator L3C should be 1.8V
  arm64: dts: qcom: sc7280: Add 3ms ramp to herobrine's
    pp3300_left_in_mlb
  arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on villager
  HID: i2c-hid: goodix: Stop tying the reset line to the regulator
  dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply
  HID: i2c-hid: goodix: Add mainboard-vddio-supply
  arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on evoker

 .../bindings/input/goodix,gt7375p.yaml        |  7 ++
 .../dts/qcom/sc7280-herobrine-evoker.dtsi     |  1 +
 .../dts/qcom/sc7280-herobrine-villager.dtsi   |  1 +
 .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  2 +
 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi    | 10 +-
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c       | 98 +++++--------------
 6 files changed, 42 insertions(+), 77 deletions(-)

-- 
2.39.1.519.gcb327c4b5f-goog

[PATCH 1/7] arm64: dts: qcom: sc7280: On QCard, regulator L3C should be 1.8V

[Douglas Anderson]

On the first sc7280 QCards the L3C rail was never really used for
anything. Stuffing options on the QCard meant that the QCard itself
didn't use this rail for anything. This rail did get sent to the
mainboard, but no existing mainboards ever did anything with it other
that route it to a testpoint.

On later sc7280 QCards, the L3C rail was repurposed. Instead of being
a (nominally) 3.3V rail, it was decided to make it a 1.8V rail. It is
now provided to the display connector (which might route it to the
touchscreen) and also used to power some buffers relating to
touchscreen IO. This rail is getting the additional tag "ts_avccio",
though some places still refer to it as "vreg_l3c_3p0" despite the
fact that the name now specifies the wrong voltage.

Since it never hurts for this rail to be 1.8V (even on old QCards /
old boards), let's just change it to 1.8V across the board and add the
extra "ts_avccio" moniker as a label in the device tree.

Future patches will start using this rail in their touchscreens.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
index df49564ae6dc..50d963957303 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
@@ -230,9 +230,15 @@ vreg_l2c_1p8: ldo2 {
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
 		};
 
+		/*
+		 * The initial design of this regulator was to use it as 3.3V,
+		 * but due to later changes in design it was changed to 1.8V.
+		 * The original name is kept due to same schematic.
+		 */
+		ts_avccio:
 		vreg_l3c_3p0: ldo3 {
-			regulator-min-microvolt = <2800000>;
-			regulator-max-microvolt = <3540000>;
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
 		};
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 1/7] arm64: dts: qcom: sc7280: On QCard, regulator L3C should be 1.8V

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:10PM -0800, Douglas Anderson wrote:
> On the first sc7280 QCards the L3C rail was never really used for
> anything. Stuffing options on the QCard meant that the QCard itself
> didn't use this rail for anything. This rail did get sent to the
> mainboard, but no existing mainboards ever did anything with it other
> that route it to a testpoint.

nit: s/that/than/

no need to re-spin just for that

> On later sc7280 QCards, the L3C rail was repurposed. Instead of being
> a (nominally) 3.3V rail, it was decided to make it a 1.8V rail. It is
> now provided to the display connector (which might route it to the
> touchscreen) and also used to power some buffers relating to
> touchscreen IO. This rail is getting the additional tag "ts_avccio",
> though some places still refer to it as "vreg_l3c_3p0" despite the
> fact that the name now specifies the wrong voltage.
> 
> Since it never hurts for this rail to be 1.8V (even on old QCards /
> old boards), let's just change it to 1.8V across the board and add the
> extra "ts_avccio" moniker as a label in the device tree.
> 
> Future patches will start using this rail in their touchscreens.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 2/7] arm64: dts: qcom: sc7280: Add 3ms ramp to herobrine's pp3300_left_in_mlb

[Douglas Anderson]

The "pp3300_left_in_mlb" rail on herobrine eventually connects up to
"vreg_edp_3p3" on the qcard. On several herobrine designs this rail
has been measured to need more than 1ms to turn on.

While technically a herobrine derivative (defined as anyone including
the "herobrine.dtsi") could change the board to make the rail rise
faster or slower, the fact that two boards (evoker and villager) both
measured it as taking more than 1ms implies that it's probably going
to be the norm. Thus, let's add a "regulator-enable-ramp-delay"
straight into the herobrine.dtsi to handle this. If a particular
derivative board needs a faster or slower one then they can override
it, though that feels unlikely.

While we measured something a bit over 1ms, we'll choose 3ms to give
us a tiny bit of margin. This isn't a rail that turns off and on all
the time anyway and 3ms is nothing compared to the total amount of
time to power on a panel.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 27f479ff9d80..ded36b5d28c7 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -108,6 +108,8 @@ pp3300_left_in_mlb: pp3300-left-in-mlb-regulator {
 		pinctrl-names = "default";
 		pinctrl-0 = <&en_pp3300_dx_edp>;
 
+		regulator-enable-ramp-delay = <3000>;
+
 		vin-supply = <&pp3300_z1>;
 	};
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 2/7] arm64: dts: qcom: sc7280: Add 3ms ramp to herobrine's pp3300_left_in_mlb

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:11PM -0800, Douglas Anderson wrote:
> The "pp3300_left_in_mlb" rail on herobrine eventually connects up to
> "vreg_edp_3p3" on the qcard. On several herobrine designs this rail
> has been measured to need more than 1ms to turn on.
> 
> While technically a herobrine derivative (defined as anyone including
> the "herobrine.dtsi") could change the board to make the rail rise
> faster or slower, the fact that two boards (evoker and villager) both
> measured it as taking more than 1ms implies that it's probably going
> to be the norm. Thus, let's add a "regulator-enable-ramp-delay"
> straight into the herobrine.dtsi to handle this. If a particular
> derivative board needs a faster or slower one then they can override
> it, though that feels unlikely.
> 
> While we measured something a bit over 1ms, we'll choose 3ms to give
> us a tiny bit of margin. This isn't a rail that turns off and on all
> the time anyway and 3ms is nothing compared to the total amount of
> time to power on a panel.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 3/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on villager

[Douglas Anderson]

On never revs of sc7280-herobrine-villager (rev2+) the L3C rail is
provided to the touchscreen as the IO voltage rail. Let's add it in
the device tree.

NOTE: Even though this is only really needed on rev2+ villagers (-rev0
had non-functioning touchscreen and -rev1 had some hacky hardware
magic), it doesn't actually hurt to do this for old villager revs. As
talked about in the patch ("arm64: dts: qcom: sc7280: On QCard,
regulator L3C should be 1.8V") the L3C regulator didn't go anywhere at
all on older revs. That means that turning it on for older revs
doesn't hurt other than drawing a tiny bit of extra power. Since -rev0
and -rev1 villagers will never make it to real customers and it's nice
not to have too many old device trees, the better tradeoff seems to be
to enable it everywhere.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7280-herobrine-villager.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager.dtsi
index 17553e0fd6fd..818d4046d2c7 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager.dtsi
@@ -55,6 +55,7 @@ ap_ts: touchscreen@10 {
 		reset-gpios = <&tlmm 54 GPIO_ACTIVE_LOW>;
 
 		vcc33-supply = <&ts_avdd>;
+		vccio-supply = <&ts_avccio>;
 	};
 };
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 3/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on villager

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:12PM -0800, Douglas Anderson wrote:
> On never revs of sc7280-herobrine-villager (rev2+) the L3C rail is

nit: s/never/newer/

no need to re-spin just for this.

> provided to the touchscreen as the IO voltage rail. Let's add it in
> the device tree.
> 
> NOTE: Even though this is only really needed on rev2+ villagers (-rev0
> had non-functioning touchscreen and -rev1 had some hacky hardware
> magic), it doesn't actually hurt to do this for old villager revs. As
> talked about in the patch ("arm64: dts: qcom: sc7280: On QCard,
> regulator L3C should be 1.8V") the L3C regulator didn't go anywhere at
> all on older revs. That means that turning it on for older revs
> doesn't hurt other than drawing a tiny bit of extra power. Since -rev0
> and -rev1 villagers will never make it to real customers and it's nice
> not to have too many old device trees, the better tradeoff seems to be
> to enable it everywhere.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 4/7] HID: i2c-hid: goodix: Stop tying the reset line to the regulator

[Douglas Anderson]

In commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to
true state of the regulator"), we started tying the reset line of
Goodix touchscreens to the regulator.

The primary motivation for that patch was some pre-production hardware
(specifically sc7180-trogdor-homestar) where it was proposed to hook
the touchscreen's main 3.3V power rail to an always-on supply. In such
a case, when we turned "off" the touchscreen in Linux it was bad to
assert the "reset" GPIO because that was causing a power drain. The
patch accomplished that goal and did it in a general sort of way that
didn't require special properties to be added in the device tree for
homestar.

It turns out that the design of using an always-on power rail for the
touchscreen was rejected soon after the patch was written and long
before sc7180-trogdor-homestar went into production. The final design
of homestar actually fully separates the rail for the touchscreen and
the display panel and both can be powered off and on. That means that
the original motivation for the feature is gone.

There are 3 other users of the goodix i2c-hid driver in mainline.

I'll first talk about 2 of the other users in mainline: coachz and
mrbland. On both coachz and mrbland the touchscreen power and panel
power _are_ shared. That means that the patch to tie the reset line to
the true state of the regulator _is_ doing something on those
boards. Specifically, the patch reduced power consumption by tens of
mA in the case where we turned the touchscreen off but left the panel
on. Other than saving a small bit of power, the patch wasn't truly
necessary. That being said, even though a small bit of power was saved
in the state of "panel on + touchscreen off", that's not actually a
state we ever expect to be in, except perhaps for very short periods
of time at boot or during suspend/resume. Thus, the patch is truly not
necessary. It should be further noted that, as documented in the
original patch, the current code still didn't optimize power for every
corner case of the "shared rail" situation.

The last user in mainline was very recently added: evoker. Evoker is
actually the motivation for me removing this bit of code. It turns out
that for evoker we need to manage a second power rail for IO to the
touchscreen. Trying to fit the management of this IO rail into the
regulator notifiers turns out to be extremely hard. To avoid lockdep
splats you shouldn't enable/disable other regulators in regulator
notifiers and trying to find a way around this was going to be fairly
difficult.

Given the lack of any true motivation to tie the reset line to the
regulator, lets go back to the simpler days and remove the code. This
is, effectively, a revert of commit bdbc65eb77ee ("HID: i2c-hid:
goodix: Fix a lockdep splat"), commit 25ddd7cfc582 ("HID: i2c-hid:
goodix: Use the devm variant of regulator_register_notifier()"), and
commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
state of the regulator").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 88 ++++---------------------
 1 file changed, 13 insertions(+), 75 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 29c6cb174032..584d833dc0aa 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -26,28 +26,28 @@ struct i2c_hid_of_goodix {
 	struct i2chid_ops ops;
 
 	struct regulator *vdd;
-	struct notifier_block nb;
 	struct gpio_desc *reset_gpio;
 	const struct goodix_i2c_hid_timing_data *timings;
 };
 
-static void goodix_i2c_hid_deassert_reset(struct i2c_hid_of_goodix *ihid_goodix,
-					  bool regulator_just_turned_on)
+static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
 {
-	if (regulator_just_turned_on && ihid_goodix->timings->post_power_delay_ms)
+	struct i2c_hid_of_goodix *ihid_goodix =
+		container_of(ops, struct i2c_hid_of_goodix, ops);
+	int ret;
+
+	ret = regulator_enable(ihid_goodix->vdd);
+	if (ret)
+		return ret;
+
+	if (ihid_goodix->timings->post_power_delay_ms)
 		msleep(ihid_goodix->timings->post_power_delay_ms);
 
 	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
 	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
 		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
-}
-
-static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
-{
-	struct i2c_hid_of_goodix *ihid_goodix =
-		container_of(ops, struct i2c_hid_of_goodix, ops);
 
-	return regulator_enable(ihid_goodix->vdd);
+	return 0;
 }
 
 static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
@@ -55,42 +55,14 @@ 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);
 	regulator_disable(ihid_goodix->vdd);
 }
 
-static int ihid_goodix_vdd_notify(struct notifier_block *nb,
-				    unsigned long event,
-				    void *ignored)
-{
-	struct i2c_hid_of_goodix *ihid_goodix =
-		container_of(nb, struct i2c_hid_of_goodix, nb);
-	int ret = NOTIFY_OK;
-
-	switch (event) {
-	case REGULATOR_EVENT_PRE_DISABLE:
-		gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
-		break;
-
-	case REGULATOR_EVENT_ENABLE:
-		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
-		break;
-
-	case REGULATOR_EVENT_ABORT_DISABLE:
-		goodix_i2c_hid_deassert_reset(ihid_goodix, false);
-		break;
-
-	default:
-		ret = NOTIFY_DONE;
-		break;
-	}
-
-	return ret;
-}
-
 static int i2c_hid_of_goodix_probe(struct i2c_client *client)
 {
 	struct i2c_hid_of_goodix *ihid_goodix;
-	int ret;
+
 	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
 				   GFP_KERNEL);
 	if (!ihid_goodix)
@@ -111,40 +83,6 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
 
 	ihid_goodix->timings = device_get_match_data(&client->dev);
 
-	/*
-	 * We need to control the "reset" line in lockstep with the regulator
-	 * actually turning on an off instead of just when we make the request.
-	 * This matters if the regulator is shared with another consumer.
-	 * - If the regulator is off then we must assert reset. The reset
-	 *   line is active low and on some boards it could cause a current
-	 *   leak if left high.
-	 * - If the regulator is on then we don't want reset asserted for very
-	 *   long. Holding the controller in reset apparently draws extra
-	 *   power.
-	 */
-	ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
-	ret = devm_regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb);
-	if (ret)
-		return dev_err_probe(&client->dev, ret,
-			"regulator notifier request failed\n");
-
-	/*
-	 * If someone else is holding the regulator on (or the regulator is
-	 * an always-on one) we might never be told to deassert reset. Do it
-	 * now... and temporarily bump the regulator reference count just to
-	 * make sure it is impossible for this to race with our own notifier!
-	 * We also assume that someone else might have _just barely_ turned
-	 * the regulator on so we'll do the full "post_power_delay" just in
-	 * case.
-	 */
-	if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) {
-		ret = regulator_enable(ihid_goodix->vdd);
-		if (ret)
-			return ret;
-		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
-		regulator_disable(ihid_goodix->vdd);
-	}
-
 	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
 }
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 4/7] HID: i2c-hid: goodix: Stop tying the reset line to the regulator

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 06:48:13PM -0800, Douglas Anderson wrote:
> In commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to
> true state of the regulator"), we started tying the reset line of
> Goodix touchscreens to the regulator.
> 
> The primary motivation for that patch was some pre-production hardware
> (specifically sc7180-trogdor-homestar) where it was proposed to hook
> the touchscreen's main 3.3V power rail to an always-on supply. In such
> a case, when we turned "off" the touchscreen in Linux it was bad to
> assert the "reset" GPIO because that was causing a power drain. The
> patch accomplished that goal and did it in a general sort of way that
> didn't require special properties to be added in the device tree for
> homestar.
> 
> It turns out that the design of using an always-on power rail for the
> touchscreen was rejected soon after the patch was written and long
> before sc7180-trogdor-homestar went into production. The final design
> of homestar actually fully separates the rail for the touchscreen and
> the display panel and both can be powered off and on. That means that
> the original motivation for the feature is gone.
> 
> There are 3 other users of the goodix i2c-hid driver in mainline.
> 
> I'll first talk about 2 of the other users in mainline: coachz and
> mrbland. On both coachz and mrbland the touchscreen power and panel
> power _are_ shared. That means that the patch to tie the reset line to
> the true state of the regulator _is_ doing something on those
> boards. Specifically, the patch reduced power consumption by tens of
> mA in the case where we turned the touchscreen off but left the panel
> on. Other than saving a small bit of power, the patch wasn't truly
> necessary. That being said, even though a small bit of power was saved
> in the state of "panel on + touchscreen off", that's not actually a
> state we ever expect to be in, except perhaps for very short periods
> of time at boot or during suspend/resume. Thus, the patch is truly not
> necessary. It should be further noted that, as documented in the
> original patch, the current code still didn't optimize power for every
> corner case of the "shared rail" situation.
> 
> The last user in mainline was very recently added: evoker. Evoker is
> actually the motivation for me removing this bit of code. It turns out
> that for evoker we need to manage a second power rail for IO to the
> touchscreen. Trying to fit the management of this IO rail into the
> regulator notifiers turns out to be extremely hard. To avoid lockdep
> splats you shouldn't enable/disable other regulators in regulator
> notifiers and trying to find a way around this was going to be fairly
> difficult.
> 
> Given the lack of any true motivation to tie the reset line to the
> regulator, lets go back to the simpler days and remove the code. This
> is, effectively, a revert of commit bdbc65eb77ee ("HID: i2c-hid:
> goodix: Fix a lockdep splat"), commit 25ddd7cfc582 ("HID: i2c-hid:
> goodix: Use the devm variant of regulator_register_notifier()"), and
> commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
> state of the regulator").
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
> 
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 88 ++++---------------------
>  1 file changed, 13 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 29c6cb174032..584d833dc0aa 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -26,28 +26,28 @@ struct i2c_hid_of_goodix {
>  	struct i2chid_ops ops;
>  
>  	struct regulator *vdd;
> -	struct notifier_block nb;
>  	struct gpio_desc *reset_gpio;
>  	const struct goodix_i2c_hid_timing_data *timings;
>  };
>  
> -static void goodix_i2c_hid_deassert_reset(struct i2c_hid_of_goodix *ihid_goodix,
> -					  bool regulator_just_turned_on)
> +static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
>  {
> -	if (regulator_just_turned_on && ihid_goodix->timings->post_power_delay_ms)
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(ops, struct i2c_hid_of_goodix, ops);
> +	int ret;
> +
> +	ret = regulator_enable(ihid_goodix->vdd);
> +	if (ret)
> +		return ret;
> +
> +	if (ihid_goodix->timings->post_power_delay_ms)
>  		msleep(ihid_goodix->timings->post_power_delay_ms);
>  
>  	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
>  	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
>  		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
> -}
> -
> -static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> -{
> -	struct i2c_hid_of_goodix *ihid_goodix =
> -		container_of(ops, struct i2c_hid_of_goodix, ops);
>  
> -	return regulator_enable(ihid_goodix->vdd);
> +	return 0;
>  }
>  
>  static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> @@ -55,42 +55,14 @@ 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);
>  	regulator_disable(ihid_goodix->vdd);
>  }
>  
> -static int ihid_goodix_vdd_notify(struct notifier_block *nb,
> -				    unsigned long event,
> -				    void *ignored)
> -{
> -	struct i2c_hid_of_goodix *ihid_goodix =
> -		container_of(nb, struct i2c_hid_of_goodix, nb);
> -	int ret = NOTIFY_OK;
> -
> -	switch (event) {
> -	case REGULATOR_EVENT_PRE_DISABLE:
> -		gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> -		break;
> -
> -	case REGULATOR_EVENT_ENABLE:
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
> -		break;
> -
> -	case REGULATOR_EVENT_ABORT_DISABLE:
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, false);
> -		break;
> -
> -	default:
> -		ret = NOTIFY_DONE;
> -		break;
> -	}
> -
> -	return ret;
> -}
> -
>  static int i2c_hid_of_goodix_probe(struct i2c_client *client)
>  {
>  	struct i2c_hid_of_goodix *ihid_goodix;
> -	int ret;
> +
>  	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
>  				   GFP_KERNEL);
>  	if (!ihid_goodix)
> @@ -111,40 +83,6 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
>  
>  	ihid_goodix->timings = device_get_match_data(&client->dev);
>  
> -	/*
> -	 * We need to control the "reset" line in lockstep with the regulator
> -	 * actually turning on an off instead of just when we make the request.
> -	 * This matters if the regulator is shared with another consumer.
> -	 * - If the regulator is off then we must assert reset. The reset
> -	 *   line is active low and on some boards it could cause a current
> -	 *   leak if left high.
> -	 * - If the regulator is on then we don't want reset asserted for very
> -	 *   long. Holding the controller in reset apparently draws extra
> -	 *   power.
> -	 */
> -	ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
> -	ret = devm_regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb);
> -	if (ret)
> -		return dev_err_probe(&client->dev, ret,
> -			"regulator notifier request failed\n");
> -
> -	/*
> -	 * If someone else is holding the regulator on (or the regulator is
> -	 * an always-on one) we might never be told to deassert reset. Do it
> -	 * now... and temporarily bump the regulator reference count just to
> -	 * make sure it is impossible for this to race with our own notifier!
> -	 * We also assume that someone else might have _just barely_ turned
> -	 * the regulator on so we'll do the full "post_power_delay" just in
> -	 * case.
> -	 */
> -	if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) {
> -		ret = regulator_enable(ihid_goodix->vdd);
> -		if (ret)
> -			return ret;
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
> -		regulator_disable(ihid_goodix->vdd);
> -	}
> -
>  	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
>  }
>  
> -- 
> 2.39.1.519.gcb327c4b5f-goog
> 

-- 
Dmitry

Re: [PATCH 4/7] HID: i2c-hid: goodix: Stop tying the reset line to the regulator

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:13PM -0800, Douglas Anderson wrote:
> In commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to
> true state of the regulator"), we started tying the reset line of
> Goodix touchscreens to the regulator.
> 
> The primary motivation for that patch was some pre-production hardware
> (specifically sc7180-trogdor-homestar) where it was proposed to hook
> the touchscreen's main 3.3V power rail to an always-on supply. In such
> a case, when we turned "off" the touchscreen in Linux it was bad to
> assert the "reset" GPIO because that was causing a power drain. The
> patch accomplished that goal and did it in a general sort of way that
> didn't require special properties to be added in the device tree for
> homestar.
> 
> It turns out that the design of using an always-on power rail for the
> touchscreen was rejected soon after the patch was written and long
> before sc7180-trogdor-homestar went into production. The final design
> of homestar actually fully separates the rail for the touchscreen and
> the display panel and both can be powered off and on. That means that
> the original motivation for the feature is gone.
> 
> There are 3 other users of the goodix i2c-hid driver in mainline.
> 
> I'll first talk about 2 of the other users in mainline: coachz and
> mrbland. On both coachz and mrbland the touchscreen power and panel
> power _are_ shared. That means that the patch to tie the reset line to
> the true state of the regulator _is_ doing something on those
> boards. Specifically, the patch reduced power consumption by tens of
> mA in the case where we turned the touchscreen off but left the panel
> on. Other than saving a small bit of power, the patch wasn't truly
> necessary. That being said, even though a small bit of power was saved
> in the state of "panel on + touchscreen off", that's not actually a
> state we ever expect to be in, except perhaps for very short periods
> of time at boot or during suspend/resume. Thus, the patch is truly not
> necessary. It should be further noted that, as documented in the
> original patch, the current code still didn't optimize power for every
> corner case of the "shared rail" situation.
> 
> The last user in mainline was very recently added: evoker. Evoker is
> actually the motivation for me removing this bit of code. It turns out
> that for evoker we need to manage a second power rail for IO to the
> touchscreen. Trying to fit the management of this IO rail into the
> regulator notifiers turns out to be extremely hard. To avoid lockdep
> splats you shouldn't enable/disable other regulators in regulator
> notifiers and trying to find a way around this was going to be fairly
> difficult.
> 
> Given the lack of any true motivation to tie the reset line to the
> regulator, lets go back to the simpler days and remove the code. This
> is, effectively, a revert of commit bdbc65eb77ee ("HID: i2c-hid:
> goodix: Fix a lockdep splat"), commit 25ddd7cfc582 ("HID: i2c-hid:
> goodix: Use the devm variant of regulator_register_notifier()"), and
> commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
> state of the regulator").
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 5/7] dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Douglas Anderson]

The goodix i2c-hid bindings currently support two models of
touchscreen: GT7375P and GT7986U. The datasheets of both touchscreens
show the following things:
* The mainboard that the touchscreen is connected to is only expected
  to supply one voltage to the touchscreen: 3.3V.
* The touchscreen, depending on stuffing options, can accept IO to the
  touchscreen as either 3.3V or 1.8V. Presumably this means that the
  touchscreen has its own way internally to make or deal with 1.8V
  signals when it's configured for 1.8V IO.

NOTE: you've got to look very carefully at the datasheet for the
touchscreen to see that the above bullets are true. Specifically, the
datasheet shows a signal called VDDIO and one might think that this is
where a mainboard would provide VDDIO to the touchscreen. Upon closer
inspection, however, a footnote can be found that says "When VDDIO is
left floating, the logic level is 1.8V [...]; when VDDIO is connected
to AVDD, the logic level is AVDD.". Thus the VDDIO pin on the
touchscreen IC is actually a selector and not a pin whre the mainboard
would pass a reference voltage.

The fact that the touchscreen isn't supplied 1.8V by the mainboard
means that when I originally submitted bindings for these touchscreens
I only listed the 3.3V rail in the bindings. It can be noted that the
original bindings and driver were added for sc7180-trogdor boards and
these boards all use 3.3V IO via a level shifter on the mainboard.

It turns out that with sc7280-herobrine-evoker, we've got a bit of a
strange monkey on our hands. Due to some very interesting but
(unfortunately) set-in-stone hardware design, we are doing 1.8V IO to
the touchscreen but we _also_ have some extra buffers on the mainboard
that need to be powered up to make the IO lines work. After much
pondering about this, it seems like the best way to handle this is to
add an optional "mainboard-vddio" rail to the bindings that is used to
power up the buffers. Specifically, the fact that the touchscreen
datasheet documents that its IOs can be at a different voltage level
than its main power rail means that there truly are two voltage rails
associated with the touchscreen, even if we don't actually provide the
IO rail to it. Thus it doesn't feel absurd for the DT node on the host
to have a 1.8V rail to power up anything related to its 1.8V logic.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../devicetree/bindings/input/goodix,gt7375p.yaml          | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index 1c191bc5a178..ce18d7dadae2 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -36,6 +36,13 @@ properties:
   vdd-supply:
     description: The 3.3V supply to the touchscreen.
 
+  mainboard-vddio-supply:
+    description:
+      The supply on the main board needed to power up IO signals going
+      to the touchscreen. This supply need not go to the touchscreen
+      itself as long as it allows the main board to make signals compatible
+      with what the touchscreen is expecting for its IO rails.
+
 required:
   - compatible
   - reg
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 5/7] dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 06:48:14PM -0800, Douglas Anderson wrote:
> The goodix i2c-hid bindings currently support two models of
> touchscreen: GT7375P and GT7986U. The datasheets of both touchscreens
> show the following things:
> * The mainboard that the touchscreen is connected to is only expected
>   to supply one voltage to the touchscreen: 3.3V.
> * The touchscreen, depending on stuffing options, can accept IO to the
>   touchscreen as either 3.3V or 1.8V. Presumably this means that the
>   touchscreen has its own way internally to make or deal with 1.8V
>   signals when it's configured for 1.8V IO.
> 
> NOTE: you've got to look very carefully at the datasheet for the
> touchscreen to see that the above bullets are true. Specifically, the
> datasheet shows a signal called VDDIO and one might think that this is
> where a mainboard would provide VDDIO to the touchscreen. Upon closer
> inspection, however, a footnote can be found that says "When VDDIO is
> left floating, the logic level is 1.8V [...]; when VDDIO is connected
> to AVDD, the logic level is AVDD.". Thus the VDDIO pin on the
> touchscreen IC is actually a selector and not a pin whre the mainboard
> would pass a reference voltage.
> 
> The fact that the touchscreen isn't supplied 1.8V by the mainboard
> means that when I originally submitted bindings for these touchscreens
> I only listed the 3.3V rail in the bindings. It can be noted that the
> original bindings and driver were added for sc7180-trogdor boards and
> these boards all use 3.3V IO via a level shifter on the mainboard.
> 
> It turns out that with sc7280-herobrine-evoker, we've got a bit of a
> strange monkey on our hands. Due to some very interesting but
> (unfortunately) set-in-stone hardware design, we are doing 1.8V IO to
> the touchscreen but we _also_ have some extra buffers on the mainboard
> that need to be powered up to make the IO lines work. After much
> pondering about this, it seems like the best way to handle this is to
> add an optional "mainboard-vddio" rail to the bindings that is used to
> power up the buffers. Specifically, the fact that the touchscreen
> datasheet documents that its IOs can be at a different voltage level
> than its main power rail means that there truly are two voltage rails
> associated with the touchscreen, even if we don't actually provide the
> IO rail to it. Thus it doesn't feel absurd for the DT node on the host
> to have a 1.8V rail to power up anything related to its 1.8V logic.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

We went over this with Doug offline, and after re-reading the spec sheet
this does make sense to me.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
> 
>  .../devicetree/bindings/input/goodix,gt7375p.yaml          | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index 1c191bc5a178..ce18d7dadae2 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -36,6 +36,13 @@ properties:
>    vdd-supply:
>      description: The 3.3V supply to the touchscreen.
>  
> +  mainboard-vddio-supply:
> +    description:
> +      The supply on the main board needed to power up IO signals going
> +      to the touchscreen. This supply need not go to the touchscreen
> +      itself as long as it allows the main board to make signals compatible
> +      with what the touchscreen is expecting for its IO rails.
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.39.1.519.gcb327c4b5f-goog
> 

-- 
Dmitry

Re: [PATCH 5/7] dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Rob Herring]

On Mon, 06 Feb 2023 18:48:14 -0800, Douglas Anderson wrote:
> The goodix i2c-hid bindings currently support two models of
> touchscreen: GT7375P and GT7986U. The datasheets of both touchscreens
> show the following things:
> * The mainboard that the touchscreen is connected to is only expected
>   to supply one voltage to the touchscreen: 3.3V.
> * The touchscreen, depending on stuffing options, can accept IO to the
>   touchscreen as either 3.3V or 1.8V. Presumably this means that the
>   touchscreen has its own way internally to make or deal with 1.8V
>   signals when it's configured for 1.8V IO.
> 
> NOTE: you've got to look very carefully at the datasheet for the
> touchscreen to see that the above bullets are true. Specifically, the
> datasheet shows a signal called VDDIO and one might think that this is
> where a mainboard would provide VDDIO to the touchscreen. Upon closer
> inspection, however, a footnote can be found that says "When VDDIO is
> left floating, the logic level is 1.8V [...]; when VDDIO is connected
> to AVDD, the logic level is AVDD.". Thus the VDDIO pin on the
> touchscreen IC is actually a selector and not a pin whre the mainboard
> would pass a reference voltage.
> 
> The fact that the touchscreen isn't supplied 1.8V by the mainboard
> means that when I originally submitted bindings for these touchscreens
> I only listed the 3.3V rail in the bindings. It can be noted that the
> original bindings and driver were added for sc7180-trogdor boards and
> these boards all use 3.3V IO via a level shifter on the mainboard.
> 
> It turns out that with sc7280-herobrine-evoker, we've got a bit of a
> strange monkey on our hands. Due to some very interesting but
> (unfortunately) set-in-stone hardware design, we are doing 1.8V IO to
> the touchscreen but we _also_ have some extra buffers on the mainboard
> that need to be powered up to make the IO lines work. After much
> pondering about this, it seems like the best way to handle this is to
> add an optional "mainboard-vddio" rail to the bindings that is used to
> power up the buffers. Specifically, the fact that the touchscreen
> datasheet documents that its IOs can be at a different voltage level
> than its main power rail means that there truly are two voltage rails
> associated with the touchscreen, even if we don't actually provide the
> IO rail to it. Thus it doesn't feel absurd for the DT node on the host
> to have a 1.8V rail to power up anything related to its 1.8V logic.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../devicetree/bindings/input/goodix,gt7375p.yaml          | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 6/7] HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Douglas Anderson]

As talked about in the patch ("dt-bindings: HID: i2c-hid: goodix: Add
mainboard-vddio-supply") we may need to power up a 1.8V rail on the
host associated with touchscreen IO. Let's add support in the driver
for it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Unfortunately, I haven't been able to actually test this on real
hardware yet. However, the change is very simple, I believe it is
correct, and it doesn't break other boards I've tested it on.

 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 584d833dc0aa..0060e3dcd775 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -26,6 +26,7 @@ struct i2c_hid_of_goodix {
 	struct i2chid_ops ops;
 
 	struct regulator *vdd;
+	struct regulator *vddio;
 	struct gpio_desc *reset_gpio;
 	const struct goodix_i2c_hid_timing_data *timings;
 };
@@ -40,6 +41,10 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
 	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);
 
@@ -56,6 +61,7 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
 		container_of(ops, struct i2c_hid_of_goodix, ops);
 
 	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+	regulator_disable(ihid_goodix->vddio);
 	regulator_disable(ihid_goodix->vdd);
 }
 
@@ -81,6 +87,10 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
 	if (IS_ERR(ihid_goodix->vdd))
 		return PTR_ERR(ihid_goodix->vdd);
 
+	ihid_goodix->vddio = devm_regulator_get(&client->dev, "mainboard-vddio");
+	if (IS_ERR(ihid_goodix->vddio))
+		return PTR_ERR(ihid_goodix->vddio);
+
 	ihid_goodix->timings = device_get_match_data(&client->dev);
 
 	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 6/7] HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 06:48:15PM -0800, Douglas Anderson wrote:
> As talked about in the patch ("dt-bindings: HID: i2c-hid: goodix: Add
> mainboard-vddio-supply") we may need to power up a 1.8V rail on the
> host associated with touchscreen IO. Let's add support in the driver
> for it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
> Unfortunately, I haven't been able to actually test this on real
> hardware yet. However, the change is very simple, I believe it is
> correct, and it doesn't break other boards I've tested it on.
> 
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 584d833dc0aa..0060e3dcd775 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -26,6 +26,7 @@ struct i2c_hid_of_goodix {
>  	struct i2chid_ops ops;
>  
>  	struct regulator *vdd;
> +	struct regulator *vddio;
>  	struct gpio_desc *reset_gpio;
>  	const struct goodix_i2c_hid_timing_data *timings;
>  };
> @@ -40,6 +41,10 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
>  	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);
>  
> @@ -56,6 +61,7 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
>  		container_of(ops, struct i2c_hid_of_goodix, ops);
>  
>  	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> +	regulator_disable(ihid_goodix->vddio);
>  	regulator_disable(ihid_goodix->vdd);
>  }
>  
> @@ -81,6 +87,10 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
>  	if (IS_ERR(ihid_goodix->vdd))
>  		return PTR_ERR(ihid_goodix->vdd);
>  
> +	ihid_goodix->vddio = devm_regulator_get(&client->dev, "mainboard-vddio");
> +	if (IS_ERR(ihid_goodix->vddio))
> +		return PTR_ERR(ihid_goodix->vddio);
> +
>  	ihid_goodix->timings = device_get_match_data(&client->dev);
>  
>  	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> -- 
> 2.39.1.519.gcb327c4b5f-goog
> 

-- 
Dmitry

Re: [PATCH 6/7] HID: i2c-hid: goodix: Add mainboard-vddio-supply

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:15PM -0800, Douglas Anderson wrote:
> As talked about in the patch ("dt-bindings: HID: i2c-hid: goodix: Add
> mainboard-vddio-supply") we may need to power up a 1.8V rail on the
> host associated with touchscreen IO. Let's add support in the driver
> for it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 7/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on evoker

[Douglas Anderson]

On older revisions of evoker, the touchscreen was either
non-functional or needed special hardware magic to get it talking
properly. It's been decided that the proper way going forward is to
use L3C to power some buffers on the QCard and then configure the
touchscreens for 1.8V. Let's do that.

Note that this is safe to do even on older revs even if it might not
make the touchscreen work there (because they didn't have a properly
stuffed QCard). As talked about in the patch ("arm64: dts: qcom:
sc7280: On QCard, regulator L3C should be 1.8V") the L3C regulator
didn't go anywhere at all on older revs.

This patch relies on the patch ("HID: i2c-hid: goodix: Add
mainboard-vddio-supply") in order to function properly. Without that
patch this one won't do any harm but it won't actually accomplish its
goal.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I haven't yet received real hardware to test this on, but it's a very
simple patch and, in the very least, highly unlikely to make anything
worse. No real users have these boards yet.

 arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dtsi
index 3d639c70a06e..0add7a2a099c 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dtsi
@@ -55,6 +55,7 @@ ap_ts: touchscreen@5d {
 		reset-gpios = <&tlmm 54 GPIO_ACTIVE_LOW>;
 
 		vdd-supply = <&ts_avdd>;
+		mainboard-vddio-supply = <&ts_avccio>;
 	};
 };
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH 7/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on evoker

[Matthias Kaehlcke]

On Mon, Feb 06, 2023 at 06:48:16PM -0800, Douglas Anderson wrote:
> On older revisions of evoker, the touchscreen was either
> non-functional or needed special hardware magic to get it talking
> properly. It's been decided that the proper way going forward is to
> use L3C to power some buffers on the QCard and then configure the
> touchscreens for 1.8V. Let's do that.
> 
> Note that this is safe to do even on older revs even if it might not
> make the touchscreen work there (because they didn't have a properly
> stuffed QCard). As talked about in the patch ("arm64: dts: qcom:
> sc7280: On QCard, regulator L3C should be 1.8V") the L3C regulator
> didn't go anywhere at all on older revs.
> 
> This patch relies on the patch ("HID: i2c-hid: goodix: Add
> mainboard-vddio-supply") in order to function properly. Without that
> patch this one won't do any harm but it won't actually accomplish its
> goal.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: (subset) [PATCH 0/7] arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards

[Bjorn Andersson]

On Mon, 6 Feb 2023 18:48:09 -0800, Douglas Anderson wrote:
> Trying to figure out how to talk to the touchscreen properly on
> sc7280-herobrine boards was a long and difficult process. Many
> Engineering hours were spent deciding how exactly one should talk over
> i2c to a peripheral. In the end, a solution has been found and this
> patch series attempts to implement it in a way that will work for all
> herobrine-based boards.
> 
> [...]

Applied, thanks!

[1/7] arm64: dts: qcom: sc7280: On QCard, regulator L3C should be 1.8V
      commit: 428df177013bad1a0a062878e3d5224122b7a5fe
[2/7] arm64: dts: qcom: sc7280: Add 3ms ramp to herobrine's pp3300_left_in_mlb
      commit: 4261cea17a2f5e0ec78eb3ceebb68dddb918aee9
[3/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on villager
      commit: d90b98f5702dccc41a5885b65361573654fcaabf
[7/7] arm64: dts: qcom: sc7280: Hook up the touchscreen IO rail on evoker
      commit: ef29188fe0b4de5c04b833378db92d3a3e0709e8

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH 0/7] arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards

[Benjamin Tissoires]

On Mon, 06 Feb 2023 18:48:09 -0800, Douglas Anderson wrote:
> Trying to figure out how to talk to the touchscreen properly on
> sc7280-herobrine boards was a long and difficult process. Many
> Engineering hours were spent deciding how exactly one should talk over
> i2c to a peripheral. In the end, a solution has been found and this
> patch series attempts to implement it in a way that will work for all
> herobrine-based boards.
> 
> [...]

Applied to hid/hid.git (for-6.3/i2c-hid), thanks!

[4/7] HID: i2c-hid: goodix: Stop tying the reset line to the regulator
      https://git.kernel.org/hid/hid/c/557e05fa9fdd
[5/7] dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply
      https://git.kernel.org/hid/hid/c/1d18c1f3b7d9
[6/7] HID: i2c-hid: goodix: Add mainboard-vddio-supply
      https://git.kernel.org/hid/hid/c/eb16f59e8e58

Cheers,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>