[PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Lukasz Majczak]

This patch extends the early bailout for probing procedure introduced in
commit: b3a81b6c4fc6730ac49e20d789a93c0faabafc98, 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.

Signed-off-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..69af0fad4f41 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1013,9 +1013,15 @@ 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;
+
+	do {
+		/* Make sure there is something at this address */
+		ret = i2c_smbus_read_byte(client);
+		if (tries > 0 && ret < 0)
+			usleep_range(400, 400);
+	} while (tries-- > 0 && ret < 0);
 
-	/* 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;
-- 
2.44.0.396.g6e790dbe36-goog

Re: [PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 3:55 AM Lukasz Majczak <lma@chromium.org> wrote:
>
> This patch extends the early bailout for probing procedure introduced in
> commit: b3a81b6c4fc6730ac49e20d789a93c0faabafc98, in order to cover devices

nit: checkpatch should have yelled at you saying that you should
specify a commit in the format:

commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
really probing")

Please fix and make sure that you're running checkpatch.


> 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.
>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>

nit: I believe your sign off should be last. It's also unclear why two
signoffs. Did Radoslaw author it and you changed it? ...or was it
Co-Developed-by, or ...? You'll probably need to adjust your tags a
bit depending on the answers.


> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Overall comment on this patch: I know that this is based on a patch
we've been carrying downstream in ChromeOS for years and some folks
considered it not upstreamable because it's too hacky. My $0.02 is
that, while it's ugly, this is needed to support real hardware that
was shipped. There's zero chance we can fix the hardware, a .00001%
chance that we could convince someone to update the firmware on the
i2c-hid device, and a .01% chance that we could convince people to try
to figure out a workaround by adding something to the main AP firmware
on this device. As I understand it, nobody has come up with a better
kernel workaround than this patch.

To me it doesn't seem terrible to have this retry here and it's not a
huge penalty for other i2c-hid users. ...so personally I'm in favor of
the idea of this landing. If I was a consumer and had one of the
affected Chromebooks I'd personally rather upstream have the support
for it.


> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..69af0fad4f41 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1013,9 +1013,15 @@ 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;
> +
> +       do {
> +               /* Make sure there is something at this address */
> +               ret = i2c_smbus_read_byte(client);
> +               if (tries > 0 && ret < 0)
> +                       usleep_range(400, 400);

Having both ends of the usleep be 400 is iffy. In this case it's at
probe time so I wonder if udelay() is better? If not, maybe give at
least _some_ margin?


> +       } while (tries-- > 0 && ret < 0);

I'm not a huge fan of having to check "tries" and "ret" twice.
Personally I'd rather see a "while (true)" loop and test the condition
once to break out. AKA:

while (true) {
  ret = i2c_...
  tries--;
  if (tries == 0 || ret >= 0)
    break;
  udelay(400);
}

...if you feel very strongly about the way you have coded it, though,
I won't stand in your way.

Pretty much all my comments are just nits and, since I'm not the
maintainer here, my opinion is just an opinion. I'd wait at least a
little while for the maintainers to comment before posting v2. I'm
happy to give a Reviewed-by tag when some of the nits are fixed.

-Doug

Re: [PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Łukasz Majczak]

> nit: checkpatch should have yelled at you saying that you should
> specify a commit in the format:
>
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing")
>
I will do it, but I did run the checkpatch (with --strict option) and
it didn't complain about anything.

>
> nit: I believe your sign off should be last. It's also unclear why two
> signoffs. Did Radoslaw author it and you changed it? ...or was it
> Co-Developed-by, or ...? You'll probably need to adjust your tags a
> bit depending on the answers.
>
Yes, we've discussed this patch together and the original
investigation was done by Rad.

> Having both ends of the usleep be 400 is iffy. In this case it's at
> probe time so I wonder if udelay() is better? If not, maybe give at
> least _some_ margin?
>
>
> > +       } while (tries-- > 0 && ret < 0);
>
According to Documentation/timers/timers-howto.rst:
" SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
                * Use usleep_range"
It was also pointed out by checkpath (when I initially used msleep).
I think giving some margin (eg. 400,500) would be ok.

> I'm not a huge fan of having to check "tries" and "ret" twice.
> Personally I'd rather see a "while (true)" loop and test the condition
> once to break out. AKA:
>
> while (true) {
>   ret = i2c_...
>   tries--;
>   if (tries == 0 || ret >= 0)
>     break;
>   udelay(400);
> }
>
> ...if you feel very strongly about the way you have coded it, though,
> I won't stand in your way.
I don't have emotional bond to this code ;), thanks.
>
> Pretty much all my comments are just nits and, since I'm not the
> maintainer here, my opinion is just an opinion. I'd wait at least a
> little while for the maintainers to comment before posting v2. I'm
> happy to give a Reviewed-by tag when some of the nits are fixed.
>
> -Doug
Thank you Doug for all your input.

Best regards,
Lukasz

Re: [PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Doug Anderson]

Hi,

On Tue, Mar 26, 2024 at 1:58 AM Łukasz Majczak <lma@chromium.org> wrote:
>
> > nit: checkpatch should have yelled at you saying that you should
> > specify a commit in the format:
> >
> > commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> > really probing")
> >
> I will do it, but I did run the checkpatch (with --strict option) and
> it didn't complain about anything.

Weird that checkpatch didn't yell, but perhaps somehow your commit
message didn't trigger its regex. ;-)



> > nit: I believe your sign off should be last. It's also unclear why two
> > signoffs. Did Radoslaw author it and you changed it? ...or was it
> > Co-Developed-by, or ...? You'll probably need to adjust your tags a
> > bit depending on the answers.
> >
> Yes, we've discussed this patch together and the original
> investigation was done by Rad.

Sounds good. Probably the best way to tag is these tags in this order:

Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Lukasz Majczak <lma@chromium.org>


> > Having both ends of the usleep be 400 is iffy. In this case it's at
> > probe time so I wonder if udelay() is better? If not, maybe give at
> > least _some_ margin?
> >
> >
> > > +       } while (tries-- > 0 && ret < 0);
> >
> According to Documentation/timers/timers-howto.rst:
> " SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>                 * Use usleep_range"
> It was also pointed out by checkpath (when I initially used msleep).
> I think giving some margin (eg. 400,500) would be ok.

Yeah, usleep_range(400, 500) would be fine. udelay(400) would also be
OK. The later would be more "accurate" but also more wasteful of CPU
cycles. Given that this is at probe time and only run a small handful
of times, it probably doesn't matter lots though perhaps the sleeping
function would allow more parallelism of other running probes.


-Doug