Hi,
On 11/6/23 19:53, Doug Anderson wrote:
> 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?
I agree that moving the mutex to the callers would be better,
I've just completed this change for v2 of the series.
>> 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...
Right, for the devices where use_override gets set
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET is also always set, so
i2c-hid-core basically does not expect reset-complete to be signalled
via IRQ on these devices.
Since these devices are all kinda weird I have chosen to just preserve
the old behavior (1) there to avoid regressions
1) just doing a msleep(100) instead of waiting for the IRQ
> 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?
That is correct, only a few devices need to read the report like this
for the reset complete IRQ to happen and since I2C_HID_QUIRK_RESET_ON_RESUME
is only set on a few devices and we know it works there already the
assumption indeed is that on those devices the reading of the report
after reset is not necessary.
Regards,
Hans
