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

[Hans de Goede <hdegoede@redhat.com>]

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