Re: [PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan_direct

[David Lechner]

On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> Currently the 'ad7606' driver supports parts with 18 and 16 bits
> resolutions.
> But when adding support for AD7607 (which has a 14-bit resolution) we
> should check for the 'realbits' field, to be able to sign-extend correctly.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@xxxxxxxxxxxx>
> ---
>  drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index d0fe9fd65f3f..a1f0c2feb04a 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -568,7 +568,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>  			      int *val)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> -	unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> +	unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
>  	const struct iio_chan_spec *chan;
>  	int ret;
>  
> @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>  
>  	chan = &indio_dev->channels[ch + 1];
>  	if (chan->scan_type.sign == 'u') {
> -		if (storagebits > 16)

I think it would be simpler to keep this if statement for choosing
which data.bufXX to use since there are only 2 choices.


> +		switch (realbits) {
> +		case 18:
>  			*val = st->data.buf32[ch];
> -		else
> +			break;
> +		case 16:
>  			*val = st->data.buf16[ch];
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
>  	} else {
> -		if (storagebits > 16)
> +		switch (realbits) {
> +		case 18:
>  			*val = sign_extend32(st->data.buf32[ch], 17);
> -		else
> +			break;
> +		case 16:
>  			*val = sign_extend32(st->data.buf16[ch], 15);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}

We can change this to:

	*val = sign_extend32(st->data.buf16[ch], reablbits - 1);

Then no additional changes should be needed to support 14-bit chips.

And similarly, we could avoid the need to use the more verbose
switch statement.

>  	}
>  
>  error_ret: