Message-ID: <20250511123416.729eb50f@jic23-huawei>
Date: Sun, 11 May 2025 12:34:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Andrew Ijano <andrew.ijano@...il.com>
Cc: andrew.lopes@...mni.usp.br, gustavobastos@....br, dlechner@...libre.com,
 nuno.sa@...log.com, andy@...nel.org, jstephan@...libre.com,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] iio: accel: sca3000: replace usages of internal read
 data helpers by spi helpers

On Sat, 10 May 2025 16:07:09 -0300
Andrew Ijano <andrew.ijano@...il.com> wrote:

> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16() 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@...mni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@....br>
> Signed-off-by: Gustavo Bastos <gustavobastos@....br>
> Suggested-by: Jonathan Cameron <jic23@...nel.org>

A few things inline but clearly main thing is to reply to Andy's other
points on v3.

> ---
>  drivers/iio/accel/sca3000.c | 169 +++++++++++++++---------------------
>  1 file changed, 72 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index aabe4491efd7..7794efc35970 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -281,10 +281,11 @@ 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)
> +static int sca3000_read_data(struct sca3000_state *st,
> +			     u8 reg_address_high,
> +			     int len)

I'd keep this where the original sca3000_read_data() is
That will give a shorter, more obvious diff and puts the code near where it
is called helping review.

>  {
> +	int ret;
>  	struct spi_transfer xfer[2] = {
>  		{
>  			.len = 1,
> @@ -296,7 +297,10 @@ static int sca3000_read_data_short(struct sca3000_state *st,
>  	};
>  	st->tx[0] = SCA3000_READ_REG(reg_address_high);
>  
> -	return spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
> +	ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
> +	if (ret)
> +		dev_err(&st->us->dev, "problem reading register\n");
> +	return ret;
>  }

...

>  /**
> @@ -412,10 +416,8 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
>  	ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
>  	if (ret)
>  		goto error_ret;
> -	ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
> -	if (ret)
> -		goto error_ret;
> -	return st->rx[0];
> +
> +	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
>  error_ret:
>  	return ret;
This shows the age of the code.  Just return in error paths above rather
than a error_ret: label

Might be a good idea to do a precursor patch tidying up any cases of this
before the one doin gthe spi changes in ehre.

>  }
> @@ -432,13 +434,13 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  
>  	mutex_lock(&st->lock);

Another patch to use guard(mutex)(&st->lock); etc would be help clean this
up by allowing direct return in the error path.

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


>  static int sca3000_read_raw(struct iio_dev *indio_dev,
> @@ -722,6 +724,7 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  	int ret;
>  	u8 address;
> +	__be16 raw_val;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -732,25 +735,24 @@ 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_w8r16(st->us, SCA3000_READ_REG(address));

spi_w8r16be() then no need for the endian conversion below.


>  			if (ret < 0) {
>  				mutex_unlock(&st->lock);
>  				return ret;
>  			}
> -			*val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
> -					     chan->scan_type.shift,
> +			raw_val = (__be16) ret;
> +			*val = sign_extend32(be16_to_cpu(raw_val) >> 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_w8r16(st->us,
> +						SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));

As above. spi_w8r16be()


>  			if (ret < 0) {
>  				mutex_unlock(&st->lock);
>  				return ret;
>  			}
> -			*val = (be16_to_cpup((__be16 *)st->rx) >>
> -				chan->scan_type.shift) &
> +			raw_val = (__be16) ret;
> +			*val = (be16_to_cpu(raw_val) >> chan->scan_type.shift) &
>  				GENMASK(chan->scan_type.realbits - 1, 0);
>  		}
>  		mutex_unlock(&st->lock);

>  

As above. Put the new implementation of this here so we can easily see what
changed.

> -static int sca3000_read_data(struct sca3000_state *st,
> -			     u8 reg_address_high,
> -			     u8 *rx,
> -			     int len)
> -{
> -	int ret;
> -	struct spi_transfer xfer[2] = {
> -		{
> -			.len = 1,
> -			.tx_buf = st->tx,
> -		}, {
> -			.len = len,
> -			.rx_buf = rx,
> -		}
> -	};
> -
> -	st->tx[0] = SCA3000_READ_REG(reg_address_high);
> -	ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
> -	if (ret) {
> -		dev_err(&st->us->dev, "problem reading register\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
Jonathan

Powered by blists - more mailing lists