[2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions

Split i2c_hid_hwreset() into:

i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and
i2c_hid_finish_hwreset() which actually waits for the reset to complete.

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 | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> 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@chromium.org>

Hi,

On 11/6/23 19:53, Doug Anderson wrote:
> Hi,
> 
> On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> 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?

I agree that moving the mutex to the callers would be better,
I've just completed this change for v2 of the series.


>> @@ -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).

One of the 2 tests goes away later, so I've kept this as is for v2.

Regards,

Hans