| Message ID | 20240325105452.529921-1-lma@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition | expand |
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
> 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
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
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;