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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).