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

[Jonathan Cameron <jic23@kernel.org>]

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.

Slightly messy to change the patches already applied to my
tree but cleaner to do so now than later as I haven't pushed 
the branch out as togreg yet (it's just the testing branch
for 0-day).

Jonathan
>