Hi, On Mon, Mar 25, 2024 at 3:55 AM Lukasz Majczak <lma@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > Signed-off-by: Radoslaw Biernacki <rad@xxxxxxxxxxxx> 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