Hi Douglas,
Thank you for the reviews.
On 11/6/23 19:50, Doug Anderson wrote:
> Hi,
>
> On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> @@ -482,21 +442,49 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>>
>> ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
>> if (ret)
>> - goto out_unlock;
>> + goto err_unlock;
>>
>> - ret = i2c_hid_execute_reset(ihid);
>> + /* Prepare reset command. Command register goes first. */
>> + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
>> + length += sizeof(__le16);
>> + /* Next is RESET command itself */
>> + length += i2c_hid_encode_command(ihid->cmdbuf + length,
>> + I2C_HID_OPCODE_RESET, 0, 0);
>> +
>> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>> +
>> + ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
>> if (ret) {
>> dev_err(&ihid->client->dev,
>> "failed to reset device: %d\n", ret);
>> - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
>> - goto out_unlock;
>> + goto err_clear_reset;
>> }
>>
>> + if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
>> + msleep(100);
>> + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>> + }
>> +
>> + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
>> + if (!wait_event_timeout(ihid->wait,
>> + !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
>> + msecs_to_jiffies(5000))) {
>> + ret = -ENODATA;
>> + goto err_clear_reset;
>> + }
>> + i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
>
> super nitty, but I wonder if your i2c_hid_dbg() message saying
> "waiting" should move above the check for
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET. Then you'll have a message printed
> before your msleep. I guess technically you could then add an "else
> if" for the second "if" statement which would make it more obvious to
> the reader that the "wait_event_timeout" won't happen when the quirk
> is present.
>
> Above is just a nit
I agree with you that it would be better to move the
mutex_[un]lock(&ihid->reset_lock) calls out of
i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() and into their
callers. I'm preparing a v2 with these changes now and I'll also
move the
i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
line to above the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET test for v2.
Regards,
Hans
