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: [email protected] # 6.0
Cc: Douglas Anderson <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++++++++++++++-------
1 file changed, 28 insertions(+), 9 deletions(-)
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);
--
2.43.2
Hi,
On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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