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, so:
Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
