From: "Nuno Sá" <noname.nuno@gmail.com>
To: Andrew Ijano <andrew.ijano@gmail.com>, jic23@kernel.org
Cc: andrew.lopes@alumni.usp.br, gustavobastos@usp.br,
dlechner@baylibre.com, nuno.sa@analog.com, andy@kernel.org,
jstephan@baylibre.com, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
Date: Thu, 12 Jun 2025 07:29:35 +0100 [thread overview]
Message-ID: <84643f3fc87425e8c5019eede275791a20872b6b.camel@gmail.com> (raw)
In-Reply-To: <20250611194648.18133-3-andrew.lopes@alumni.usp.br>
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;
next prev parent reply other threads:[~2025-06-12 7:29 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-11 19:39 [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
2025-06-12 6:20 ` Nuno Sá
2025-06-12 6:41 ` Nuno Sá
2025-06-14 19:12 ` Andrew Ijano
2025-06-12 12:56 ` Andy Shevchenko
2025-06-14 11:48 ` Jonathan Cameron
2025-06-14 19:16 ` Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
2025-06-12 6:29 ` Nuno Sá [this message]
2025-06-14 19:33 ` Andrew Ijano
2025-06-18 2:36 ` Andrew Ijano
2025-06-12 13:22 ` Andy Shevchenko
2025-06-14 21:06 ` Andrew Ijano
2025-06-14 21:50 ` Andy Shevchenko
2025-06-15 3:21 ` Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2025-06-12 6:38 ` Nuno Sá
2025-06-14 11:54 ` Jonathan Cameron
2025-06-14 21:17 ` Andrew Ijano
2025-06-12 14:48 ` kernel test robot
2025-06-12 15:52 ` kernel test robot
2025-06-12 16:06 ` David Lechner
2025-06-14 21:21 ` Andrew Ijano
2025-06-14 12:01 ` Jonathan Cameron
2025-06-14 21:40 ` Andrew Ijano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84643f3fc87425e8c5019eede275791a20872b6b.camel@gmail.com \
--to=noname.nuno@gmail.com \
--cc=andrew.ijano@gmail.com \
--cc=andrew.lopes@alumni.usp.br \
--cc=andy@kernel.org \
--cc=dlechner@baylibre.com \
--cc=gustavobastos@usp.br \
--cc=jic23@kernel.org \
--cc=jstephan@baylibre.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nuno.sa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.