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

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

[Fernando Yang]

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);
+
 	return ad7266_wakeup(st);
 }
 
 static int ad7266_postdisable(struct iio_dev *indio_dev)
 {
 	struct ad7266_state *st = iio_priv(indio_dev);
+
 	return ad7266_powerdown(st);
 }
 
@@ -151,15 +153,16 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
 
 	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);
 
-		return IIO_VAL_INT;
+			return IIO_VAL_INT;
+		}
 		unreachable();
 	case IIO_CHAN_INFO_SCALE:
 		scale_mv = st->vref_mv;
-- 
2.34.1

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

[Marcelo Schmitt]

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

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

[Jonathan Cameron]

On Wed,  8 May 2024 12:54:35 -0300
Fernando Yang <hagisf@usp.br> 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);
> +
>  	return ad7266_wakeup(st);
>  }
>  
>  static int ad7266_postdisable(struct iio_dev *indio_dev)
>  {
>  	struct ad7266_state *st = iio_priv(indio_dev);
> +
>  	return ad7266_powerdown(st);
>  }
>  
> @@ -151,15 +153,16 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
This isn't the existing code. I'm guessing you have another commit ahead
of this one on your branch?

> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>  			ret = ad7266_read_single(st, val, chan->address);
Not checking ret here is a bug in the existing code.  Please fix that as
a precursor patch before making the _scoped change.

The check will have to be immediately after iio_device_release_direct_mode()
in the existing code.

>  
> -		*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);
>  
> -		return IIO_VAL_INT;
> +			return IIO_VAL_INT;
> +		}
>  		unreachable();
>  	case IIO_CHAN_INFO_SCALE:
>  		scale_mv = st->vref_mv;

[PATCH v2 1/3] iio: adc: ad7266: Fix variable checking bug

[Fernando Yang]

The ret variable was not checked after iio_device_release_direct_mode(),
which could possibly cause errors

Signed-off-by: Fernando Yang <hagisf@usp.br>
---
 drivers/iio/adc/ad7266.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 353a97f9c..13ea8a107 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -157,6 +157,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
 		ret = ad7266_read_single(st, val, chan->address);
 		iio_device_release_direct_mode(indio_dev);
 
+		if (ret < 0)
+			return ret;
 		*val = (*val >> 2) & 0xfff;
 		if (chan->scan_type.sign == 's')
 			*val = sign_extend32(*val,
-- 
2.34.1

[PATCH v2 2/3] iio: adc: ad7266: Use iio_device_claim_direct_scoped()

[Fernando Yang]

Switching to the _scoped() version can make the error handling more
natural instead of delayed until direct mode was released.

Signed-off-by: Fernando Yang <hagisf@usp.br>
---
 drivers/iio/adc/ad7266.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 13ea8a107..356c2fe07 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -151,20 +151,19 @@ static int ad7266_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 = ad7266_read_single(st, val, chan->address);
-		iio_device_release_direct_mode(indio_dev);
-
-		if (ret < 0)
-			return ret;
-		*val = (*val >> 2) & 0xfff;
-		if (chan->scan_type.sign == 's')
-			*val = sign_extend32(*val,
-					     chan->scan_type.realbits - 1);
-
-		return IIO_VAL_INT;
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+			ret = ad7266_read_single(st, val, chan->address);
+			
+			if (ret < 0)
+				return ret;
+			*val = (*val >> 2) & 0xfff;
+			if (chan->scan_type.sign == 's')
+				*val = sign_extend32(*val,
+							 chan->scan_type.realbits - 1);
+
+			return IIO_VAL_INT;
+		}
+		unreachable();
 	case IIO_CHAN_INFO_SCALE:
 		scale_mv = st->vref_mv;
 		if (st->mode == AD7266_MODE_DIFF)
-- 
2.34.1

Re: [PATCH v2 2/3] iio: adc: ad7266: Use iio_device_claim_direct_scoped()

[Marcelo Schmitt]

Looks good overall, but a couple of untidy bits slipped through into the patch.

On 06/03, Fernando Yang wrote:
> Switching to the _scoped() version can make the error handling more
> natural instead of delayed until direct mode was released.
> 
> Signed-off-by: Fernando Yang <hagisf@usp.br>
> ---
>  drivers/iio/adc/ad7266.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 13ea8a107..356c2fe07 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -151,20 +151,19 @@ static int ad7266_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 = ad7266_read_single(st, val, chan->address);
> -		iio_device_release_direct_mode(indio_dev);
> -
> -		if (ret < 0)
> -			return ret;
> -		*val = (*val >> 2) & 0xfff;
> -		if (chan->scan_type.sign == 's')
> -			*val = sign_extend32(*val,
> -					     chan->scan_type.realbits - 1);
> -
> -		return IIO_VAL_INT;
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			ret = ad7266_read_single(st, val, chan->address);
The line right bellow adds 3 tabs. Don't add them so no need to remove them in patch 3. ;)
> +			
> +			if (ret < 0)
> +				return ret;
> +			*val = (*val >> 2) & 0xfff;
> +			if (chan->scan_type.sign == 's')
Keep chan aligned to *val
> +				*val = sign_extend32(*val,
> +							 chan->scan_type.realbits - 1);

				*val = sign_extend32(*val,
						     chan->scan_type.realbits - 1);

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

[PATCH v2 3/3] iio: adc: ad7266: Fix codestyle error

[Fernando Yang]

Fix some codestyle errors indicated by checkpatch.pl

Signed-off-by: Fernando Yang <hagisf@usp.br>
---
 drivers/iio/adc/ad7266.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 356c2fe07..5a2f1bb27 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);
+
 	return ad7266_wakeup(st);
 }
 
 static int ad7266_postdisable(struct iio_dev *indio_dev)
 {
 	struct ad7266_state *st = iio_priv(indio_dev);
+
 	return ad7266_powerdown(st);
 }
 
@@ -153,7 +155,7 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
 			ret = ad7266_read_single(st, val, chan->address);
-			
+
 			if (ret < 0)
 				return ret;
 			*val = (*val >> 2) & 0xfff;
-- 
2.34.1

Re: [PATCH v2 3/3] iio: adc: ad7266: Fix codestyle error

[Marcelo Schmitt]

Looks good.
One minor neat being the last change won't be needed after patch 2 is updated.

Also, I think it will help visualize the patch series if a cover letter is added.
If running format-patch, the --cover-letter should help with it. e.g.
git format-patch -v3 --thread=shallow --cover-letter --to="..." --cc="..." -3

On 06/03, Fernando Yang wrote:
> Fix some codestyle errors indicated by checkpatch.pl
> 
> Signed-off-by: Fernando Yang <hagisf@usp.br>
> ---
>  drivers/iio/adc/ad7266.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 356c2fe07..5a2f1bb27 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);
> +
>  	return ad7266_wakeup(st);
>  }
>  
>  static int ad7266_postdisable(struct iio_dev *indio_dev)
>  {
>  	struct ad7266_state *st = iio_priv(indio_dev);
> +
>  	return ad7266_powerdown(st);
>  }
>  
> @@ -153,7 +155,7 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>  			ret = ad7266_read_single(st, val, chan->address);
This is not needed as comment on patch 2.
> -			
> +
>  			if (ret < 0)
>  				return ret;
>  			*val = (*val >> 2) & 0xfff;
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2 1/3] iio: adc: ad7266: Fix variable checking bug

[Marcelo Schmitt]

Hi Fernando,

This patch looks good.
I think a fixes tag would also be appropriate for this one.

Fixes: c70df20e3159 ("iio: adc: ad7266: claim direct mode during sensor read")

On 06/03, Fernando Yang wrote:
> The ret variable was not checked after iio_device_release_direct_mode(),
> which could possibly cause errors
> 
> Signed-off-by: Fernando Yang <hagisf@usp.br>
> ---
>  drivers/iio/adc/ad7266.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 353a97f9c..13ea8a107 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -157,6 +157,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
>  		ret = ad7266_read_single(st, val, chan->address);
>  		iio_device_release_direct_mode(indio_dev);
>  
> +		if (ret < 0)
> +			return ret;
>  		*val = (*val >> 2) & 0xfff;
>  		if (chan->scan_type.sign == 's')
>  			*val = sign_extend32(*val,
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2 1/3] iio: adc: ad7266: Fix variable checking bug

[Jonathan Cameron]

On Sat, 8 Jun 2024 02:06:21 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hi Fernando,
> 
> This patch looks good.
> I think a fixes tag would also be appropriate for this one.
> 
> Fixes: c70df20e3159 ("iio: adc: ad7266: claim direct mode during sensor read")
Agreed. 
I've picked this one for the fixes-togreg branch of iio.git and marked it
for inclusion in linux-stable.

Note this will delay when I can pick up v3 of the other two patches, but
good to get this fix upstream in the meantime.

Thanks,

Jonathan

> 
> On 06/03, Fernando Yang wrote:
> > The ret variable was not checked after iio_device_release_direct_mode(),
> > which could possibly cause errors
> > 
> > Signed-off-by: Fernando Yang <hagisf@usp.br>
> > ---
> >  drivers/iio/adc/ad7266.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> > index 353a97f9c..13ea8a107 100644
> > --- a/drivers/iio/adc/ad7266.c
> > +++ b/drivers/iio/adc/ad7266.c
> > @@ -157,6 +157,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
> >  		ret = ad7266_read_single(st, val, chan->address);
> >  		iio_device_release_direct_mode(indio_dev);
> >  
> > +		if (ret < 0)
> > +			return ret;
> >  		*val = (*val >> 2) & 0xfff;
> >  		if (chan->scan_type.sign == 's')
> >  			*val = sign_extend32(*val,
> > -- 
> > 2.34.1
> > 
> >   

Re: [PATCH v2 1/3] iio: adc: ad7266: Fix variable checking bug

[Jonathan Cameron]

On Mon,  3 Jun 2024 15:07:54 -0300
Fernando Yang <hagisf@usp.br> wrote:

> The ret variable was not checked after iio_device_release_direct_mode(),
> which could possibly cause errors
> 
> Signed-off-by: Fernando Yang <hagisf@usp.br>
For future reference, in IIO at least (and 'most' of the rest of the kernel)
don't send a patch set in reply to a previous one.

I can't remember who once gave a good explanation of why, but key to
limited time management when reviewing kernel patches is we start with
latest info and maybe never get back to the beginning.

Given everyone uses threading email clients, your email is a lot of
pages up from where I'd start if I wasn't aiming to clear the whole IIO
backlog this weekend...


> ---
>  drivers/iio/adc/ad7266.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 353a97f9c..13ea8a107 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -157,6 +157,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
>  		ret = ad7266_read_single(st, val, chan->address);
>  		iio_device_release_direct_mode(indio_dev);
>  
> +		if (ret < 0)
> +			return ret;
>  		*val = (*val >> 2) & 0xfff;
>  		if (chan->scan_type.sign == 's')
>  			*val = sign_extend32(*val,