On 19/02/2025 21:05, Jonathan Cameron wrote:
On Wed, 19 Feb 2025 09:25:00 -0600
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
Cc: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
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@xxxxxxxxx>
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
