| Message ID | 20240405102436.3479210-1-lma@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition | expand |
Hi, On Fri, Apr 5, 2024 at 3:24 AM Lukasz Majczak <lma@chromium.org> wrote: > > This patch extends the early bailout for probing procedure introduced in > commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before > really probing"), in order to cover devices > based on STM microcontrollers. For touchpads based on STM uC, > the probe sequence needs to take into account the increased response time > for i2c transaction if the device already entered a deep power state. > STM specify the wakeup time as 400us between positive strobe of > the clock line. Deep sleep is controlled by Touchpad FW, > though some devices enter it pretty early to conserve power > in case of lack of activity on the i2c bus. > Failing to follow this requirement will result in touchpad device not being > detected due initial transaction being dropped by STM i2c slave controller. > By adding additional try, first transaction will wake up the touchpad > STM controller, and the second will produce proper detection response. > > v1->v2: > * Updated commit message with short sha of a base commit and proper tags > * Rearranged while loop to perform check only once > * Loosened sleeping range > > Co-developed-by: Radoslaw Biernacki <rad@chromium.org> > Signed-off-by: Radoslaw Biernacki <rad@chromium.org> > Signed-off-by: Lukasz Majczak <lma@chromium.org> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) As per my review comments in response to your v1 [1], this seems reasonable to me. Reviewed-by: Douglas Anderson <dianders@chromium.org> [1] https://lore.kernel.org/r/20240325105452.529921-1-lma@chromium.org
On Tue, Apr 09, 2024 at 12:53:43PM +0200, Łukasz Majczak wrote: > > Can you please explain why this would not a problem for all future > > transactions as well? > The problem is that the probe function calling i2c_smbus_read_byte() > is not aware that > uC on the other end is in a deep sleep state so the first read will > fail and so the whole probe. > > In a normal scenario, when a user touches the touchpad (when it is in > a deep sleep), the touch will first wake up the uC and > then generate an interrupt to AP, so in this case the touchpad is > fully awake and operational. Sure, but what about other transactions that are initiated by the host (e.g. SET_POWER)? Perhaps this hack at probe is enough for your use case, but is an incomplete hack and at a minimum you'd need to add a comment explaining why it is there. Johan
> Sure, but what about other transactions that are initiated by the host > (e.g. SET_POWER)? > Somehow it is problematic only on reboot and works just fine on suspend/resume and set_power. I will dig more and try to find out what the difference is. > Perhaps this hack at probe is enough for your use case, but is an > incomplete hack and at a minimum you'd need to add a comment explaining > why it is there. > You mean a comment in the code ? Lukasz
Hi Łukasz, On Thu, Apr 11, 2024 at 10:23 PM Łukasz Majczak <lma@chromium.org> wrote: > > > Sure, but what about other transactions that are initiated by the host > > (e.g. SET_POWER)? > > > Somehow it is problematic only on reboot and works just fine on > suspend/resume and > set_power. > I will dig more and try to find out what the difference is. If cold boot doesn't have such issue, maybe I2C_HID_PWR_SLEEP shouldn't be used by reboot? Kai-Heng > > > Perhaps this hack at probe is enough for your use case, but is an > > incomplete hack and at a minimum you'd need to add a comment explaining > > why it is there. > > > You mean a comment in the code ? > > Lukasz
On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote: > > Sure, but what about other transactions that are initiated by the host > > (e.g. SET_POWER)? > > > Somehow it is problematic only on reboot and works just fine on > suspend/resume and > set_power. > I will dig more and try to find out what the difference is. Sounds like it may be related to the i2c_hid_set_power() on shutdown() then as Kai-Heng pointed out. That function already handles a similar retry for I2C_HID_PWR_ON during resume. > > Perhaps this hack at probe is enough for your use case, but is an > > incomplete hack and at a minimum you'd need to add a comment explaining > > why it is there. > > > You mean a comment in the code ? Yes, if this turns out to be needed then there should be a comment explaining why it is there (and currently also as the delays you used seem specific for your particular platform). But hopefully you can find a generic solution to this. Johan
Hi Johan, On Mon, Apr 15, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote: > > On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote: > > > Sure, but what about other transactions that are initiated by the host > > > (e.g. SET_POWER)? > > > > > Somehow it is problematic only on reboot and works just fine on > > suspend/resume and > > set_power. > > I will dig more and try to find out what the difference is. > > Sounds like it may be related to the i2c_hid_set_power() on shutdown() > then as Kai-Heng pointed out. > > That function already handles a similar retry for I2C_HID_PWR_ON during > resume. > > > > Perhaps this hack at probe is enough for your use case, but is an > > > incomplete hack and at a minimum you'd need to add a comment explaining > > > why it is there. > > > > > You mean a comment in the code ? > > Yes, if this turns out to be needed then there should be a comment > explaining why it is there (and currently also as the delays you used > seem specific for your particular platform). > > But hopefully you can find a generic solution to this. Yes, we might need a more generic solution though it is not yet clear for us which would be the cleanest one. As I wrote in the reply to Kenny, the design back in the day was made to use events rather than level driven IO line, to drive the power state of the device. Consequence is we need to request a low power state before the kernel goes down as there is no guarantee the kernel will wake up soon (prevent battery power leak). This event/level logic problem (event design for level type problem). Let us get back to you with more info as we look deeper into some newly found power sequence limitations of the second I2C node on this device. > > Johan
On Mon, Apr 15, 2024 at 02:26:42PM +0200, Radoslaw Biernacki wrote: > On Mon, Apr 15, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote: > > > > On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote: > > > > Sure, but what about other transactions that are initiated by the host > > > > (e.g. SET_POWER)? > > > > > > > Somehow it is problematic only on reboot and works just fine on > > > suspend/resume and > > > set_power. > > > I will dig more and try to find out what the difference is. > > > > Sounds like it may be related to the i2c_hid_set_power() on shutdown() > > then as Kai-Heng pointed out. > > Yes, if this turns out to be needed then there should be a comment > > explaining why it is there (and currently also as the delays you used > > seem specific for your particular platform). > > > > But hopefully you can find a generic solution to this. > > Yes, we might need a more generic solution though it is not yet clear > for us which would be the cleanest one. > As I wrote in the reply to Kenny, the design back in the day was made > to use events rather than > level driven IO line, to drive the power state of the device. > Consequence is we need to request > a low power state before the kernel goes down as there is no guarantee > the kernel will wake up soon > (prevent battery power leak). This event/level logic problem (event > design for level type problem). Ok, and do I understand you correctly that it is indeed the SET_POWER command on shutdown() that makes the device enter the sleep state and which then needs an extra transaction on next boot to be woken up? And that the firmware will not enter that low power state on its own based on lack of activity as the current commit message suggests? If so, then it seems a variant of this patch would be ok as long as the commit message and a comment in the code explains the problem correctly. Johan
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2df1ab3c31cc..ece1a5815e0b 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid) struct i2c_client *client = ihid->client; struct hid_device *hid = ihid->hid; int ret; + int tries = 2; + + while (true) { + /* Make sure there is something at this address */ + ret = i2c_smbus_read_byte(client); + tries--; + if (tries == 0 || ret >= 0) + break; + usleep_range(400, 500); + } - /* Make sure there is something at this address */ - ret = i2c_smbus_read_byte(client); if (ret < 0) { i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret); return -ENXIO;
This patch extends the early bailout for probing procedure introduced in commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before really probing"), in order to cover devices based on STM microcontrollers. For touchpads based on STM uC, the probe sequence needs to take into account the increased response time for i2c transaction if the device already entered a deep power state. STM specify the wakeup time as 400us between positive strobe of the clock line. Deep sleep is controlled by Touchpad FW, though some devices enter it pretty early to conserve power in case of lack of activity on the i2c bus. Failing to follow this requirement will result in touchpad device not being detected due initial transaction being dropped by STM i2c slave controller. By adding additional try, first transaction will wake up the touchpad STM controller, and the second will produce proper detection response. v1->v2: * Updated commit message with short sha of a base commit and proper tags * Rearranged while loop to perform check only once * Loosened sleeping range Co-developed-by: Radoslaw Biernacki <rad@chromium.org> Signed-off-by: Radoslaw Biernacki <rad@chromium.org> Signed-off-by: Lukasz Majczak <lma@chromium.org> --- drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)