Remove usages of sca3000_read_data() and sca3000_read_data_short()
functions, replacing it by spi_w8r8() and spi_w8r16be() 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>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
drivers/iio/accel/sca3000.c | 166 +++++++++++++++---------------------
1 file changed, 68 insertions(+), 98 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index bfa8a3f5a92f..d41759c68fb4 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,22 @@ 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 +798,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)
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,
@@ -964,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;
@@ -974,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;
}
/**
@@ -1001,16 +972,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;
@@ -1045,7 +1015,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 +1023,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 +1039,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 +1048,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 +1057,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 +1083,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 +1098,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 +1126,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 +1176,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 +1249,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 +1284,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 +1315,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 +1345,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 +1371,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 +1385,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 +1472,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.0On Wed, Jun 11, 2025 at 04:39:20PM -0300, Andrew Ijano wrote:
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16be() 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.
...
> + ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
Unneeded parentheses.
...
> + ret = spi_w8r16be(st->us,
> + SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
Make it simply one line. The above formatting is ugly.
...
> 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;
Ideally it's better to split them and len should never be signed.
Moreover, the function should be switched to sysfs_emit_at() if this is part
of ABI.
> 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;
...
> }, {
> .len = len,
> - .rx_buf = rx,
> + .rx_buf = st->rx,
> }
> };
> -
Stray change. Doesn't checkpatch complain on this?
> - (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
> - (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
In the original code and still now too many parentheses.
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > + ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
>
> Unneeded parentheses.
>
...
>
> > + ret = spi_w8r16be(st->us,
> > + SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
>
> Make it simply one line. The above formatting is ugly.
That's right! I'll fix them.
>
> > 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;
>
> Ideally it's better to split them and len should never be signed.
Nice! I can make this change.
> Moreover, the function should be switched to sysfs_emit_at() if this is part
> of ABI.
Great! I didn't know that.
In this case, sca3000_read_av_freq() is described as a "sysfs function
to get available frequencies", so I guess it's the case, right?
Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
then? If so, I could do that in a following patch, it seems that
sca3000_show_available_3db_freqs() is also using sprintf().
>
> > 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;
>
> ...
>
> > }, {
> > .len = len,
> > - .rx_buf = rx,
> > + .rx_buf = st->rx,
> > }
> > };
>
> > -
>
> Stray change. Doesn't checkpatch complain on this?
I don't recall getting any warning from checkpatch but I can check
again for this next version.
> > - (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> > + (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> > - (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> > + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> In the original code and still now too many parentheses.
Ok! I'll remove them.
Thanks,
Andrew
On Sun, Jun 15, 2025 at 12:06 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
...
> > Moreover, the function should be switched to sysfs_emit_at() if this is part
> > of ABI.
>
> Great! I didn't know that.
>
> In this case, sca3000_read_av_freq() is described as a "sysfs function
> to get available frequencies", so I guess it's the case, right?
> Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
> then? If so, I could do that in a following patch, it seems that
> sca3000_show_available_3db_freqs() is also using sprintf().
Yes. This is written in the Documentation.
...
> > > }, {
> > > .len = len,
> > > - .rx_buf = rx,
> > > + .rx_buf = st->rx,
> > > }
> > > };
> >
> > > -
> >
> > Stray change. Doesn't checkpatch complain on this?
>
> I don't recall getting any warning from checkpatch but I can check
> again for this next version.
The problem here is the absence of a blank line between the definition
block (where variables are declared/defined) and the first line of the
actual code.
--
With Best Regards,
Andy Shevchenko
On Sat, Jun 14, 2025 at 6:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 15, 2025 at 12:06 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > > Moreover, the function should be switched to sysfs_emit_at() if this is part
> > > of ABI.
> >
> > Great! I didn't know that.
> >
> > In this case, sca3000_read_av_freq() is described as a "sysfs function
> > to get available frequencies", so I guess it's the case, right?
> > Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
> > then? If so, I could do that in a following patch, it seems that
> > sca3000_show_available_3db_freqs() is also using sprintf().
>
> Yes. This is written in the Documentation.
Nice! I'll try to make a new patch later to address that then.
...
> > > > }, {
> > > > .len = len,
> > > > - .rx_buf = rx,
> > > > + .rx_buf = st->rx,
> > > > }
> > > > };
> > >
> > > > -
> > >
> > > Stray change. Doesn't checkpatch complain on this?
> >
> > I don't recall getting any warning from checkpatch but I can check
> > again for this next version.
>
> The problem here is the absence of a blank line between the definition
> block (where variables are declared/defined) and the first line of the
> actual code.
Thanks for the explanation. I double checked and checkpatch doesn't
seem to warn about that, but I fixed it anyway.
Thanks,
Andrew
On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16be() 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>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> ---
Looks good. Just one comment from me...
> drivers/iio/accel/sca3000.c | 166 +++++++++++++++---------------------
> 1 file changed, 68 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index bfa8a3f5a92f..d41759c68fb4 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(struct sca3000_state *st,
> u8 reg_address_high,
> - u8 *rx,
> int len)
> {
> int ret;
> @@ -974,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;
Unless I'm missing something, the above seems unrelated to the rest of the patch.
- Nuno Sá
> }
>
> /**
> @@ -1001,16 +972,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;
> @@ -1045,7 +1015,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 +1023,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 +1039,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 +1048,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 +1057,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 +1083,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 +1098,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 +1126,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 +1176,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 +1249,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 +1284,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 +1315,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 +1345,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 +1371,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 +1385,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 +1472,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;
On Thu, Jun 12, 2025 at 4:29 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
...
>
> Looks good. Just one comment from me...
>
> > drivers/iio/accel/sca3000.c | 166 +++++++++++++++---------------------
> > 1 file changed, 68 insertions(+), 98 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index bfa8a3f5a92f..d41759c68fb4 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(struct sca3000_state *st,
> > u8 reg_address_high,
> > - u8 *rx,
> > int len)
> > {
> > int ret;
> > @@ -974,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;
>
> Unless I'm missing something, the above seems unrelated to the rest of the patch.
>
Oh, I can see why that might feel unrelated. Actually, this was
related to the first version of the patch which was focused on
removing the duplicated behavior of sca3000_read_data() and
sca3000_read_data_short(), unifying it in only one function. Also,
with Andy's suggestions we cleaned up the function as you're seeing
here. However, we later replaced all usages of
sca3000_read_data_short() by spi helpers, leaving just one place
calling sca3000_read_data(), so this change isn't as necessary as
before.
Do you think it's still a valid cleanup for this patch or would you
prefer moving it to a separated one?
Thanks,
Andrew
On Sat, Jun 14, 2025 at 4:33 PM Andrew Ijano <andrew.ijano@gmail.com> wrote: > > On Thu, Jun 12, 2025 at 4:29 AM Nuno Sá <noname.nuno@gmail.com> wrote: ... > > > > Unless I'm missing something, the above seems unrelated to the rest of the patch. > > > Oh, I can see why that might feel unrelated. Actually, this was > related to the first version of the patch which was focused on > removing the duplicated behavior of sca3000_read_data() and > sca3000_read_data_short(), unifying it in only one function. Also, > with Andy's suggestions we cleaned up the function as you're seeing > here. However, we later replaced all usages of > sca3000_read_data_short() by spi helpers, leaving just one place > calling sca3000_read_data(), so this change isn't as necessary as > before. > > Do you think it's still a valid cleanup for this patch or would you > prefer moving it to a separated one? Looking back to this, I think we really could benefit from having two separate patches to handle these different changes. I'll try to make a new version that addresses all the comments for the current patchset. Thanks, Andrew
© 2016 - 2025 Red Hat, Inc.