Re: [PATCH 5/8] iio: accel: kx022a: Switch to sparse friendly iio_device_claim/release_direct()

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 19/02/2025 21:05, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 09:25:00 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 2/19/25 6:21 AM, Matti Vaittinen wrote:
>>> On 19/02/2025 12:51, Nuno Sá wrote:
>>>> On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote:
>>>>> On 18/02/2025 17:42, David Lechner wrote:
>>>>>> On 2/18/25 1:39 AM, Matti Vaittinen wrote:
>>>>>>> On 17/02/2025 16:01, Jonathan Cameron wrote:
>>>>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>>>>
>>>>>>>> These new functions allow sparse to find failures to release
>>>>>>>> direct mode reducing chances of bugs over the claim_direct_mode()
>>>>>>>> functions that are deprecated.
>>>>>>>>
>>>>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com>
>>>>>>>> ---
>>>>>>>>      drivers/iio/accel/kionix-kx022a.c | 14 ++++++--------
>>>>>>>>      1 file changed, 6 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c
>>>>>>>> b/drivers/iio/accel/kionix-kx022a.c
>>>>>>>> index 727e007c5fc1..07dcf5f0599f 100644
>>>>>>>> --- a/drivers/iio/accel/kionix-kx022a.c
>>>>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c
>>>>>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev,
>>>>>>>>           * issues if users trust the watermark to be reached within known
>>>>>>>>           * time-limit).
>>>>>>>>           */
>>>>>>>> -    ret = iio_device_claim_direct_mode(idev);
>>>>>>>> -    if (ret)
>>>>>>>> -        return ret;
>>>>>>>> +    if (!iio_device_claim_direct(idev))
>>>>>>>> +        return -EBUSY;
>>>>>>>
>>>>>>> Not really in the scope of this review - but in my opinion the logic of
>>>>>>> this check is terribly counter intuitive. I mean,
>>>>>>>   
>>>>>>>> +    if (iio_device_claim_direct(idev))
>>>>>>>> +        return -EBUSY;
>>>>>>
>>>>>> I'm curious how you read this then. I read this as:
>>>>>>
>>>>>> "If claiming direct mode succeeded, then return an error!"
>>>>>
>>>>> I am used to seeing a pattern where function returning zero indicates a
>>>>> success. I have no statistics but I believe this is true for a vast
>>>>> majority of functions in the kernel. I believe this was the case with
>>>>> the old 'iio_device_claim_direct_mode(idev)' too.
>>>>>   
>>>>
>>>> Fair enough... Note though this is returning a boolean where true makes total
>>>> sense for the "good" case. I do agree it's not super clear just by reading the
>>>> code that the API is supposed to return a boolean.
>>>
>>> Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens.
>>>
>>> Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding:
>>>
>>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>
>>> Yours,
>>>    -- Matti
>>>
>>>    
>>
>> Would a name like iio_device_try_claim_direct_mode() make it more
>> obvious that it returned a bool instead of int?
> 
> FWIW I'd consider this a reasonable change if people in general
> find it more intuitive.  Conveys to those not familiar with the
> fun of IIO that failure is something we kind of expect to happen.

As I replied to David's mail - for me renaming is not likely to make a 
big difference - but maybe it would help someone who is more used to the 
mutex_trylock() and alike. I'd still like to see someone else thinking 
that renaming would help before asking for anyone to go through that hassle.

Yours,
	-- Matti