Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> @@ -746,8 +752,6 @@ static int i2c_hid_parse(struct hid_device *hid)
>
> do {
> ret = i2c_hid_start_hwreset(ihid);
> - if (ret == 0)
> - ret = i2c_hid_finish_hwreset(ihid);
The mutex contract (talked about in a previous patch) is a little more
confusing now. ;-) Feels like it needs a comment somewhere in here so
the reader of the code understands that the reset_mutex might or might
not be locked here.
...or would it make more sense for the caller to just handle the mutex
to make it more obvious. The "I2C_HID_QUIRK_RESET_ON_RESUME" code
would need to grab the mutex too, but that really doesn't seem
terrible. In fact, I suspect it cleans up your error handling and
makes everything cleaner?
> if (ret)
> msleep(1000);
> } while (tries-- > 0 && ret);
> @@ -763,9 +767,8 @@ static int i2c_hid_parse(struct hid_device *hid)
> i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
> } else {
> rdesc = kzalloc(rsize, GFP_KERNEL);
> -
> if (!rdesc) {
> - dbg_hid("couldn't allocate rdesc memory\n");
> + i2c_hid_abort_hwreset(ihid);
> return -ENOMEM;
> }
>
> @@ -776,10 +779,21 @@ static int i2c_hid_parse(struct hid_device *hid)
> rdesc, rsize);
> if (ret) {
> hid_err(hid, "reading report descriptor failed\n");
> + i2c_hid_abort_hwreset(ihid);
> goto out;
> }
> }
>
> + /*
> + * Windows directly reads the report-descriptor after sending reset
> + * and then waits for resets completion afterwards. Some touchpads
> + * actually wait for the report-descriptor to be read before signalling
> + * reset completion.
> + */
> + ret = i2c_hid_finish_hwreset(ihid);
> + if (ret)
> + goto out;
Given your new understanding, I wonder if you should start reading the
report descriptor even if "use_override" is set? You'd throw away what
you read but maybe it would be important to make the touchscreen
properly de-assert reset? I guess this is the subject of the next
patch...
Also: I guess there's the assumption that the touchscreens needing the
other caller of the reset functions (I2C_HID_QUIRK_RESET_ON_RESUME)
never need to read the report like this?
-Doug
