Message ID | 20240507144821.12275-5-johan+linaro@kernel.org |
---|---|
State | New |
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
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 = {