| Message ID | 20240423134611.31979-5-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand |
Hi, On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > ihid_elan->ops.power_up = elan_i2c_hid_power_up; > ihid_elan->ops.power_down = elan_i2c_hid_power_down; > > - /* Start out with reset asserted */ > - ihid_elan->reset_gpio = > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_ASIS); I'm not a huge fan of this part of the change. It feels like the GPIO state should be initialized by the probe function. Right before we call i2c_hid_core_probe() we should be in the state of "powered off" and the reset line should be in a consistent state. If "no_reset_on_power_off" then it should be de-asserted. Else it should be asserted. I think GPIOD_ASIS doesn't actually do anything useful for you, right? i2c_hid_core_probe() will power on and the first thing that'll happen there is that the reset line will be unconditionally asserted. Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow able to maintain continuity of this GPIO line from the BIOS state to the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS" property before because I've always wanted the ability to have GPIOs that could more seamlessly transition their firmware state to their kernel state. I don't think the API actually allows it. The fact that GPIO regulators don't support this seamless transition (even though it would be an obvious feature to add) supports my theory that the API doesn't currently allow it. It may be possible to make something work on some implementations but I think it's not guaranteed. Specifically, the docs say: * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set later with one of the dedicated functions. So that means that you can't read the pin without making it an input (which might change the state if it was previously driving a value) and you can't write the pin without making it an output and choosing a value to set it to. Basically grabbing a pin with "asis" doesn't allow you to do anything with it--it just claims it and doesn't let anyone else have it. -Doug
On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote: > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > > ihid_elan->ops.power_up = elan_i2c_hid_power_up; > > ihid_elan->ops.power_down = elan_i2c_hid_power_down; > > > > - /* Start out with reset asserted */ > > - ihid_elan->reset_gpio = > > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); > > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > > + GPIOD_ASIS); > > I'm not a huge fan of this part of the change. It feels like the GPIO > state should be initialized by the probe function. Right before we > call i2c_hid_core_probe() we should be in the state of "powered off" > and the reset line should be in a consistent state. If > "no_reset_on_power_off" then it should be de-asserted. Else it should > be asserted. First, the reset gpio will be set before probe() returns, just not immediately when it is requested. [ Sure, your panel follower implementation may defer the actual probe of the touchscreen even further but I think that's a design flaw in the current implementation. ] Second, the device is not necessarily in the "powered off" state as the driver leaves the power supplies in whatever state that the boot firmware left them in. Not immediately asserting reset and instead leaving it in the state that the boot firmware left it in is also no different from what happens when a probe function bails out before requesting the reset line. > I think GPIOD_ASIS doesn't actually do anything useful for you, right? > i2c_hid_core_probe() will power on and the first thing that'll happen > there is that the reset line will be unconditionally asserted. It avoids asserting reset before we need to and thus also avoid the need to deassert it on early probe failures (e.g. if one of the regulator lookups fails). We also don't need to worry about timing requirements, which can all be handled in one place (i.e. in the power up and power down callbacks). > Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow > able to maintain continuity of this GPIO line from the BIOS state to > the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS" > property before because I've always wanted the ability to have GPIOs > that could more seamlessly transition their firmware state to their > kernel state. I don't think the API actually allows it. The fact that > GPIO regulators don't support this seamless transition (even though it > would be an obvious feature to add) supports my theory that the API > doesn't currently allow it. It may be possible to make something work > on some implementations but I think it's not guaranteed. > > Specifically, the docs say: > > * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set > later with one of the dedicated functions. > > So that means that you can't read the pin without making it an input > (which might change the state if it was previously driving a value) > and you can't write the pin without making it an output and choosing a > value to set it to. Basically grabbing a pin with "asis" doesn't allow > you to do anything with it--it just claims it and doesn't let anyone > else have it. These properties may prevent it from being used by the regulator framework, but GPIOD_ASIS works well in the case of a reset gpio where we simply leave it in whatever state the firmware left it in if probe fails before we get to powering on the device. Johan
Hi, On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote: > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > > > ihid_elan->ops.power_up = elan_i2c_hid_power_up; > > > ihid_elan->ops.power_down = elan_i2c_hid_power_down; > > > > > > - /* Start out with reset asserted */ > > > - ihid_elan->reset_gpio = > > > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); > > > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > > > + GPIOD_ASIS); > > > > I'm not a huge fan of this part of the change. It feels like the GPIO > > state should be initialized by the probe function. Right before we > > call i2c_hid_core_probe() we should be in the state of "powered off" > > and the reset line should be in a consistent state. If > > "no_reset_on_power_off" then it should be de-asserted. Else it should > > be asserted. > > First, the reset gpio will be set before probe() returns, just not > immediately when it is requested. > > [ Sure, your panel follower implementation may defer the actual probe of > the touchscreen even further but I think that's a design flaw in the > current implementation. ] > > Second, the device is not necessarily in the "powered off" state Logically, the driver treats it as being in "powered off" state, though. That's why the i2c-hid core makes the call to power it on. IMO we should strive to make it more of a consistent state, not less of one. > as the > driver leaves the power supplies in whatever state that the boot > firmware left them in. I guess it depends on the regulator. ;-) For GPIO-regulators they aren't in whatever state the boot firmware left them in. For non-GPIO regulators we (usually) do preserve the state that the boot firmware left them in. > Not immediately asserting reset and instead leaving it in the state that > the boot firmware left it in is also no different from what happens when > a probe function bails out before requesting the reset line. > > > I think GPIOD_ASIS doesn't actually do anything useful for you, right? > > i2c_hid_core_probe() will power on and the first thing that'll happen > > there is that the reset line will be unconditionally asserted. > > It avoids asserting reset before we need to and thus also avoid the need > to deassert it on early probe failures (e.g. if one of the regulator > lookups fails). I guess so, though I'm of the opinion that we should be robust against the state that firmware left things in. The firmware's job is to boot the kernel and make sure that the system is running in a safe/reliable way, not to optimize the power consumption of the board. If the firmware left the line configured as "output low" then you'd let that stand. If it's important for the line to be left in a certain state, isn't it better to make that explicit? Also note: if we really end up keeping GPIOD_ASIS, which I'm still not convinced is the right move, the docs seem to imply that you need to explicitly set a direction before using it. Your current patch doesn't do that. -Doug
On Wed, Apr 24, 2024 at 09:24:33AM -0700, Doug Anderson wrote: > On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote: > > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > > > > ihid_elan->ops.power_up = elan_i2c_hid_power_up; > > > > ihid_elan->ops.power_down = elan_i2c_hid_power_down; > > > > > > > > - /* Start out with reset asserted */ > > > > - ihid_elan->reset_gpio = > > > > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); > > > > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > > > > + GPIOD_ASIS); > > > > > > I'm not a huge fan of this part of the change. It feels like the GPIO > > > state should be initialized by the probe function. Right before we > > > call i2c_hid_core_probe() we should be in the state of "powered off" > > > and the reset line should be in a consistent state. If > > > "no_reset_on_power_off" then it should be de-asserted. Else it should > > > be asserted. > > Second, the device is not necessarily in the "powered off" state > > Logically, the driver treats it as being in "powered off" state, > though. That's why the i2c-hid core makes the call to power it on. IMO > we should strive to make it more of a consistent state, not less of > one. That's not really true. The device is often in an undefined power state and we try to make sure that the hand over is as smooth as possible to avoid resetting displays and similar unnecessarily. The power-on sequence is what brings the device into a defined power state. > > as the > > driver leaves the power supplies in whatever state that the boot > > firmware left them in. > > I guess it depends on the regulator. ;-) For GPIO-regulators they > aren't in whatever state the boot firmware left them in. For non-GPIO > regulators we (usually) do preserve the state that the boot firmware > left them in. Even for GPIO regulators we have the "regulator-boot-on" devicetree property which is supposed to be set if the boot firmware has left a regulator on so that the regulator initialisation can preserve the state. > > Not immediately asserting reset and instead leaving it in the state that > > the boot firmware left it in is also no different from what happens when > > a probe function bails out before requesting the reset line. > > > > > I think GPIOD_ASIS doesn't actually do anything useful for you, right? > > > i2c_hid_core_probe() will power on and the first thing that'll happen > > > there is that the reset line will be unconditionally asserted. > > > > It avoids asserting reset before we need to and thus also avoid the need > > to deassert it on early probe failures (e.g. if one of the regulator > > lookups fails). > > I guess so, though I'm of the opinion that we should be robust against > the state that firmware left things in. The firmware's job is to boot > the kernel and make sure that the system is running in a safe/reliable > way, not to optimize the power consumption of the board. Agreed. > If the > firmware left the line configured as "output low" then you'd let that > stand. If it's important for the line to be left in a certain state, > isn't it better to make that explicit? As I pointed out above we already do this for any error paths before requesting the reset line. And I also don't think we need to worry too much about power consumption in case of errors. But there is one case I had not considered before, and that is your gpio regulator example but where the boot-on flag does not match the actual regulator state. If the supply is on and reset deasserted, but the regulator-boot-on flag is not set, then we want to make sure that reset is asserted before disabling the supply when requesting the regulator. > Also note: if we really end up keeping GPIOD_ASIS, which I'm still not > convinced is the right move, the docs seem to imply that you need to > explicitly set a direction before using it. Your current patch doesn't > do that. You're right. It will work in my case because of the gpiolib open-drain implementation, but not generally. I'll add back the reset during early probe and add error handling for deasserting reset on machines like the X13s. On these, the touchscreen may now be reset a couple of times in case of probe deferrals, but device links should generally prevent that. Johan
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c index 5b91fb106cfc..8a905027d5e9 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); @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) ihid_elan->ops.power_up = elan_i2c_hid_power_up; ihid_elan->ops.power_down = elan_i2c_hid_power_down; - /* Start out with reset asserted */ - ihid_elan->reset_gpio = - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", + GPIOD_ASIS); 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);
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> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-)