| Message ID | 20240423134611.31979-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand |
On 23/04/2024 15:46, Johan Hovold wrote: > The Ilitek ILI2901 touch screen controller was apparently incorrectly > added to the Elan eKTH6915 schema simply because it also has a reset > gpio and is currently managed by the Elan driver in Linux. > > The two controllers are not related even if an unfortunate wording in > the commit message adding the Ilitek compatible made it sound like they > were. > > Add a dedicated schema for the ILI2901 which does not specify the I2C > address (which is likely 0x41 rather than 0x10 as for other Ilitek touch > controllers) to avoid cluttering the Elan schema with unrelated devices > and to make it easier to find the correct schema when adding further > Ilitek controllers. > > Fixes: d74ac6f60a7e ("dt-bindings: HID: i2c-hid: elan: Introduce Ilitek ili2901") > Cc: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > .../bindings/input/elan,ekth6915.yaml | 5 +- > .../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++ > 2 files changed, 68 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml > > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > index dc4ac41f2441..3e2d216c6432 100644 > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml > @@ -18,9 +18,8 @@ allOf: > > properties: > compatible: > - enum: > - - elan,ekth6915 > - - ilitek,ili2901 > + items: Drop items, that's just const. Or keep it as enum, which makes patch diff smaller here. > + - const: elan,ekth6915 With items dropped: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi, On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > before sending commands after having deasserted reset during power on. > > This series switches the X13s devicetree to use the Elan specific > binding so that the OS can determine the required power-on sequence and > make sure that the controller is always detected during boot. [1] > > The Elan hid-i2c driver currently asserts reset unconditionally during > suspend, which does not work on the X13s where the touch controller > supply is shared with other peripherals that may remain powered. Holding > the controller in reset can increase power consumption and also leaks > current through the reset circuitry pull ups. Can you provide more details about which devices exactly it shares power with? I'm worried that you may be shooting yourself in the foot to avoid shooting yourself in the arm. Specifically, if those other peripherals that may remain powered ever power themselves off then you'll end up back-driving the touchscreen through the reset line, won't you? Since reset is active low then not asserting reset drives the reset line high and, if you power it off, it can leach power backwards through the reset line. The "goodix,no-reset-during-suspend" property that I added earlier specifically worked on systems where the rail was always-on so I could guarantee that didn't happen.
On Tue, Apr 23, 2024 at 02:34:20PM -0500, Steev Klimaszewski wrote: > On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > Note that my X13s does not have a touchscreen, but I have done partial > > verification of the implementation using that machine and the sc8280xp > > CRD reference design. Bjorn has promised to help out with final > > verification on an X13s with a touchscreen. > I thought that I'd purchased a Thinkpad X13s without touchscreen, but > it turns out that I do have one, and since I do, I was able to test > this patchset, and it works on mine. > > Tested-by: Steev Klimaszewski <steev@kali.org> Thanks for testing, Steev. Johan
On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote: > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > > before sending commands after having deasserted reset during power on. > > > > This series switches the X13s devicetree to use the Elan specific > > binding so that the OS can determine the required power-on sequence and > > make sure that the controller is always detected during boot. [1] > > > > The Elan hid-i2c driver currently asserts reset unconditionally during > > suspend, which does not work on the X13s where the touch controller > > supply is shared with other peripherals that may remain powered. Holding > > the controller in reset can increase power consumption and also leaks > > current through the reset circuitry pull ups. > > Can you provide more details about which devices exactly it shares > power with? I'm worried that you may be shooting yourself in the foot > to avoid shooting yourself in the arm. > > Specifically, if those other peripherals that may remain powered ever > power themselves off then you'll end up back-driving the touchscreen > through the reset line, won't you? Since reset is active low then not > asserting reset drives the reset line high and, if you power it off, > it can leach power backwards through the reset line. The > "goodix,no-reset-during-suspend" property that I added earlier > specifically worked on systems where the rail was always-on so I could > guarantee that didn't happen. > > From looking at your dts patch it looks like your power _is_ on an > always-on rail so you should be OK, but it should be documented that > this only works for always-on rails. > > ..also, from your patch description it sounds as if (maybe?) you > intend to eventually let the rail power off if the trackpad isn't a > wakeup source. If you eventually plan to do that then you definitely > need something more complex here... No, that's the whole point: the hardware is designed so that the reset line can be left deasserted by the CPU also when the supply is off. The supply in this case is shared with the keyboard and touchpad, but also some other devices which are not yet fully described. As you rightly noted, the intention is to allow the supply to eventually be disabled when none of these devices are enabled as wakeup sources. I did not want to get in to too much details on exactly how this particular reset circuit is designed, but basically you have a pull up to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a pull up to the supply voltage on the peripheral side. With this design, the reset line can be left deasserted by the CPU (tri-stated or driven high), but the important part is that the reset signal that goes into the controller will be pulled to 3.3 V only when the supply is left on and otherwise it will be connected to ground. > > Note that the latter also affects X13s variants where the touchscreen is > > not populated as the driver also exits probe() with reset asserted. > > I assume driving against an external pull is _probably_ not a huge > deal (should be a pretty small amount of power), but I agree it would > be nice to fix. > > I'm a bit leery of actively driving the reset pin high (deasserting > the reset) just to match the pull. It feels like in your case it would > be better to make it an input w/ no pulls. It almost feels like > something in the pinctrl system should handle this. Something where > the pin is default "input no pull" at the board level and when the > driver exits it should go back to the pinctrl default... If you look at the DT patch that's essentially what I'm doing by describing the reset pin as open-drain so that it will be configured as an input (tristated) when reset is deasserted and only driven low when reset is asserted. > I guess one last thought is: what do we do if/when someone needs the > same solution but they want multiple sources of touchscreens, assuming > we ever get the second-sourcing problem solved well. In that case the > different touchscreen drivers might have a different idea of how the > GPIO should be left when the driver exits... The second-source problem is arguable a separate one, and as we've discussed in the past, the current approach of describing both devices in the devicetree only works when the devices are truly compatible in terms of external resources (supplies, gpios, pinconfig). For anything more complex, we need a more elaborate implementation. In this case it should not be a problem, though, as the reset circuit should have the same properties regardless of which controller you connect (e.g. both nodes would have the 'no-reset-on-power-off' property). Johan
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
Hi, On Wed, Apr 24, 2024 at 1:50 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote: > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > > > before sending commands after having deasserted reset during power on. > > > > > > This series switches the X13s devicetree to use the Elan specific > > > binding so that the OS can determine the required power-on sequence and > > > make sure that the controller is always detected during boot. [1] > > > > > > The Elan hid-i2c driver currently asserts reset unconditionally during > > > suspend, which does not work on the X13s where the touch controller > > > supply is shared with other peripherals that may remain powered. Holding > > > the controller in reset can increase power consumption and also leaks > > > current through the reset circuitry pull ups. > > > > Can you provide more details about which devices exactly it shares > > power with? I'm worried that you may be shooting yourself in the foot > > to avoid shooting yourself in the arm. > > > > Specifically, if those other peripherals that may remain powered ever > > power themselves off then you'll end up back-driving the touchscreen > > through the reset line, won't you? Since reset is active low then not > > asserting reset drives the reset line high and, if you power it off, > > it can leach power backwards through the reset line. The > > "goodix,no-reset-during-suspend" property that I added earlier > > specifically worked on systems where the rail was always-on so I could > > guarantee that didn't happen. > > > > From looking at your dts patch it looks like your power _is_ on an > > always-on rail so you should be OK, but it should be documented that > > this only works for always-on rails. > > > > ..also, from your patch description it sounds as if (maybe?) you > > intend to eventually let the rail power off if the trackpad isn't a > > wakeup source. If you eventually plan to do that then you definitely > > need something more complex here... > > No, that's the whole point: the hardware is designed so that the reset > line can be left deasserted by the CPU also when the supply is off. > > The supply in this case is shared with the keyboard and touchpad, but > also some other devices which are not yet fully described. As you > rightly noted, the intention is to allow the supply to eventually be > disabled when none of these devices are enabled as wakeup sources. > > I did not want to get in to too much details on exactly how this > particular reset circuit is designed, but basically you have a pull up > to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a > pull up to the supply voltage on the peripheral side. > > With this design, the reset line can be left deasserted by the CPU > (tri-stated or driven high), but the important part is that the reset > signal that goes into the controller will be pulled to 3.3 V only when > the supply is left on and otherwise it will be connected to ground. Ah, got it. The level shifter isolating things makes sense. > > I guess one last thought is: what do we do if/when someone needs the > > same solution but they want multiple sources of touchscreens, assuming > > we ever get the second-sourcing problem solved well. In that case the > > different touchscreen drivers might have a different idea of how the > > GPIO should be left when the driver exits... > > The second-source problem is arguable a separate one, and as we've > discussed in the past, the current approach of describing both devices > in the devicetree only works when the devices are truly compatible in > terms of external resources (supplies, gpios, pinconfig). For anything > more complex, we need a more elaborate implementation. > > In this case it should not be a problem, though, as the reset circuit > should have the same properties regardless of which controller you > connect (e.g. both nodes would have the 'no-reset-on-power-off' > property). The reset circuitry may be the same, but the properties of the touchscreen might not be. It would be easy to imagine a different touchscreen that consumes less power when held in reset. In any case, not a problem we need to solve right now. -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
The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay before sending commands after having deasserted reset during power on. This series switches the X13s devicetree to use the Elan specific binding so that the OS can determine the required power-on sequence and make sure that the controller is always detected during boot. [1] The Elan hid-i2c driver currently asserts reset unconditionally during suspend, which does not work on the X13s where the touch controller supply is shared with other peripherals that may remain powered. Holding the controller in reset can increase power consumption and also leaks current through the reset circuitry pull ups. Note that the latter also affects X13s variants where the touchscreen is not populated as the driver also exits probe() with reset asserted. Fix this by adding a new 'no-reset-on-power-off' devicetree property which can be used by the OS to determine when reset needs to be asserted on power down and when it safe and desirable to leave it deasserted. I tried to look for drivers that had already addressed this but it was only after I finished implementing this that I noticed Doug's reference to commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator"), which tried to solve a related problem. That commit has since been reverted but ultimately resulted in commit 7607f12ba735 ("HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property") being merged to handle the related case where the touch controller supply is always on. The implementation is very similar, but I decided to use the slightly more generic 'no-reset-on-power-off' property name after considering a number of alternatives (including trying to describe the hardware configuration in the name). (And as this is not vendor specific, I left out the prefix.) Note that my X13s does not have a touchscreen, but I have done partial verification of the implementation using that machine and the sc8280xp CRD reference design. Bjorn has promised to help out with final verification on an X13s with a touchscreen. The devicetree changes are expected to go in through the Qualcomm tree once the binding and driver updates have been merged. Johan [1] The reset signal is currently deasserted using the pin configuration and the controller would be detected if probe is deferred or if user space triggers a reprobe through sysfs. Johan Hovold (6): dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property HID: i2c-hid: elan: fix reset suspend current leakage arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset .../bindings/input/elan,ekth6915.yaml | 20 ++++-- .../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++ arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 15 +++-- drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++--- 5 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml