* [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-02-04 16:26 ` andy.shevchenko
2024-01-28 15:05 ` [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup Jonathan Cameron
` (9 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Allows use of:
iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
}
to automatically call iio_device_release_direct_mode() based on scope.
Typically seen in combination with local device specific locks which
are already have automated cleanup options via guard(mutex)(&st->lock)
and scoped_guard(). Using both together allows most error handling to
be automated.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Drop the IS_ERR() protection in iio_device_release_direct_mode()
as nothing in this new infrastructure results in this function
being passed an error pointer.
- Add some blank lines to make the header slightly easier to read.
---
include/linux/iio/iio.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index c5b36d2c1e73..28e78af3614b 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -9,6 +9,7 @@
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/cleanup.h>
#include <linux/slab.h>
#include <linux/iio/types.h>
/* IIO TODO LIST */
@@ -638,6 +639,30 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
void iio_device_release_direct_mode(struct iio_dev *indio_dev);
+
+/* This autocleanup logic is normally used via iio_claim_direct_scoped */
+DEFINE_GUARD(iio_claim_direct, struct iio_dev *, iio_device_claim_direct_mode(_T),
+ iio_device_release_direct_mode(_T))
+
+DEFINE_GUARD_COND(iio_claim_direct, _try, ({
+ struct iio_dev *dev;
+ int d = iio_device_claim_direct_mode(_T);
+
+ if (d < 0)
+ dev = NULL;
+ else
+ dev = _T;
+ dev;
+ }))
+
+/**
+ * iio_device_claim_direct_scoped() - Scoped call to iio_device_claim_direct.
+ * @fail: What to do on failure to claim device.
+ * @iio_dev: Pointer to the IIO devices structure
+ */
+#define iio_device_claim_direct_scoped(fail, iio_dev) \
+ scoped_cond_guard(iio_claim_direct_try, fail, iio_dev)
+
int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure
2024-01-28 15:05 ` [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure Jonathan Cameron
@ 2024-02-04 16:26 ` andy.shevchenko
2024-02-04 17:42 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: andy.shevchenko @ 2024-02-04 16:26 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
Sun, Jan 28, 2024 at 03:05:28PM +0000, Jonathan Cameron kirjoitti:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Allows use of:
>
> iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> }
>
> to automatically call iio_device_release_direct_mode() based on scope.
> Typically seen in combination with local device specific locks which
> are already have automated cleanup options via guard(mutex)(&st->lock)
> and scoped_guard(). Using both together allows most error handling to
> be automated.
...
> +/* This autocleanup logic is normally used via iio_claim_direct_scoped */
Wrong name? And missing parentheses.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure
2024-02-04 16:26 ` andy.shevchenko
@ 2024-02-04 17:42 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-02-04 17:42 UTC (permalink / raw)
To: andy.shevchenko
Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
On Sun, 4 Feb 2024 18:26:16 +0200
andy.shevchenko@gmail.com wrote:
> Sun, Jan 28, 2024 at 03:05:28PM +0000, Jonathan Cameron kirjoitti:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Allows use of:
> >
> > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > }
> >
> > to automatically call iio_device_release_direct_mode() based on scope.
> > Typically seen in combination with local device specific locks which
> > are already have automated cleanup options via guard(mutex)(&st->lock)
> > and scoped_guard(). Using both together allows most error handling to
> > be automated.
>
> ...
>
> > +/* This autocleanup logic is normally used via iio_claim_direct_scoped */
>
> Wrong name? And missing parentheses.
>
Oops. I renamed it and missed the comment. Will fix up.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
2024-01-28 15:05 ` [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 21:52 ` David Lechner
2024-02-04 16:29 ` andy.shevchenko
2024-01-28 15:05 ` [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode Jonathan Cameron
` (8 subsequent siblings)
10 siblings, 2 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Given we now have iio_device_claim_direct_scoped() to perform automatic
releasing of direct mode at exit from the scope that follows it, this can
be used in conjunction with guard(mutex) etc remove a lot of special case
handling.
Note that in this particular example code, there is no real reason you can't
read channels via sysfs at the same time as filling the software buffer.
To make it look more like a real driver constrain raw and processed
channel reads from occurring whilst the buffer is in use.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Dropped a stale comment about a local variable that existed
in an earlier version of this patch before the new scoped_guard_cond()
infrastructure was added.
- Use unreachable() to convince the compiler we can't get to code
at end of the pattern.
iio_device_claim_direct_scoped(return -EBUSY, iio_dev) {
return 0;
}
unreacahable();
---
drivers/iio/dummy/iio_simple_dummy.c | 182 +++++++++++++--------------
1 file changed, 88 insertions(+), 94 deletions(-)
diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
index c24f609c2ade..d6ef556698fb 100644
--- a/drivers/iio/dummy/iio_simple_dummy.c
+++ b/drivers/iio/dummy/iio_simple_dummy.c
@@ -283,65 +283,63 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
long mask)
{
struct iio_dummy_state *st = iio_priv(indio_dev);
- int ret = -EINVAL;
- mutex_lock(&st->lock);
switch (mask) {
case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
- switch (chan->type) {
- case IIO_VOLTAGE:
- if (chan->output) {
- /* Set integer part to cached value */
- *val = st->dac_val;
- ret = IIO_VAL_INT;
- } else if (chan->differential) {
- if (chan->channel == 1)
- *val = st->differential_adc_val[0];
- else
- *val = st->differential_adc_val[1];
- ret = IIO_VAL_INT;
- } else {
- *val = st->single_ended_adc_val;
- ret = IIO_VAL_INT;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ guard(mutex)(&st->lock);
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (chan->output) {
+ /* Set integer part to cached value */
+ *val = st->dac_val;
+ return IIO_VAL_INT;
+ } else if (chan->differential) {
+ if (chan->channel == 1)
+ *val = st->differential_adc_val[0];
+ else
+ *val = st->differential_adc_val[1];
+ return IIO_VAL_INT;
+ } else {
+ *val = st->single_ended_adc_val;
+ return IIO_VAL_INT;
+ }
+
+ case IIO_ACCEL:
+ *val = st->accel_val;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
}
- break;
- case IIO_ACCEL:
- *val = st->accel_val;
- ret = IIO_VAL_INT;
- break;
- default:
- break;
}
- break;
+ unreachable();
case IIO_CHAN_INFO_PROCESSED:
- switch (chan->type) {
- case IIO_STEPS:
- *val = st->steps;
- ret = IIO_VAL_INT;
- break;
- case IIO_ACTIVITY:
- switch (chan->channel2) {
- case IIO_MOD_RUNNING:
- *val = st->activity_running;
- ret = IIO_VAL_INT;
- break;
- case IIO_MOD_WALKING:
- *val = st->activity_walking;
- ret = IIO_VAL_INT;
- break;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ guard(mutex)(&st->lock);
+ switch (chan->type) {
+ case IIO_STEPS:
+ *val = st->steps;
+ return IIO_VAL_INT;
+ case IIO_ACTIVITY:
+ switch (chan->channel2) {
+ case IIO_MOD_RUNNING:
+ *val = st->activity_running;
+ return IIO_VAL_INT;
+ case IIO_MOD_WALKING:
+ *val = st->activity_walking;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
default:
- break;
+ return -EINVAL;
}
- break;
- default:
- break;
}
- break;
+ unreachable();
case IIO_CHAN_INFO_OFFSET:
/* only single ended adc -> 7 */
*val = 7;
- ret = IIO_VAL_INT;
- break;
+ return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_VOLTAGE:
@@ -350,60 +348,57 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
/* only single ended adc -> 0.001333 */
*val = 0;
*val2 = 1333;
- ret = IIO_VAL_INT_PLUS_MICRO;
- break;
+ return IIO_VAL_INT_PLUS_MICRO;
case 1:
/* all differential adc -> 0.000001344 */
*val = 0;
*val2 = 1344;
- ret = IIO_VAL_INT_PLUS_NANO;
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return -EINVAL;
}
- break;
default:
- break;
+ return -EINVAL;
}
- break;
- case IIO_CHAN_INFO_CALIBBIAS:
+ case IIO_CHAN_INFO_CALIBBIAS: {
+ guard(mutex)(&st->lock);
/* only the acceleration axis - read from cache */
*val = st->accel_calibbias;
- ret = IIO_VAL_INT;
- break;
- case IIO_CHAN_INFO_CALIBSCALE:
+ return IIO_VAL_INT;
+ }
+ case IIO_CHAN_INFO_CALIBSCALE: {
+ guard(mutex)(&st->lock);
*val = st->accel_calibscale->val;
*val2 = st->accel_calibscale->val2;
- ret = IIO_VAL_INT_PLUS_MICRO;
- break;
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
case IIO_CHAN_INFO_SAMP_FREQ:
*val = 3;
*val2 = 33;
- ret = IIO_VAL_INT_PLUS_NANO;
- break;
- case IIO_CHAN_INFO_ENABLE:
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_ENABLE: {
+ guard(mutex)(&st->lock);
switch (chan->type) {
case IIO_STEPS:
*val = st->steps_enabled;
- ret = IIO_VAL_INT;
- break;
+ return IIO_VAL_INT;
default:
- break;
+ return -EINVAL;
}
- break;
- case IIO_CHAN_INFO_CALIBHEIGHT:
+ }
+ case IIO_CHAN_INFO_CALIBHEIGHT: {
+ guard(mutex)(&st->lock);
switch (chan->type) {
case IIO_STEPS:
*val = st->height;
- ret = IIO_VAL_INT;
- break;
+ return IIO_VAL_INT;
default:
- break;
+ return -EINVAL;
}
- break;
-
+ }
default:
- break;
+ return -EINVAL;
}
- mutex_unlock(&st->lock);
- return ret;
}
/**
@@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
if (chan->output == 0)
return -EINVAL;
- /* Locking not required as writing single value */
- mutex_lock(&st->lock);
- st->dac_val = val;
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ /* Locking not required as writing single value */
+ st->dac_val = val;
+ }
return 0;
default:
return -EINVAL;
@@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_STEPS:
- mutex_lock(&st->lock);
- st->steps = val;
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ st->steps = val;
+ }
return 0;
case IIO_ACTIVITY:
if (val < 0)
@@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
- case IIO_CHAN_INFO_CALIBSCALE:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_CALIBSCALE: {
+ guard(mutex)(&st->lock);
/* Compare against table - hard matching here */
for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
if (val == dummy_scales[i].val &&
val2 == dummy_scales[i].val2)
break;
if (i == ARRAY_SIZE(dummy_scales))
- ret = -EINVAL;
- else
- st->accel_calibscale = &dummy_scales[i];
- mutex_unlock(&st->lock);
+ return -EINVAL;
+ st->accel_calibscale = &dummy_scales[i];
return ret;
+ }
case IIO_CHAN_INFO_CALIBBIAS:
- mutex_lock(&st->lock);
- st->accel_calibbias = val;
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ st->accel_calibbias = val;
+ }
return 0;
case IIO_CHAN_INFO_ENABLE:
switch (chan->type) {
case IIO_STEPS:
- mutex_lock(&st->lock);
- st->steps_enabled = val;
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ st->steps_enabled = val;
+ }
return 0;
default:
return -EINVAL;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-28 15:05 ` [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup Jonathan Cameron
@ 2024-01-28 21:52 ` David Lechner
2024-01-29 11:46 ` Jonathan Cameron
2024-02-04 16:29 ` andy.shevchenko
1 sibling, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-01-28 21:52 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Peter Zijlstra, Jonathan Cameron
On Sun, Jan 28, 2024 at 9:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Given we now have iio_device_claim_direct_scoped() to perform automatic
> releasing of direct mode at exit from the scope that follows it, this can
> be used in conjunction with guard(mutex) etc remove a lot of special case
> handling.
>
> Note that in this particular example code, there is no real reason you can't
> read channels via sysfs at the same time as filling the software buffer.
> To make it look more like a real driver constrain raw and processed
> channel reads from occurring whilst the buffer is in use.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Since RFC:
> - Dropped a stale comment about a local variable that existed
> in an earlier version of this patch before the new scoped_guard_cond()
> infrastructure was added.
> - Use unreachable() to convince the compiler we can't get to code
> at end of the pattern.
>
> iio_device_claim_direct_scoped(return -EBUSY, iio_dev) {
> return 0;
> }
> unreacahable();
> ---
> drivers/iio/dummy/iio_simple_dummy.c | 182 +++++++++++++--------------
> 1 file changed, 88 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index c24f609c2ade..d6ef556698fb 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -283,65 +283,63 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct iio_dummy_state *st = iio_priv(indio_dev);
> - int ret = -EINVAL;
>
> - mutex_lock(&st->lock);
> switch (mask) {
> case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
> - switch (chan->type) {
> - case IIO_VOLTAGE:
> - if (chan->output) {
> - /* Set integer part to cached value */
> - *val = st->dac_val;
> - ret = IIO_VAL_INT;
> - } else if (chan->differential) {
> - if (chan->channel == 1)
> - *val = st->differential_adc_val[0];
> - else
> - *val = st->differential_adc_val[1];
> - ret = IIO_VAL_INT;
> - } else {
> - *val = st->single_ended_adc_val;
> - ret = IIO_VAL_INT;
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (chan->output) {
> + /* Set integer part to cached value */
> + *val = st->dac_val;
> + return IIO_VAL_INT;
> + } else if (chan->differential) {
> + if (chan->channel == 1)
> + *val = st->differential_adc_val[0];
> + else
> + *val = st->differential_adc_val[1];
> + return IIO_VAL_INT;
> + } else {
> + *val = st->single_ended_adc_val;
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_ACCEL:
> + *val = st->accel_val;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> }
> - break;
> - case IIO_ACCEL:
> - *val = st->accel_val;
> - ret = IIO_VAL_INT;
> - break;
> - default:
> - break;
> }
> - break;
> + unreachable();
> case IIO_CHAN_INFO_PROCESSED:
> - switch (chan->type) {
> - case IIO_STEPS:
> - *val = st->steps;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_ACTIVITY:
> - switch (chan->channel2) {
> - case IIO_MOD_RUNNING:
> - *val = st->activity_running;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_MOD_WALKING:
> - *val = st->activity_walking;
> - ret = IIO_VAL_INT;
> - break;
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> + switch (chan->type) {
> + case IIO_STEPS:
> + *val = st->steps;
> + return IIO_VAL_INT;
> + case IIO_ACTIVITY:
> + switch (chan->channel2) {
> + case IIO_MOD_RUNNING:
> + *val = st->activity_running;
> + return IIO_VAL_INT;
> + case IIO_MOD_WALKING:
> + *val = st->activity_walking;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - default:
> - break;
> }
> - break;
> + unreachable();
> case IIO_CHAN_INFO_OFFSET:
> /* only single ended adc -> 7 */
> *val = 7;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_VOLTAGE:
> @@ -350,60 +348,57 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
> /* only single ended adc -> 0.001333 */
> *val = 0;
> *val2 = 1333;
> - ret = IIO_VAL_INT_PLUS_MICRO;
> - break;
> + return IIO_VAL_INT_PLUS_MICRO;
> case 1:
> /* all differential adc -> 0.000001344 */
> *val = 0;
> *val2 = 1344;
> - ret = IIO_VAL_INT_PLUS_NANO;
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> }
> - break;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - case IIO_CHAN_INFO_CALIBBIAS:
> + case IIO_CHAN_INFO_CALIBBIAS: {
> + guard(mutex)(&st->lock);
> /* only the acceleration axis - read from cache */
> *val = st->accel_calibbias;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_CHAN_INFO_CALIBSCALE:
> + return IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_CALIBSCALE: {
> + guard(mutex)(&st->lock);
> *val = st->accel_calibscale->val;
> *val2 = st->accel_calibscale->val2;
> - ret = IIO_VAL_INT_PLUS_MICRO;
> - break;
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> case IIO_CHAN_INFO_SAMP_FREQ:
> *val = 3;
> *val2 = 33;
> - ret = IIO_VAL_INT_PLUS_NANO;
> - break;
> - case IIO_CHAN_INFO_ENABLE:
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_ENABLE: {
> + guard(mutex)(&st->lock);
> switch (chan->type) {
> case IIO_STEPS:
> *val = st->steps_enabled;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - case IIO_CHAN_INFO_CALIBHEIGHT:
> + }
> + case IIO_CHAN_INFO_CALIBHEIGHT: {
> + guard(mutex)(&st->lock);
> switch (chan->type) {
> case IIO_STEPS:
> *val = st->height;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> -
> + }
> default:
> - break;
> + return -EINVAL;
> }
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> /**
> @@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> if (chan->output == 0)
> return -EINVAL;
>
> - /* Locking not required as writing single value */
> - mutex_lock(&st->lock);
> - st->dac_val = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + /* Locking not required as writing single value */
> + st->dac_val = val;
> + }
> return 0;
> default:
> return -EINVAL;
> @@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_STEPS:
> - mutex_lock(&st->lock);
> - st->steps = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->steps = val;
> + }
> return 0;
> case IIO_ACTIVITY:
> if (val < 0)
> @@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> - case IIO_CHAN_INFO_CALIBSCALE:
> - mutex_lock(&st->lock);
> + case IIO_CHAN_INFO_CALIBSCALE: {
> + guard(mutex)(&st->lock);
> /* Compare against table - hard matching here */
> for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
> if (val == dummy_scales[i].val &&
> val2 == dummy_scales[i].val2)
> break;
> if (i == ARRAY_SIZE(dummy_scales))
> - ret = -EINVAL;
> - else
> - st->accel_calibscale = &dummy_scales[i];
> - mutex_unlock(&st->lock);
> + return -EINVAL;
> + st->accel_calibscale = &dummy_scales[i];
> return ret;
Can we change this to `return 0;` and get rid of the `ret = 0`
initialization at the beginning of the function?
> + }
> case IIO_CHAN_INFO_CALIBBIAS:
> - mutex_lock(&st->lock);
> - st->accel_calibbias = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->accel_calibbias = val;
> + }
> return 0;
> case IIO_CHAN_INFO_ENABLE:
> switch (chan->type) {
> case IIO_STEPS:
> - mutex_lock(&st->lock);
> - st->steps_enabled = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->steps_enabled = val;
> + }
> return 0;
> default:
> return -EINVAL;
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-28 21:52 ` David Lechner
@ 2024-01-29 11:46 ` Jonathan Cameron
2024-01-29 19:58 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-29 11:46 UTC (permalink / raw)
To: David Lechner; +Cc: Jonathan Cameron, linux-iio, Peter Zijlstra
> > @@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > if (chan->output == 0)
> > return -EINVAL;
> >
> > - /* Locking not required as writing single value */
> > - mutex_lock(&st->lock);
> > - st->dac_val = val;
> > - mutex_unlock(&st->lock);
> > + scoped_guard(mutex, &st->lock) {
> > + /* Locking not required as writing single value */
> > + st->dac_val = val;
> > + }
> > return 0;
> > default:
> > return -EINVAL;
> > @@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_PROCESSED:
> > switch (chan->type) {
> > case IIO_STEPS:
> > - mutex_lock(&st->lock);
> > - st->steps = val;
> > - mutex_unlock(&st->lock);
> > + scoped_guard(mutex, &st->lock) {
> > + st->steps = val;
> > + }
> > return 0;
> > case IIO_ACTIVITY:
> > if (val < 0)
> > @@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > - case IIO_CHAN_INFO_CALIBSCALE:
> > - mutex_lock(&st->lock);
> > + case IIO_CHAN_INFO_CALIBSCALE: {
> > + guard(mutex)(&st->lock);
> > /* Compare against table - hard matching here */
> > for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
> > if (val == dummy_scales[i].val &&
> > val2 == dummy_scales[i].val2)
> > break;
> > if (i == ARRAY_SIZE(dummy_scales))
> > - ret = -EINVAL;
> > - else
> > - st->accel_calibscale = &dummy_scales[i];
> > - mutex_unlock(&st->lock);
> > + return -EINVAL;
> > + st->accel_calibscale = &dummy_scales[i];
> > return ret;
>
> Can we change this to `return 0;` and get rid of the `ret = 0`
> initialization at the beginning of the function?
Yes. That would make sense.
>
> > + }
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-29 11:46 ` Jonathan Cameron
@ 2024-01-29 19:58 ` Jonathan Cameron
2024-01-29 20:05 ` David Lechner
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-29 19:58 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: David Lechner, linux-iio, Peter Zijlstra
On Mon, 29 Jan 2024 11:46:22 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > @@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > if (chan->output == 0)
> > > return -EINVAL;
> > >
> > > - /* Locking not required as writing single value */
> > > - mutex_lock(&st->lock);
> > > - st->dac_val = val;
> > > - mutex_unlock(&st->lock);
> > > + scoped_guard(mutex, &st->lock) {
> > > + /* Locking not required as writing single value */
> > > + st->dac_val = val;
> > > + }
> > > return 0;
> > > default:
> > > return -EINVAL;
> > > @@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > case IIO_CHAN_INFO_PROCESSED:
> > > switch (chan->type) {
> > > case IIO_STEPS:
> > > - mutex_lock(&st->lock);
> > > - st->steps = val;
> > > - mutex_unlock(&st->lock);
> > > + scoped_guard(mutex, &st->lock) {
> > > + st->steps = val;
> > > + }
> > > return 0;
> > > case IIO_ACTIVITY:
> > > if (val < 0)
> > > @@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > default:
> > > return -EINVAL;
> > > }
> > > - case IIO_CHAN_INFO_CALIBSCALE:
> > > - mutex_lock(&st->lock);
> > > + case IIO_CHAN_INFO_CALIBSCALE: {
> > > + guard(mutex)(&st->lock);
> > > /* Compare against table - hard matching here */
> > > for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
> > > if (val == dummy_scales[i].val &&
> > > val2 == dummy_scales[i].val2)
> > > break;
> > > if (i == ARRAY_SIZE(dummy_scales))
> > > - ret = -EINVAL;
> > > - else
> > > - st->accel_calibscale = &dummy_scales[i];
> > > - mutex_unlock(&st->lock);
> > > + return -EINVAL;
> > > + st->accel_calibscale = &dummy_scales[i];
> > > return ret;
> >
> > Can we change this to `return 0;` and get rid of the `ret = 0`
> > initialization at the beginning of the function?
>
> Yes. That would make sense.
Given it's fairly trivial, I may not post it again but instead just
tidy that up whilst applying. Diff will also git rid of the bonus space
in this block. oops.
diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
index d6ef556698fb..09efacaf8f78 100644
--- a/drivers/iio/dummy/iio_simple_dummy.c
+++ b/drivers/iio/dummy/iio_simple_dummy.c
@@ -421,7 +421,6 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
long mask)
{
int i;
- int ret = 0;
struct iio_dummy_state *st = iio_priv(indio_dev);
switch (mask) {
@@ -473,9 +472,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
val2 == dummy_scales[i].val2)
break;
if (i == ARRAY_SIZE(dummy_scales))
- return -EINVAL;
+ return -EINVAL;
st->accel_calibscale = &dummy_scales[i];
- return ret;
+ return 0;
}
case IIO_CHAN_INFO_CALIBBIAS:
scoped_guard(mutex, &st->lock) {
> >
> > > + }
>
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-29 19:58 ` Jonathan Cameron
@ 2024-01-29 20:05 ` David Lechner
0 siblings, 0 replies; 24+ messages in thread
From: David Lechner @ 2024-01-29 20:05 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Jonathan Cameron, linux-iio, Peter Zijlstra
On Mon, Jan 29, 2024 at 1:58 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 29 Jan 2024 11:46:22 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
> > > > @@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > > if (chan->output == 0)
> > > > return -EINVAL;
> > > >
> > > > - /* Locking not required as writing single value */
> > > > - mutex_lock(&st->lock);
> > > > - st->dac_val = val;
> > > > - mutex_unlock(&st->lock);
> > > > + scoped_guard(mutex, &st->lock) {
> > > > + /* Locking not required as writing single value */
> > > > + st->dac_val = val;
> > > > + }
> > > > return 0;
> > > > default:
> > > > return -EINVAL;
> > > > @@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > > case IIO_CHAN_INFO_PROCESSED:
> > > > switch (chan->type) {
> > > > case IIO_STEPS:
> > > > - mutex_lock(&st->lock);
> > > > - st->steps = val;
> > > > - mutex_unlock(&st->lock);
> > > > + scoped_guard(mutex, &st->lock) {
> > > > + st->steps = val;
> > > > + }
> > > > return 0;
> > > > case IIO_ACTIVITY:
> > > > if (val < 0)
> > > > @@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> > > > default:
> > > > return -EINVAL;
> > > > }
> > > > - case IIO_CHAN_INFO_CALIBSCALE:
> > > > - mutex_lock(&st->lock);
> > > > + case IIO_CHAN_INFO_CALIBSCALE: {
> > > > + guard(mutex)(&st->lock);
> > > > /* Compare against table - hard matching here */
> > > > for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
> > > > if (val == dummy_scales[i].val &&
> > > > val2 == dummy_scales[i].val2)
> > > > break;
> > > > if (i == ARRAY_SIZE(dummy_scales))
> > > > - ret = -EINVAL;
> > > > - else
> > > > - st->accel_calibscale = &dummy_scales[i];
> > > > - mutex_unlock(&st->lock);
> > > > + return -EINVAL;
> > > > + st->accel_calibscale = &dummy_scales[i];
> > > > return ret;
> > >
> > > Can we change this to `return 0;` and get rid of the `ret = 0`
> > > initialization at the beginning of the function?
> >
> > Yes. That would make sense.
>
> Given it's fairly trivial, I may not post it again but instead just
> tidy that up whilst applying. Diff will also git rid of the bonus space
> in this block. oops.
In that case:
Reviewed-by: David Lechner <dlechner@baylibre.com>
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index d6ef556698fb..09efacaf8f78 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -421,7 +421,6 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> long mask)
> {
> int i;
> - int ret = 0;
> struct iio_dummy_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> @@ -473,9 +472,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> val2 == dummy_scales[i].val2)
> break;
> if (i == ARRAY_SIZE(dummy_scales))
> - return -EINVAL;
> + return -EINVAL;
> st->accel_calibscale = &dummy_scales[i];
> - return ret;
> + return 0;
> }
> case IIO_CHAN_INFO_CALIBBIAS:
> scoped_guard(mutex, &st->lock) {
>
> > >
> > > > + }
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-01-28 15:05 ` [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup Jonathan Cameron
2024-01-28 21:52 ` David Lechner
@ 2024-02-04 16:29 ` andy.shevchenko
2024-02-04 17:41 ` Jonathan Cameron
1 sibling, 1 reply; 24+ messages in thread
From: andy.shevchenko @ 2024-02-04 16:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
Sun, Jan 28, 2024 at 03:05:29PM +0000, Jonathan Cameron kirjoitti:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Given we now have iio_device_claim_direct_scoped() to perform automatic
> releasing of direct mode at exit from the scope that follows it, this can
> be used in conjunction with guard(mutex) etc remove a lot of special case
> handling.
>
> Note that in this particular example code, there is no real reason you can't
> read channels via sysfs at the same time as filling the software buffer.
> To make it look more like a real driver constrain raw and processed
> channel reads from occurring whilst the buffer is in use.
...
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (chan->output) {
> + /* Set integer part to cached value */
> + *val = st->dac_val;
> + return IIO_VAL_INT;
> + } else if (chan->differential) {
> + if (chan->channel == 1)
> + *val = st->differential_adc_val[0];
> + else
> + *val = st->differential_adc_val[1];
> + return IIO_VAL_INT;
> + } else {
> + *val = st->single_ended_adc_val;
> + return IIO_VAL_INT;
> + }
Now you may go further and use only single return statement here.
> + case IIO_ACCEL:
> + *val = st->accel_val;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> }
...
> + unreachable();
Hmm... Is it really required? Why?
...
P.S> I hope you are using --histogram diff algo when preparing patches.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.
2024-02-04 16:29 ` andy.shevchenko
@ 2024-02-04 17:41 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-02-04 17:41 UTC (permalink / raw)
To: andy.shevchenko
Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
On Sun, 4 Feb 2024 18:29:25 +0200
andy.shevchenko@gmail.com wrote:
> Sun, Jan 28, 2024 at 03:05:29PM +0000, Jonathan Cameron kirjoitti:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Given we now have iio_device_claim_direct_scoped() to perform automatic
> > releasing of direct mode at exit from the scope that follows it, this can
> > be used in conjunction with guard(mutex) etc remove a lot of special case
> > handling.
> >
> > Note that in this particular example code, there is no real reason you can't
> > read channels via sysfs at the same time as filling the software buffer.
> > To make it look more like a real driver constrain raw and processed
> > channel reads from occurring whilst the buffer is in use.
>
> ...
>
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > + guard(mutex)(&st->lock);
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + if (chan->output) {
> > + /* Set integer part to cached value */
> > + *val = st->dac_val;
> > + return IIO_VAL_INT;
> > + } else if (chan->differential) {
> > + if (chan->channel == 1)
> > + *val = st->differential_adc_val[0];
> > + else
> > + *val = st->differential_adc_val[1];
> > + return IIO_VAL_INT;
> > + } else {
> > + *val = st->single_ended_adc_val;
> > + return IIO_VAL_INT;
> > + }
>
> Now you may go further and use only single return statement here.
True though this is an example driver and it's only really coincidence those returns
are the same so I'd rather keep it as explicitly matching *val with the return.
>
> > + case IIO_ACCEL:
> > + *val = st->accel_val;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > }
>
> ...
>
> > + unreachable();
>
> Hmm... Is it really required? Why?
Try compiling without it. (I'm running with C=1 W=1 but think this will show up anyway.
Seems it can't tell we never leave the for loop.
>
In file included from ./include/linux/preempt.h:11,
from ./include/linux/spinlock.h:56,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:7,
from ./include/linux/slab.h:16,
from drivers/iio/dummy/iio_simple_dummy.c:15:
drivers/iio/dummy/iio_simple_dummy.c: In function ‘iio_dummy_read_raw’:
./include/linux/cleanup.h:173:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
173 | for (CLASS(_name, scope)(args), \
| ^~~
./include/linux/iio/iio.h:667:9: note: in expansion of macro ‘scoped_cond_guard’
667 | scoped_cond_guard(iio_claim_direct_try, fail, iio_dev)
| ^~~~~~~~~~~~~~~~~
drivers/iio/dummy/iio_simple_dummy.c:289:17: note: in expansion of macro ‘iio_device_claim_direct_scoped’
289 | iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dummy/iio_simple_dummy.c:316:9: note: here
316 | case IIO_CHAN_INFO_PROCESSED:
| ^~~~
> ...
>
> P.S> I hope you are using --histogram diff algo when preparing patches.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
2024-01-28 15:05 ` [PATCH 01/10] iio: locking: introduce __cleanup() based direct mode claiming infrastructure Jonathan Cameron
2024-01-28 15:05 ` [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-30 12:53 ` Nuno Sá
2024-01-28 15:05 ` [PATCH 04/10] iio: imu: bmi323: Use cleanup handling for iio_device_claim_direct_mode() Jonathan Cameron
` (7 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Switching to the iio_device_claim_direct_scoped() for state
and to guard() based unlocking of mutexes simplifies error handling
by allowing direct returns when an error is encountered.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Use unreachable() to stop complier moaning if we have
iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
return 0;
}
unreachable(); /* can't get here */
- Update some code from earlier attempt to handle this that was left
behind from before iio_device_claim_direct_scoped()
- Reduce scope of some local variables only used within
iio_device_claim_direct_scoped() blocks.
---
drivers/iio/accel/adxl367.c | 297 ++++++++++++++----------------------
1 file changed, 118 insertions(+), 179 deletions(-)
diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 90b7ae6d42b7..834ee6d63947 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -339,22 +339,17 @@ static int adxl367_set_act_threshold(struct adxl367_state *st,
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = _adxl367_set_act_threshold(st, act, threshold);
if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
}
static int adxl367_set_act_proc_mode(struct adxl367_state *st,
@@ -482,51 +477,45 @@ static int adxl367_set_fifo_watermark(struct adxl367_state *st,
static int adxl367_set_range(struct iio_dev *indio_dev,
enum adxl367_range range)
{
- struct adxl367_state *st = iio_priv(indio_dev);
- int ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct adxl367_state *st = iio_priv(indio_dev);
+ int ret;
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- mutex_lock(&st->lock);
-
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- goto out;
+ guard(mutex)(&st->lock);
- ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
- ADXL367_FILTER_CTL_RANGE_MASK,
- FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
- range));
- if (ret)
- goto out;
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
- adxl367_scale_act_thresholds(st, st->range, range);
+ ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
+ ADXL367_FILTER_CTL_RANGE_MASK,
+ FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
+ range));
+ if (ret)
+ return ret;
- /* Activity thresholds depend on range */
- ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
- st->act_threshold);
- if (ret)
- goto out;
+ adxl367_scale_act_thresholds(st, st->range, range);
- ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
- st->inact_threshold);
- if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
- if (ret)
- goto out;
+ /* Activity thresholds depend on range */
+ ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
+ st->act_threshold);
+ if (ret)
+ return ret;
- st->range = range;
+ ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
+ st->inact_threshold);
+ if (ret)
+ return ret;
-out:
- mutex_unlock(&st->lock);
+ ret = adxl367_set_measure_en(st, true);
+ if (ret)
+ return ret;
- iio_device_release_direct_mode(indio_dev);
+ st->range = range;
- return ret;
+ return 0;
+ }
+ unreachable();
}
static int adxl367_time_ms_to_samples(struct adxl367_state *st, unsigned int ms)
@@ -587,11 +576,11 @@ static int adxl367_set_act_time_ms(struct adxl367_state *st,
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
if (act == ADXL367_ACTIVITY)
ret = _adxl367_set_act_time_ms(st, ms);
@@ -599,14 +588,9 @@ static int adxl367_set_act_time_ms(struct adxl367_state *st,
ret = _adxl367_set_inact_time_ms(st, ms);
if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
}
static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
@@ -636,31 +620,23 @@ static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
{
- struct adxl367_state *st = iio_priv(indio_dev);
- int ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct adxl367_state *st = iio_priv(indio_dev);;
+ int ret;
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
+ guard(mutex)(&st->lock);
- mutex_lock(&st->lock);
-
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- goto out;
-
- ret = _adxl367_set_odr(st, odr);
- if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
-
-out:
- mutex_unlock(&st->lock);
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
- iio_device_release_direct_mode(indio_dev);
+ ret = _adxl367_set_odr(st, odr);
+ if (ret)
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
+ }
+ unreachable();
}
static int adxl367_set_temp_adc_en(struct adxl367_state *st, unsigned int reg,
@@ -749,36 +725,32 @@ static int adxl367_read_sample(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val)
{
- struct adxl367_state *st = iio_priv(indio_dev);
- u16 sample;
- int ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct adxl367_state *st = iio_priv(indio_dev);
+ u16 sample;
+ int ret;
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
+ guard(mutex)(&st->lock);
- mutex_lock(&st->lock);
-
- ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
- if (ret)
- goto out;
-
- ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
- sizeof(st->sample_buf));
- if (ret)
- goto out;
-
- sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
- *val = sign_extend32(sample, chan->scan_type.realbits - 1);
+ ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
+ if (ret)
+ return ret;
- ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
+ ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
+ sizeof(st->sample_buf));
+ if (ret)
+ return ret;
-out:
- mutex_unlock(&st->lock);
+ sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
+ *val = sign_extend32(sample, chan->scan_type.realbits - 1);
- iio_device_release_direct_mode(indio_dev);
+ ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
+ if (ret)
+ return ret;
- return ret ?: IIO_VAL_INT;
+ return IIO_VAL_INT;
+ }
+ unreachable();
}
static int adxl367_get_status(struct adxl367_state *st, u8 *status,
@@ -886,12 +858,12 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
return adxl367_read_sample(indio_dev, chan, val);
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
- case IIO_ACCEL:
- mutex_lock(&st->lock);
+ case IIO_ACCEL: {
+ guard(mutex)(&st->lock);
*val = adxl367_range_scale_tbl[st->range][0];
*val2 = adxl367_range_scale_tbl[st->range][1];
- mutex_unlock(&st->lock);
return IIO_VAL_INT_PLUS_NANO;
+ }
case IIO_TEMP:
*val = 1000;
*val2 = ADXL367_TEMP_PER_C;
@@ -914,12 +886,12 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
- case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_SAMP_FREQ: {
+ guard(mutex)(&st->lock);
*val = adxl367_samp_freq_tbl[st->odr][0];
*val2 = adxl367_samp_freq_tbl[st->odr][1];
- mutex_unlock(&st->lock);
return IIO_VAL_INT_PLUS_MICRO;
+ }
default:
return -EINVAL;
}
@@ -1004,18 +976,15 @@ static int adxl367_read_event_value(struct iio_dev *indio_dev,
{
struct adxl367_state *st = iio_priv(indio_dev);
+ guard(mutex)(&st->lock);
switch (info) {
case IIO_EV_INFO_VALUE: {
switch (dir) {
case IIO_EV_DIR_RISING:
- mutex_lock(&st->lock);
*val = st->act_threshold;
- mutex_unlock(&st->lock);
return IIO_VAL_INT;
case IIO_EV_DIR_FALLING:
- mutex_lock(&st->lock);
*val = st->inact_threshold;
- mutex_unlock(&st->lock);
return IIO_VAL_INT;
default:
return -EINVAL;
@@ -1024,15 +993,11 @@ static int adxl367_read_event_value(struct iio_dev *indio_dev,
case IIO_EV_INFO_PERIOD:
switch (dir) {
case IIO_EV_DIR_RISING:
- mutex_lock(&st->lock);
*val = st->act_time_ms;
- mutex_unlock(&st->lock);
*val2 = 1000;
return IIO_VAL_FRACTIONAL;
case IIO_EV_DIR_FALLING:
- mutex_lock(&st->lock);
*val = st->inact_time_ms;
- mutex_unlock(&st->lock);
*val2 = 1000;
return IIO_VAL_FRACTIONAL;
default:
@@ -1110,9 +1075,7 @@ static int adxl367_write_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir,
int state)
{
- struct adxl367_state *st = iio_priv(indio_dev);
enum adxl367_activity_type act;
- int ret;
switch (dir) {
case IIO_EV_DIR_RISING:
@@ -1125,33 +1088,28 @@ static int adxl367_write_event_config(struct iio_dev *indio_dev,
return -EINVAL;
}
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- mutex_lock(&st->lock);
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct adxl367_state *st = iio_priv(indio_dev);
+ int ret;
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- goto out;
+ guard(mutex)(&st->lock);
- ret = adxl367_set_act_interrupt_en(st, act, state);
- if (ret)
- goto out;
-
- ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
- : ADXL367_ACT_DISABLED);
- if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
-out:
- mutex_unlock(&st->lock);
+ ret = adxl367_set_act_interrupt_en(st, act, state);
+ if (ret)
+ return ret;
- iio_device_release_direct_mode(indio_dev);
+ ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
+ : ADXL367_ACT_DISABLED);
+ if (ret)
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
+ }
+ unreachable();
}
static ssize_t adxl367_get_fifo_enabled(struct device *dev,
@@ -1176,9 +1134,8 @@ static ssize_t adxl367_get_fifo_watermark(struct device *dev,
struct adxl367_state *st = iio_priv(dev_to_iio_dev(dev));
unsigned int fifo_watermark;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
fifo_watermark = st->fifo_watermark;
- mutex_unlock(&st->lock);
return sysfs_emit(buf, "%d\n", fifo_watermark);
}
@@ -1207,22 +1164,17 @@ static int adxl367_set_watermark(struct iio_dev *indio_dev, unsigned int val)
if (val > ADXL367_FIFO_MAX_WATERMARK)
return -EINVAL;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_watermark(st, val);
if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
}
static bool adxl367_find_mask_fifo_format(const unsigned long *scan_mask,
@@ -1253,27 +1205,24 @@ static int adxl367_update_scan_mode(struct iio_dev *indio_dev,
if (!adxl367_find_mask_fifo_format(active_scan_mask, &fifo_format))
return -EINVAL;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_format(st, fifo_format);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_measure_en(st, true);
if (ret)
- goto out;
+ return ret;
st->fifo_set_size = bitmap_weight(active_scan_mask,
indio_dev->masklength);
-out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static int adxl367_buffer_postenable(struct iio_dev *indio_dev)
@@ -1281,31 +1230,26 @@ static int adxl367_buffer_postenable(struct iio_dev *indio_dev)
struct adxl367_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
true);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_watermark_interrupt_en(st, true);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_mode(st, ADXL367_FIFO_MODE_STREAM);
if (ret)
- goto out;
-
- ret = adxl367_set_measure_en(st, true);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return adxl367_set_measure_en(st, true);
}
static int adxl367_buffer_predisable(struct iio_dev *indio_dev)
@@ -1313,31 +1257,26 @@ static int adxl367_buffer_predisable(struct iio_dev *indio_dev)
struct adxl367_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = adxl367_set_measure_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_mode(st, ADXL367_FIFO_MODE_DISABLED);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_fifo_watermark_interrupt_en(st, false);
if (ret)
- goto out;
+ return ret;
ret = adxl367_set_measure_en(st, true);
if (ret)
- goto out;
-
- ret = adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
- false);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
+ false);
}
static const struct iio_buffer_setup_ops adxl367_buffer_ops = {
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode.
2024-01-28 15:05 ` [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode Jonathan Cameron
@ 2024-01-30 12:53 ` Nuno Sá
2024-02-04 13:48 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Nuno Sá @ 2024-01-30 12:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
On Sun, 2024-01-28 at 15:05 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Switching to the iio_device_claim_direct_scoped() for state
> and to guard() based unlocking of mutexes simplifies error handling
> by allowing direct returns when an error is encountered.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Since RFC:
> - Use unreachable() to stop complier moaning if we have
>
> iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> return 0;
> }
> unreachable(); /* can't get here */
> - Update some code from earlier attempt to handle this that was left
> behind from before iio_device_claim_direct_scoped()
> - Reduce scope of some local variables only used within
> iio_device_claim_direct_scoped() blocks.
> ---
> drivers/iio/accel/adxl367.c | 297 ++++++++++++++----------------------
> 1 file changed, 118 insertions(+), 179 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index 90b7ae6d42b7..834ee6d63947 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c
> @@ -339,22 +339,17 @@ static int adxl367_set_act_threshold(struct
> adxl367_state *st,
> {
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
>
> ret = adxl367_set_measure_en(st, false);
> if (ret)
> - goto out;
> + return ret;
>
> ret = _adxl367_set_act_threshold(st, act, threshold);
> if (ret)
> - goto out;
> -
> - ret = adxl367_set_measure_en(st, true);
> -
> -out:
> - mutex_unlock(&st->lock);
> + return ret;
>
> - return ret;
> + return adxl367_set_measure_en(st, true);
> }
>
> static int adxl367_set_act_proc_mode(struct adxl367_state *st,
> @@ -482,51 +477,45 @@ static int adxl367_set_fifo_watermark(struct
> adxl367_state *st,
> static int adxl367_set_range(struct iio_dev *indio_dev,
> enum adxl367_range range)
> {
> - struct adxl367_state *st = iio_priv(indio_dev);
> - int ret;
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + struct adxl367_state *st = iio_priv(indio_dev);
> + int ret;
>
> - ret = iio_device_claim_direct_mode(indio_dev);
> - if (ret)
> - return ret;
> -
> - mutex_lock(&st->lock);
> -
> - ret = adxl367_set_measure_en(st, false);
> - if (ret)
> - goto out;
> + guard(mutex)(&st->lock);
>
> - ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
> - ADXL367_FILTER_CTL_RANGE_MASK,
> - FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
> - range));
> - if (ret)
> - goto out;
> + ret = adxl367_set_measure_en(st, false);
> + if (ret)
> + return ret;
>
> - adxl367_scale_act_thresholds(st, st->range, range);
> + ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
> + ADXL367_FILTER_CTL_RANGE_MASK,
> +
> FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
> + range));
> + if (ret)
> + return ret;
>
> - /* Activity thresholds depend on range */
> - ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
> - st->act_threshold);
> - if (ret)
> - goto out;
> + adxl367_scale_act_thresholds(st, st->range, range);
>
> - ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
> - st->inact_threshold);
> - if (ret)
> - goto out;
> -
> - ret = adxl367_set_measure_en(st, true);
> - if (ret)
> - goto out;
> + /* Activity thresholds depend on range */
> + ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
> + st->act_threshold);
> + if (ret)
> + return ret;
>
> - st->range = range;
> + ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
> + st->inact_threshold);
> + if (ret)
> + return ret;
>
> -out:
> - mutex_unlock(&st->lock);
> + ret = adxl367_set_measure_en(st, true);
> + if (ret)
> + return ret;
>
> - iio_device_release_direct_mode(indio_dev);
> + st->range = range;
>
> - return ret;
> + return 0;
> + }
> + unreachable();
> }
I do agree this is irritating. Personally I would prefer to return 0 (or the
last ret value) instead of the unusual unreachable() builtin. But that's me :)
- Nuno Sá
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode.
2024-01-30 12:53 ` Nuno Sá
@ 2024-02-04 13:48 ` Jonathan Cameron
2024-02-05 8:27 ` Nuno Sá
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2024-02-04 13:48 UTC (permalink / raw)
To: Nuno Sá; +Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
> > -out:
> > - mutex_unlock(&st->lock);
> > + ret = adxl367_set_measure_en(st, true);
> > + if (ret)
> > + return ret;
> >
> > - iio_device_release_direct_mode(indio_dev);
> > + st->range = range;
> >
> > - return ret;
> > + return 0;
> > + }
> > + unreachable();
> > }
>
> I do agree this is irritating. Personally I would prefer to return 0 (or the
> last ret value) instead of the unusual unreachable() builtin. But that's me :)
Definitely would be an error, not 0 or ret, but I'm still in two minds about this
in general. I think I'll go with unreachable for now and see if we get an
push back.
>
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode.
2024-02-04 13:48 ` Jonathan Cameron
@ 2024-02-05 8:27 ` Nuno Sá
0 siblings, 0 replies; 24+ messages in thread
From: Nuno Sá @ 2024-02-05 8:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
On Sun, 2024-02-04 at 13:48 +0000, Jonathan Cameron wrote:
>
> > > -out:
> > > - mutex_unlock(&st->lock);
> > > + ret = adxl367_set_measure_en(st, true);
> > > + if (ret)
> > > + return ret;
> > >
> > > - iio_device_release_direct_mode(indio_dev);
> > > + st->range = range;
> > >
> > > - return ret;
> > > + return 0;
> > > + }
> > > + unreachable();
> > > }
> >
> > I do agree this is irritating. Personally I would prefer to return 0 (or the
> > last ret value) instead of the unusual unreachable() builtin. But that's me :)
> Definitely would be an error, not 0 or ret, but I'm still in two minds about this
Agreed with that... In theory, would just be about making the compiler happy.
- Nuno Sá
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 04/10] iio: imu: bmi323: Use cleanup handling for iio_device_claim_direct_mode()
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (2 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 03/10] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 05/10] iio: adc: max1363: Use automatic cleanup for locks and iio mode claiming Jonathan Cameron
` (6 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Similar to existing use of guard() in this driver,
iio_device_claim_direct_scoped() will ensure that scope based cleanup
occurs.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Use unreachable() to avoid false warning from compiler in the
pattern
iio_device_claim_direct_scoped(return -EBUSY, iio_dev) {
return 0;
}
unreachable();
- Switch one guard_scoped(mutex,..) to guard(mutex) to avoid need for
an unreachable() marking (ended up a little neater).
---
drivers/iio/imu/bmi323/bmi323_core.c | 78 +++++++++++-----------------
1 file changed, 31 insertions(+), 47 deletions(-)
diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
index 183af482828f..5d42ab9b176a 100644
--- a/drivers/iio/imu/bmi323/bmi323_core.c
+++ b/drivers/iio/imu/bmi323/bmi323_core.c
@@ -1668,52 +1668,41 @@ static int bmi323_write_raw(struct iio_dev *indio_dev,
int val2, long mask)
{
struct bmi323_data *data = iio_priv(indio_dev);
- int ret;
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
- val, val2);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return bmi323_set_odr(data,
+ bmi323_iio_to_sensor(chan->type),
+ val, val2);
+ unreachable();
case IIO_CHAN_INFO_SCALE:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
- val, val2);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return bmi323_set_scale(data,
+ bmi323_iio_to_sensor(chan->type),
+ val, val2);
+ unreachable();
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = bmi323_set_average(data, bmi323_iio_to_sensor(chan->type),
- val);
-
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return bmi323_set_average(data,
+ bmi323_iio_to_sensor(chan->type),
+ val);
+ unreachable();
case IIO_CHAN_INFO_ENABLE:
return bmi323_enable_steps(data, val);
- case IIO_CHAN_INFO_PROCESSED:
- scoped_guard(mutex, &data->mutex) {
- if (val || !FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
- data->feature_events))
- return -EINVAL;
+ case IIO_CHAN_INFO_PROCESSED: {
+ guard(mutex)(&data->mutex);
- /* Clear step counter value */
- ret = bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
- BMI323_STEP_SC1_RST_CNT_MSK,
- FIELD_PREP(BMI323_STEP_SC1_RST_CNT_MSK,
- 1));
- }
- return ret;
+ if (val || !FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
+ data->feature_events))
+ return -EINVAL;
+
+ /* Clear step counter value */
+ return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
+ BMI323_STEP_SC1_RST_CNT_MSK,
+ FIELD_PREP(BMI323_STEP_SC1_RST_CNT_MSK,
+ 1));
+ }
default:
return -EINVAL;
}
@@ -1724,7 +1713,6 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
int *val2, long mask)
{
struct bmi323_data *data = iio_priv(indio_dev);
- int ret;
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
@@ -1733,14 +1721,10 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_ACCEL:
case IIO_ANGL_VEL:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = bmi323_read_axis(data, chan, val);
-
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY,
+ indio_dev)
+ return bmi323_read_axis(data, chan, val);
+ unreachable();
case IIO_TEMP:
return bmi323_get_temp_data(data, val);
default:
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 05/10] iio: adc: max1363: Use automatic cleanup for locks and iio mode claiming.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (3 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 04/10] iio: imu: bmi323: Use cleanup handling for iio_device_claim_direct_mode() Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 06/10] iio: proximity: sx9360: Use automated cleanup for locks and IIO " Jonathan Cameron
` (5 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This simplifies error return paths.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Reduce scope of some local variables to within
iio_device_claim_direct_scoped() covered blocks.
- Don't initialize ret in a few paths where it's never used without
being assigned.
- Use unreachable in one case to make the code flow to return 0 more
obvious (only if claiming direct mode succeeds).
---
drivers/iio/adc/max1363.c | 171 ++++++++++++++++++--------------------
1 file changed, 79 insertions(+), 92 deletions(-)
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 7c2a98b8c3a9..8b5bc96cb9fb 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -357,62 +357,55 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
int *val,
long m)
{
- int ret = 0;
- s32 data;
- u8 rxbuf[2];
- struct max1363_state *st = iio_priv(indio_dev);
- struct i2c_client *client = st->client;
-
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
- mutex_lock(&st->lock);
-
- /*
- * If monitor mode is enabled, the method for reading a single
- * channel will have to be rather different and has not yet
- * been implemented.
- *
- * Also, cannot read directly if buffered capture enabled.
- */
- if (st->monitor_on) {
- ret = -EBUSY;
- goto error_ret;
- }
-
- /* Check to see if current scan mode is correct */
- if (st->current_mode != &max1363_mode_table[chan->address]) {
- /* Update scan mode if needed */
- st->current_mode = &max1363_mode_table[chan->address];
- ret = max1363_set_scan_mode(st);
- if (ret < 0)
- goto error_ret;
- }
- if (st->chip_info->bits != 8) {
- /* Get reading */
- data = st->recv(client, rxbuf, 2);
- if (data < 0) {
- ret = data;
- goto error_ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ s32 data;
+ u8 rxbuf[2];
+ struct max1363_state *st = iio_priv(indio_dev);
+ struct i2c_client *client = st->client;
+
+ guard(mutex)(&st->lock);
+
+ /*
+ * If monitor mode is enabled, the method for reading a single
+ * channel will have to be rather different and has not yet
+ * been implemented.
+ *
+ * Also, cannot read directly if buffered capture enabled.
+ */
+ if (st->monitor_on)
+ return -EBUSY;
+
+ /* Check to see if current scan mode is correct */
+ if (st->current_mode != &max1363_mode_table[chan->address]) {
+ int ret;
+
+ /* Update scan mode if needed */
+ st->current_mode = &max1363_mode_table[chan->address];
+ ret = max1363_set_scan_mode(st);
+ if (ret < 0)
+ return ret;
}
- data = (rxbuf[1] | rxbuf[0] << 8) &
- ((1 << st->chip_info->bits) - 1);
- } else {
- /* Get reading */
- data = st->recv(client, rxbuf, 1);
- if (data < 0) {
- ret = data;
- goto error_ret;
+ if (st->chip_info->bits != 8) {
+ /* Get reading */
+ data = st->recv(client, rxbuf, 2);
+ if (data < 0)
+ return data;
+
+ data = (rxbuf[1] | rxbuf[0] << 8) &
+ ((1 << st->chip_info->bits) - 1);
+ } else {
+ /* Get reading */
+ data = st->recv(client, rxbuf, 1);
+ if (data < 0)
+ return data;
+
+ data = rxbuf[0];
}
- data = rxbuf[0];
- }
- *val = data;
-
-error_ret:
- mutex_unlock(&st->lock);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ *val = data;
+ return 0;
+ }
+ unreachable();
}
static int max1363_read_raw(struct iio_dev *indio_dev,
@@ -710,9 +703,8 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
if (!found)
return -EINVAL;
- mutex_lock(&st->lock);
- st->monitor_speed = i;
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock)
+ st->monitor_speed = i;
return 0;
}
@@ -815,12 +807,11 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
int val;
int number = chan->channel;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (dir == IIO_EV_DIR_FALLING)
val = (1 << number) & st->mask_low;
else
val = (1 << number) & st->mask_high;
- mutex_unlock(&st->lock);
return val;
}
@@ -962,46 +953,42 @@ static int max1363_write_event_config(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, enum iio_event_type type,
enum iio_event_direction dir, int state)
{
- int ret = 0;
struct max1363_state *st = iio_priv(indio_dev);
- u16 unifiedmask;
- int number = chan->channel;
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
- mutex_lock(&st->lock);
-
- unifiedmask = st->mask_low | st->mask_high;
- if (dir == IIO_EV_DIR_FALLING) {
-
- if (state == 0)
- st->mask_low &= ~(1 << number);
- else {
- ret = __max1363_check_event_mask((1 << number),
- unifiedmask);
- if (ret)
- goto error_ret;
- st->mask_low |= (1 << number);
- }
- } else {
- if (state == 0)
- st->mask_high &= ~(1 << number);
- else {
- ret = __max1363_check_event_mask((1 << number),
- unifiedmask);
- if (ret)
- goto error_ret;
- st->mask_high |= (1 << number);
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ int number = chan->channel;
+ u16 unifiedmask;
+ int ret;
+
+ guard(mutex)(&st->lock);
+
+ unifiedmask = st->mask_low | st->mask_high;
+ if (dir == IIO_EV_DIR_FALLING) {
+
+ if (state == 0)
+ st->mask_low &= ~(1 << number);
+ else {
+ ret = __max1363_check_event_mask((1 << number),
+ unifiedmask);
+ if (ret)
+ return ret;
+ st->mask_low |= (1 << number);
+ }
+ } else {
+ if (state == 0)
+ st->mask_high &= ~(1 << number);
+ else {
+ ret = __max1363_check_event_mask((1 << number),
+ unifiedmask);
+ if (ret)
+ return ret;
+ st->mask_high |= (1 << number);
+ }
}
}
-
max1363_monitor_mode_update(st, !!(st->mask_high | st->mask_low));
-error_ret:
- mutex_unlock(&st->lock);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ return 0;
}
/*
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 06/10] iio: proximity: sx9360: Use automated cleanup for locks and IIO mode claiming.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (4 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 05/10] iio: adc: max1363: Use automatic cleanup for locks and iio mode claiming Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 07/10] iio: proximity: sx9324: " Jonathan Cameron
` (4 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This simplifies error handling paths and generallly removes a bunch
of boilerplate.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Changes since RFC:
- use unreachable() to convince compiler we can't reach paths beyond
iio_device_claim_direct_scoped(return -EBUSY, iio_dev) {
return 0;
}
unreachable();
- reduce scope of int ret in resume callback.
---
drivers/iio/proximity/sx9360.c | 115 +++++++++++----------------------
1 file changed, 39 insertions(+), 76 deletions(-)
diff --git a/drivers/iio/proximity/sx9360.c b/drivers/iio/proximity/sx9360.c
index 2c4e14a4fe9f..75a1c29f14eb 100644
--- a/drivers/iio/proximity/sx9360.c
+++ b/drivers/iio/proximity/sx9360.c
@@ -322,25 +322,16 @@ static int sx9360_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
- int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx_common_read_proximity(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx_common_read_proximity(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_HARDWAREGAIN:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx9360_read_gain(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx9360_read_gain(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9360_read_samp_freq(data, val, val2);
default:
@@ -387,19 +378,15 @@ static int sx9360_read_avail(struct iio_dev *indio_dev,
static int sx9360_set_samp_freq(struct sx_common_data *data,
int val, int val2)
{
- int ret, reg;
+ int reg;
__be16 buf;
reg = val * 8192 / SX9360_FOSC_HZ + val2 * 8192 / (SX9360_FOSC_MHZ);
buf = cpu_to_be16(reg);
- mutex_lock(&data->mutex);
-
- ret = regmap_bulk_write(data->regmap, SX9360_REG_GNRL_CTRL1, &buf,
- sizeof(buf));
+ guard(mutex)(&data->mutex);
- mutex_unlock(&data->mutex);
-
- return ret;
+ return regmap_bulk_write(data->regmap, SX9360_REG_GNRL_CTRL1, &buf,
+ sizeof(buf));
}
static int sx9360_read_thresh(struct sx_common_data *data, int *val)
@@ -510,7 +497,6 @@ static int sx9360_read_event_val(struct iio_dev *indio_dev,
static int sx9360_write_thresh(struct sx_common_data *data, int _val)
{
unsigned int val = _val;
- int ret;
if (val >= 1)
val = int_sqrt(2 * val);
@@ -518,11 +504,8 @@ static int sx9360_write_thresh(struct sx_common_data *data, int _val)
if (val > 0xff)
return -EINVAL;
- mutex_lock(&data->mutex);
- ret = regmap_write(data->regmap, SX9360_REG_PROX_CTRL5, val);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_write(data->regmap, SX9360_REG_PROX_CTRL5, val);
}
static int sx9360_write_hysteresis(struct sx_common_data *data, int _val)
@@ -546,18 +529,14 @@ static int sx9360_write_hysteresis(struct sx_common_data *data, int _val)
return -EINVAL;
hyst = FIELD_PREP(SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
- SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+ SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
}
static int sx9360_write_far_debounce(struct sx_common_data *data, int _val)
{
unsigned int regval, val = _val;
- int ret;
if (val > 0)
val = ilog2(val);
@@ -566,19 +545,15 @@ static int sx9360_write_far_debounce(struct sx_common_data *data, int _val)
regval = FIELD_PREP(SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
- SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+ SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK,
+ regval);
}
static int sx9360_write_close_debounce(struct sx_common_data *data, int _val)
{
unsigned int regval, val = _val;
- int ret;
if (val > 0)
val = ilog2(val);
@@ -587,13 +562,10 @@ static int sx9360_write_close_debounce(struct sx_common_data *data, int _val)
regval = FIELD_PREP(SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
- SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+ SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK,
+ regval);
}
static int sx9360_write_event_val(struct iio_dev *indio_dev,
@@ -630,19 +602,15 @@ static int sx9360_write_gain(struct sx_common_data *data,
const struct iio_chan_spec *chan, int val)
{
unsigned int gain, reg;
- int ret;
gain = ilog2(val);
reg = SX9360_REG_PROX_CTRL0_PHR + chan->channel;
gain = FIELD_PREP(SX9360_REG_PROX_CTRL0_GAIN_MASK, gain);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, reg,
- SX9360_REG_PROX_CTRL0_GAIN_MASK,
- gain);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, reg,
+ SX9360_REG_PROX_CTRL0_GAIN_MASK,
+ gain);
}
static int sx9360_write_raw(struct iio_dev *indio_dev,
@@ -827,36 +795,31 @@ static int sx9360_suspend(struct device *dev)
disable_irq_nosync(data->client->irq);
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = regmap_read(data->regmap, SX9360_REG_GNRL_CTRL0, ®val);
+ if (ret < 0)
+ return ret;
data->suspend_ctrl =
FIELD_GET(SX9360_REG_GNRL_CTRL0_PHEN_MASK, regval);
- if (ret < 0)
- goto out;
/* Disable all phases, send the device to sleep. */
- ret = regmap_write(data->regmap, SX9360_REG_GNRL_CTRL0, 0);
-
-out:
- mutex_unlock(&data->mutex);
- return ret;
+ return regmap_write(data->regmap, SX9360_REG_GNRL_CTRL0, 0);
}
static int sx9360_resume(struct device *dev)
{
struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
- int ret;
-
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9360_REG_GNRL_CTRL0,
- SX9360_REG_GNRL_CTRL0_PHEN_MASK,
- data->suspend_ctrl);
- mutex_unlock(&data->mutex);
- if (ret)
- return ret;
+ scoped_guard(mutex, &data->mutex) {
+ int ret = regmap_update_bits(data->regmap,
+ SX9360_REG_GNRL_CTRL0,
+ SX9360_REG_GNRL_CTRL0_PHEN_MASK,
+ data->suspend_ctrl);
+ if (ret)
+ return ret;
+ }
enable_irq(data->client->irq);
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 07/10] iio: proximity: sx9324: Use automated cleanup for locks and IIO mode claiming.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (5 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 06/10] iio: proximity: sx9360: Use automated cleanup for locks and IIO " Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 08/10] iio: proximity: sx9310: " Jonathan Cameron
` (3 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This simplifies error handling paths and generallly removes a bunch
of boilerplate.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Use unreachable() rather than return -EINVAL; to convince compiler
that the code is really unreachable and stop it complaining.
- Reduce scope of int ret in resume() callback.
---
drivers/iio/proximity/sx9324.c | 109 ++++++++++++---------------------
1 file changed, 39 insertions(+), 70 deletions(-)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index ac2ed2da21cc..3bf51bd2ec21 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -429,25 +429,16 @@ static int sx9324_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
- int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx_common_read_proximity(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx_common_read_proximity(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_HARDWAREGAIN:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx9324_read_gain(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx9324_read_gain(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9324_read_samp_freq(data, val, val2);
default:
@@ -484,7 +475,7 @@ static int sx9324_read_avail(struct iio_dev *indio_dev,
static int sx9324_set_samp_freq(struct sx_common_data *data,
int val, int val2)
{
- int i, ret;
+ int i;
for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
if (val == sx9324_samp_freq_table[i].val &&
@@ -494,15 +485,11 @@ static int sx9324_set_samp_freq(struct sx_common_data *data,
if (i == ARRAY_SIZE(sx9324_samp_freq_table))
return -EINVAL;
- mutex_lock(&data->mutex);
-
- ret = regmap_update_bits(data->regmap,
- SX9324_REG_GNRL_CTRL0,
- SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
+ guard(mutex)(&data->mutex);
- mutex_unlock(&data->mutex);
-
- return ret;
+ return regmap_update_bits(data->regmap,
+ SX9324_REG_GNRL_CTRL0,
+ SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
}
static int sx9324_read_thresh(struct sx_common_data *data,
@@ -623,7 +610,6 @@ static int sx9324_write_thresh(struct sx_common_data *data,
const struct iio_chan_spec *chan, int _val)
{
unsigned int reg, val = _val;
- int ret;
reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
@@ -633,11 +619,9 @@ static int sx9324_write_thresh(struct sx_common_data *data,
if (val > 0xff)
return -EINVAL;
- mutex_lock(&data->mutex);
- ret = regmap_write(data->regmap, reg, val);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return regmap_write(data->regmap, reg, val);
}
static int sx9324_write_hysteresis(struct sx_common_data *data,
@@ -662,18 +646,15 @@ static int sx9324_write_hysteresis(struct sx_common_data *data,
return -EINVAL;
hyst = FIELD_PREP(SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
- SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
}
static int sx9324_write_far_debounce(struct sx_common_data *data, int _val)
{
unsigned int regval, val = _val;
- int ret;
if (val > 0)
val = ilog2(val);
@@ -682,19 +663,16 @@ static int sx9324_write_far_debounce(struct sx_common_data *data, int _val)
regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
- SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
+ regval);
}
static int sx9324_write_close_debounce(struct sx_common_data *data, int _val)
{
unsigned int regval, val = _val;
- int ret;
if (val > 0)
val = ilog2(val);
@@ -703,13 +681,11 @@ static int sx9324_write_close_debounce(struct sx_common_data *data, int _val)
regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
- SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
+ regval);
}
static int sx9324_write_event_val(struct iio_dev *indio_dev,
@@ -746,7 +722,6 @@ static int sx9324_write_gain(struct sx_common_data *data,
const struct iio_chan_spec *chan, int val)
{
unsigned int gain, reg;
- int ret;
reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
@@ -756,13 +731,11 @@ static int sx9324_write_gain(struct sx_common_data *data,
gain = FIELD_PREP(SX9324_REG_PROX_CTRL0_GAIN_MASK, gain);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, reg,
- SX9324_REG_PROX_CTRL0_GAIN_MASK,
- gain);
- mutex_unlock(&data->mutex);
+ guard(mutex)(&data->mutex);
- return ret;
+ return regmap_update_bits(data->regmap, reg,
+ SX9324_REG_PROX_CTRL0_GAIN_MASK,
+ gain);
}
static int sx9324_write_raw(struct iio_dev *indio_dev,
@@ -1081,34 +1054,30 @@ static int sx9324_suspend(struct device *dev)
disable_irq_nosync(data->client->irq);
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL1, ®val);
+ if (ret < 0)
+ return ret;
data->suspend_ctrl =
FIELD_GET(SX9324_REG_GNRL_CTRL1_PHEN_MASK, regval);
- if (ret < 0)
- goto out;
/* Disable all phases, send the device to sleep. */
- ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
-
-out:
- mutex_unlock(&data->mutex);
- return ret;
+ return regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
}
static int sx9324_resume(struct device *dev)
{
struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
- int ret;
- mutex_lock(&data->mutex);
- ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
- data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
- mutex_unlock(&data->mutex);
- if (ret)
- return ret;
+ scoped_guard(mutex, &data->mutex) {
+ int ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
+ data->suspend_ctrl |
+ SX9324_REG_GNRL_CTRL1_PAUSECTRL);
+ if (ret)
+ return ret;
+ }
enable_irq(data->client->irq);
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 08/10] iio: proximity: sx9310: Use automated cleanup for locks and IIO mode claiming.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (6 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 07/10] iio: proximity: sx9324: " Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 09/10] iio: adc: ad4130: Use automatic cleanup of locks and direct mode Jonathan Cameron
` (2 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This simplifies error handling paths and generallly removes a bunch
of boilerplate.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Use unreachable() to convince compiler of paths we can't get to rather
than adding pointless returns.
---
drivers/iio/proximity/sx9310.c | 114 +++++++++++----------------------
1 file changed, 39 insertions(+), 75 deletions(-)
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 0d230a0dff56..427c9343d6d1 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -337,28 +337,19 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
- int ret;
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx_common_read_proximity(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx_common_read_proximity(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_HARDWAREGAIN:
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
-
- ret = sx9310_read_gain(data, chan, val);
- iio_device_release_direct_mode(indio_dev);
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return sx9310_read_gain(data, chan, val);
+ unreachable();
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9310_read_samp_freq(data, val, val2);
default:
@@ -546,12 +537,10 @@ static int sx9310_write_thresh(struct sx_common_data *data,
return -EINVAL;
regval = FIELD_PREP(SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, reg,
- SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
- mutex_unlock(&data->mutex);
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, reg,
+ SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
}
static int sx9310_write_hysteresis(struct sx_common_data *data,
@@ -576,17 +565,14 @@ static int sx9310_write_hysteresis(struct sx_common_data *data,
return -EINVAL;
hyst = FIELD_PREP(SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
- SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
- mutex_unlock(&data->mutex);
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+ SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
}
static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
{
- int ret;
unsigned int regval;
if (val > 0)
@@ -596,18 +582,14 @@ static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
regval = FIELD_PREP(SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
- SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+ SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK,
+ regval);
}
static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
{
- int ret;
unsigned int regval;
if (val > 0)
@@ -617,13 +599,10 @@ static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
regval = FIELD_PREP(SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK, val);
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
- SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK,
- regval);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+ SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK,
+ regval);
}
static int sx9310_write_event_val(struct iio_dev *indio_dev,
@@ -658,7 +637,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
{
- int i, ret;
+ int i;
for (i = 0; i < ARRAY_SIZE(sx9310_samp_freq_table); i++)
if (val == sx9310_samp_freq_table[i].val &&
@@ -668,23 +647,17 @@ static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
if (i == ARRAY_SIZE(sx9310_samp_freq_table))
return -EINVAL;
- mutex_lock(&data->mutex);
-
- ret = regmap_update_bits(
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(
data->regmap, SX9310_REG_PROX_CTRL0,
SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK,
FIELD_PREP(SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK, i));
-
- mutex_unlock(&data->mutex);
-
- return ret;
}
static int sx9310_write_gain(struct sx_common_data *data,
const struct iio_chan_spec *chan, int val)
{
unsigned int gain, mask;
- int ret;
gain = ilog2(val);
@@ -703,12 +676,9 @@ static int sx9310_write_gain(struct sx_common_data *data,
return -EINVAL;
}
- mutex_lock(&data->mutex);
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL3, mask,
- gain);
- mutex_unlock(&data->mutex);
-
- return ret;
+ guard(mutex)(&data->mutex);
+ return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL3, mask,
+ gain);
}
static int sx9310_write_raw(struct iio_dev *indio_dev,
@@ -969,22 +939,18 @@ static int sx9310_suspend(struct device *dev)
disable_irq_nosync(data->client->irq);
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
&data->suspend_ctrl);
if (ret)
- goto out;
+ return ret;
ctrl0 = data->suspend_ctrl & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
if (ret)
- goto out;
-
- ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
+ return ret;
-out:
- mutex_unlock(&data->mutex);
- return ret;
+ return regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
}
static int sx9310_resume(struct device *dev)
@@ -992,18 +958,16 @@ static int sx9310_resume(struct device *dev)
struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
int ret;
- mutex_lock(&data->mutex);
- ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
- if (ret)
- goto out;
-
- ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
- data->suspend_ctrl);
+ scoped_guard(mutex, &data->mutex) {
+ ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
+ if (ret)
+ return ret;
-out:
- mutex_unlock(&data->mutex);
- if (ret)
- return ret;
+ ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
+ data->suspend_ctrl);
+ if (ret)
+ return ret;
+ }
enable_irq(data->client->irq);
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 09/10] iio: adc: ad4130: Use automatic cleanup of locks and direct mode.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (7 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 08/10] iio: proximity: sx9310: " Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-28 15:05 ` [PATCH 10/10] iio: adc: ad7091r-base: Use auto cleanup of locks Jonathan Cameron
2024-01-30 14:08 ` [PATCH 00/10] IIO: Use the new cleanup.h magic Nuno Sá
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reduces boilerplate and allows for simpler to follow direct returns.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Since RFC:
- Reduced scope of local variable.
- Used unreachable() to inform compiler some paths are never reached and
avoid need for missleading return -EINVAL; statements.
---
drivers/iio/adc/ad4130.c | 131 ++++++++++++++++-----------------------
1 file changed, 52 insertions(+), 79 deletions(-)
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index feb86fe6c422..53e19a863198 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -887,9 +887,9 @@ static int ad4130_set_filter_mode(struct iio_dev *indio_dev,
unsigned int old_fs;
int ret = 0;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (setup_info->filter_mode == val)
- goto out;
+ return 0;
old_fs = setup_info->fs;
old_filter_mode = setup_info->filter_mode;
@@ -911,12 +911,10 @@ static int ad4130_set_filter_mode(struct iio_dev *indio_dev,
if (ret) {
setup_info->fs = old_fs;
setup_info->filter_mode = old_filter_mode;
+ return ret;
}
- out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static int ad4130_get_filter_mode(struct iio_dev *indio_dev,
@@ -927,9 +925,8 @@ static int ad4130_get_filter_mode(struct iio_dev *indio_dev,
struct ad4130_setup_info *setup_info = &st->chans_info[channel].setup;
enum ad4130_filter_mode filter_mode;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
filter_mode = setup_info->filter_mode;
- mutex_unlock(&st->lock);
return filter_mode;
}
@@ -971,7 +968,7 @@ static int ad4130_set_channel_pga(struct ad4130_state *st, unsigned int channel,
struct ad4130_chan_info *chan_info = &st->chans_info[channel];
struct ad4130_setup_info *setup_info = &chan_info->setup;
unsigned int pga, old_pga;
- int ret = 0;
+ int ret;
for (pga = 0; pga < AD4130_MAX_PGA; pga++)
if (val == st->scale_tbls[setup_info->ref_sel][pga][0] &&
@@ -981,21 +978,20 @@ static int ad4130_set_channel_pga(struct ad4130_state *st, unsigned int channel,
if (pga == AD4130_MAX_PGA)
return -EINVAL;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (pga == setup_info->pga)
- goto out;
+ return 0;
old_pga = setup_info->pga;
setup_info->pga = pga;
ret = ad4130_write_channel_setup(st, channel, false);
- if (ret)
+ if (ret) {
setup_info->pga = old_pga;
+ return ret;
+ }
-out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static int ad4130_set_channel_freq(struct ad4130_state *st,
@@ -1004,26 +1000,25 @@ static int ad4130_set_channel_freq(struct ad4130_state *st,
struct ad4130_chan_info *chan_info = &st->chans_info[channel];
struct ad4130_setup_info *setup_info = &chan_info->setup;
unsigned int fs, old_fs;
- int ret = 0;
+ int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
old_fs = setup_info->fs;
ad4130_freq_to_fs(setup_info->filter_mode, val, val2, &fs);
if (fs == setup_info->fs)
- goto out;
+ return 0;
setup_info->fs = fs;
ret = ad4130_write_channel_setup(st, channel, false);
- if (ret)
+ if (ret) {
setup_info->fs = old_fs;
+ return ret;
+ }
-out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static int _ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
@@ -1065,20 +1060,13 @@ static int _ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
static int ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
int *val)
{
- struct ad4130_state *st = iio_priv(indio_dev);
- int ret;
-
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct ad4130_state *st = iio_priv(indio_dev);
- mutex_lock(&st->lock);
- ret = _ad4130_read_sample(indio_dev, channel, val);
- mutex_unlock(&st->lock);
-
- iio_device_release_direct_mode(indio_dev);
-
- return ret;
+ guard(mutex)(&st->lock);
+ return _ad4130_read_sample(indio_dev, channel, val);
+ }
+ unreachable();
}
static int ad4130_read_raw(struct iio_dev *indio_dev,
@@ -1092,24 +1080,24 @@ static int ad4130_read_raw(struct iio_dev *indio_dev,
switch (info) {
case IIO_CHAN_INFO_RAW:
return ad4130_read_sample(indio_dev, channel, val);
- case IIO_CHAN_INFO_SCALE:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_SCALE: {
+ guard(mutex)(&st->lock);
*val = st->scale_tbls[setup_info->ref_sel][setup_info->pga][0];
*val2 = st->scale_tbls[setup_info->ref_sel][setup_info->pga][1];
- mutex_unlock(&st->lock);
return IIO_VAL_INT_PLUS_NANO;
+ }
case IIO_CHAN_INFO_OFFSET:
*val = st->bipolar ? -BIT(chan->scan_type.realbits - 1) : 0;
return IIO_VAL_INT;
- case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&st->lock);
+ case IIO_CHAN_INFO_SAMP_FREQ: {
+ guard(mutex)(&st->lock);
ad4130_fs_to_freq(setup_info->filter_mode, setup_info->fs,
val, val2);
- mutex_unlock(&st->lock);
return IIO_VAL_INT_PLUS_NANO;
+ }
default:
return -EINVAL;
}
@@ -1134,9 +1122,9 @@ static int ad4130_read_avail(struct iio_dev *indio_dev,
return IIO_AVAIL_LIST;
case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&st->lock);
- filter_config = &ad4130_filter_configs[setup_info->filter_mode];
- mutex_unlock(&st->lock);
+ scoped_guard(mutex, &st->lock) {
+ filter_config = &ad4130_filter_configs[setup_info->filter_mode];
+ }
*vals = (int *)filter_config->samp_freq_avail;
*length = filter_config->samp_freq_avail_len * 2;
@@ -1197,21 +1185,18 @@ static int ad4130_update_scan_mode(struct iio_dev *indio_dev,
unsigned int val = 0;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
for_each_set_bit(channel, scan_mask, indio_dev->num_channels) {
ret = ad4130_set_channel_enable(st, channel, true);
if (ret)
- goto out;
+ return ret;
val++;
}
st->num_enabled_channels = val;
-out:
- mutex_unlock(&st->lock);
-
return 0;
}
@@ -1232,22 +1217,19 @@ static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val
*/
eff = rounddown(AD4130_FIFO_SIZE, st->num_enabled_channels);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_update_bits(st->regmap, AD4130_FIFO_CONTROL_REG,
AD4130_FIFO_CONTROL_WM_MASK,
FIELD_PREP(AD4130_FIFO_CONTROL_WM_MASK,
ad4130_watermark_reg_val(eff)));
if (ret)
- goto out;
+ return ret;
st->effective_watermark = eff;
st->watermark = val;
-out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static const struct iio_info ad4130_info = {
@@ -1265,26 +1247,21 @@ static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
struct ad4130_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = ad4130_set_watermark_interrupt_en(st, true);
if (ret)
- goto out;
+ return ret;
ret = irq_set_irq_type(st->spi->irq, st->inv_irq_trigger);
if (ret)
- goto out;
+ return ret;
ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_WM);
if (ret)
- goto out;
-
- ret = ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
-
-out:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
}
static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
@@ -1293,23 +1270,23 @@ static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
unsigned int i;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = ad4130_set_mode(st, AD4130_MODE_IDLE);
if (ret)
- goto out;
+ return ret;
ret = irq_set_irq_type(st->spi->irq, st->irq_trigger);
if (ret)
- goto out;
+ return ret;
ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_DISABLED);
if (ret)
- goto out;
+ return ret;
ret = ad4130_set_watermark_interrupt_en(st, false);
if (ret)
- goto out;
+ return ret;
/*
* update_scan_mode() is not called in the disable path, disable all
@@ -1318,13 +1295,10 @@ static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
for (i = 0; i < indio_dev->num_channels; i++) {
ret = ad4130_set_channel_enable(st, i, false);
if (ret)
- goto out;
+ return ret;
}
-out:
- mutex_unlock(&st->lock);
-
- return ret;
+ return 0;
}
static const struct iio_buffer_setup_ops ad4130_buffer_ops = {
@@ -1338,9 +1312,8 @@ static ssize_t hwfifo_watermark_show(struct device *dev,
struct ad4130_state *st = iio_priv(dev_to_iio_dev(dev));
unsigned int val;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
val = st->watermark;
- mutex_unlock(&st->lock);
return sysfs_emit(buf, "%d\n", val);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 10/10] iio: adc: ad7091r-base: Use auto cleanup of locks.
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (8 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 09/10] iio: adc: ad4130: Use automatic cleanup of locks and direct mode Jonathan Cameron
@ 2024-01-28 15:05 ` Jonathan Cameron
2024-01-30 14:08 ` [PATCH 00/10] IIO: Use the new cleanup.h magic Nuno Sá
10 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-01-28 15:05 UTC (permalink / raw)
To: linux-iio; +Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Done to reduce boilerplate and simplify code flow by allowing early
returns with the lock automatically released.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad7091r-base.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index f4255b91acfc..d6876259ad14 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -86,28 +86,25 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev,
unsigned int read_val;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
switch (m) {
case IIO_CHAN_INFO_RAW:
- if (st->mode != AD7091R_MODE_COMMAND) {
- ret = -EBUSY;
- goto unlock;
- }
+ if (st->mode != AD7091R_MODE_COMMAND)
+ return -EBUSY;
ret = ad7091r_read_one(iio_dev, chan->channel, &read_val);
if (ret)
- goto unlock;
+ return ret;
*val = read_val;
- ret = IIO_VAL_INT;
- break;
+ return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
if (st->vref) {
ret = regulator_get_voltage(st->vref);
if (ret < 0)
- goto unlock;
+ return ret;
*val = ret / 1000;
} else {
@@ -115,17 +112,11 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev,
}
*val2 = chan->scan_type.realbits;
- ret = IIO_VAL_FRACTIONAL_LOG2;
- break;
+ return IIO_VAL_FRACTIONAL_LOG2;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
-
-unlock:
- mutex_unlock(&st->lock);
- return ret;
}
static int ad7091r_read_event_config(struct iio_dev *indio_dev,
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 00/10] IIO: Use the new cleanup.h magic
2024-01-28 15:05 [PATCH 00/10] IIO: Use the new cleanup.h magic Jonathan Cameron
` (9 preceding siblings ...)
2024-01-28 15:05 ` [PATCH 10/10] iio: adc: ad7091r-base: Use auto cleanup of locks Jonathan Cameron
@ 2024-01-30 14:08 ` Nuno Sá
2024-02-04 16:06 ` Jonathan Cameron
10 siblings, 1 reply; 24+ messages in thread
From: Nuno Sá @ 2024-01-30 14:08 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Peter Zijlstra, Jonathan Cameron
On Sun, 2024-01-28 at 15:05 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The prerequisites are now in place upstream, so this series can now
> introduce the infrastructure and apply it to a few drivers.
>
> Changes since RFC v2: Thanks to David Lechner for review
> - Use unreachable() instead of misleading returns in paths we can't reach.
> - Various minor tweaks and local variable scope reduction.
>
> A lot of the advantages of the automated cleanup added for locks and similar
> are not that useful in IIO unless we also deal with the
> iio_device_claim_direct_mode() / iio_device_release_direct_mode()
> calls that prevent IIO device drivers from transitioning into buffered
> mode whilst calls are in flight + prevent sysfs reads and writes from
> interfering with buffered capture if it is enabled.
>
> This can now be neatly done using new scoped_cond_guard() to elegantly
> return if the attempt to claim direct mode fails.
>
> The need to always handle what happens after
> iio_device_claim_direct_scoped() {} is a little irritating but the
> compiler will warn if you don't do it and it's not obvious how to
> let the compiler know the magic loop (hidden in the cleanup.h macros)
> always runs once. Example:
>
> iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> return 42;
> }
> /* Can't get here, but compiler about no return val without this */
> unreachable();
> }
>
> Jonathan Cameron (10):
> iio: locking: introduce __cleanup() based direct mode claiming
> infrastructure
> iio: dummy: Use automatic lock and direct mode cleanup.
> iio: accel: adxl367: Use automated cleanup for locks and iio direct
> mode.
> iio: imu: bmi323: Use cleanup handling for
> iio_device_claim_direct_mode()
> iio: adc: max1363: Use automatic cleanup for locks and iio mode
> claiming.
> iio: proximity: sx9360: Use automated cleanup for locks and IIO mode
> claiming.
> iio: proximity: sx9324: Use automated cleanup for locks and IIO mode
> claiming.
> iio: proximity: sx9310: Use automated cleanup for locks and IIO mode
> claiming.
> iio: adc: ad4130: Use automatic cleanup of locks and direct mode.
> iio: adc: ad7091r-base: Use auto cleanup of locks.
>
> drivers/iio/accel/adxl367.c | 297 +++++++++++----------------
> drivers/iio/adc/ad4130.c | 131 +++++-------
> drivers/iio/adc/ad7091r-base.c | 25 +--
> drivers/iio/adc/max1363.c | 171 +++++++--------
> drivers/iio/dummy/iio_simple_dummy.c | 182 ++++++++--------
> drivers/iio/imu/bmi323/bmi323_core.c | 78 +++----
> drivers/iio/proximity/sx9310.c | 114 ++++------
> drivers/iio/proximity/sx9324.c | 109 ++++------
> drivers/iio/proximity/sx9360.c | 115 ++++-------
> include/linux/iio/iio.h | 25 +++
> 10 files changed, 518 insertions(+), 729 deletions(-)
>
Just one comment that boils down to preference... So, LGTM:
Reviewed-by: Nuno Sa <nuno.a@analog.com>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 00/10] IIO: Use the new cleanup.h magic
2024-01-30 14:08 ` [PATCH 00/10] IIO: Use the new cleanup.h magic Nuno Sá
@ 2024-02-04 16:06 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2024-02-04 16:06 UTC (permalink / raw)
To: Nuno Sá; +Cc: linux-iio, David Lechner, Peter Zijlstra, Jonathan Cameron
On Tue, 30 Jan 2024 15:08:39 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sun, 2024-01-28 at 15:05 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The prerequisites are now in place upstream, so this series can now
> > introduce the infrastructure and apply it to a few drivers.
> >
> > Changes since RFC v2: Thanks to David Lechner for review
> > - Use unreachable() instead of misleading returns in paths we can't reach.
> > - Various minor tweaks and local variable scope reduction.
> >
> > A lot of the advantages of the automated cleanup added for locks and similar
> > are not that useful in IIO unless we also deal with the
> > iio_device_claim_direct_mode() / iio_device_release_direct_mode()
> > calls that prevent IIO device drivers from transitioning into buffered
> > mode whilst calls are in flight + prevent sysfs reads and writes from
> > interfering with buffered capture if it is enabled.
> >
> > This can now be neatly done using new scoped_cond_guard() to elegantly
> > return if the attempt to claim direct mode fails.
> >
> > The need to always handle what happens after
> > iio_device_claim_direct_scoped() {} is a little irritating but the
> > compiler will warn if you don't do it and it's not obvious how to
> > let the compiler know the magic loop (hidden in the cleanup.h macros)
> > always runs once. Example:
> >
> > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > return 42;
> > }
> > /* Can't get here, but compiler about no return val without this */
> > unreachable();
> > }
> >
> > Jonathan Cameron (10):
> > iio: locking: introduce __cleanup() based direct mode claiming
> > infrastructure
> > iio: dummy: Use automatic lock and direct mode cleanup.
> > iio: accel: adxl367: Use automated cleanup for locks and iio direct
> > mode.
> > iio: imu: bmi323: Use cleanup handling for
> > iio_device_claim_direct_mode()
> > iio: adc: max1363: Use automatic cleanup for locks and iio mode
> > claiming.
> > iio: proximity: sx9360: Use automated cleanup for locks and IIO mode
> > claiming.
> > iio: proximity: sx9324: Use automated cleanup for locks and IIO mode
> > claiming.
> > iio: proximity: sx9310: Use automated cleanup for locks and IIO mode
> > claiming.
> > iio: adc: ad4130: Use automatic cleanup of locks and direct mode.
> > iio: adc: ad7091r-base: Use auto cleanup of locks.
> >
> > drivers/iio/accel/adxl367.c | 297 +++++++++++----------------
> > drivers/iio/adc/ad4130.c | 131 +++++-------
> > drivers/iio/adc/ad7091r-base.c | 25 +--
> > drivers/iio/adc/max1363.c | 171 +++++++--------
> > drivers/iio/dummy/iio_simple_dummy.c | 182 ++++++++--------
> > drivers/iio/imu/bmi323/bmi323_core.c | 78 +++----
> > drivers/iio/proximity/sx9310.c | 114 ++++------
> > drivers/iio/proximity/sx9324.c | 109 ++++------
> > drivers/iio/proximity/sx9360.c | 115 ++++-------
> > include/linux/iio/iio.h | 25 +++
> > 10 files changed, 518 insertions(+), 729 deletions(-)
> >
>
>
> Just one comment that boils down to preference... So, LGTM:
>
> Reviewed-by: Nuno Sa <nuno.a@analog.com>
>
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to poke at it. I tweaked patch 2 as discussed in the thread.
Thanks!
Jonathan
^ permalink raw reply [flat|nested] 24+ messages in thread