| Message ID | 20240507144821.12275-5-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 0eafc58f2194dbd01d4be40f99a697681171995b |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand |
Hi, On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > @@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) > container_of(ops, struct i2c_hid_of_elan, ops); > int ret; > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); Could probably use a comment above it saying that this is important when we have "no_reset_on_power_off" and doesn't do anything bad when we don't so we can just do it unconditionally. > + > if (ihid_elan->vcc33) { > ret = regulator_enable(ihid_elan->vcc33); > if (ret) > - return ret; > + goto err_deassert_reset; > } > > ret = regulator_enable(ihid_elan->vccio); > - if (ret) { > - regulator_disable(ihid_elan->vcc33); > - return ret; > - } > + if (ret) > + goto err_disable_vcc33; > > if (ihid_elan->chip_data->post_power_delay_ms) > msleep(ihid_elan->chip_data->post_power_delay_ms); > @@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) > msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms); > > return 0; > + > +err_disable_vcc33: > + if (ihid_elan->vcc33) > + regulator_disable(ihid_elan->vcc33); > +err_deassert_reset: > + if (ihid_elan->no_reset_on_power_off) > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0); Small nit about the error label: it sounds as if when you go here you _will_ deassert reset when in actuality you might or might not (depending on no_reset_on_power_off). Personally I prefer to label error jumps based on things I've done instead of things that the error jump needs to do, so you could call them "err_enabled_vcc33" and "err_asserted_reset"... I don't feel that strongly about it, though, so if you love the label you have then no need to change it. > @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > struct i2c_hid_of_elan *ihid_elan = > container_of(ops, struct i2c_hid_of_elan, ops); > > - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > + /* > + * Do not assert reset when the hardware allows for it to remain > + * deasserted regardless of the state of the (shared) power supply to > + * avoid wasting power when the supply is left on. > + */ > + if (!ihid_elan->no_reset_on_power_off) > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > + > if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms) > msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms); Shouldn't the above two lines be inside the "if (!ihid_elan->no_reset_on_power_off)" test? If you're not setting the reset GPIO then you don't need to do the delay, right? > @@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > static int i2c_hid_of_elan_probe(struct i2c_client *client) > { > struct i2c_hid_of_elan *ihid_elan; > + int ret; > > ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL); > if (!ihid_elan) > @@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > if (IS_ERR(ihid_elan->reset_gpio)) > return PTR_ERR(ihid_elan->reset_gpio); > > + ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node, > + "no-reset-on-power-off"); Personally, I'd rather you query for the property before you request the GPIO and then request the GPIO in the "powered off" state just to keep everything in the most consistent state possible. -Doug
Hi Doug, and sorry about the late reply. Was travelling last week. On Fri, May 10, 2024 at 04:36:08PM -0700, Doug Anderson wrote: > On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > @@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) > > container_of(ops, struct i2c_hid_of_elan, ops); > > int ret; > > > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > Could probably use a comment above it saying that this is important > when we have "no_reset_on_power_off" and doesn't do anything bad when > we don't so we can just do it unconditionally. Possibly, but I'd prefer not to add comments for things like this, which should be apparent from just looking at the code. And explicitly asserting reset before deasserting it is not unusual in any way. > > + > > if (ihid_elan->vcc33) { > > ret = regulator_enable(ihid_elan->vcc33); > > if (ret) > > - return ret; > > + goto err_deassert_reset; > > } > > > > ret = regulator_enable(ihid_elan->vccio); > > - if (ret) { > > - regulator_disable(ihid_elan->vcc33); > > - return ret; > > - } > > + if (ret) > > + goto err_disable_vcc33; > > > > if (ihid_elan->chip_data->post_power_delay_ms) > > msleep(ihid_elan->chip_data->post_power_delay_ms); > > @@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) > > msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms); > > > > return 0; > > + > > +err_disable_vcc33: > > + if (ihid_elan->vcc33) > > + regulator_disable(ihid_elan->vcc33); > > +err_deassert_reset: > > + if (ihid_elan->no_reset_on_power_off) > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0); > > Small nit about the error label: it sounds as if when you go here you > _will_ deassert reset when in actuality you might or might not > (depending on no_reset_on_power_off). Yes, this is similar to how err_disable_vcc33 may or may not disable the optional regulator. > Personally I prefer to label > error jumps based on things I've done instead of things that the error > jump needs to do, so you could call them "err_enabled_vcc33" and > "err_asserted_reset"... Naming labels after what they do is less error prone and also explicitly recommended by the coding style. > I don't feel that strongly about it, though, so if you love the label > you have then no need to change it. So I'd prefer keeping things this way. > > @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > > struct i2c_hid_of_elan *ihid_elan = > > container_of(ops, struct i2c_hid_of_elan, ops); > > > > - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > + /* > > + * Do not assert reset when the hardware allows for it to remain > > + * deasserted regardless of the state of the (shared) power supply to > > + * avoid wasting power when the supply is left on. > > + */ > > + if (!ihid_elan->no_reset_on_power_off) > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > + > > if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms) > > msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms); > > Shouldn't the above two lines be inside the "if > (!ihid_elan->no_reset_on_power_off)" test? If you're not setting the > reset GPIO then you don't need to do the delay, right? Yes, I guess you're right. The off-delay is weird and not normally used, but apparently it is needed by some panel-follower use case. AFAICT it's not even related to the reset line, just a hack to add a delay before the panel is reset by some other driver (see f2f43bf15d7a ("HID: i2c-hid: elan: Add ili9882t timing")). I think that's why I just looked the other way and left this little oddity here unchanged. > > @@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > > static int i2c_hid_of_elan_probe(struct i2c_client *client) > > { > > struct i2c_hid_of_elan *ihid_elan; > > + int ret; > > > > ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL); > > if (!ihid_elan) > > @@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > > if (IS_ERR(ihid_elan->reset_gpio)) > > return PTR_ERR(ihid_elan->reset_gpio); > > > > + ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node, > > + "no-reset-on-power-off"); > > Personally, I'd rather you query for the property before you request > the GPIO and then request the GPIO in the "powered off" state just to > keep everything in the most consistent state possible. No, I don't know what state the reset line is in before the driver probes. So either I leave it unchanged as I did in v1 or I assert it here unconditionally as I do in v2 (e.g. to avoid deasserting reset before the supply is stable). Johan
On Mon, May 20, 2024 at 01:44:06PM +0200, Johan Hovold wrote: > On Fri, May 10, 2024 at 04:36:08PM -0700, Doug Anderson wrote: > > On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > > > struct i2c_hid_of_elan *ihid_elan = > > > container_of(ops, struct i2c_hid_of_elan, ops); > > > > > > - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > > + /* > > > + * Do not assert reset when the hardware allows for it to remain > > > + * deasserted regardless of the state of the (shared) power supply to > > > + * avoid wasting power when the supply is left on. > > > + */ > > > + if (!ihid_elan->no_reset_on_power_off) > > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > > + > > > if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms) > > > msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms); > > > > Shouldn't the above two lines be inside the "if > > (!ihid_elan->no_reset_on_power_off)" test? If you're not setting the > > reset GPIO then you don't need to do the delay, right? > > Yes, I guess you're right. The off-delay is weird and not normally used, > but apparently it is needed by some panel-follower use case. AFAICT it's > not even related to the reset line, just a hack to add a delay before > the panel is reset by some other driver (see f2f43bf15d7a ("HID: > i2c-hid: elan: Add ili9882t timing")). > > I think that's why I just looked the other way and left this little > oddity here unchanged. Hit send too soon. Since this hack does not appear to be related to the reset line, I think it's correct to not have it depend on whether the reset line is asserted or not (e.g. as there could be 'panel-followers' with 'no_reset_on_power_off'): The datasheet specifies there should be 60ms between touch SDA sleep and panel RESX. Doug's series[1] allows panels and touchscreens to power on/off together, so we can add the 65 ms delay in i2c_hid_core_suspend before panel_unprepare. The power-off delay variable should probably be renamed, but that's a separate change. So I think v2 of this series is good to go. Johan
Hi, On Mon, May 20, 2024 at 4:52 AM Johan Hovold <johan@kernel.org> wrote: > > On Mon, May 20, 2024 at 01:44:06PM +0200, Johan Hovold wrote: > > On Fri, May 10, 2024 at 04:36:08PM -0700, Doug Anderson wrote: > > > On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > > @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) > > > > struct i2c_hid_of_elan *ihid_elan = > > > > container_of(ops, struct i2c_hid_of_elan, ops); > > > > > > > > - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > > > + /* > > > > + * Do not assert reset when the hardware allows for it to remain > > > > + * deasserted regardless of the state of the (shared) power supply to > > > > + * avoid wasting power when the supply is left on. > > > > + */ > > > > + if (!ihid_elan->no_reset_on_power_off) > > > > + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); > > > > + > > > > if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms) > > > > msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms); > > > > > > Shouldn't the above two lines be inside the "if > > > (!ihid_elan->no_reset_on_power_off)" test? If you're not setting the > > > reset GPIO then you don't need to do the delay, right? > > > > Yes, I guess you're right. The off-delay is weird and not normally used, > > but apparently it is needed by some panel-follower use case. AFAICT it's > > not even related to the reset line, just a hack to add a delay before > > the panel is reset by some other driver (see f2f43bf15d7a ("HID: > > i2c-hid: elan: Add ili9882t timing")). > > > > I think that's why I just looked the other way and left this little > > oddity here unchanged. > > Hit send too soon. > > Since this hack does not appear to be related to the reset line, I think > it's correct to not have it depend on whether the reset line is asserted > or not (e.g. as there could be 'panel-followers' with > 'no_reset_on_power_off'): > > The datasheet specifies there should be 60ms between touch SDA > sleep and panel RESX. Doug's series[1] allows panels and > touchscreens to power on/off together, so we can add the 65 ms > delay in i2c_hid_core_suspend before panel_unprepare. > > The power-off delay variable should probably be renamed, but that's a > separate change. > > So I think v2 of this series is good to go. Sure. As I think we've seen in the past, my choice of bikeshed paint color seems to be quite different than yours, but nothing here seems like it needs to block landing, so: Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c index 5b91fb106cfc..091e37933225 100644 --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c @@ -31,6 +31,7 @@ struct i2c_hid_of_elan { struct regulator *vcc33; struct regulator *vccio; struct gpio_desc *reset_gpio; + bool no_reset_on_power_off; const struct elan_i2c_hid_chip_data *chip_data; }; @@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) container_of(ops, struct i2c_hid_of_elan, ops); int ret; + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); + if (ihid_elan->vcc33) { ret = regulator_enable(ihid_elan->vcc33); if (ret) - return ret; + goto err_deassert_reset; } ret = regulator_enable(ihid_elan->vccio); - if (ret) { - regulator_disable(ihid_elan->vcc33); - return ret; - } + if (ret) + goto err_disable_vcc33; if (ihid_elan->chip_data->post_power_delay_ms) msleep(ihid_elan->chip_data->post_power_delay_ms); @@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops) msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms); return 0; + +err_disable_vcc33: + if (ihid_elan->vcc33) + regulator_disable(ihid_elan->vcc33); +err_deassert_reset: + if (ihid_elan->no_reset_on_power_off) + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0); + + return ret; } static void elan_i2c_hid_power_down(struct i2chid_ops *ops) @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) struct i2c_hid_of_elan *ihid_elan = container_of(ops, struct i2c_hid_of_elan, ops); - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); + /* + * Do not assert reset when the hardware allows for it to remain + * deasserted regardless of the state of the (shared) power supply to + * avoid wasting power when the supply is left on. + */ + if (!ihid_elan->no_reset_on_power_off) + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1); + if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms) msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms); @@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops) static int i2c_hid_of_elan_probe(struct i2c_client *client) { struct i2c_hid_of_elan *ihid_elan; + int ret; ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL); if (!ihid_elan) @@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) if (IS_ERR(ihid_elan->reset_gpio)) return PTR_ERR(ihid_elan->reset_gpio); + ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node, + "no-reset-on-power-off"); + ihid_elan->vccio = devm_regulator_get(&client->dev, "vccio"); - if (IS_ERR(ihid_elan->vccio)) - return PTR_ERR(ihid_elan->vccio); + if (IS_ERR(ihid_elan->vccio)) { + ret = PTR_ERR(ihid_elan->vccio); + goto err_deassert_reset; + } ihid_elan->chip_data = device_get_match_data(&client->dev); if (ihid_elan->chip_data->main_supply_name) { ihid_elan->vcc33 = devm_regulator_get(&client->dev, ihid_elan->chip_data->main_supply_name); - if (IS_ERR(ihid_elan->vcc33)) - return PTR_ERR(ihid_elan->vcc33); + if (IS_ERR(ihid_elan->vcc33)) { + ret = PTR_ERR(ihid_elan->vcc33); + goto err_deassert_reset; + } } - return i2c_hid_core_probe(client, &ihid_elan->ops, - ihid_elan->chip_data->hid_descriptor_address, 0); + ret = i2c_hid_core_probe(client, &ihid_elan->ops, + ihid_elan->chip_data->hid_descriptor_address, 0); + if (ret) + goto err_deassert_reset; + + return 0; + +err_deassert_reset: + if (ihid_elan->no_reset_on_power_off) + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0); + + return ret; } static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
The Elan eKTH5015M touch controller found on the Lenovo ThinkPad X13s shares the VCC33 supply with other peripherals that may remain powered during suspend (e.g. when enabled as wakeup sources). The reset line is also wired so that it can be left deasserted when the supply is off. This is important as it avoids holding the controller in reset for extended periods of time when it remains powered, which can lead to increased power consumption, and also avoids leaking current through the X13s reset circuitry during suspend (and after driver unbind). Use the new 'no-reset-on-power-off' devicetree property to determine when reset needs to be asserted on power down. Notably this also avoids wasting power on machine variants without a touchscreen for which the driver would otherwise exit probe with reset asserted. Fixes: bd3cba00dcc6 ("HID: i2c-hid: elan: Add support for Elan eKTH6915 i2c-hid touchscreens") Cc: stable@vger.kernel.org # 6.0 Cc: Douglas Anderson <dianders@chromium.org> Tested-by: Steev Klimaszewski <steev@kali.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/hid/i2c-hid/i2c-hid-of-elan.c | 59 +++++++++++++++++++++------ 1 file changed, 47 insertions(+), 12 deletions(-)