Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> @@ -460,6 +460,20 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
> goto err_clear_reset;
> }
>
> + return 0;
The mutex "contract" between i2c_hid_start_hwreset() and
i2c_hid_finish_hwreset() is non-obvious and, IMO, deserves a comment.
Specifically i2c_hid_start_hwreset() will grab and leave the mutex
locked if it returns 0. Then i2c_hid_finish_hwreset() expects the
mutex pre-locked and will always release it.
While reviewing later patches, I actually realized that _I think_
things would be cleaner by moving the mutex lock/unlock to the
callers. Maybe take a look at how the code looks with that?
> @@ -732,7 +745,9 @@ static int i2c_hid_parse(struct hid_device *hid)
> }
>
> do {
> - ret = i2c_hid_hwreset(ihid);
> + ret = i2c_hid_start_hwreset(ihid);
> + if (ret == 0)
> + ret = i2c_hid_finish_hwreset(ihid);
> if (ret)
> msleep(1000);
nit: it's slightly weird that two "if" tests next to each other use
different style. One compares against 0 and the other just implicitly
treats an int as a bool. I'm fine with either way, but it's nice to
keep the style the same within any given function (even better if it
can be the same throughout the driver).
> @@ -975,10 +990,13 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
> * However some ALPS touchpads generate IRQ storm without reset, so
> * let's still reset them here.
> */
> - if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
> - ret = i2c_hid_hwreset(ihid);
> - else
> + if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) {
> + ret = i2c_hid_start_hwreset(ihid);
> + if (ret == 0)
> + ret = i2c_hid_finish_hwreset(ihid);
nit: Above is also a "if (ret == 0)" vs below "if (ret)"...
> + } else {
> ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> + }
>
> if (ret)
> return ret;
The above is mostly nits. I'd be OK with:
Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
