Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

From: Nuno Sá
Date: Thu Jun 12 2025 - 03:29:16 EST

Next message: Viresh Kumar: "Re: drivers/opp/of.o: warning: objtool: _opp_table_alloc_required_tables+0x3b0: stack state mismatch: reg1[22]=-1+0 reg2[22]=-2-16"
Previous message: Eugenio Perez Martin: "Re: [RFC 5/6] vduse: reset group asid in reset"
In reply to: Andrew Ijano: "[PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers"
Next in thread: Andrew Ijano: "Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
> Co-developed-by: Gustavo Bastos <gustavobastos@xxxxxx>
> Signed-off-by: Gustavo Bastos <gustavobastos@xxxxxx>
> Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> ---

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;

Next message: Viresh Kumar: "Re: drivers/opp/of.o: warning: objtool: _opp_table_alloc_required_tables+0x3b0: stack state mismatch: reg1[22]=-1+0 reg2[22]=-2-16"
Previous message: Eugenio Perez Martin: "Re: [RFC 5/6] vduse: reset group asid in reset"
In reply to: Andrew Ijano: "[PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers"
Next in thread: Andrew Ijano: "Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]