| Message ID | 20230206184744.4.I085b32b6140c7d1ac4e7e97b712bff9dd5962b62@changeid (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 557e05fa9fdd0184890ca206cdec250cdd30375e |
| Delegated to: | Jiri Kosina |
| 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: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>
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); }
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(-)