Re: [PATCH] iio: adc: adc7266: Use iio_device_claim_direct_scoped()

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi Fernando, Eduardo,

The patch idea looks good. Though, there are some aspects to improve.
First thing is this patch doesn't really apply to IIO testing branch [1].
Please, check the commits you have in your local branch.
Most often, each commit generates one patch. Looks like the commit from this
patch should be squashed with some other commit.
Then, certify that the patches you generate apply cleanly to IIO testing branch.
e.g.
git am 0001-my-awesome-patch.patch

More comments inline.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing

On 05/08, Fernando Yang wrote:
> Switching to the _scoped() version can make the error
> handling more natural instead of delayed until direct
> mode was released.
> 
> Co-developed-by: Eduardo Figueredo <eduardofp@usp.br>
> Signed-off-by: Eduardo Figueredo <eduardofp@usp.br>
> Signed-off-by: Fernando Yang <hagisf@usp.br>
> ---
>  drivers/iio/adc/ad7266.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 8b03d4469..3fc34a3a8 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -63,12 +63,14 @@ static int ad7266_powerdown(struct ad7266_state *st)
>  static int ad7266_preenable(struct iio_dev *indio_dev)
>  {
>  	struct ad7266_state *st = iio_priv(indio_dev);
> +
This should belong to a separate patch. Conceptually, this is a different type
of change compared to the update to use claim direct mode scoped advertised in
patch message and body. The blank line additions would make up a code style
patch.
>  	return ad7266_wakeup(st);
>  }
>  
>  static int ad7266_postdisable(struct iio_dev *indio_dev)
>  {
>  	struct ad7266_state *st = iio_priv(indio_dev);
> +
This also goes into the code style patch.
>  	return ad7266_powerdown(st);
>  }
>  
> @@ -151,15 +153,16 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,

I think after updating to call claim direct mode scoped the ret variable that is
nearby can be removed. You may have removed the variable in your local branch
but it would be nice to also have the removal in this patch.
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>  			ret = ad7266_read_single(st, val, chan->address);
>  
> -		*val = (*val >> 2) & 0xfff;
> -		if (chan->scan_type.sign == 's')
> -			*val = sign_extend32(*val,
> -						 chan->scan_type.realbits - 1);
> +			*val = (*val >> 2) & 0xfff;
> +			if (chan->scan_type.sign == 's')
> +				*val = sign_extend32(*val,
> +							 chan->scan_type.realbits - 1);
This change actually introduces another code style issue for checkpatch to nitpick on.
Please, consider also running checkpatch on generated patch files. e.g.
./scripts/checkpatch.pl --terse --codespell --color=always -strict 0001-use-claim-scoped.patch

>  
> -		return IIO_VAL_INT;
> +			return IIO_VAL_INT;
> +		}
>  		unreachable();
>  	case IIO_CHAN_INFO_SCALE:
>  		scale_mv = st->vref_mv;
> -- 
> 2.34.1
> 
>