Re: [PATCH v6 1/4] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 Jun 2025 00:12:16 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Remove sca3000_read_data_short() function, replacing it by spi_w8r8()
> and spi_w8r16be() helpers.
> 
> 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>

Hi. I made two related tweaks.  A few comments inline for further possible cleanup.

Applied to the togreg branch of iio.git and initially pushed out as testing
for 0-day to take a look at it.

Thanks,

Jonathan


diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index c85a06cbea37..eb0cad22474e 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -589,8 +589,7 @@ static int sca3000_read_raw_samp_freq(struct sca3000_state *st, int *val)
                return ret;
 
        if (*val > 0) {
-               ret &= SCA3000_REG_OUT_CTRL_BUF_DIV_MASK;
-               switch (ret) {
+               switch (ret & SCA3000_REG_OUT_CTRL_BUF_DIV_MASK) {
                case SCA3000_REG_OUT_CTRL_BUF_DIV_2:
                        *val /= 2;
                        break;
@@ -644,8 +643,7 @@ static int sca3000_read_3db_freq(struct sca3000_state *st, int *val)
                return ret;
 
        /* mask bottom 2 bits - only ones that are relevant */
-       ret &= SCA3000_REG_MODE_MODE_MASK;
-       switch (ret) {
+       switch (ret & SCA3000_REG_MODE_MODE_MASK) {
        case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
                *val = st->info->measurement_mode_3db_freq;
                return IIO_VAL_INT;



>  /**
> @@ -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);
Hmm. doesn't belong in this patch but it is rather odd to not see
a shift on one of these.

Hmm. Time to miss quote whoever it was who said:

"kernel development is like a murder mystery where you try to solve
 the crime only to realize you were the murderer."

Below I mention using FIELD_GET() in appropriate places as a possible additional
cleanup.  Fix this up when you do that (and note it in the patch description for
that patch).

>  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) {
See discussion below.  This can be simplified as
	switch (reg & SCA3000_REG_MODE_MASK)
avoiding the modified 'ret' value in place comment.

This one I'll tweak as it is easy to avoid the ugly pattern.

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

For a further potential cleanup it would be good to use FIELD_GET() and FIELD_PREP()
in appropriate places in this driver. Do that into a separate local variable
as using ret for all this is a little confusing as quite a bit of the time
it's not a variable we are ever going to return.

As a rule of thumb if we are modifying the ret only a little in a single reuse
(i.e. masking out a bit in a parameter being passed to something else) then
a local variable is probably overkill. If we are building up a new register
value it is not really appropriate to do it into ret.

I'm not asking for changes in this patch though as what you have here
is the simplest and easiest to review change.  If you add those extra
local variables as part of using FIELD_GET/ FIELD_PREP though that would
be great.



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