1. Patchew
  2. linux
  3. View series

[PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

Andrew Ijano posted 1 patch 9 months ago
There is a newer version of this series
drivers/iio/accel/sca3000.c | 153 +++++++++++++++---------------------
1 file changed, 65 insertions(+), 88 deletions(-)
[PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Posted by Andrew Ijano 9 months ago 
Remove usages of sca3000_read_data() and sca3000_read_data_short()
functions, replacing it by spi_w8r8() and spi_w8r16() helpers. Just
one case that reads large buffers is left using an internal helper.

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>
---
Even though we reviewed every change and it compiled without
any errors, we don't have the required devices to manually
test its behavior.

drivers/iio/accel/sca3000.c | 153 +++++++++++++++---------------------
 1 file changed, 65 insertions(+), 88 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index aabe4491efd7..4a9ec0639aaa 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -281,10 +281,11 @@ 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)
+static int sca3000_read_data(struct sca3000_state *st,
+			     u8 reg_address_high,
+			     int len)
 {
+	int ret;
 	struct spi_transfer xfer[2] = {
 		{
 			.len = 1,
@@ -296,7 +297,10 @@ static int sca3000_read_data_short(struct sca3000_state *st,
 	};
 	st->tx[0] = SCA3000_READ_REG(reg_address_high);
 
-	return spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
+	ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
+	if (ret)
+		dev_err(&st->us->dev, "problem reading register\n");
+	return ret;
 }
 
 /**
@@ -309,11 +313,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);
 }
 
 /**
@@ -412,10 +416,11 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
 	ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
 	if (ret)
 		goto error_ret;
-	ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
+
+	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
 	if (ret)
 		goto error_ret;
-	return st->rx[0];
+	return ret;
 error_ret:
 	return ret;
 }
@@ -432,13 +437,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);
 
@@ -575,10 +580,10 @@ 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)
 		goto error_ret;
-	switch (SCA3000_REG_MODE_MODE_MASK & st->rx[0]) {
+	switch (SCA3000_REG_MODE_MODE_MASK & ret) {
 	case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
 		*base_freq = info->measurement_mode_freq;
 		break;
@@ -665,13 +670,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;
@@ -703,14 +708,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,
@@ -732,24 +737,23 @@ 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_w8r16(st->us, SCA3000_READ_REG(address));
 			if (ret < 0) {
 				mutex_unlock(&st->lock);
 				return ret;
 			}
-			*val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
+			*val = sign_extend32(be16_to_cpu((__be16) 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_w8r16(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) >>
+			*val = (be16_to_cpu((__be16) ret) >>
 				chan->scan_type.shift) &
 				GENMASK(chan->scan_type.realbits - 1, 0);
 		}
@@ -827,16 +831,15 @@ 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;
+	int len = 0, 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)
 		goto error_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,
@@ -969,32 +972,6 @@ static const struct attribute_group sca3000_attribute_group = {
 	.attrs = sca3000_attributes,
 };
 
-static int sca3000_read_data(struct sca3000_state *st,
-			     u8 reg_address_high,
-			     u8 *rx,
-			     int len)
-{
-	int ret;
-	struct spi_transfer xfer[2] = {
-		{
-			.len = 1,
-			.tx_buf = st->tx,
-		}, {
-			.len = len,
-			.rx_buf = rx,
-		}
-	};
-
-	st->tx[0] = SCA3000_READ_REG(reg_address_high);
-	ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
-	if (ret) {
-		dev_err(&st->us->dev, "problem reading register\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 /**
  * sca3000_ring_int_process() - ring specific interrupt handling.
  * @val: Value of the interrupt status register.
@@ -1008,16 +985,15 @@ 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.
 		 */
-		ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
+		ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
 					num_available * 2);
 		if (ret)
 			goto error_ret;
@@ -1060,8 +1036,8 @@ 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));
+	val = ret;
 	mutex_unlock(&st->lock);
 	if (ret)
 		goto done;
@@ -1121,13 +1097,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:
@@ -1136,7 +1112,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 {
@@ -1164,18 +1140,18 @@ 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))
+	else 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);
 	else
 		return 0;
 }
@@ -1214,22 +1190,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;
 }
@@ -1287,18 +1263,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);
 
@@ -1322,12 +1298,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;
 
@@ -1353,12 +1329,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;
@@ -1383,7 +1359,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;
 
@@ -1409,7 +1385,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,
@@ -1423,11 +1399,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);
@@ -1510,11 +1486,12 @@ 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] &
+				(ret &
 				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
 				   SCA3000_REG_INT_MASK_RING_HALF |
 				   SCA3000_REG_INT_MASK_ALL_INTS)));
-- 
2.49.0
Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Posted by kernel test robot 9 months ago 
Hi Andrew,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.15-rc5 next-20250509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrew-Ijano/iio-accel-sca3000-replace-usages-of-internal-read-data-helpers-by-spi-helpers/20250509-094127
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20250509013931.47524-1-andrew.lopes%40alumni.usp.br
patch subject: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
config: i386-randconfig-r123-20250510 (https://download.01.org/0day-ci/archive/20250510/202505100631.nlBOlbYm-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250510/202505100631.nlBOlbYm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505100631.nlBOlbYm-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/accel/sca3000.c:745:46: sparse: sparse: cast to restricted __be16
   drivers/iio/accel/sca3000.c:756:33: sparse: sparse: cast to restricted __be16

vim +745 drivers/iio/accel/sca3000.c

   720	
   721	static int sca3000_read_raw(struct iio_dev *indio_dev,
   722				    struct iio_chan_spec const *chan,
   723				    int *val,
   724				    int *val2,
   725				    long mask)
   726	{
   727		struct sca3000_state *st = iio_priv(indio_dev);
   728		int ret;
   729		u8 address;
   730	
   731		switch (mask) {
   732		case IIO_CHAN_INFO_RAW:
   733			mutex_lock(&st->lock);
   734			if (chan->type == IIO_ACCEL) {
   735				if (st->mo_det_use_count) {
   736					mutex_unlock(&st->lock);
   737					return -EBUSY;
   738				}
   739				address = sca3000_addresses[chan->address][0];
   740				ret = spi_w8r16(st->us, SCA3000_READ_REG(address));
   741				if (ret < 0) {
   742					mutex_unlock(&st->lock);
   743					return ret;
   744				}
 > 745				*val = sign_extend32(be16_to_cpu((__be16) ret) >>
   746						     chan->scan_type.shift,
   747						     chan->scan_type.realbits - 1);
   748			} else {
   749				/* get the temperature when available */
   750				ret = spi_w8r16(st->us,
   751							SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
   752				if (ret < 0) {
   753					mutex_unlock(&st->lock);
   754					return ret;
   755				}
   756				*val = (be16_to_cpu((__be16) ret) >>
   757					chan->scan_type.shift) &
   758					GENMASK(chan->scan_type.realbits - 1, 0);
   759			}
   760			mutex_unlock(&st->lock);
   761			return IIO_VAL_INT;
   762		case IIO_CHAN_INFO_SCALE:
   763			*val = 0;
   764			if (chan->type == IIO_ACCEL)
   765				*val2 = st->info->scale;
   766			else /* temperature */
   767				*val2 = 555556;
   768			return IIO_VAL_INT_PLUS_MICRO;
   769		case IIO_CHAN_INFO_OFFSET:
   770			*val = -214;
   771			*val2 = 600000;
   772			return IIO_VAL_INT_PLUS_MICRO;
   773		case IIO_CHAN_INFO_SAMP_FREQ:
   774			mutex_lock(&st->lock);
   775			ret = sca3000_read_raw_samp_freq(st, val);
   776			mutex_unlock(&st->lock);
   777			return ret ? ret : IIO_VAL_INT;
   778		case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
   779			mutex_lock(&st->lock);
   780			ret = sca3000_read_3db_freq(st, val);
   781			mutex_unlock(&st->lock);
   782			return ret;
   783		default:
   784			return -EINVAL;
   785		}
   786	}
   787	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Posted by Andy Shevchenko 9 months ago 
On Fri, May 9, 2025 at 4:39 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
>
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16() helpers. Just
> one case that reads large buffers is left using an internal helper.
>
> This is an old driver that was not making full use of the newer
> infrastructure.

Suggested-by: ? (IIRC Jonathan suggested this, but ignore if I am mistaken)

...

>         ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
>         if (ret)
>                 goto error_ret;
> -       ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
> +
> +       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
>         if (ret)
>                 goto error_ret;
> -       return st->rx[0];

> +       return ret;
>  error_ret:
>         return ret;

Doesn't feel like a good cleanup. Please, drop this error handling
completely, just return instead of goto above.

...

> -                       *val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
> +                       *val = sign_extend32(be16_to_cpu((__be16) ret) >>

This doesn't look good, can you define a proper __be16 variable on
stack and use it instead of ret?

>                                              chan->scan_type.shift,

With the above done, move this parameter to the previous line.

>                                              chan->scan_type.realbits - 1);
>                 } else {

...

> -                       *val = (be16_to_cpup((__be16 *)st->rx) >>
> +                       *val = (be16_to_cpu((__be16) ret) >>
>                                 chan->scan_type.shift) &
>                                 GENMASK(chan->scan_type.realbits - 1, 0);

Ditto.

...

>         /* 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))
> +       else if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))

Remove redundant 'else'

>                 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);
>         else

Ditto.

>                 return 0;

...

>         ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> -                               (st->rx[0] & SCA3000_MODE_PROT_MASK));
> +                               (ret & SCA3000_MODE_PROT_MASK));

Remove unneeded parentheses.

...

> -       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));

> +

Stray blank line.

>         if (ret)
>                 goto error_ret;

Perhaps you wanted it to be here?

>         ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,

> -                               (st->rx[0] &
> +                               (ret &
>                                  ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
>                                    SCA3000_REG_INT_MASK_RING_HALF |
>                                    SCA3000_REG_INT_MASK_ALL_INTS)));

Remove unneeded parentheses (outer for the last parameter).

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Posted by Andrew Ijano 9 months ago 
Going back to this one to reply to the unaddressed comments.

On Fri, May 9, 2025 at 6:06 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
...
>
> >                                              chan->scan_type.shift,
>
> With the above done, move this parameter to the previous line.

Ok! I reorganized this shift operation to be all in one line.

 ...
>
> > -                       *val = (be16_to_cpup((__be16 *)st->rx) >>
> > +                       *val = (be16_to_cpu((__be16) ret) >>
> >                                 chan->scan_type.shift) &
> >                                 GENMASK(chan->scan_type.realbits - 1, 0);
>
> Ditto.

Thanks, I changed here too.
 ...

> > -       else if (!state && (st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
> > +       else if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
>
> Remove redundant 'else'
>
> >                 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);
> >         else
>
> Ditto.
Ok! Removed both of them.

 ...
>
> >         ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > -                               (st->rx[0] & SCA3000_MODE_PROT_MASK));
> > +                               (ret & SCA3000_MODE_PROT_MASK));
>
> Remove unneeded parentheses.
...
>
> >         ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
>
> > -                               (st->rx[0] &
> > +                               (ret &
> >                                  ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> >                                    SCA3000_REG_INT_MASK_RING_HALF |
> >                                    SCA3000_REG_INT_MASK_ALL_INTS)));
>
> Remove unneeded parentheses (outer for the last parameter).
Great! Removed all the unneeded parentheses then.

--
Thanks again for these comments,
Andrew
Re: [PATCH v3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Posted by Jonathan Cameron 9 months ago 
> ...
> 
> > -                       *val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
> > +                       *val = sign_extend32(be16_to_cpu((__be16) ret) >>  
> 
> This doesn't look good, can you define a proper __be16 variable on
> stack and use it instead of ret?

As I mention in v4 quick review, we have spi_w8r16be() to avoid the need
to have this dance at all.

>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.