Received-SPF: pass (google.com: domain of linux-kernel+bounces-168610-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sat, 4 May 2024 12:38:24 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Jorge Harrisonn <jorge.harrisonn@usp.br>
Cc: lars@metafoo.de, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org, laisnuto@usp.br
Subject: Re: [PATCH 2/2] iio: adc: ad7923: using claim_direct_scoped for
 code simplification
Message-ID: <20240504123824.3cbf28c5@jic23-huawei>
In-Reply-To: <20240501215724.26655-3-jorge.harrisonn@usp.br>
References: <20240501215724.26655-1-jorge.harrisonn@usp.br>
	<20240501215724.26655-3-jorge.harrisonn@usp.br>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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)