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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 20 Feb 2025 08:31:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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.
Ok. I'll leave it as is for now. I don't mind circling back to this
eventually. I just don't want to have a mass rename in the middle of
making the change to the new ABI as it would be really messy.

Jonathan

> 
> Yours,
> 	-- Matti
>