[PATCH 0/8] IIO: Accelerometers: Sparse friendly claim of direct mode

[PATCH 0/8] IIO: Accelerometers: Sparse friendly claim of direct mode

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Note that a number of the drivers touched in this series have no
obvious active maintainer, so it would be much appreciated if anyone
has time to take a look!

This is effectively part 2 of what will probably be around 5 series
focused on moving from iio_device_claim/release_direct_mode() to
iio_device_claim/release_direct(). The new form is more consistent
with conditional locking semantics and sparse markings have been
added that let us detect miss-balance between claim and release.

More details can be found in the cover letter of the first series:
https://lore.kernel.org/all/20250209180624.701140-1-jic23@kernel.org/

This series focuses on the accelerometer drivers. There are a couple
of fixes in here that showed up during the conversion.  They are
old issues in error paths that I guess no one hit in practice,
so I see no reason to rush them in before the next merge window.

Jonathan Cameron (8):
  iio: accel: mma8452: Ensure error return on failure to matching
    oversampling ratio
  iio: accel: mma8452: Factor out guts of write_raw() to simplify
    locking
  iio: accel: mma8452: Switch to sparse friendly
    iio_device_claim/release_direct()
  iio: accel: kx022a: Factor out guts of write_raw() to allow direct
    returns
  iio: accel: kx022a: Switch to sparse friendly
    iio_device_claim/release_direct()
  iio: accel: msa311: Fix failure to release runtime pm if direct mode
    claim fails.
  iio: accel: msa311: Switch to sparse friendly
    iio_device_claim/release_direct()
  iio: accel: Switch to sparse friendly
    iio_device_claim/release_direct()

 drivers/iio/accel/adxl372.c           |  7 +--
 drivers/iio/accel/adxl380.c           |  7 +--
 drivers/iio/accel/bma180.c            |  7 +--
 drivers/iio/accel/bmi088-accel-core.c |  7 ++-
 drivers/iio/accel/fxls8962af-core.c   | 21 +++----
 drivers/iio/accel/kionix-kx022a.c     | 74 ++++++++++++-----------
 drivers/iio/accel/mma8452.c           | 86 +++++++++++++--------------
 drivers/iio/accel/msa311.c            | 32 +++++-----
 8 files changed, 119 insertions(+), 122 deletions(-)

-- 
2.48.1

[PATCH 1/8] iio: accel: mma8452: Ensure error return on failure to matching oversampling ratio

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

If a match was not found, then the write_raw() callback would return
the odr index, not an error. Return -EINVAL if this occurs.
To avoid similar issues in future, introduce j, a new indexing variable
rather than using ret for this purpose.

Fixes: 79de2ee469aa ("iio: accel: mma8452: claim direct mode during write raw")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma8452.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 962d289065ab..1b2014c4c4b4 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -712,7 +712,7 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	int i, ret;
+	int i, j, ret;
 
 	ret = iio_device_claim_direct_mode(indio_dev);
 	if (ret)
@@ -772,14 +772,18 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 		break;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		ret = mma8452_get_odr_index(data);
+		j = mma8452_get_odr_index(data);
 
 		for (i = 0; i < ARRAY_SIZE(mma8452_os_ratio); i++) {
-			if (mma8452_os_ratio[i][ret] == val) {
+			if (mma8452_os_ratio[i][j] == val) {
 				ret = mma8452_set_power_mode(data, i);
 				break;
 			}
 		}
+		if (i == ARRAY_SIZE(mma8452_os_ratio)) {
+			ret = -EINVAL;
+			break;
+		}
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.48.1

[PATCH 2/8] iio: accel: mma8452: Factor out guts of write_raw() to simplify locking

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Factoring out those parts of write_raw() in which direct mode is held
allows for direct returns on errors, simplifying the code.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma8452.c | 78 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 1b2014c4c4b4..8ce4ddadc559 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -707,55 +707,45 @@ static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
 	return mma8452_change_config(data, MMA8452_HP_FILTER_CUTOFF, reg);
 }
 
-static int mma8452_write_raw(struct iio_dev *indio_dev,
+static int __mma8452_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int i, j, ret;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		i = mma8452_get_samp_freq_index(data, val, val2);
-		if (i < 0) {
-			ret = i;
-			break;
-		}
+		if (i < 0)
+			return i;
+
 		data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
 		data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
 
 		data->sleep_val = mma8452_calculate_sleep(data);
 
-		ret = mma8452_change_config(data, MMA8452_CTRL_REG1,
-					    data->ctrl_reg1);
-		break;
+		return mma8452_change_config(data, MMA8452_CTRL_REG1,
+					     data->ctrl_reg1);
+
 	case IIO_CHAN_INFO_SCALE:
 		i = mma8452_get_scale_index(data, val, val2);
-		if (i < 0) {
-			ret = i;
-			break;
-		}
+		if (i < 0)
+			return  i;
 
 		data->data_cfg &= ~MMA8452_DATA_CFG_FS_MASK;
 		data->data_cfg |= i;
 
-		ret = mma8452_change_config(data, MMA8452_DATA_CFG,
-					    data->data_cfg);
-		break;
+		return mma8452_change_config(data, MMA8452_DATA_CFG,
+					     data->data_cfg);
+
 	case IIO_CHAN_INFO_CALIBBIAS:
-		if (val < -128 || val > 127) {
-			ret = -EINVAL;
-			break;
-		}
+		if (val < -128 || val > 127)
+			return -EINVAL;
 
-		ret = mma8452_change_config(data,
-					    MMA8452_OFF_X + chan->scan_index,
-					    val);
-		break;
+		return mma8452_change_config(data,
+					     MMA8452_OFF_X + chan->scan_index,
+					     val);
 
 	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
 		if (val == 0 && val2 == 0) {
@@ -764,32 +754,38 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 			data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
 			ret = mma8452_set_hp_filter_frequency(data, val, val2);
 			if (ret < 0)
-				break;
+				return ret;
 		}
 
-		ret = mma8452_change_config(data, MMA8452_DATA_CFG,
+		return mma8452_change_config(data, MMA8452_DATA_CFG,
 					     data->data_cfg);
-		break;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		j = mma8452_get_odr_index(data);
 
 		for (i = 0; i < ARRAY_SIZE(mma8452_os_ratio); i++) {
-			if (mma8452_os_ratio[i][j] == val) {
-				ret = mma8452_set_power_mode(data, i);
-				break;
-			}
+			if (mma8452_os_ratio[i][j] == val)
+				return mma8452_set_power_mode(data, i);
 		}
-		if (i == ARRAY_SIZE(mma8452_os_ratio)) {
-			ret = -EINVAL;
-			break;
-		}
-		break;
+
+		return -EINVAL;
+
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
+}
+
+static int mma8452_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 
+	ret = __mma8452_write_raw(indio_dev, chan, val, val2, mask);
 	iio_device_release_direct_mode(indio_dev);
 	return ret;
 }
-- 
2.48.1

[PATCH 3/8] iio: accel: mma8452: Switch to sparse friendly iio_device_claim/release_direct()

[Jonathan Cameron]

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>
---
 drivers/iio/accel/mma8452.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 8ce4ddadc559..05f5482f366e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -497,14 +497,13 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		mutex_lock(&data->lock);
 		ret = mma8452_read(data, buffer);
 		mutex_unlock(&data->lock);
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
 
@@ -781,12 +780,11 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 {
 	int ret;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
 
 	ret = __mma8452_write_raw(indio_dev, chan, val, val2, mask);
-	iio_device_release_direct_mode(indio_dev);
+	iio_device_release_direct(indio_dev);
 	return ret;
 }
 
-- 
2.48.1

[PATCH 4/8] iio: accel: kx022a: Factor out guts of write_raw() to allow direct returns

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Create a new utility function for the actions taken when direct mode
is held. This allows for direct returns, simplifying the code flow.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/accel/kionix-kx022a.c | 66 ++++++++++++++++---------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 3a56ab00791a..727e007c5fc1 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -510,26 +510,13 @@ static int kx022a_write_raw_get_fmt(struct iio_dev *idev,
 	}
 }
 
-static int kx022a_write_raw(struct iio_dev *idev,
-			    struct iio_chan_spec const *chan,
-			    int val, int val2, long mask)
+static int __kx022a_write_raw(struct iio_dev *idev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
 {
 	struct kx022a_data *data = iio_priv(idev);
 	int ret, n;
 
-	/*
-	 * We should not allow changing scale or frequency when FIFO is running
-	 * as it will mess the timestamp/scale for samples existing in the
-	 * buffer. If this turns out to be an issue we can later change logic
-	 * to internally flush the fifo before reconfiguring so the samples in
-	 * fifo keep matching the freq/scale settings. (Such setup could cause
-	 * issues if users trust the watermark to be reached within known
-	 * time-limit).
-	 */
-	ret = iio_device_claim_direct_mode(idev);
-	if (ret)
-		return ret;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
@@ -538,20 +525,19 @@ static int kx022a_write_raw(struct iio_dev *idev,
 			if (val == kx022a_accel_samp_freq_table[n][0] &&
 			    val2 == kx022a_accel_samp_freq_table[n][1])
 				break;
-		if (n < 0) {
-			ret = -EINVAL;
-			goto unlock_out;
-		}
+		if (n < 0)
+			return -EINVAL;
+
 		ret = kx022a_turn_off_lock(data);
 		if (ret)
-			break;
+			return ret;
 
 		ret = regmap_update_bits(data->regmap,
 					 data->chip_info->odcntl,
 					 KX022A_MASK_ODR, n);
 		data->odr_ns = kx022a_odrs[n];
 		kx022a_turn_on_unlock(data);
-		break;
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		n = data->chip_info->scale_table_size / 2;
 
@@ -559,26 +545,44 @@ static int kx022a_write_raw(struct iio_dev *idev,
 			if (val == data->chip_info->scale_table[n][0] &&
 			    val2 == data->chip_info->scale_table[n][1])
 				break;
-		if (n < 0) {
-			ret = -EINVAL;
-			goto unlock_out;
-		}
+		if (n < 0)
+			return -EINVAL;
 
 		ret = kx022a_turn_off_lock(data);
 		if (ret)
-			break;
+			return ret;
 
 		ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
 					 KX022A_MASK_GSEL,
 					 n << KX022A_GSEL_SHIFT);
 		kx022a_turn_on_unlock(data);
-		break;
+		return ret;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
+}
+
+static int kx022a_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	int ret;
+
+	/*
+	 * We should not allow changing scale or frequency when FIFO is running
+	 * as it will mess the timestamp/scale for samples existing in the
+	 * buffer. If this turns out to be an issue we can later change logic
+	 * to internally flush the fifo before reconfiguring so the samples in
+	 * fifo keep matching the freq/scale settings. (Such setup could cause
+	 * issues if users trust the watermark to be reached within known
+	 * time-limit).
+	 */
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	ret = __kx022a_write_raw(idev, chan, val, val2, mask);
 
-unlock_out:
 	iio_device_release_direct_mode(idev);
 
 	return ret;
-- 
2.48.1

Re: [PATCH 4/8] iio: accel: kx022a: Factor out guts of write_raw() to allow direct returns

[Matti Vaittinen]

On 17/02/2025 16:01, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Create a new utility function for the actions taken when direct mode
> is held. This allows for direct returns, simplifying the code flow.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matti Vaittinen <mazziesaccount@gmail.com>

Thanks Jonathan!
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Yours,
	-- Matti

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

[Jonathan Cameron]

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;
 
 	ret = __kx022a_write_raw(idev, chan, val, val2, mask);
 
-	iio_device_release_direct_mode(idev);
+	iio_device_release_direct(idev);
 
 	return ret;
 }
@@ -624,15 +623,14 @@ static int kx022a_read_raw(struct iio_dev *idev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(idev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(idev))
+			return -EBUSY;
 
 		mutex_lock(&data->mutex);
 		ret = kx022a_get_axis(data, chan, val);
 		mutex_unlock(&data->mutex);
 
-		iio_device_release_direct_mode(idev);
+		iio_device_release_direct(idev);
 
 		return ret;
 
-- 
2.48.1

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

[Matti Vaittinen]

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;

would feel much more familiar. I actually had to look up the 
implementation of the iio_device_claim_direct() to see this was not a bug.

Other than that this looks very good to me.

Yours,
	-- Matti

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

[David Lechner]

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!"

Returning an error on success seem very counterintuitive to me.
And the way Jonathan implemented it seems the logical way to do it.

"If claiming direct mode did not succeed, then return an error."

> 
> would feel much more familiar. I actually had to look up the implementation of the iio_device_claim_direct() to see this was not a bug.
> 
> Other than that this looks very good to me.
> 
> Yours,
>     -- Matti

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

[Matti Vaittinen]

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.

I am not saying this is 'absolutely' bad. I can only tell that _I_ 
really had to go and look up the implementation of the 
iio_device_claim_direct() in order to review this change to ensure the 
return value check was not inverted.

Yours,
	-- Matti

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

[Nuno Sá]

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.


- Nuno Sá

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

[Matti Vaittinen]

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

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

[David Lechner]

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?

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

[Jonathan Cameron]

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
> 

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

[Matti Vaittinen]

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

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

[Jonathan Cameron]

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
> 

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

[Matti Vaittinen]

On 19/02/2025 17:25, David Lechner 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?

In general? I don't know. For me ... I am afraid I wouldn't have guessed 
the type of the return value (or 0 == "failure to claim direct") even 
with such name. It's still fair to say that I do _really_ rarely use 
stuff like mutex_trylock(), so I can't say if different naming would 
help someone else who uses those variants more.

What I would expect is -EBUSY when claiming fails, 0 if it succeeds :)

If this won't work for what ever reasons, then I'll just live with this 
function using bool and returning true on success, and move on ;)

Yours,
	-- Matti

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

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:21:51 +0200
Matti Vaittinen <mazziesaccount@gmail.com> 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>

This is copying what happens for the locks that can fail. I agree
that it would have been nice to get the advantages of sparse with
the old interface but from what I recall I got a lot more false positives
so wanted it to look more lock like.

Jonathan

> 
> Yours,
>    -- Matti
> 
> 

[PATCH 6/8] iio: accel: msa311: Fix failure to release runtime pm if direct mode claim fails.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reorder the claiming of direct mode and runtime pm calls to simplify
handling a little.  For correct error handling, after the reorder
iio_device_release_direct_mode() must be claimed in an error occurs
in pm_runtime_resume_and_get()

Fixes: 1ca2cfbc0c33 ("iio: add MEMSensing MSA311 3-axis accelerometer driver")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/msa311.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
index fe4c32a9558a..7f4ab7cdbc4a 100644
--- a/drivers/iio/accel/msa311.c
+++ b/drivers/iio/accel/msa311.c
@@ -594,23 +594,25 @@ static int msa311_read_raw_data(struct iio_dev *indio_dev,
 	__le16 axis;
 	int err;
 
-	err = pm_runtime_resume_and_get(dev);
+	err = iio_device_claim_direct_mode(indio_dev);
 	if (err)
 		return err;
 
-	err = iio_device_claim_direct_mode(indio_dev);
-	if (err)
+	err = pm_runtime_resume_and_get(dev);
+	if (err) {
+		iio_device_release_direct_mode(indio_dev);
 		return err;
+	}
 
 	mutex_lock(&msa311->lock);
 	err = msa311_get_axis(msa311, chan, &axis);
 	mutex_unlock(&msa311->lock);
 
-	iio_device_release_direct_mode(indio_dev);
-
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
+	iio_device_release_direct_mode(indio_dev);
+
 	if (err) {
 		dev_err(dev, "can't get axis %s (%pe)\n",
 			chan->datasheet_name, ERR_PTR(err));
@@ -756,10 +758,6 @@ static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	unsigned int odr;
 	int err;
 
-	err = pm_runtime_resume_and_get(dev);
-	if (err)
-		return err;
-
 	/*
 	 * Sampling frequency changing is prohibited when buffer mode is
 	 * enabled, because sometimes MSA311 chip returns outliers during
@@ -769,6 +767,12 @@ static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	if (err)
 		return err;
 
+	err = pm_runtime_resume_and_get(dev);
+	if (err) {
+		iio_device_release_direct_mode(indio_dev);
+		return err;
+	}
+
 	err = -EINVAL;
 	for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); odr++)
 		if (val == msa311_odr_table[odr].integral &&
@@ -779,11 +783,11 @@ static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 			break;
 		}
 
-	iio_device_release_direct_mode(indio_dev);
-
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
+	iio_device_release_direct_mode(indio_dev);
+
 	if (err)
 		dev_err(dev, "can't update frequency (%pe)\n", ERR_PTR(err));
 
-- 
2.48.1

[PATCH 7/8] iio: accel: msa311: Switch to sparse friendly iio_device_claim/release_direct()

[Jonathan Cameron]

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>
---
 drivers/iio/accel/msa311.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
index 7f4ab7cdbc4a..d31c11fbbe68 100644
--- a/drivers/iio/accel/msa311.c
+++ b/drivers/iio/accel/msa311.c
@@ -594,13 +594,12 @@ static int msa311_read_raw_data(struct iio_dev *indio_dev,
 	__le16 axis;
 	int err;
 
-	err = iio_device_claim_direct_mode(indio_dev);
-	if (err)
-		return err;
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
 
 	err = pm_runtime_resume_and_get(dev);
 	if (err) {
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		return err;
 	}
 
@@ -611,7 +610,7 @@ static int msa311_read_raw_data(struct iio_dev *indio_dev,
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
-	iio_device_release_direct_mode(indio_dev);
+	iio_device_release_direct(indio_dev);
 
 	if (err) {
 		dev_err(dev, "can't get axis %s (%pe)\n",
@@ -763,13 +762,12 @@ static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	 * enabled, because sometimes MSA311 chip returns outliers during
 	 * frequency values growing up in the read operation moment.
 	 */
-	err = iio_device_claim_direct_mode(indio_dev);
-	if (err)
-		return err;
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
 
 	err = pm_runtime_resume_and_get(dev);
 	if (err) {
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		return err;
 	}
 
@@ -786,7 +784,7 @@ static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
-	iio_device_release_direct_mode(indio_dev);
+	iio_device_release_direct(indio_dev);
 
 	if (err)
 		dev_err(dev, "can't update frequency (%pe)\n", ERR_PTR(err));
-- 
2.48.1

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

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Single patch for the simple cases in accelerometer drivers.

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: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
Given we had so many complex cases, maybe it makes sense to break
these out as well.
---
 drivers/iio/accel/adxl372.c           |  7 +++----
 drivers/iio/accel/adxl380.c           |  7 +++----
 drivers/iio/accel/bma180.c            |  7 +++----
 drivers/iio/accel/bmi088-accel-core.c |  7 ++++---
 drivers/iio/accel/fxls8962af-core.c   | 21 +++++++++------------
 5 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 8ba5fbe6e1f5..961145b50293 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -763,12 +763,11 @@ static int adxl372_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		ret = adxl372_read_axis(st, chan->address);
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
 
diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
index 90340f134722..0cf3c6815829 100644
--- a/drivers/iio/accel/adxl380.c
+++ b/drivers/iio/accel/adxl380.c
@@ -1175,12 +1175,11 @@ static int adxl380_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		ret = adxl380_read_chn(st, chan->address);
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
 
diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 128db14ba726..aa664a923f91 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -540,14 +540,13 @@ static int bma180_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		mutex_lock(&data->mutex);
 		ret = bma180_get_data_reg(data, chan->scan_index);
 		mutex_unlock(&data->mutex);
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
 		if (chan->scan_type.sign == 's') {
diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index 36e5d06ffd33..dea126f993c1 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -313,12 +313,13 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 
-			ret = iio_device_claim_direct_mode(indio_dev);
-			if (ret)
+			if (!iio_device_claim_direct(indio_dev)) {
+				ret = -EBUSY;
 				goto out_read_raw_pm_put;
+			}
 
 			ret = bmi088_accel_get_axis(data, chan, val);
-			iio_device_release_direct_mode(indio_dev);
+			iio_device_release_direct(indio_dev);
 			if (!ret)
 				ret = IIO_VAL_INT;
 
diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 987212a7c038..48e4282964a0 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -460,22 +460,20 @@ static int fxls8962af_write_raw(struct iio_dev *indio_dev,
 		if (val != 0)
 			return -EINVAL;
 
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		ret = fxls8962af_set_full_scale(data, val2);
 
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		return ret;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		ret = fxls8962af_set_samp_freq(data, val, val2);
 
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 		return ret;
 	default:
 		return -EINVAL;
@@ -683,14 +681,13 @@ fxls8962af_write_event_config(struct iio_dev *indio_dev,
 		fxls8962af_active(data);
 		ret = fxls8962af_power_on(data);
 	} else {
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
 
 		/* Not in buffered mode so disable power */
 		ret = fxls8962af_power_off(data);
 
-		iio_device_release_direct_mode(indio_dev);
+		iio_device_release_direct(indio_dev);
 	}
 
 	return ret;
-- 
2.48.1

Re: [PATCH 0/8] IIO: Accelerometers: Sparse friendly claim of direct mode

[David Lechner]

On 2/17/25 8:01 AM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Note that a number of the drivers touched in this series have no
> obvious active maintainer, so it would be much appreciated if anyone
> has time to take a look!
> 
Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH 0/8] IIO: Accelerometers: Sparse friendly claim of direct mode

[Jonathan Cameron]

On Mon, 17 Feb 2025 16:21:58 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 2/17/25 8:01 AM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Note that a number of the drivers touched in this series have no
> > obvious active maintainer, so it would be much appreciated if anyone
> > has time to take a look!
> >   
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> 

Thanks!
 Applied.