Hi,
On 11/6/23 19:53, Doug Anderson wrote:
> 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?
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
