[PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andrew Ijano]

The sca3000 driver is old and could be simplified by using newer
infrastructure.

Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
v5 -> v6:
- break up changes related to read data helpers in two patches
- fix formatting
- add local scope for switch() cases that use guard()
- use sysfs_emit_at() instead of sprintf()

v4 -> v5:
- break up the changes in three patches
- replace error_ret labels by simple returns
- use spi_w8r16be() for be16 reads
- use guard(mutex) for handling mutex lock

v3 -> v4:
- clean the code and remove redundant operations

v2 -> v3:
- replace usages of internal read data helpers by spi helpers

v1 -> v2:
- simplify the return of the internal read data function
---
Andrew Ijano (4):
  iio: accel: sca3000: replace usages of internal read data helpers by
    spi helpers
  iio: accel: sca3000: clean sca3000_read_data()
  iio: accel: sca3000: use lock guards
  iio: accel: sca3000: use sysfs_emit_at() instead of sprintf()

 drivers/iio/accel/sca3000.c | 384 ++++++++++++++----------------------
 1 file changed, 145 insertions(+), 239 deletions(-)

-- 
2.49.0

[PATCH v6 1/4] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Andrew Ijano]

Remove sca3000_read_data_short() function, replacing it by spi_w8r8()
and spi_w8r16be() helpers.

This is an old driver that was not making full use of the newer
infrastructure.

Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/iio/accel/sca3000.c | 152 +++++++++++++++---------------------
 1 file changed, 63 insertions(+), 89 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index bfa8a3f5a92f..c85a06cbea37 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -281,24 +281,6 @@ static int sca3000_write_reg(struct sca3000_state *st, u8 address, u8 val)
 	return spi_write(st->us, st->tx, 2);
 }
 
-static int sca3000_read_data_short(struct sca3000_state *st,
-				   u8 reg_address_high,
-				   int len)
-{
-	struct spi_transfer xfer[2] = {
-		{
-			.len = 1,
-			.tx_buf = st->tx,
-		}, {
-			.len = len,
-			.rx_buf = st->rx,
-		}
-	};
-	st->tx[0] = SCA3000_READ_REG(reg_address_high);
-
-	return spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
-}
-
 /**
  * sca3000_reg_lock_on() - test if the ctrl register lock is on
  * @st: Driver specific device instance data.
@@ -309,11 +291,11 @@ static int sca3000_reg_lock_on(struct sca3000_state *st)
 {
 	int ret;
 
-	ret = sca3000_read_data_short(st, SCA3000_REG_STATUS_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_STATUS_ADDR));
 	if (ret < 0)
 		return ret;
 
-	return !(st->rx[0] & SCA3000_LOCKED);
+	return !(ret & SCA3000_LOCKED);
 }
 
 /**
@@ -409,10 +391,7 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
 	ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
 	if (ret)
 		return ret;
-	ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
-	if (ret)
-		return ret;
-	return st->rx[0];
+	return spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
 }
 
 /**
@@ -427,13 +406,13 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
 	struct sca3000_state *st = iio_priv(indio_dev);
 
 	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_REVID_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
 	if (ret < 0)
 		goto error_ret;
 	dev_info(&indio_dev->dev,
 		 "sca3000 revision major=%lu, minor=%lu\n",
-		 st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
-		 st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
+		 ret & SCA3000_REG_REVID_MAJOR_MASK,
+		 ret & SCA3000_REG_REVID_MINOR_MASK);
 error_ret:
 	mutex_unlock(&st->lock);
 
@@ -570,7 +549,7 @@ static inline int __sca3000_get_base_freq(struct sca3000_state *st,
 {
 	int ret;
 
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		return ret;
 
@@ -660,13 +639,13 @@ static int sca3000_read_3db_freq(struct sca3000_state *st, int *val)
 {
 	int ret;
 
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		return ret;
 
 	/* mask bottom 2 bits - only ones that are relevant */
-	st->rx[0] &= SCA3000_REG_MODE_MODE_MASK;
-	switch (st->rx[0]) {
+	ret &= SCA3000_REG_MODE_MODE_MASK;
+	switch (ret) {
 	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
 		*val = st->info->measurement_mode_3db_freq;
 		return IIO_VAL_INT;
@@ -698,14 +677,14 @@ static int sca3000_write_3db_freq(struct sca3000_state *st, int val)
 		mode = SCA3000_REG_MODE_MEAS_MODE_OP_2;
 	else
 		return -EINVAL;
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		return ret;
 
-	st->rx[0] &= ~SCA3000_REG_MODE_MODE_MASK;
-	st->rx[0] |= (mode & SCA3000_REG_MODE_MODE_MASK);
+	ret &= ~SCA3000_REG_MODE_MODE_MASK;
+	ret |= mode & SCA3000_REG_MODE_MODE_MASK;
 
-	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, st->rx[0]);
+	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, ret);
 }
 
 static int sca3000_read_raw(struct iio_dev *indio_dev,
@@ -727,25 +706,21 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
 				return -EBUSY;
 			}
 			address = sca3000_addresses[chan->address][0];
-			ret = sca3000_read_data_short(st, address, 2);
+			ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
 			if (ret < 0) {
 				mutex_unlock(&st->lock);
 				return ret;
 			}
-			*val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
-					     chan->scan_type.shift,
+			*val = sign_extend32(ret >> chan->scan_type.shift,
 					     chan->scan_type.realbits - 1);
 		} else {
 			/* get the temperature when available */
-			ret = sca3000_read_data_short(st,
-						      SCA3000_REG_TEMP_MSB_ADDR,
-						      2);
+			ret = spi_w8r16be(st->us, SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
 			if (ret < 0) {
 				mutex_unlock(&st->lock);
 				return ret;
 			}
-			*val = (be16_to_cpup((__be16 *)st->rx) >>
-				chan->scan_type.shift) &
+			*val = (ret >> chan->scan_type.shift) &
 				GENMASK(chan->scan_type.realbits - 1, 0);
 		}
 		mutex_unlock(&st->lock);
@@ -822,16 +797,16 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int len = 0, ret, val;
+	unsigned int len = 0;
+	int ret;
 
 	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
-	val = st->rx[0];
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	mutex_unlock(&st->lock);
 	if (ret)
 		return ret;
 
-	switch (val & SCA3000_REG_MODE_MODE_MASK) {
+	switch (ret & SCA3000_REG_MODE_MODE_MASK) {
 	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
 		len += sprintf(buf + len, "%d %d %d\n",
 			       st->info->measurement_mode_freq,
@@ -1001,11 +976,10 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
 	mutex_lock(&st->lock);
 
 	if (val & SCA3000_REG_INT_STATUS_HALF) {
-		ret = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR,
-					      1);
+		ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
 		if (ret)
 			goto error_ret;
-		num_available = st->rx[0];
+		num_available = ret;
 		/*
 		 * num_available is the total number of samples available
 		 * i.e. number of time points * number of channels.
@@ -1045,7 +1019,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int ret, val;
+	int ret;
 	s64 last_timestamp = iio_get_time_ns(indio_dev);
 
 	/*
@@ -1053,15 +1027,14 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 	 * but ensures no interrupt is missed.
 	 */
 	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
-	val = st->rx[0];
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
 	mutex_unlock(&st->lock);
 	if (ret)
 		goto done;
 
-	sca3000_ring_int_process(val, indio_dev);
+	sca3000_ring_int_process(ret, indio_dev);
 
-	if (val & SCA3000_INT_STATUS_FREE_FALL)
+	if (ret & SCA3000_INT_STATUS_FREE_FALL)
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1070,7 +1043,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 						  IIO_EV_DIR_FALLING),
 			       last_timestamp);
 
-	if (val & SCA3000_INT_STATUS_Y_TRIGGER)
+	if (ret & SCA3000_INT_STATUS_Y_TRIGGER)
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1079,7 +1052,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 						  IIO_EV_DIR_RISING),
 			       last_timestamp);
 
-	if (val & SCA3000_INT_STATUS_X_TRIGGER)
+	if (ret & SCA3000_INT_STATUS_X_TRIGGER)
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1088,7 +1061,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 						  IIO_EV_DIR_RISING),
 			       last_timestamp);
 
-	if (val & SCA3000_INT_STATUS_Z_TRIGGER)
+	if (ret & SCA3000_INT_STATUS_Z_TRIGGER)
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1114,13 +1087,13 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
 	/* read current value of mode register */
 	mutex_lock(&st->lock);
 
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		goto error_ret;
 
 	switch (chan->channel2) {
 	case IIO_MOD_X_AND_Y_AND_Z:
-		ret = !!(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT);
+		ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
 		break;
 	case IIO_MOD_X:
 	case IIO_MOD_Y:
@@ -1129,7 +1102,7 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
 		 * Motion detection mode cannot run at the same time as
 		 * acceleration data being read.
 		 */
-		if ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+		if ((ret & SCA3000_REG_MODE_MODE_MASK)
 		    != SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
 			ret = 0;
 		} else {
@@ -1157,20 +1130,20 @@ static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
 	int ret;
 
 	/* read current value of mode register */
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		return ret;
 
 	/* if off and should be on */
-	if (state && !(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
+	if (state && !(ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
 		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
-					 st->rx[0] | SCA3000_REG_MODE_FREE_FALL_DETECT);
+					 ret | SCA3000_REG_MODE_FREE_FALL_DETECT);
 	/* if on and should be off */
-	else if (!state && (st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
+	if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
 		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
-					 st->rx[0] & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
-	else
-		return 0;
+					 ret & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
+
+	return 0;
 }
 
 static int sca3000_motion_detect_set_state(struct iio_dev *indio_dev, int axis,
@@ -1207,22 +1180,22 @@ static int sca3000_motion_detect_set_state(struct iio_dev *indio_dev, int axis,
 	}
 
 	/* read current value of mode register */
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		return ret;
 	/* if off and should be on */
 	if ((st->mo_det_use_count) &&
-	    ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+	    ((ret & SCA3000_REG_MODE_MODE_MASK)
 	     != SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
 		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
-			(st->rx[0] & ~SCA3000_REG_MODE_MODE_MASK)
+			(ret & ~SCA3000_REG_MODE_MODE_MASK)
 			| SCA3000_REG_MODE_MEAS_MODE_MOT_DET);
 	/* if on and should be off */
 	else if (!(st->mo_det_use_count) &&
-		 ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+		 ((ret & SCA3000_REG_MODE_MODE_MASK)
 		  == SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
 		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
-			st->rx[0] & SCA3000_REG_MODE_MODE_MASK);
+			ret & SCA3000_REG_MODE_MODE_MASK);
 	else
 		return 0;
 }
@@ -1280,18 +1253,18 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		goto error_ret;
 	if (state) {
 		dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
 		ret = sca3000_write_reg(st,
 			SCA3000_REG_MODE_ADDR,
-			(st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
+			ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
 	} else
 		ret = sca3000_write_reg(st,
 			SCA3000_REG_MODE_ADDR,
-			(st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
+			ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
 error_ret:
 	mutex_unlock(&st->lock);
 
@@ -1315,12 +1288,12 @@ static int sca3000_hw_ring_preenable(struct iio_dev *indio_dev)
 	mutex_lock(&st->lock);
 
 	/* Enable the 50% full interrupt */
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
 		goto error_unlock;
 	ret = sca3000_write_reg(st,
 				SCA3000_REG_INT_MASK_ADDR,
-				st->rx[0] | SCA3000_REG_INT_MASK_RING_HALF);
+				ret | SCA3000_REG_INT_MASK_RING_HALF);
 	if (ret)
 		goto error_unlock;
 
@@ -1346,12 +1319,12 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
 	/* Disable the 50% full interrupt */
 	mutex_lock(&st->lock);
 
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
 		goto unlock;
 	ret = sca3000_write_reg(st,
 				SCA3000_REG_INT_MASK_ADDR,
-				st->rx[0] & ~SCA3000_REG_INT_MASK_RING_HALF);
+				ret & ~SCA3000_REG_INT_MASK_RING_HALF);
 unlock:
 	mutex_unlock(&st->lock);
 	return ret;
@@ -1376,7 +1349,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 
 	mutex_lock(&st->lock);
 	/* Ensure all interrupts have been acknowledged */
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
 		goto error_ret;
 
@@ -1402,7 +1375,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 	if (ret)
 		goto error_ret;
 	/* Enable interrupts, relevant to mode and set up as active low */
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
 		goto error_ret;
 	ret = sca3000_write_reg(st,
@@ -1416,11 +1389,11 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 	 * Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
 	 * as that occurs in one of the example on the datasheet
 	 */
-	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
 		goto error_ret;
 	ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
-				(st->rx[0] & SCA3000_MODE_PROT_MASK));
+				ret & SCA3000_MODE_PROT_MASK);
 
 error_ret:
 	mutex_unlock(&st->lock);
@@ -1503,14 +1476,15 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
 		goto error_ret;
+
 	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
-				(st->rx[0] &
-				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
-				   SCA3000_REG_INT_MASK_RING_HALF |
-				   SCA3000_REG_INT_MASK_ALL_INTS)));
+				ret &
+				~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
+				  SCA3000_REG_INT_MASK_RING_HALF |
+				  SCA3000_REG_INT_MASK_ALL_INTS));
 error_ret:
 	mutex_unlock(&st->lock);
 	return ret;
-- 
2.49.0

[PATCH v6 2/4] iio: accel: sca3000: clean sca3000_read_data()

[Andrew Ijano]

Clean internal sca3000_read_data() helper by removing unnecessary
arguments and return logic.

Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
---
 drivers/iio/accel/sca3000.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index c85a06cbea37..7145b4541264 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -939,7 +939,6 @@ static const struct attribute_group sca3000_attribute_group = {
 
 static int sca3000_read_data(struct sca3000_state *st,
 			     u8 reg_address_high,
-			     u8 *rx,
 			     int len)
 {
 	int ret;
@@ -949,18 +948,15 @@ static int sca3000_read_data(struct sca3000_state *st,
 			.tx_buf = st->tx,
 		}, {
 			.len = len,
-			.rx_buf = rx,
+			.rx_buf = st->rx,
 		}
 	};
 
 	st->tx[0] = SCA3000_READ_REG(reg_address_high);
 	ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
-	if (ret) {
+	if (ret)
 		dev_err(&st->us->dev, "problem reading register\n");
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 /**
@@ -984,8 +980,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
 		 * num_available is the total number of samples available
 		 * i.e. number of time points * number of channels.
 		 */
-		ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
-					num_available * 2);
+		ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, num_available * 2);
 		if (ret)
 			goto error_ret;
 		for (i = 0; i < num_available / 3; i++) {
-- 
2.49.0

[PATCH v6 3/4] iio: accel: sca3000: use lock guards

[Andrew Ijano]

Use guard() and scoped_guard() for handling mutex lock instead of
manually locking and unlocking the mutex. This prevents forgotten
locks due to early exits and remove the need of gotos.

Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/iio/accel/sca3000.c | 205 +++++++++++++-----------------------
 1 file changed, 73 insertions(+), 132 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 7145b4541264..058a2d67c91c 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -405,17 +405,14 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
 	int ret;
 	struct sca3000_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
 	if (ret < 0)
-		goto error_ret;
+		return ret;
 	dev_info(&indio_dev->dev,
 		 "sca3000 revision major=%lu, minor=%lu\n",
 		 ret & SCA3000_REG_REVID_MAJOR_MASK,
 		 ret & SCA3000_REG_REVID_MINOR_MASK);
-error_ret:
-	mutex_unlock(&st->lock);
-
 	return ret;
 }
 
@@ -698,33 +695,27 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
 	u8 address;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&st->lock);
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&st->lock);
 		if (chan->type == IIO_ACCEL) {
-			if (st->mo_det_use_count) {
-				mutex_unlock(&st->lock);
+			if (st->mo_det_use_count)
 				return -EBUSY;
-			}
 			address = sca3000_addresses[chan->address][0];
 			ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
-			if (ret < 0) {
-				mutex_unlock(&st->lock);
+			if (ret < 0)
 				return ret;
-			}
 			*val = sign_extend32(ret >> chan->scan_type.shift,
 					     chan->scan_type.realbits - 1);
 		} else {
 			/* get the temperature when available */
 			ret = spi_w8r16be(st->us, SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
-			if (ret < 0) {
-				mutex_unlock(&st->lock);
+			if (ret < 0)
 				return ret;
-			}
 			*val = (ret >> chan->scan_type.shift) &
 				GENMASK(chan->scan_type.realbits - 1, 0);
 		}
-		mutex_unlock(&st->lock);
 		return IIO_VAL_INT;
+	}
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
 		if (chan->type == IIO_ACCEL)
@@ -736,16 +727,16 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
 		*val = -214;
 		*val2 = 600000;
 		return IIO_VAL_INT_PLUS_MICRO;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		mutex_lock(&st->lock);
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		guard(mutex)(&st->lock);
 		ret = sca3000_read_raw_samp_freq(st, val);
-		mutex_unlock(&st->lock);
 		return ret ? ret : IIO_VAL_INT;
-	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		mutex_lock(&st->lock);
+	}
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
+		guard(mutex)(&st->lock);
 		ret = sca3000_read_3db_freq(st, val);
-		mutex_unlock(&st->lock);
 		return ret;
+	}
 	default:
 		return -EINVAL;
 	}
@@ -756,28 +747,23 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int ret;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_SAMP_FREQ:
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		guard(mutex)(&st->lock);
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&st->lock);
-		ret = sca3000_write_raw_samp_freq(st, val);
-		mutex_unlock(&st->lock);
-		return ret;
-	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return sca3000_write_raw_samp_freq(st, val);
+	}
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
+		guard(mutex)(&st->lock);
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&st->lock);
-		ret = sca3000_write_3db_freq(st, val);
-		mutex_unlock(&st->lock);
-		return ret;
+		return sca3000_write_3db_freq(st, val);
+	}
 	default:
 		return -EINVAL;
 	}
-
-	return ret;
 }
 
 /**
@@ -800,9 +786,9 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
 	unsigned int len = 0;
 	int ret;
 
-	mutex_lock(&st->lock);
-	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
-	mutex_unlock(&st->lock);
+	scoped_guard(mutex, &st->lock) {
+		ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
+	}
 	if (ret)
 		return ret;
 
@@ -850,11 +836,10 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
 	int i;
 
 	switch (info) {
-	case IIO_EV_INFO_VALUE:
-		mutex_lock(&st->lock);
-		ret = sca3000_read_ctrl_reg(st,
-					    sca3000_addresses[chan->address][1]);
-		mutex_unlock(&st->lock);
+	case IIO_EV_INFO_VALUE: {
+		scoped_guard(mutex, &st->lock) {
+			ret = sca3000_read_ctrl_reg(st, sca3000_addresses[chan->address][1]);
+		}
 		if (ret < 0)
 			return ret;
 		*val = 0;
@@ -868,6 +853,7 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
 				*val += st->info->mot_det_mult_xz[i];
 
 		return IIO_VAL_INT;
+	}
 	case IIO_EV_INFO_PERIOD:
 		*val = 0;
 		*val2 = 226000;
@@ -898,7 +884,6 @@ static int sca3000_write_event_value(struct iio_dev *indio_dev,
 				     int val, int val2)
 {
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int ret;
 	int i;
 	u8 nonlinear = 0;
 
@@ -918,13 +903,10 @@ static int sca3000_write_event_value(struct iio_dev *indio_dev,
 			}
 	}
 
-	mutex_lock(&st->lock);
-	ret = sca3000_write_ctrl_reg(st,
+	guard(mutex)(&st->lock);
+	return sca3000_write_ctrl_reg(st,
 				     sca3000_addresses[chan->address][1],
 				     nonlinear);
-	mutex_unlock(&st->lock);
-
-	return ret;
 }
 
 static struct attribute *sca3000_attributes[] = {
@@ -969,12 +951,12 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret, i, num_available;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	if (val & SCA3000_REG_INT_STATUS_HALF) {
 		ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
 		if (ret)
-			goto error_ret;
+			return;
 		num_available = ret;
 		/*
 		 * num_available is the total number of samples available
@@ -982,7 +964,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
 		 */
 		ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, num_available * 2);
 		if (ret)
-			goto error_ret;
+			return;
 		for (i = 0; i < num_available / 3; i++) {
 			/*
 			 * Dirty hack to cover for 11 bit in fifo, 13 bit
@@ -994,8 +976,6 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
 			iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
 		}
 	}
-error_ret:
-	mutex_unlock(&st->lock);
 }
 
 /**
@@ -1021,9 +1001,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
 	 * Could lead if badly timed to an extra read of status reg,
 	 * but ensures no interrupt is missed.
 	 */
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
-	mutex_unlock(&st->lock);
 	if (ret)
 		goto done;
 
@@ -1080,16 +1059,15 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret;
 	/* read current value of mode register */
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
-		goto error_ret;
+		return ret;
 
 	switch (chan->channel2) {
 	case IIO_MOD_X_AND_Y_AND_Z:
-		ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
-		break;
+		return !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
 	case IIO_MOD_X:
 	case IIO_MOD_Y:
 	case IIO_MOD_Z:
@@ -1099,24 +1077,18 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
 		 */
 		if ((ret & SCA3000_REG_MODE_MODE_MASK)
 		    != SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
-			ret = 0;
+			return 0;
 		} else {
 			ret = sca3000_read_ctrl_reg(st,
 						SCA3000_REG_CTRL_SEL_MD_CTRL);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 			/* only supporting logical or's for now */
-			ret = !!(ret & sca3000_addresses[chan->address][2]);
+			return !!(ret & sca3000_addresses[chan->address][2]);
 		}
-		break;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret;
 }
 
 static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
@@ -1217,28 +1189,20 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
 				      bool state)
 {
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	switch (chan->channel2) {
 	case IIO_MOD_X_AND_Y_AND_Z:
-		ret = sca3000_freefall_set_state(indio_dev, state);
-		break;
-
+		return sca3000_freefall_set_state(indio_dev, state);
 	case IIO_MOD_X:
 	case IIO_MOD_Y:
 	case IIO_MOD_Z:
-		ret = sca3000_motion_detect_set_state(indio_dev,
+		return sca3000_motion_detect_set_state(indio_dev,
 						      chan->address,
 						      state);
-		break;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
-	mutex_unlock(&st->lock);
-
-	return ret;
 }
 
 static inline
@@ -1247,23 +1211,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
 	struct sca3000_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
-		goto error_ret;
+		return ret;
+
 	if (state) {
 		dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
-		ret = sca3000_write_reg(st,
+		return sca3000_write_reg(st,
 			SCA3000_REG_MODE_ADDR,
 			ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
-	} else
-		ret = sca3000_write_reg(st,
-			SCA3000_REG_MODE_ADDR,
-			ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret;
+	}
+	return sca3000_write_reg(st,
+		SCA3000_REG_MODE_ADDR,
+		ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
 }
 
 /**
@@ -1280,26 +1240,18 @@ static int sca3000_hw_ring_preenable(struct iio_dev *indio_dev)
 	int ret;
 	struct sca3000_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->lock);
-
+	guard(mutex)(&st->lock);
 	/* Enable the 50% full interrupt */
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
-		goto error_unlock;
+		return ret;
 	ret = sca3000_write_reg(st,
 				SCA3000_REG_INT_MASK_ADDR,
 				ret | SCA3000_REG_INT_MASK_RING_HALF);
 	if (ret)
-		goto error_unlock;
-
-	mutex_unlock(&st->lock);
+		return ret;
 
 	return __sca3000_hw_ring_state_set(indio_dev, 1);
-
-error_unlock:
-	mutex_unlock(&st->lock);
-
-	return ret;
 }
 
 static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
@@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
 	int ret;
 	struct sca3000_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	ret = __sca3000_hw_ring_state_set(indio_dev, 0);
 	if (ret)
 		return ret;
 
 	/* Disable the 50% full interrupt */
-	mutex_lock(&st->lock);
-
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
-		goto unlock;
-	ret = sca3000_write_reg(st,
+		return ret;
+	return sca3000_write_reg(st,
 				SCA3000_REG_INT_MASK_ADDR,
 				ret & ~SCA3000_REG_INT_MASK_RING_HALF);
-unlock:
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
@@ -1342,25 +1290,25 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 {
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	/* Ensure all interrupts have been acknowledged */
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
-		goto error_ret;
+		return ret;
 
 	/* Turn off all motion detection channels */
 	ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL);
 	if (ret < 0)
-		goto error_ret;
+		return ret;
 	ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL,
 				     ret & SCA3000_MD_CTRL_PROT_MASK);
 	if (ret)
-		goto error_ret;
+		return ret;
 
 	/* Disable ring buffer */
 	ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL);
 	if (ret < 0)
-		goto error_ret;
+		return ret;
 	ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL,
 				     (ret & SCA3000_REG_OUT_CTRL_PROT_MASK)
 				     | SCA3000_REG_OUT_CTRL_BUF_X_EN
@@ -1368,17 +1316,17 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 				     | SCA3000_REG_OUT_CTRL_BUF_Z_EN
 				     | SCA3000_REG_OUT_CTRL_BUF_DIV_4);
 	if (ret)
-		goto error_ret;
+		return ret;
 	/* Enable interrupts, relevant to mode and set up as active low */
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
-		goto error_ret;
+		return ret;
 	ret = sca3000_write_reg(st,
 				SCA3000_REG_INT_MASK_ADDR,
 				(ret & SCA3000_REG_INT_MASK_PROT_MASK)
 				| SCA3000_REG_INT_MASK_ACTIVE_LOW);
 	if (ret)
-		goto error_ret;
+		return ret;
 	/*
 	 * Select normal measurement mode, free fall off, ring off
 	 * Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
@@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
 	 */
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
 	if (ret)
-		goto error_ret;
-	ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
+		return ret;
+	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
 				ret & SCA3000_MODE_PROT_MASK);
-
-error_ret:
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 static const struct iio_info sca3000_info = {
@@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
 {
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
 	if (ret)
-		goto error_ret;
+		return ret;
 
-	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
+	return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
 				ret &
 				~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
 				  SCA3000_REG_INT_MASK_RING_HALF |
 				  SCA3000_REG_INT_MASK_ALL_INTS));
-error_ret:
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 static void sca3000_remove(struct spi_device *spi)
-- 
2.49.0

[PATCH v6 4/4] iio: accel: sca3000: use sysfs_emit_at() instead of sprintf()

[Andrew Ijano]

Use sysfs_emit_at() instead of sprintf() for sysfs operations as
suggested in the documentation, since it is aware of PAGE_SIZE buffer.

Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/sca3000.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 058a2d67c91c..bc0046b19511 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -423,16 +423,16 @@ sca3000_show_available_3db_freqs(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct sca3000_state *st = iio_priv(indio_dev);
-	int len;
+	unsigned int len = 0;
 
-	len = sprintf(buf, "%d", st->info->measurement_mode_3db_freq);
+	len = sysfs_emit_at(buf, len, "%d", st->info->measurement_mode_3db_freq);
 	if (st->info->option_mode_1)
-		len += sprintf(buf + len, " %d",
+		len += sysfs_emit_at(buf, len, " %d",
 			       st->info->option_mode_1_3db_freq);
 	if (st->info->option_mode_2)
-		len += sprintf(buf + len, " %d",
+		len += sysfs_emit_at(buf, len, " %d",
 			       st->info->option_mode_2_3db_freq);
-	len += sprintf(buf + len, "\n");
+	len += sysfs_emit_at(buf, len, "\n");
 
 	return len;
 }
@@ -783,7 +783,6 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct sca3000_state *st = iio_priv(indio_dev);
-	unsigned int len = 0;
 	int ret;
 
 	scoped_guard(mutex, &st->lock) {
@@ -794,25 +793,22 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
 
 	switch (ret & SCA3000_REG_MODE_MODE_MASK) {
 	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
-		len += sprintf(buf + len, "%d %d %d\n",
+		return sysfs_emit(buf, "%d %d %d\n",
 			       st->info->measurement_mode_freq,
 			       st->info->measurement_mode_freq / 2,
 			       st->info->measurement_mode_freq / 4);
-		break;
 	case SCA3000_REG_MODE_MEAS_MODE_OP_1:
-		len += sprintf(buf + len, "%d %d %d\n",
+		return sysfs_emit(buf, "%d %d %d\n",
 			       st->info->option_mode_1_freq,
 			       st->info->option_mode_1_freq / 2,
 			       st->info->option_mode_1_freq / 4);
-		break;
 	case SCA3000_REG_MODE_MEAS_MODE_OP_2:
-		len += sprintf(buf + len, "%d %d %d\n",
+		return sysfs_emit(buf, "%d %d %d\n",
 			       st->info->option_mode_2_freq,
 			       st->info->option_mode_2_freq / 2,
 			       st->info->option_mode_2_freq / 4);
-		break;
 	}
-	return len;
+	return 0;
 }
 
 /*
-- 
2.49.0

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andy Shevchenko]

On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
>
> The sca3000 driver is old and could be simplified by using newer
> infrastructure.

I haven't found any reference to a base commit here. Have you
forgotten to use --base when preparing the series?
In any case, please clarify what this series is based on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andrew Ijano]

On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> >
> > The sca3000 driver is old and could be simplified by using newer
> > infrastructure.
>
> I haven't found any reference to a base commit here. Have you
> forgotten to use --base when preparing the series?
> In any case, please clarify what this series is based on.

Thank you for pointing this out! I think I forgot to use --base for
it. In this case, should I submit a new version of the whole patchset
with this information or is there a better way to do it?

Thanks,
Andrew

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andy Shevchenko]

On Wed, Jun 18, 2025 at 09:24:19AM -0300, Andrew Ijano wrote:
> On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > >
> > > The sca3000 driver is old and could be simplified by using newer
> > > infrastructure.
> >
> > I haven't found any reference to a base commit here. Have you
> > forgotten to use --base when preparing the series?
> > In any case, please clarify what this series is based on.
> 
> Thank you for pointing this out! I think I forgot to use --base for
> it. In this case, should I submit a new version of the whole patchset
> with this information or is there a better way to do it?

For now just reply here what is the base. I asked this question above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andrew Ijano]

On Wed, Jun 18, 2025 at 2:41 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Jun 18, 2025 at 09:24:19AM -0300, Andrew Ijano wrote:
> > On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > > >
> > > > The sca3000 driver is old and could be simplified by using newer
> > > > infrastructure.
> > >
> > > I haven't found any reference to a base commit here. Have you
> > > forgotten to use --base when preparing the series?
> > > In any case, please clarify what this series is based on.
> >
> > Thank you for pointing this out! I think I forgot to use --base for
> > it. In this case, should I submit a new version of the whole patchset
> > with this information or is there a better way to do it?
>
> For now just reply here what is the base. I asked this question above.
>

Ok! No problem. So the base for this patchset is the commit
3c23416f69f2870bea83697d7ab03c6a8497daa7.

Thanks,
Andrew

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andy Shevchenko]

On Wed, Jun 18, 2025 at 03:20:06PM -0300, Andrew Ijano wrote:
> On Wed, Jun 18, 2025 at 2:41 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Jun 18, 2025 at 09:24:19AM -0300, Andrew Ijano wrote:
> > > On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > > > >
> > > > > The sca3000 driver is old and could be simplified by using newer
> > > > > infrastructure.
> > > >
> > > > I haven't found any reference to a base commit here. Have you
> > > > forgotten to use --base when preparing the series?
> > > > In any case, please clarify what this series is based on.
> > >
> > > Thank you for pointing this out! I think I forgot to use --base for
> > > it. In this case, should I submit a new version of the whole patchset
> > > with this information or is there a better way to do it?
> >
> > For now just reply here what is the base. I asked this question above.
> 
> Ok! No problem. So the base for this patchset is the commit
> 3c23416f69f2870bea83697d7ab03c6a8497daa7.

No such commit in the repository. :-(
You are doing something interesting here [1].

So, make sure you are based on the iio/testing or so, make sure that the base
commit is the one that may be found on git.kernel.org. Use that in the next
version. Due to above this version is ambiguous to even start reviewing it.

[1] I have connected IIO subsystem as a remote, so I have access to many trees
from kernel.org (but not to all of them).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Jonathan Cameron]

On Wed, 18 Jun 2025 00:12:16 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Remove sca3000_read_data_short() function, replacing it by spi_w8r8()
> and spi_w8r16be() helpers.
> 
> This is an old driver that was not making full use of the newer
> infrastructure.
> 
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>

Hi. I made two related tweaks.  A few comments inline for further possible cleanup.

Applied to the togreg branch of iio.git and initially pushed out as testing
for 0-day to take a look at it.

Thanks,

Jonathan


diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index c85a06cbea37..eb0cad22474e 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -589,8 +589,7 @@ static int sca3000_read_raw_samp_freq(struct sca3000_state *st, int *val)
                return ret;
 
        if (*val > 0) {
-               ret &= SCA3000_REG_OUT_CTRL_BUF_DIV_MASK;
-               switch (ret) {
+               switch (ret & SCA3000_REG_OUT_CTRL_BUF_DIV_MASK) {
                case SCA3000_REG_OUT_CTRL_BUF_DIV_2:
                        *val /= 2;
                        break;
@@ -644,8 +643,7 @@ static int sca3000_read_3db_freq(struct sca3000_state *st, int *val)
                return ret;
 
        /* mask bottom 2 bits - only ones that are relevant */
-       ret &= SCA3000_REG_MODE_MODE_MASK;
-       switch (ret) {
+       switch (ret & SCA3000_REG_MODE_MODE_MASK) {
        case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
                *val = st->info->measurement_mode_3db_freq;
                return IIO_VAL_INT;



>  /**
> @@ -427,13 +406,13 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  
>  	mutex_lock(&st->lock);
> -	ret = sca3000_read_data_short(st, SCA3000_REG_REVID_ADDR, 1);
> +	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
>  	if (ret < 0)
>  		goto error_ret;
>  	dev_info(&indio_dev->dev,
>  		 "sca3000 revision major=%lu, minor=%lu\n",
> -		 st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
> -		 st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
> +		 ret & SCA3000_REG_REVID_MAJOR_MASK,
> +		 ret & SCA3000_REG_REVID_MINOR_MASK);
Hmm. doesn't belong in this patch but it is rather odd to not see
a shift on one of these.

Hmm. Time to miss quote whoever it was who said:

"kernel development is like a murder mystery where you try to solve
 the crime only to realize you were the murderer."

Below I mention using FIELD_GET() in appropriate places as a possible additional
cleanup.  Fix this up when you do that (and note it in the patch description for
that patch).

>  error_ret:
>  	mutex_unlock(&st->lock);
>  
> @@ -570,7 +549,7 @@ static inline int __sca3000_get_base_freq(struct sca3000_state *st,
>  {
>  	int ret;
>  
> -	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> +	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
>  		return ret;
>  
> @@ -660,13 +639,13 @@ static int sca3000_read_3db_freq(struct sca3000_state *st, int *val)
>  {
>  	int ret;
>  
> -	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> +	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
>  		return ret;
>  
>  	/* mask bottom 2 bits - only ones that are relevant */
> -	st->rx[0] &= SCA3000_REG_MODE_MODE_MASK;
> -	switch (st->rx[0]) {
> +	ret &= SCA3000_REG_MODE_MODE_MASK;
> +	switch (ret) {
See discussion below.  This can be simplified as
	switch (reg & SCA3000_REG_MODE_MASK)
avoiding the modified 'ret' value in place comment.

This one I'll tweak as it is easy to avoid the ugly pattern.

>  	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
>  		*val = st->info->measurement_mode_3db_freq;
>  		return IIO_VAL_INT;
> @@ -698,14 +677,14 @@ static int sca3000_write_3db_freq(struct sca3000_state *st, int val)
>  		mode = SCA3000_REG_MODE_MEAS_MODE_OP_2;
>  	else
>  		return -EINVAL;
> -	ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> +	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
>  		return ret;
>  
> -	st->rx[0] &= ~SCA3000_REG_MODE_MODE_MASK;
> -	st->rx[0] |= (mode & SCA3000_REG_MODE_MODE_MASK);
> +	ret &= ~SCA3000_REG_MODE_MODE_MASK;

For a further potential cleanup it would be good to use FIELD_GET() and FIELD_PREP()
in appropriate places in this driver. Do that into a separate local variable
as using ret for all this is a little confusing as quite a bit of the time
it's not a variable we are ever going to return.

As a rule of thumb if we are modifying the ret only a little in a single reuse
(i.e. masking out a bit in a parameter being passed to something else) then
a local variable is probably overkill. If we are building up a new register
value it is not really appropriate to do it into ret.

I'm not asking for changes in this patch though as what you have here
is the simplest and easiest to review change.  If you add those extra
local variables as part of using FIELD_GET/ FIELD_PREP though that would
be great.



> +	ret |= mode & SCA3000_REG_MODE_MODE_MASK;
>  
> -	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, st->rx[0]);
> +	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, ret);
>  }

Re: [PATCH v6 2/4] iio: accel: sca3000: clean sca3000_read_data()

[Jonathan Cameron]

On Wed, 18 Jun 2025 00:12:17 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Clean internal sca3000_read_data() helper by removing unnecessary
> arguments and return logic.
> 
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Applied

Re: [PATCH v6 3/4] iio: accel: sca3000: use lock guards

[Jonathan Cameron]

On Wed, 18 Jun 2025 00:12:18 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Use guard() and scoped_guard() for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.

Please add extra description for where you have pushed locks up
a level in the call tree and why that is fine to do.

> 
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>

A few comments inline.

Thanks

Jonathan

> @@ -756,28 +747,23 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
>  			     int val, int val2, long mask)
>  {
>  	struct sca3000_state *st = iio_priv(indio_dev);
> -	int ret;
>  
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		guard(mutex)(&st->lock);
As below

>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&st->lock);
> -		ret = sca3000_write_raw_samp_freq(st, val);
> -		mutex_unlock(&st->lock);
> -		return ret;
> -	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		return sca3000_write_raw_samp_freq(st, val);
> +	}
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> +		guard(mutex)(&st->lock);
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&st->lock);

You can keep the old ordering here.  It doesn't matter much but
easier to be sure about a patch that makes no functional change.

> -		ret = sca3000_write_3db_freq(st, val);
> -		mutex_unlock(&st->lock);
> -		return ret;
> +		return sca3000_write_3db_freq(st, val);
> +	}

>  
>  /**
> @@ -1021,9 +1001,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
>  	 * Could lead if badly timed to an extra read of status reg,
>  	 * but ensures no interrupt is missed.
>  	 */
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

This is a very large increase in scope.  Use scoped_guard() here instead to avoid
holding the lock over a whole load of code that doesn't need it.

>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> -	mutex_unlock(&st->lock);
>  	if (ret)
>  		goto done;
>  

>  }

>  
>  static inline
> @@ -1247,23 +1211,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
> -		goto error_ret;
> +		return ret;
> +
>  	if (state) {
>  		dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> -		ret = sca3000_write_reg(st,
> +		return sca3000_write_reg(st,
>  			SCA3000_REG_MODE_ADDR,
>  			ret | SCA3000_REG_MODE_RING_BUF_ENABLE);

This indent was already nasty so as we are touching the code good to clean it up.
For cases like this we can be more relaxed and if it helps readability go a little
over 80 chars (I think this will be 80 ish)

		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
					 ret | SCA3000_REG_MODE_RING_BUF_ENABLE);


> -	} else
> -		ret = sca3000_write_reg(st,
> -			SCA3000_REG_MODE_ADDR,
> -			ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
> -error_ret:
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> +	}
> +	return sca3000_write_reg(st,
> +		SCA3000_REG_MODE_ADDR,
> +		ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
>  }
>
>  
>  static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
>  	int ret;
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  
> +	guard(mutex)(&st->lock);

See comment at the top - Making this change is fine but call it out in the patch
description as it isn't simple change to using guards, but instead to holding
the lock for longer.  Change is fine but point it out as it needs
more review than the mechanical changes.

>  	ret = __sca3000_hw_ring_state_set(indio_dev, 0);
>  	if (ret)
>  		return ret;
>  
>  	/* Disable the 50% full interrupt */
> -	mutex_lock(&st->lock);
> -
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
>  	if (ret)
> -		goto unlock;
> -	ret = sca3000_write_reg(st,
> +		return ret;
> +	return sca3000_write_reg(st,
>  				SCA3000_REG_INT_MASK_ADDR,
>  				ret & ~SCA3000_REG_INT_MASK_RING_HALF);

As below.

> -unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }

> @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
>  	 */
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
> -		goto error_ret;
> -	ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> +		return ret;
> +	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
>  				ret & SCA3000_MODE_PROT_MASK);

As below.

> -
> -error_ret:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }
>  
>  static const struct iio_info sca3000_info = {
> @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
>  {
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
>  	if (ret)
> -		goto error_ret;
> +		return ret;
>  
> -	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> +	return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
This adds a character to this line which means that either the indent of
the following lines was previously wrong, or it is now.
I think you need to add a space to the following lines.

Check for other similar cases.

>  				ret &
>  				~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
>  				  SCA3000_REG_INT_MASK_RING_HALF |
>  				  SCA3000_REG_INT_MASK_ALL_INTS));

Re: [PATCH v6 4/4] iio: accel: sca3000: use sysfs_emit_at() instead of sprintf()

[Jonathan Cameron]

On Wed, 18 Jun 2025 00:12:19 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Use sysfs_emit_at() instead of sprintf() for sysfs operations as
> suggested in the documentation, since it is aware of PAGE_SIZE buffer.
> 
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi.

A few comments inline,

Thanks,

J
> ---
>  drivers/iio/accel/sca3000.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 058a2d67c91c..bc0046b19511 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -423,16 +423,16 @@ sca3000_show_available_3db_freqs(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct sca3000_state *st = iio_priv(indio_dev);
> -	int len;
> +	unsigned int len = 0;

No need to initialize as set on the next line

>  
> -	len = sprintf(buf, "%d", st->info->measurement_mode_3db_freq);
> +	len = sysfs_emit_at(buf, len, "%d", st->info->measurement_mode_3db_freq);

sysfs_emit() when you know you are at the start.


>  	if (st->info->option_mode_1)
> -		len += sprintf(buf + len, " %d",
> +		len += sysfs_emit_at(buf, len, " %d",
>  			       st->info->option_mode_1_3db_freq);
Fix alignment.

>  	if (st->info->option_mode_2)
> -		len += sprintf(buf + len, " %d",
> +		len += sysfs_emit_at(buf, len, " %d",
>  			       st->info->option_mode_2_3db_freq);

same here.

> -	len += sprintf(buf + len, "\n");
> +	len += sysfs_emit_at(buf, len, "\n");
>  
>  	return len;
>  }
> @@ -783,7 +783,6 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct sca3000_state *st = iio_priv(indio_dev);
> -	unsigned int len = 0;
>  	int ret;
>  
>  	scoped_guard(mutex, &st->lock) {
> @@ -794,25 +793,22 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
>  
>  	switch (ret & SCA3000_REG_MODE_MODE_MASK) {
>  	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
> -		len += sprintf(buf + len, "%d %d %d\n",
> +		return sysfs_emit(buf, "%d %d %d\n",
>  			       st->info->measurement_mode_freq,
>  			       st->info->measurement_mode_freq / 2,
>  			       st->info->measurement_mode_freq / 4);
> -		break;
>  	case SCA3000_REG_MODE_MEAS_MODE_OP_1:
> -		len += sprintf(buf + len, "%d %d %d\n",
> +		return sysfs_emit(buf, "%d %d %d\n",
>  			       st->info->option_mode_1_freq,
>  			       st->info->option_mode_1_freq / 2,
>  			       st->info->option_mode_1_freq / 4);
> -		break;
>  	case SCA3000_REG_MODE_MEAS_MODE_OP_2:
> -		len += sprintf(buf + len, "%d %d %d\n",
> +		return sysfs_emit(buf, "%d %d %d\n",
>  			       st->info->option_mode_2_freq,
>  			       st->info->option_mode_2_freq / 2,
>  			       st->info->option_mode_2_freq / 4);
> -		break;
>  	}
> -	return len;
> +	return 0;
>  }
>  
>  /*

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andrew Ijano]

On Thu, Jun 19, 2025 at 12:23 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Jun 18, 2025 at 03:20:06PM -0300, Andrew Ijano wrote:
> > On Wed, Jun 18, 2025 at 2:41 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Wed, Jun 18, 2025 at 09:24:19AM -0300, Andrew Ijano wrote:
> > > > On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > > > > >
> > > > > > The sca3000 driver is old and could be simplified by using newer
> > > > > > infrastructure.
> > > > >
> > > > > I haven't found any reference to a base commit here. Have you
> > > > > forgotten to use --base when preparing the series?
> > > > > In any case, please clarify what this series is based on.
> > > >
> > > > Thank you for pointing this out! I think I forgot to use --base for
> > > > it. In this case, should I submit a new version of the whole patchset
> > > > with this information or is there a better way to do it?
> > >
> > > For now just reply here what is the base. I asked this question above.
> >
> > Ok! No problem. So the base for this patchset is the commit
> > 3c23416f69f2870bea83697d7ab03c6a8497daa7.
>
> No such commit in the repository. :-(
> You are doing something interesting here [1].
>
> So, make sure you are based on the iio/testing or so, make sure that the base
> commit is the one that may be found on git.kernel.org. Use that in the next
> version. Due to above this version is ambiguous to even start reviewing it.
>
> [1] I have connected IIO subsystem as a remote, so I have access to many trees
> from kernel.org (but not to all of them).
>

Hi, Andy. Sorry for the late response.

Actually, I think I didn't fully understand this part of the
contribution process and that's what was causing confusion.
Basically, the base commit appeared in the previous version of this
patchset but I removed it after it was approved, to prevent it from
being reviewed again. However, I think I could just add the
reviewed-by tag.

I'll send a next version with other corrections and the missing commit
based on iio/testing.

Thanks,
Andrew

Re: [PATCH v6 1/4] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Andrew Ijano]

On Sat, Jun 21, 2025 at 2:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Hi. I made two related tweaks.  A few comments inline for further possible cleanup.
>
> Applied to the togreg branch of iio.git and initially pushed out as testing
> for 0-day to take a look at it.
>
Thanks. Since there was a problem with my implementation, I'll send a
new version along with your tweaks too.

> >       if (ret < 0)
> >               goto error_ret;
> >       dev_info(&indio_dev->dev,
> >                "sca3000 revision major=%lu, minor=%lu\n",
> > -              st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
> > -              st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
> > +              ret & SCA3000_REG_REVID_MAJOR_MASK,
> > +              ret & SCA3000_REG_REVID_MINOR_MASK);
> Hmm. doesn't belong in this patch but it is rather odd to not see
> a shift on one of these.
>
> Hmm. Time to miss quote whoever it was who said:
>
> "kernel development is like a murder mystery where you try to solve
>  the crime only to realize you were the murderer."
>
> Below I mention using FIELD_GET() in appropriate places as a possible additional
> cleanup.  Fix this up when you do that (and note it in the patch description for
> that patch).
Ok! I'll do that.

> >       /* mask bottom 2 bits - only ones that are relevant */
> > -     st->rx[0] &= SCA3000_REG_MODE_MODE_MASK;
> > -     switch (st->rx[0]) {
> > +     ret &= SCA3000_REG_MODE_MODE_MASK;
> > +     switch (ret) {
> See discussion below.  This can be simplified as
>         switch (reg & SCA3000_REG_MODE_MASK)
> avoiding the modified 'ret' value in place comment.
>
> This one I'll tweak as it is easy to avoid the ugly pattern.
>
Nice! I'll pay attention to similar cases in the future.

> >
> > -     st->rx[0] &= ~SCA3000_REG_MODE_MODE_MASK;
> > -     st->rx[0] |= (mode & SCA3000_REG_MODE_MODE_MASK);
> > +     ret &= ~SCA3000_REG_MODE_MODE_MASK;
>
> For a further potential cleanup it would be good to use FIELD_GET() and FIELD_PREP()
> in appropriate places in this driver. Do that into a separate local variable
> as using ret for all this is a little confusing as quite a bit of the time
> it's not a variable we are ever going to return.
>
> As a rule of thumb if we are modifying the ret only a little in a single reuse
> (i.e. masking out a bit in a parameter being passed to something else) then
> a local variable is probably overkill. If we are building up a new register
> value it is not really appropriate to do it into ret.
>
> I'm not asking for changes in this patch though as what you have here
> is the simplest and easiest to review change.  If you add those extra
> local variables as part of using FIELD_GET/ FIELD_PREP though that would
> be great.

That's great! I didn't know about FIELD_GET() and FIELD_PREP() before.
This is really something that would be better in a separate patch, I
could try to do that later.

Thanks,
Andrew

Re: [PATCH v6 3/4] iio: accel: sca3000: use lock guards

[Andrew Ijano]

On Sat, Jun 21, 2025 at 2:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Please add extra description for where you have pushed locks up
> a level in the call tree and why that is fine to do.
>
Ok! I'll do that.

> >       switch (mask) {
> > -     case IIO_CHAN_INFO_SAMP_FREQ:
> > +     case IIO_CHAN_INFO_SAMP_FREQ: {
> > +             guard(mutex)(&st->lock);
> As below
>
> >               if (val2)
> >                       return -EINVAL;
> > -             mutex_lock(&st->lock);
> > -             ret = sca3000_write_raw_samp_freq(st, val);
> > -             mutex_unlock(&st->lock);
> > -             return ret;
> > -     case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +             return sca3000_write_raw_samp_freq(st, val);
> > +     }
> > +     case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> > +             guard(mutex)(&st->lock);
> >               if (val2)
> >                       return -EINVAL;
> > -             mutex_lock(&st->lock);
>
> You can keep the old ordering here.  It doesn't matter much but
> easier to be sure about a patch that makes no functional change.
Ok!


> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
>
> This is a very large increase in scope.  Use scoped_guard() here instead to avoid
> holding the lock over a whole load of code that doesn't need it.
>
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> > -     mutex_unlock(&st->lock);

That makes sense! I don't know why I didn't use scoped_guard() in this
case before.


> >       if (state) {
> >               dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> > -             ret = sca3000_write_reg(st,
> > +             return sca3000_write_reg(st,
> >                       SCA3000_REG_MODE_ADDR,
> >                       ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
>
> This indent was already nasty so as we are touching the code good to clean it up.
> For cases like this we can be more relaxed and if it helps readability go a little
> over 80 chars (I think this will be 80 ish)

Great! I'll pay attention to that.

> >
> >
> >  static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> > @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> >       int ret;
> >       struct sca3000_state *st = iio_priv(indio_dev);
> >
> > +     guard(mutex)(&st->lock);
>
> See comment at the top - Making this change is fine but call it out in the patch
> description as it isn't simple change to using guards, but instead to holding
> the lock for longer.  Change is fine but point it out as it needs
> more review than the mechanical changes.

Ok!

>
> >       ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> >       if (ret)
> >               return ret;
> >
> >       /* Disable the 50% full interrupt */
> > -     mutex_lock(&st->lock);
> > -
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> >       if (ret)
> > -             goto unlock;
> > -     ret = sca3000_write_reg(st,
> > +             return ret;
> > +     return sca3000_write_reg(st,
> >                               SCA3000_REG_INT_MASK_ADDR,
> >                               ret & ~SCA3000_REG_INT_MASK_RING_HALF);
>
> As below.
>
> > -unlock:
> > -     mutex_unlock(&st->lock);
> > -     return ret;
> >  }
>
> > @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> >        */
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> >       if (ret)
> > -             goto error_ret;
> > -     ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > +             return ret;
> > +     return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> >                               ret & SCA3000_MODE_PROT_MASK);
>
> As below.
>
> > -
> > -error_ret:
> > -     mutex_unlock(&st->lock);
> > -     return ret;
> >  }
> >
> >  static const struct iio_info sca3000_info = {
> > @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> >  {
> >       int ret;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> >       if (ret)
> > -             goto error_ret;
> > +             return ret;
> >
> > -     ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > +     return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> This adds a character to this line which means that either the indent of
> the following lines was previously wrong, or it is now.
> I think you need to add a space to the following lines.
>
> Check for other similar cases.
>
> >                               ret &
> >                               ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> >                                 SCA3000_REG_INT_MASK_RING_HALF |
> >                                 SCA3000_REG_INT_MASK_ALL_INTS));

Hm, correct me if I'm mistaken but I couldn't find this extra
character, I used the same number of tabs in both cases. Even in this
email it shows as having the same number of white spaces.

Thanks,
Andrew

Re: [PATCH v6 4/4] iio: accel: sca3000: use sysfs_emit_at() instead of sprintf()

[Andrew Ijano]

On Sat, Jun 21, 2025 at 2:58 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > @@ -423,16 +423,16 @@ sca3000_show_available_3db_freqs(struct device *dev,
> >  {
> >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >       struct sca3000_state *st = iio_priv(indio_dev);
> > -     int len;
> > +     unsigned int len = 0;
>
> No need to initialize as set on the next line

That makes sense! I´ll change that.

>
> >
> > -     len = sprintf(buf, "%d", st->info->measurement_mode_3db_freq);
> > +     len = sysfs_emit_at(buf, len, "%d", st->info->measurement_mode_3db_freq);
>
> sysfs_emit() when you know you are at the start.
>
Ok! Thanks.

>
> >       if (st->info->option_mode_1)
> > -             len += sprintf(buf + len, " %d",
> > +             len += sysfs_emit_at(buf, len, " %d",
> >                              st->info->option_mode_1_3db_freq);
> Fix alignment.
>
> >       if (st->info->option_mode_2)
> > -             len += sprintf(buf + len, " %d",
> > +             len += sysfs_emit_at(buf, len, " %d",
> >                              st->info->option_mode_2_3db_freq);
>
> same here.

Actually, both cases are aligned. I checked the code and they have the
same number of tabs, and in this email they have the same number of
spaces.
However, since I'm not reading this diff with a monospaced font, for
me it appears to be different but this is caused by the difference in
size of "-" and "+".
Maybe this is why it appears to be different for you too?

Thanks,
Andrew

Re: [PATCH v6 0/4] iio: accel: sca3000: simplify by using newer infrastructure

[Andy Shevchenko]

Sat, Jul 05, 2025 at 12:03:37AM -0300, Andrew Ijano kirjoitti:
> On Thu, Jun 19, 2025 at 12:23 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Jun 18, 2025 at 03:20:06PM -0300, Andrew Ijano wrote:
> > > On Wed, Jun 18, 2025 at 2:41 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Wed, Jun 18, 2025 at 09:24:19AM -0300, Andrew Ijano wrote:
> > > > > On Wed, Jun 18, 2025 at 2:56 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Wed, Jun 18, 2025 at 6:17 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:

...

> > > > > > I haven't found any reference to a base commit here. Have you
> > > > > > forgotten to use --base when preparing the series?
> > > > > > In any case, please clarify what this series is based on.
> > > > >
> > > > > Thank you for pointing this out! I think I forgot to use --base for
> > > > > it. In this case, should I submit a new version of the whole patchset
> > > > > with this information or is there a better way to do it?
> > > >
> > > > For now just reply here what is the base. I asked this question above.
> > >
> > > Ok! No problem. So the base for this patchset is the commit
> > > 3c23416f69f2870bea83697d7ab03c6a8497daa7.
> >
> > No such commit in the repository. :-(
> > You are doing something interesting here [1].
> >
> > So, make sure you are based on the iio/testing or so, make sure that the base
> > commit is the one that may be found on git.kernel.org. Use that in the next
> > version. Due to above this version is ambiguous to even start reviewing it.
> >
> > [1] I have connected IIO subsystem as a remote, so I have access to many trees
> > from kernel.org (but not to all of them).
> 
> Actually, I think I didn't fully understand this part of the
> contribution process and that's what was causing confusion.
> Basically, the base commit appeared in the previous version of this
> patchset but I removed it after it was approved, to prevent it from
> being reviewed again. However, I think I could just add the
> reviewed-by tag.
> 
> I'll send a next version with other corrections and the missing commit
> based on iio/testing.

What you just described is a normal process of rebasing your local tree against
the (updated) upstream branch (in this case we are taling about iio/testing or
iio/togreg whichever suits better). Hence, if the commit was approved, the new
base should be provided. Under "approved" means that it made the subsystem tree
and pending for the upstream.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/4] iio: accel: sca3000: use lock guards

[Jonathan Cameron]

> > > -     ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > > +     return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,  
> > This adds a character to this line which means that either the indent of
> > the following lines was previously wrong, or it is now.
> > I think you need to add a space to the following lines.
> >
> > Check for other similar cases.
> >  
> > >                               ret &
> > >                               ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> > >                                 SCA3000_REG_INT_MASK_RING_HALF |
> > >                                 SCA3000_REG_INT_MASK_ALL_INTS));  
> 
> Hm, correct me if I'm mistaken but I couldn't find this extra
> character, I used the same number of tabs in both cases. Even in this
> email it shows as having the same number of white spaces.
return sca3000_write_reg(
is one character longer than 
ret = sca3000_write_reg(
and seeing as parameters on later lines should align just after this they all
need one additional space.

> 
> Thanks,
> Andrew

Re: [PATCH v6 4/4] iio: accel: sca3000: use sysfs_emit_at() instead of sprintf()

[Jonathan Cameron]

On Sat, 5 Jul 2025 00:45:05 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> On Sat, Jun 21, 2025 at 2:58 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > @@ -423,16 +423,16 @@ sca3000_show_available_3db_freqs(struct device *dev,
> > >  {
> > >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > >       struct sca3000_state *st = iio_priv(indio_dev);
> > > -     int len;
> > > +     unsigned int len = 0;  
> >
> > No need to initialize as set on the next line  
> 
> That makes sense! I´ll change that.
> 
> >  
> > >
> > > -     len = sprintf(buf, "%d", st->info->measurement_mode_3db_freq);
> > > +     len = sysfs_emit_at(buf, len, "%d", st->info->measurement_mode_3db_freq);  
> >
> > sysfs_emit() when you know you are at the start.
> >  
> Ok! Thanks.
> 
> >  
> > >       if (st->info->option_mode_1)
> > > -             len += sprintf(buf + len, " %d",
> > > +             len += sysfs_emit_at(buf, len, " %d",
> > >                              st->info->option_mode_1_3db_freq);  
> > Fix alignment.
> >  
> > >       if (st->info->option_mode_2)
> > > -             len += sprintf(buf + len, " %d",
> > > +             len += sysfs_emit_at(buf, len, " %d",
> > >                              st->info->option_mode_2_3db_freq);  
> >
> > same here.  
> 
> Actually, both cases are aligned. I checked the code and they have the
> same number of tabs, and in this email they have the same number of
> spaces.
> However, since I'm not reading this diff with a monospaced font, for
> me it appears to be different but this is caused by the difference in
> size of "-" and "+".
> Maybe this is why it appears to be different for you too?
Key as in earlier reply is parameters that need to go on an additional line
are not aligned as particular number of tabs, they are aligned to under
the parameters on the first line.

The only normal reason to break with that rule is on particularly long
lines.

Jonathan

> 
> Thanks,
> Andrew