[PATCH 0/9] iio: imu: adis: make use the cleanup.h magic

[PATCH 0/9] iio: imu: adis: make use the cleanup.h magic

[Nuno Sa]

This is a simple series for moving the adis library into using the
cleanup.h based locks.

It also introduces new helpers (a normal and a scoped lock) for grabbing
the adis device lock using the automatic cleanup guards. In the end
(after updating all users) these new helpers fully replace the legacy
adis_dev_lock() and adis_dev_unlock(). 

---
Nuno Sa (9):
      iio: imu: adis_buffer: split trigger handling
      iio: imu: adis: move to the cleanup magic
      iio: imu: adis: add cleanup based lock helpers
      iio: gyro: adis16260: make use of the new lock helpers
      iio: gyro: adis16260: make use of the new lock helpers
      iio: imu: adis16400: make use of the new lock helpers
      iio: imu: adis16480: make use of the new lock helpers
      iio: imu: adis16475: make use of the new lock helpers
      iio: imu: adis: remove legacy lock helpers

 drivers/iio/gyro/adis16136.c  | 26 ++++++----------
 drivers/iio/gyro/adis16260.c  | 19 +++++-------
 drivers/iio/imu/adis.c        | 11 +++----
 drivers/iio/imu/adis16400.c   | 72 +++++++++++++++++++++----------------------
 drivers/iio/imu/adis16475.c   | 43 +++++++++-----------------
 drivers/iio/imu/adis16480.c   | 65 +++++++++++++++-----------------------
 drivers/iio/imu/adis_buffer.c | 52 ++++++++++++++++---------------
 include/linux/iio/imu/adis.h  | 66 ++++++++++-----------------------------
 8 files changed, 138 insertions(+), 216 deletions(-)
---
base-commit: cc1ce839526a65620778617da0b022bd88e8a139
change-id: 20240618-dev-iio-adis-cleanup-b6571fddfe13
--

Thanks!
- Nuno Sá

[PATCH 1/9] iio: imu: adis_buffer: split trigger handling

[Nuno Sa]

Split trigger handling for devices that have paging and need to
select the correct page to get the data. Although this actually
introduces more LOC, it makes the code and the locking clear. It will
also make the following move to the cleanup magic cleaner.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 56 ++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 871b78b225e2..d1d1e4f512b9 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -126,6 +126,30 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_NS_GPL(adis_update_scan_mode, IIO_ADISLIB);
 
+static int adis_paging_trigger_handler(struct adis *adis)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	if (adis->current_page != 0) {
+		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
+		adis->tx[1] = 0;
+		ret = spi_write(adis->spi, adis->tx, 2);
+		if (ret) {
+			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+			mutex_unlock(&adis->state_lock);
+			return ret;
+		}
+
+		adis->current_page = 0;
+	}
+
+	ret = spi_sync(adis->spi, &adis->msg);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
+
 static irqreturn_t adis_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -133,34 +157,16 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	struct adis *adis = iio_device_get_drvdata(indio_dev);
 	int ret;
 
-	if (adis->data->has_paging) {
-		mutex_lock(&adis->state_lock);
-		if (adis->current_page != 0) {
-			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
-			adis->tx[1] = 0;
-			ret = spi_write(adis->spi, adis->tx, 2);
-			if (ret) {
-				dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
-				mutex_unlock(&adis->state_lock);
-				goto irq_done;
-			}
-
-			adis->current_page = 0;
-		}
-	}
-
-	ret = spi_sync(adis->spi, &adis->msg);
 	if (adis->data->has_paging)
-		mutex_unlock(&adis->state_lock);
-	if (ret) {
+		ret = adis_paging_trigger_handler(adis);
+	else
+		ret = spi_sync(adis->spi, &adis->msg);
+	if (ret)
 		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
-		goto irq_done;
-	}
+	else
+		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
+						   pf->timestamp);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
-					   pf->timestamp);
-
-irq_done:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;

-- 
2.45.2

Re: [PATCH 1/9] iio: imu: adis_buffer: split trigger handling

[Jonathan Cameron]

On Tue, 18 Jun 2024 15:32:04 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Split trigger handling for devices that have paging and need to
> select the correct page to get the data. Although this actually
> introduces more LOC, it makes the code and the locking clear. It will
> also make the following move to the cleanup magic cleaner.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,

Just one thing,

> +		ret = spi_sync(adis->spi, &adis->msg);
> +	if (ret)
>  		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> -		goto irq_done;
> -	}
> +	else
> +		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> +						   pf->timestamp);

Keep the goto as having an indented 'good' path is not great for readability.

>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> -					   pf->timestamp);
> -
> -irq_done:
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> 

Re: [PATCH 1/9] iio: imu: adis_buffer: split trigger handling

[Jonathan Cameron]

On Sun, 23 Jun 2024 17:03:19 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 18 Jun 2024 15:32:04 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Split trigger handling for devices that have paging and need to
> > select the correct page to get the data. Although this actually
> > introduces more LOC, it makes the code and the locking clear. It will
> > also make the following move to the cleanup magic cleaner.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> Hi Nuno,
> 
> Just one thing,
> 
> > +		ret = spi_sync(adis->spi, &adis->msg);
> > +	if (ret)
> >  		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> > -		goto irq_done;
> > -	}
> > +	else
> > +		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > +						   pf->timestamp);  
> 
> Keep the goto as having an indented 'good' path is not great for readability.
> 
Meh. That was (almost) all I found to comment on so I changed it back whilst applying.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > -					   pf->timestamp);
> > -
> > -irq_done:
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  
> >  	return IRQ_HANDLED;
> >   
> 
> 

[PATCH 2/9] iio: imu: adis: move to the cleanup magic

[Nuno Sa]

This makes locking and handling error paths simpler.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis.c        | 11 ++++-----
 drivers/iio/imu/adis_buffer.c |  8 ++-----
 include/linux/iio/imu/adis.h  | 54 +++++++++++--------------------------------
 3 files changed, 19 insertions(+), 54 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 495caf4ce87a..876848b42f69 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -466,17 +466,17 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 	unsigned int uval;
 	int ret;
 
-	mutex_lock(&adis->state_lock);
+	guard(mutex)(&adis->state_lock);
 
 	ret = __adis_read_reg(adis, chan->address, &uval,
 			      chan->scan_type.storagebits / 8);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	if (uval & error_mask) {
 		ret = __adis_check_status(adis);
 		if (ret)
-			goto err_unlock;
+			return ret;
 	}
 
 	if (chan->scan_type.sign == 's')
@@ -484,10 +484,7 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 	else
 		*val = uval & ((1 << chan->scan_type.realbits) - 1);
 
-	ret = IIO_VAL_INT;
-err_unlock:
-	mutex_unlock(&adis->state_lock);
-	return ret;
+	return IIO_VAL_INT;
 }
 EXPORT_SYMBOL_NS_GPL(adis_single_conversion, IIO_ADISLIB);
 
diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index d1d1e4f512b9..29cd8564cd32 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -130,24 +130,20 @@ static int adis_paging_trigger_handler(struct adis *adis)
 {
 	int ret;
 
-	mutex_lock(&adis->state_lock);
+	guard(mutex)(&adis->state_lock);
 	if (adis->current_page != 0) {
 		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
 		adis->tx[1] = 0;
 		ret = spi_write(adis->spi, adis->tx, 2);
 		if (ret) {
 			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
-			mutex_unlock(&adis->state_lock);
 			return ret;
 		}
 
 		adis->current_page = 0;
 	}
 
-	ret = spi_sync(adis->spi, &adis->msg);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	return spi_sync(adis->spi, &adis->msg);
 }
 
 static irqreturn_t adis_trigger_handler(int irq, void *p)
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 4bb0a53cf7ea..93dad627378f 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -9,6 +9,7 @@
 #ifndef __IIO_ADIS_H__
 #define __IIO_ADIS_H__
 
+#include <linux/cleanup.h>
 #include <linux/spi/spi.h>
 #include <linux/interrupt.h>
 #include <linux/iio/iio.h>
@@ -150,13 +151,8 @@ int __adis_reset(struct adis *adis);
  */
 static inline int adis_reset(struct adis *adis)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_reset(adis);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_reset(adis);
 }
 
 int __adis_write_reg(struct adis *adis, unsigned int reg,
@@ -248,13 +244,8 @@ static inline int __adis_read_reg_32(struct adis *adis, unsigned int reg,
 static inline int adis_write_reg(struct adis *adis, unsigned int reg,
 				 unsigned int val, unsigned int size)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_write_reg(adis, reg, val, size);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_write_reg(adis, reg, val, size);
 }
 
 /**
@@ -267,13 +258,8 @@ static inline int adis_write_reg(struct adis *adis, unsigned int reg,
 static int adis_read_reg(struct adis *adis, unsigned int reg,
 			 unsigned int *val, unsigned int size)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_read_reg(adis, reg, val, size);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_read_reg(adis, reg, val, size);
 }
 
 /**
@@ -365,12 +351,8 @@ int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask,
 static inline int adis_update_bits_base(struct adis *adis, unsigned int reg,
 					const u32 mask, const u32 val, u8 size)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_update_bits_base(adis, reg, mask, val, size);
-	mutex_unlock(&adis->state_lock);
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_update_bits_base(adis, reg, mask, val, size);
 }
 
 /**
@@ -411,24 +393,14 @@ int __adis_enable_irq(struct adis *adis, bool enable);
 
 static inline int adis_enable_irq(struct adis *adis, bool enable)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_enable_irq(adis, enable);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_enable_irq(adis, enable);
 }
 
 static inline int adis_check_status(struct adis *adis)
 {
-	int ret;
-
-	mutex_lock(&adis->state_lock);
-	ret = __adis_check_status(adis);
-	mutex_unlock(&adis->state_lock);
-
-	return ret;
+	guard(mutex)(&adis->state_lock);
+	return __adis_check_status(adis);
 }
 
 static inline void adis_dev_lock(struct adis *adis)

-- 
2.45.2

[PATCH 3/9] iio: imu: adis: add cleanup based lock helpers

[Nuno Sa]

Add two new lock helpers that make use of the cleanup guard() and
scoped_guard() macros. Thus, users won't have to worry about unlocking
which is less prone to errors and allows for simpler error paths.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/imu/adis.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 93dad627378f..bc7332d12c44 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -403,6 +403,10 @@ static inline int adis_check_status(struct adis *adis)
 	return __adis_check_status(adis);
 }
 
+#define adis_dev_auto_lock(adis)	guard(mutex)(&(adis)->state_lock)
+#define adis_dev_auto_scoped_lock(adis) \
+	scoped_guard(mutex, &(adis)->state_lock)
+
 static inline void adis_dev_lock(struct adis *adis)
 {
 	mutex_lock(&adis->state_lock);

-- 
2.45.2

[PATCH 4/9] iio: gyro: adis16260: make use of the new lock helpers

[Nuno Sa]

Use the new auto cleanup based locks so error paths are simpler.

While at it, turned a sprintf() call into sysfs_emit().

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/gyro/adis16136.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index c95cf41be34b..da83adc684d0 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -221,13 +221,12 @@ static ssize_t adis16136_read_frequency(struct device *dev,
 	unsigned int freq;
 	int ret;
 
-	adis_dev_lock(&adis16136->adis);
+	adis_dev_auto_lock(&adis16136->adis);
 	ret = __adis16136_get_freq(adis16136, &freq);
-	adis_dev_unlock(&adis16136->adis);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%d\n", freq);
+	return sysfs_emit(buf, "%d\n", freq);
 }
 
 static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
@@ -251,21 +250,17 @@ static int adis16136_set_filter(struct iio_dev *indio_dev, int val)
 	unsigned int freq;
 	int i, ret;
 
-	adis_dev_lock(&adis16136->adis);
+	adis_dev_auto_lock(&adis16136->adis);
 	ret = __adis16136_get_freq(adis16136, &freq);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	for (i = ARRAY_SIZE(adis16136_3db_divisors) - 1; i >= 1; i--) {
 		if (freq / adis16136_3db_divisors[i] >= val)
 			break;
 	}
 
-	ret = __adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
-out_unlock:
-	adis_dev_unlock(&adis16136->adis);
-
-	return ret;
+	return __adis_write_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT, i);
 }
 
 static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
@@ -275,23 +270,20 @@ static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
 	uint16_t val16;
 	int ret;
 
-	adis_dev_lock(&adis16136->adis);
+	adis_dev_auto_lock(&adis16136->adis);
 
 	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT,
 				 &val16);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	ret = __adis16136_get_freq(adis16136, &freq);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	*val = freq / adis16136_3db_divisors[val16 & 0x07];
 
-err_unlock:
-	adis_dev_unlock(&adis16136->adis);
-
-	return ret ? ret : IIO_VAL_INT;
+	return IIO_VAL_INT;
 }
 
 static int adis16136_read_raw(struct iio_dev *indio_dev,

-- 
2.45.2

[PATCH 5/9] iio: gyro: adis16260: make use of the new lock helpers

[Nuno Sa]

Use the new auto cleanup based locks so error paths are simpler.

While at it, reduce a bit the scope of the lock as we did not needed it
protecting all the data in the switch() branch.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/gyro/adis16260.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index 112d635b7dfd..495b64a27061 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -270,7 +270,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
 {
 	struct adis16260 *adis16260 = iio_priv(indio_dev);
 	struct adis *adis = &adis16260->adis;
-	int ret;
 	u8 addr;
 	u8 t;
 
@@ -288,7 +287,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
 		addr = adis16260_addresses[chan->scan_index][1];
 		return adis_write_reg_16(adis, addr, val);
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		adis_dev_lock(adis);
 		if (spi_get_device_id(adis->spi)->driver_data)
 			t = 256 / val;
 		else
@@ -298,15 +296,14 @@ static int adis16260_write_raw(struct iio_dev *indio_dev,
 			t = ADIS16260_SMPL_PRD_DIV_MASK;
 		else if (t > 0)
 			t--;
-
-		if (t >= 0x0A)
-			adis->spi->max_speed_hz = ADIS16260_SPI_SLOW;
-		else
-			adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
-		ret = __adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
-
-		adis_dev_unlock(adis);
-		return ret;
+		adis_dev_auto_scoped_lock(adis) {
+			if (t >= 0x0A)
+				adis->spi->max_speed_hz = ADIS16260_SPI_SLOW;
+			else
+				adis->spi->max_speed_hz = ADIS16260_SPI_FAST;
+			return __adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t);
+		}
+		unreachable();
 	}
 	return -EINVAL;
 }

-- 
2.45.2

[PATCH 6/9] iio: imu: adis16400: make use of the new lock helpers

[Nuno Sa]

Use the new auto cleanup based locks so error paths are simpler.

While at it, removed 'ret' from adis16400_write_raw() by doing

	return adis_write_reg_16();

instead of

	ret = adis_write_reg_16();
	return ret;

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 72 ++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 3eda32e12a53..0bfd6205f5f6 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -497,41 +497,38 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
 	struct iio_chan_spec const *chan, int val, int val2, long info)
 {
 	struct adis16400_state *st = iio_priv(indio_dev);
-	int ret, sps;
+	int sps;
 
 	switch (info) {
 	case IIO_CHAN_INFO_CALIBBIAS:
-		ret = adis_write_reg_16(&st->adis,
-				adis16400_addresses[chan->scan_index], val);
-		return ret;
+		return adis_write_reg_16(&st->adis,
+					 adis16400_addresses[chan->scan_index],
+					 val);
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		/*
 		 * Need to cache values so we can update if the frequency
 		 * changes.
 		 */
-		adis_dev_lock(&st->adis);
-		st->filt_int = val;
-		/* Work out update to current value */
-		sps = st->variant->get_freq(st);
-		if (sps < 0) {
-			adis_dev_unlock(&st->adis);
-			return sps;
-		}
+		adis_dev_auto_scoped_lock(&st->adis) {
+			st->filt_int = val;
+			/* Work out update to current value */
+			sps = st->variant->get_freq(st);
+			if (sps < 0)
+				return sps;
 
-		ret = __adis16400_set_filter(indio_dev, sps,
-			val * 1000 + val2 / 1000);
-		adis_dev_unlock(&st->adis);
-		return ret;
+			return __adis16400_set_filter(indio_dev, sps,
+						      val * 1000 + val2 / 1000);
+		}
+		unreachable();
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		sps = val * 1000 + val2 / 1000;
 
 		if (sps <= 0)
 			return -EINVAL;
 
-		adis_dev_lock(&st->adis);
-		ret = st->variant->set_freq(st, sps);
-		adis_dev_unlock(&st->adis);
-		return ret;
+		adis_dev_auto_scoped_lock(&st->adis)
+			return st->variant->set_freq(st, sps);
+		unreachable();
 	default:
 		return -EINVAL;
 	}
@@ -596,29 +593,30 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		*val = st->variant->temp_offset;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		adis_dev_lock(&st->adis);
-		/* Need both the number of taps and the sampling frequency */
-		ret = __adis_read_reg_16(&st->adis,
-						ADIS16400_SENS_AVG,
-						&val16);
-		if (ret) {
-			adis_dev_unlock(&st->adis);
-			return ret;
+		adis_dev_auto_scoped_lock(&st->adis) {
+			/*
+			 * Need both the number of taps and the sampling
+			 * frequency
+			 */
+			ret = __adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG,
+						 &val16);
+			if (ret)
+				return ret;
+
+			ret = st->variant->get_freq(st);
+			if (ret)
+				return ret;
 		}
-		ret = st->variant->get_freq(st);
-		adis_dev_unlock(&st->adis);
-		if (ret)
-			return ret;
 		ret /= adis16400_3db_divisors[val16 & 0x07];
 		*val = ret / 1000;
 		*val2 = (ret % 1000) * 1000;
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		adis_dev_lock(&st->adis);
-		ret = st->variant->get_freq(st);
-		adis_dev_unlock(&st->adis);
-		if (ret)
-			return ret;
+		adis_dev_auto_scoped_lock(&st->adis) {
+			ret = st->variant->get_freq(st);
+			if (ret)
+				return ret;
+		}
 		*val = ret / 1000;
 		*val2 = (ret % 1000) * 1000;
 		return IIO_VAL_INT_PLUS_MICRO;

-- 
2.45.2

[PATCH 7/9] iio: imu: adis16480: make use of the new lock helpers

[Nuno Sa]

Use the new auto cleanup based locks so error paths are simpler.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16480.c | 65 +++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 56ca5a09fbbf..c59ef6f7cfd4 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -345,7 +345,7 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 	if (t == 0)
 		return -EINVAL;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 	/*
 	 * When using PPS mode, the input clock needs to be scaled so that we have an IMU
 	 * sample rate between (optimally) 4000 and 4250. After this, we can use the
@@ -388,7 +388,7 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 		sync_scale = scaled_rate / st->clk_freq;
 		ret = __adis_write_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, sync_scale);
 		if (ret)
-			goto error;
+			return ret;
 
 		sample_rate = scaled_rate;
 	}
@@ -400,10 +400,7 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 	if (t > st->chip_info->max_dec_rate)
 		t = st->chip_info->max_dec_rate;
 
-	ret = __adis_write_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, t);
-error:
-	adis_dev_unlock(&st->adis);
-	return ret;
+	return __adis_write_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, t);
 }
 
 static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
@@ -413,23 +410,21 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 	int ret;
 	unsigned int freq, sample_rate = st->clk_freq;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	if (st->clk_mode == ADIS16480_CLK_PPS) {
 		u16 sync_scale;
 
 		ret = __adis_read_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, &sync_scale);
 		if (ret)
-			goto error;
+			return ret;
 
 		sample_rate = st->clk_freq * sync_scale;
 	}
 
 	ret = __adis_read_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, &t);
 	if (ret)
-		goto error;
-
-	adis_dev_unlock(&st->adis);
+		return ret;
 
 	freq = DIV_ROUND_CLOSEST(sample_rate, (t + 1));
 
@@ -437,9 +432,6 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 	*val2 = (freq % 1000) * 1000;
 
 	return IIO_VAL_INT_PLUS_MICRO;
-error:
-	adis_dev_unlock(&st->adis);
-	return ret;
 }
 
 enum {
@@ -630,11 +622,11 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 	offset = ad16480_filter_data[chan->scan_index][1];
 	enable_mask = BIT(offset + 2);
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	ret = __adis_read_reg_16(&st->adis, reg, &val);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	if (freq == 0) {
 		val &= ~enable_mask;
@@ -656,11 +648,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 		val |= enable_mask;
 	}
 
-	ret = __adis_write_reg_16(&st->adis, reg, val);
-out_unlock:
-	adis_dev_unlock(&st->adis);
-
-	return ret;
+	return __adis_write_reg_16(&st->adis, reg, val);
 }
 
 static int adis16480_read_raw(struct iio_dev *indio_dev,
@@ -1355,29 +1343,26 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	u32 crc;
 	bool valid;
 
-	adis_dev_lock(adis);
-	if (adis->current_page != 0) {
-		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
-		adis->tx[1] = 0;
-		ret = spi_write(adis->spi, adis->tx, 2);
-		if (ret) {
-			dev_err(dev, "Failed to change device page: %d\n", ret);
-			adis_dev_unlock(adis);
-			goto irq_done;
+	adis_dev_auto_scoped_lock(adis) {
+		if (adis->current_page != 0) {
+			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
+			adis->tx[1] = 0;
+			ret = spi_write(adis->spi, adis->tx, 2);
+			if (ret) {
+				dev_err(dev, "Failed to change device page: %d\n", ret);
+				goto irq_done;
+			}
+
+			adis->current_page = 0;
 		}
 
-		adis->current_page = 0;
+		ret = spi_sync(adis->spi, &adis->msg);
+		if (ret) {
+			dev_err(dev, "Failed to read data: %d\n", ret);
+			goto irq_done;
+		}
 	}
 
-	ret = spi_sync(adis->spi, &adis->msg);
-	if (ret) {
-		dev_err(dev, "Failed to read data: %d\n", ret);
-		adis_dev_unlock(adis);
-		goto irq_done;
-	}
-
-	adis_dev_unlock(adis);
-
 	/*
 	 * After making the burst request, the response can have one or two
 	 * 16-bit responses containing the BURST_ID depending on the sclk. If

-- 
2.45.2

[PATCH 8/9] iio: imu: adis16475: make use of the new lock helpers

[Nuno Sa]

Use the new auto cleanup based locks so error paths are simpler.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 43 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index cf73c6f46c79..78ceebf5023c 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -302,30 +302,25 @@ static int adis16475_get_freq(struct adis16475 *st, u32 *freq)
 	u16 dec;
 	u32 sample_rate = st->clk_freq;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	if (st->sync_mode == ADIS16475_SYNC_SCALED) {
 		u16 sync_scale;
 
 		ret = __adis_read_reg_16(&st->adis, ADIS16475_REG_UP_SCALE, &sync_scale);
 		if (ret)
-			goto error;
+			return ret;
 
 		sample_rate = st->clk_freq * sync_scale;
 	}
 
 	ret = __adis_read_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, &dec);
 	if (ret)
-		goto error;
-
-	adis_dev_unlock(&st->adis);
+		return ret;
 
 	*freq = DIV_ROUND_CLOSEST(sample_rate, dec + 1);
 
 	return 0;
-error:
-	adis_dev_unlock(&st->adis);
-	return ret;
 }
 
 static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
@@ -340,7 +335,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 	if (!freq)
 		return -EINVAL;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 	/*
 	 * When using sync scaled mode, the input clock needs to be scaled so that we have
 	 * an IMU sample rate between (optimally) int_clk - 100 and int_clk + 100.
@@ -385,7 +380,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 		sync_scale = scaled_rate / st->clk_freq;
 		ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_UP_SCALE, sync_scale);
 		if (ret)
-			goto error;
+			return ret;
 
 		sample_rate = scaled_rate;
 	}
@@ -400,7 +395,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 
 	ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, dec);
 	if (ret)
-		goto error;
+		return ret;
 
 	adis_dev_unlock(&st->adis);
 	/*
@@ -410,9 +405,6 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 	assign_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag, dec);
 
 	return 0;
-error:
-	adis_dev_unlock(&st->adis);
-	return ret;
 }
 
 /* The values are approximated. */
@@ -541,19 +533,15 @@ static int adis16475_buffer_postdisable(struct iio_dev *indio_dev)
 	struct adis *adis = &st->adis;
 	int ret;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	ret = __adis_update_bits(adis, ADIS16475_REG_FIFO_CTRL,
 				 ADIS16575_FIFO_EN_MASK, (u16)ADIS16575_FIFO_EN(0));
 	if (ret)
-		goto unlock;
+		return ret;
 
-	ret = __adis_write_reg_16(adis, ADIS16475_REG_GLOB_CMD,
-				  ADIS16575_FIFO_FLUSH_CMD);
-
-unlock:
-	adis_dev_unlock(&st->adis);
-	return ret;
+	return __adis_write_reg_16(adis, ADIS16475_REG_GLOB_CMD,
+				   ADIS16575_FIFO_FLUSH_CMD);
 }
 
 static const struct iio_buffer_setup_ops adis16475_buffer_ops = {
@@ -567,20 +555,18 @@ static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned int val)
 	int ret;
 	u16 wm_lvl;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	val = min_t(unsigned int, val, ADIS16575_MAX_FIFO_WM);
 
 	wm_lvl = ADIS16575_WM_LVL(val - 1);
 	ret = __adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16575_WM_LVL_MASK, wm_lvl);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	st->fifo_watermark = val;
 
-unlock:
-	adis_dev_unlock(&st->adis);
-	return ret;
+	return 0;
 }
 
 static const u32 adis16475_calib_regs[] = {
@@ -1745,7 +1731,7 @@ static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
 	int ret;
 	u16 fifo_cnt, i;
 
-	adis_dev_lock(&st->adis);
+	adis_dev_auto_lock(&st->adis);
 
 	ret = __adis_read_reg_16(adis, ADIS16575_REG_FIFO_CNT, &fifo_cnt);
 	if (ret)
@@ -1781,7 +1767,6 @@ static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
 	 * reading data from registers will impact the FIFO reading.
 	 */
 	adis16475_burst32_check(st);
-	adis_dev_unlock(&st->adis);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;

-- 
2.45.2

Re: [PATCH 8/9] iio: imu: adis16475: make use of the new lock helpers

[Jonathan Cameron]

On Tue, 18 Jun 2024 15:32:11 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Use the new auto cleanup based locks so error paths are simpler.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
This failed a build test...

>  static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
> @@ -340,7 +335,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
>  	if (!freq)
>  		return -EINVAL;
>  
> -	adis_dev_lock(&st->adis);
> +	adis_dev_auto_lock(&st->adis);
>  	/*
>  	 * When using sync scaled mode, the input clock needs to be scaled so that we have
>  	 * an IMU sample rate between (optimally) int_clk - 100 and int_clk + 100.
> @@ -385,7 +380,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
>  		sync_scale = scaled_rate / st->clk_freq;
>  		ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_UP_SCALE, sync_scale);
>  		if (ret)
> -			goto error;
> +			return ret;
>  
>  		sample_rate = scaled_rate;
>  	}
> @@ -400,7 +395,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
>  
>  	ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, dec);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	adis_dev_unlock(&st->adis);
You missed this one... I dropped it whilst applying.

>  	/*
> @@ -410,9 +405,6 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
>  	assign_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag, dec);
>  
>  	return 0;
> -error:
> -	adis_dev_unlock(&st->adis);
> -	return ret;
>  }

[PATCH 9/9] iio: imu: adis: remove legacy lock helpers

[Nuno Sa]

Since all users were converted to the new cleanup based helper,
adis_dev_lock() and adis_dev_unlock() can now be removed from the lib.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/imu/adis.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index bc7332d12c44..e6a75356567a 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -407,16 +407,6 @@ static inline int adis_check_status(struct adis *adis)
 #define adis_dev_auto_scoped_lock(adis) \
 	scoped_guard(mutex, &(adis)->state_lock)
 
-static inline void adis_dev_lock(struct adis *adis)
-{
-	mutex_lock(&adis->state_lock);
-}
-
-static inline void adis_dev_unlock(struct adis *adis)
-{
-	mutex_unlock(&adis->state_lock);
-}
-
 int adis_single_conversion(struct iio_dev *indio_dev,
 			   const struct iio_chan_spec *chan,
 			   unsigned int error_mask, int *val);

-- 
2.45.2