diff mbox series

[1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

Message ID 20231104111743.14668-2-hdegoede@redhat.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: i2c-hid: Rework wait for reset to match Windows | expand

Commit Message

Hans de Goede Nov. 4, 2023, 11:17 a.m. UTC 
i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(),
fold the latter into the former.

This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.

No functional changes intended.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 80 +++++++++++++-----------------
 1 file changed, 34 insertions(+), 46 deletions(-)

Comments

Doug Anderson Nov. 6, 2023, 6:50 p.m. UTC | #1 
Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> 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@chromium.org>
Hans de Goede Nov. 17, 2023, 7:34 p.m. UTC | #2 
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@redhat.com> 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
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2735cd585af0..12a9edb23f82 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -426,49 +426,9 @@  static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	return ret;
 }
 
-static int i2c_hid_execute_reset(struct i2c_hid *ihid)
-{
-	size_t length = 0;
-	int ret;
-
-	i2c_hid_dbg(ihid, "resetting...\n");
-
-	/* 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.\n");
-		goto out;
-	}
-
-	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
-		msleep(100);
-		goto out;
-	}
-
-	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 out;
-	}
-	i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
-
-out:
-	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-	return ret;
-}
-
 static int i2c_hid_hwreset(struct i2c_hid *ihid)
 {
+	size_t length = 0;
 	int ret;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -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__);
+
 	/* At least some SIS devices need this after reset */
 	if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
-out_unlock:
+	mutex_unlock(&ihid->reset_lock);
+	return ret;
+
+err_clear_reset:
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+err_unlock:
 	mutex_unlock(&ihid->reset_lock);
 	return ret;
 }