Re: [PATCH 2/2] iio: adc: ad7923: using claim_direct_scoped for code simplification

[Jonathan Cameron <jic23@kernel.org>]

On Wed,  1 May 2024 18:57:24 -0300
Jorge Harrisonn <jorge.harrisonn@usp.br> wrote:

> Using iio_device_claim_direct_scoped instead of calling `iio_device
> _claim_direct_modeand later callingiio_device_release_direct_mode`
> 
> This should make code cleaner and error handling easier
> 
> Co-authored-by: Lais Nuto <laisnuto@usp.br>
> Signed-off-by: Lais Nuto <laisnuto@usp.br>
> Signed-off-by: Jorge Harrisonn <jorge.harrisonn@usp.br>
Hi Jorge, Lais,

Comments inline.

> ---
>  drivers/iio/adc/ad7923.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> index 9d6bf6d0927a..a646521b2ef3 100644
> --- a/drivers/iio/adc/ad7923.c
> +++ b/drivers/iio/adc/ad7923.c
> @@ -260,22 +260,20 @@ static int ad7923_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> -			return ret;
> -		ret = ad7923_scan_direct(st, chan->address);
> -		iio_device_release_direct_mode(indio_dev);
> -
> -		if (ret < 0)
> -			return ret;
> -
> -		if (chan->address == EXTRACT(ret, 12, 4))
> -			*val = EXTRACT(ret, chan->scan_type.shift,
> -				       chan->scan_type.realbits);
> -		else
> -			return -EIO;
> -
> -		return IIO_VAL_INT;
> +		    iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {         	

Lots of stray white space + why this indent for the line above?

> +			ret = ad7923_scan_direct(st, chan->address); 
> +				
No blank line here as the check should be clearly associated with
the line above.
> +			if (ret < 0)
> +				return ret;
> +
> +			if (chan->address == EXTRACT(ret, 12, 4))
> +				*val = EXTRACT(ret, 0, 12);
> +			else
> +				return -EIO;
Flip this logic
			if (chan->address != EXTRACT(ret, 12, 4))
				return -EIO;

			*val = EXTRACT(ret, 0, 12);

However, better to also change the cases where the masks is fixed at
compile time to use the standard FIELD_GET() and provide appropriate
mask defines.

That change would be a separate patch, so up to you whether you want
to do that as an addition for v2.



> +
> +			return IIO_VAL_INT;
> +		}
> +    	unreachable();
>  	case IIO_CHAN_INFO_SCALE:
>  		ret = ad7923_get_range(st);
>  		if (ret < 0)