Message ID | 20230207024816.525938-1-dianders@chromium.org |
---|---|
Headers | show |
Series | arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards | expand |
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 >
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 >
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>
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>
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>
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>
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>
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>
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,
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,