[PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging

[PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging

[Lucas Stankus]

Cleans up the driver by removing vague comments, fixing code alignment and
refactoring the probe function return. This patch set also contains a small
bug fix when setting the amount of iio channels in AD7745 devices.

These small patches are a starting point for improving the ad7746 driver,
hopefully to a point where it's possible to get it out of staging. I'm
looking up to feedback on what could be improved to accomplish that.

Lucas Stankus (2):
  staging: iio: cdc: clean up driver comments and probe return
  staging: iio: cdc: avoid overwrite of num_channels

 drivers/staging/iio/cdc/ad7746.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return

[Lucas Stankus]

Remove vague comments, align temperature comment with indent block and
simplify probe return on device register.

Also fix the following checkpatch warning:
CHECK: Alignment should match open parenthesis

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index dfd71e99e872..e03d010b2f4c 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -84,10 +84,6 @@
 #define AD7746_CAPDAC_DACEN		BIT(7)
 #define AD7746_CAPDAC_DACP(x)		((x) & 0x7F)
 
-/*
- * struct ad7746_chip_info - chip specific information
- */
-
 struct ad7746_chip_info {
 	struct i2c_client *client;
 	struct mutex lock; /* protect sensor state */
@@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
 
 		if (chip->capdac_set != chan->channel) {
 			ret = i2c_smbus_write_byte_data(chip->client,
-				AD7746_REG_CAPDACA,
-				chip->capdac[chan->channel][0]);
+							AD7746_REG_CAPDACA,
+							chip->capdac[chan->channel][0]);
 			if (ret < 0)
 				return ret;
+
 			ret = i2c_smbus_write_byte_data(chip->client,
-				AD7746_REG_CAPDACB,
-				chip->capdac[chan->channel][1]);
+							AD7746_REG_CAPDACB,
+							chip->capdac[chan->channel][1]);
 			if (ret < 0)
 				return ret;
 
@@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
 
 		switch (chan->type) {
 		case IIO_TEMP:
-		/*
-		 * temperature in milli degrees Celsius
-		 * T = ((*val / 2048) - 4096) * 1000
-		 */
+			/*
+			 * temperature in milli degrees Celsius
+			 * T = ((*val / 2048) - 4096) * 1000
+			 */
 			*val = (*val * 125) / 256;
 			break;
 		case IIO_VOLTAGE:
@@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = {
 	.write_raw = ad7746_write_raw,
 };
 
-/*
- * device probe and remove
- */
-
 static int ad7746_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -730,11 +723,7 @@ static int ad7746_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
-	if (ret)
-		return ret;
-
-	return 0;
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 }
 
 static const struct i2c_device_id ad7746_id[] = {
-- 
2.31.1

[PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Lucas Stankus]

AD7745 devices don't have the CIN2 pins and therefore can't handle related
channels. Forcing the number of AD7746 channels may lead to enabling more
channels than what the hardware actually supports.
Avoid num_channels being overwritten after first assignment.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index e03d010b2f4c..9e0da43b2871 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
 		indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
 	else
 		indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
-	indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (pdata) {
-- 
2.31.1

Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return

[Fabio Aiuto]

Hi Lucas,

On Tue, May 11, 2021 at 05:54:01PM -0300, Lucas Stankus wrote:
> Remove vague comments, align temperature comment with indent block and
> simplify probe return on device register.
> 
> Also fix the following checkpatch warning:
> CHECK: Alignment should match open parenthesis
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index dfd71e99e872..e03d010b2f4c 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -84,10 +84,6 @@
>  #define AD7746_CAPDAC_DACEN		BIT(7)
>  #define AD7746_CAPDAC_DACP(x)		((x) & 0x7F)
>  
> -/*
> - * struct ad7746_chip_info - chip specific information
> - */
> -

Comment deletions should go in a separate patch


>  struct ad7746_chip_info {
>  	struct i2c_client *client;
>  	struct mutex lock; /* protect sensor state */
> @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
>  
>  		if (chip->capdac_set != chan->channel) {
>  			ret = i2c_smbus_write_byte_data(chip->client,
> -				AD7746_REG_CAPDACA,
> -				chip->capdac[chan->channel][0]);
> +							AD7746_REG_CAPDACA,
> +							chip->capdac[chan->channel][0]);
>  			if (ret < 0)
>  				return ret;
> +
>  			ret = i2c_smbus_write_byte_data(chip->client,
> -				AD7746_REG_CAPDACB,
> -				chip->capdac[chan->channel][1]);
> +							AD7746_REG_CAPDACB,
> +							chip->capdac[chan->channel][1]);
>  			if (ret < 0)
>  				return ret;

Alignments of function argument form a different logical change
and should go on a separate patch...
 
>  
> @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  
>  		switch (chan->type) {
>  		case IIO_TEMP:
> -		/*
> -		 * temperature in milli degrees Celsius
> -		 * T = ((*val / 2048) - 4096) * 1000
> -		 */
> +			/*
> +			 * temperature in milli degrees Celsius
> +			 * T = ((*val / 2048) - 4096) * 1000
> +			 */
>  			*val = (*val * 125) / 256;
>  			break;
>  		case IIO_VOLTAGE:
> @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = {
>  	.write_raw = ad7746_write_raw,
>  };
>  
> -/*
> - * device probe and remove
> - */
> -
>  static int ad7746_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -730,11 +723,7 @@ static int ad7746_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }

this return value fix should go in a separate patch

>  
>  static const struct i2c_device_id ad7746_id[] = {
> -- 
> 2.31.1
> 
> 

so, in my opinion, this patch could be split
in three different patches.

Thank you,

fabio

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Alexandru Ardelean]

On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
<lucas.p.stankus@gmail.com> wrote:
>
> AD7745 devices don't have the CIN2 pins and therefore can't handle related
> channels. Forcing the number of AD7746 channels may lead to enabling more
> channels than what the hardware actually supports.
> Avoid num_channels being overwritten after first assignment.
>
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index e03d010b2f4c..9e0da43b2871 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
>                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>         else
>                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);

ohh; good catch

this falls into the category of a fix, so a Fixes tag is required;
this looks so old, that i did not bother tracking it before
83e416f458d53  [which is 2011]

so, maybe something like:

Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
scratch.")

>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         if (pdata) {
> --
> 2.31.1
>

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Alexandru Ardelean]

On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
<lucas.p.stankus@gmail.com> wrote:
>
> AD7745 devices don't have the CIN2 pins and therefore can't handle related
> channels. Forcing the number of AD7746 channels may lead to enabling more
> channels than what the hardware actually supports.
> Avoid num_channels being overwritten after first assignment.
>
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index e03d010b2f4c..9e0da43b2871 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
>                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>         else
>                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);

ohh; good catch

this falls into the category of a fix, so a Fixes tag is required;
this looks so old, that i did not bother tracking it before
83e416f458d53  [which is 2011]

so, maybe something like:

Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
scratch.")

>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         if (pdata) {
> --
> 2.31.1
>

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Jonathan Cameron]

On Wed, 12 May 2021 20:20:02 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> <lucas.p.stankus@gmail.com> wrote:
> >
> > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > channels. Forcing the number of AD7746 channels may lead to enabling more
> > channels than what the hardware actually supports.
> > Avoid num_channels being overwritten after first assignment.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index e03d010b2f4c..9e0da43b2871 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         else
> >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);  
> 
> ohh; good catch
> 
> this falls into the category of a fix, so a Fixes tag is required;
> this looks so old, that i did not bother tracking it before
> 83e416f458d53  [which is 2011]
> 
> so, maybe something like:
> 
> Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> scratch.")

ouch.  Given I was queuing up some fixes I've added this one to the fixes-togreg
branch of iio.git and marked it for stable.

So drop this one from your v2 series with the changes requested in patch 1.

Thanks,

Jonathan

> 
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         if (pdata) {
> > --
> > 2.31.1
> >  

Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return

[Jonathan Cameron]

On Tue, 11 May 2021 17:54:01 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> Remove vague comments, align temperature comment with indent block and
> simplify probe return on device register.
> 
> Also fix the following checkpatch warning:
> CHECK: Alignment should match open parenthesis
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>

As Fabio pointed out, finer grained patches with one type of change per
patch would be good.

> ---
>  drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index dfd71e99e872..e03d010b2f4c 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -84,10 +84,6 @@
>  #define AD7746_CAPDAC_DACEN		BIT(7)
>  #define AD7746_CAPDAC_DACP(x)		((x) & 0x7F)
>  
> -/*
> - * struct ad7746_chip_info - chip specific information
> - */
> -
>  struct ad7746_chip_info {
>  	struct i2c_client *client;
>  	struct mutex lock; /* protect sensor state */
> @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
>  
>  		if (chip->capdac_set != chan->channel) {
>  			ret = i2c_smbus_write_byte_data(chip->client,
> -				AD7746_REG_CAPDACA,
> -				chip->capdac[chan->channel][0]);
> +							AD7746_REG_CAPDACA,
> +							chip->capdac[chan->channel][0]);
>  			if (ret < 0)
>  				return ret;
> +
>  			ret = i2c_smbus_write_byte_data(chip->client,
> -				AD7746_REG_CAPDACB,
> -				chip->capdac[chan->channel][1]);
> +							AD7746_REG_CAPDACB,
> +							chip->capdac[chan->channel][1]);
>  			if (ret < 0)
>  				return ret;

I wondered if it might be sensible to factor this code out to reduce the indent
and make things more readable.  Having taken a look it seems there is another
place with exactly the same call sequence.  From how it's used there, I'm
assuming this is updating the offsets.  As such, I would introduce an

ad7746_offsets_set(struct iio_dev *indio_dev, int channel)

or similar.


>  
> @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  
>  		switch (chan->type) {
>  		case IIO_TEMP:
> -		/*
> -		 * temperature in milli degrees Celsius
> -		 * T = ((*val / 2048) - 4096) * 1000
> -		 */
> +			/*
> +			 * temperature in milli degrees Celsius
> +			 * T = ((*val / 2048) - 4096) * 1000
> +			 */
>  			*val = (*val * 125) / 256;
>  			break;
>  		case IIO_VOLTAGE:
> @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = {
>  	.write_raw = ad7746_write_raw,
>  };
>  
> -/*
> - * device probe and remove
> - */
> -
>  static int ad7746_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -730,11 +723,7 @@ static int ad7746_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }
>  
>  static const struct i2c_device_id ad7746_id[] = {

Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return

[Lucas Stankus]

On Thu, May 13, 2021 at 12:59 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 11 May 2021 17:54:01 -0300
> Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> > Remove vague comments, align temperature comment with indent block and
> > simplify probe return on device register.
> >
> > Also fix the following checkpatch warning:
> > CHECK: Alignment should match open parenthesis
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
>
> As Fabio pointed out, finer grained patches with one type of change per
> patch would be good.

Thank you both for the review and sorry for the radio silence, I'll split
the patch in the v2.

>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++---------------------
> >  1 file changed, 10 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index dfd71e99e872..e03d010b2f4c 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -84,10 +84,6 @@
> >  #define AD7746_CAPDAC_DACEN          BIT(7)
> >  #define AD7746_CAPDAC_DACP(x)                ((x) & 0x7F)
> >
> > -/*
> > - * struct ad7746_chip_info - chip specific information
> > - */
> > -
> >  struct ad7746_chip_info {
> >       struct i2c_client *client;
> >       struct mutex lock; /* protect sensor state */
> > @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> >
> >               if (chip->capdac_set != chan->channel) {
> >                       ret = i2c_smbus_write_byte_data(chip->client,
> > -                             AD7746_REG_CAPDACA,
> > -                             chip->capdac[chan->channel][0]);
> > +                                                     AD7746_REG_CAPDACA,
> > +                                                     chip->capdac[chan->channel][0]);
> >                       if (ret < 0)
> >                               return ret;
> > +                       ret = i2c_smbus_write_byte_data(chip->client,
> > -                             AD7746_REG_CAPDACB,
> > -                             chip->capdac[chan->channel][1]);
> > +                                                     AD7746_REG_CAPDACB,
> > +                                                     chip->capdac[chan->channel][1]);
> >                       if (ret < 0)
> >                               return ret;
>
> I wondered if it might be sensible to factor this code out to reduce the indent
> and make things more readable.  Having taken a look it seems there is another
> place with exactly the same call sequence.  From how it's used there, I'm
> assuming this is updating the offsets.  As such, I would introduce an
>
> ad7746_offsets_set(struct iio_dev *indio_dev, int channel)
>
> or similar.
>

Makes sense, I'll do that in the v2 as well.

>
> >
> > @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
> >
> >               switch (chan->type) {
> >               case IIO_TEMP:
> > -             /*
> > -              * temperature in milli degrees Celsius
> > -              * T = ((*val / 2048) - 4096) * 1000
> > -              */
> > +                     /*
> > +                      * temperature in milli degrees Celsius
> > +                      * T = ((*val / 2048) - 4096) * 1000
> > +                      */
> >                       *val = (*val * 125) / 256;
> >                       break;
> >               case IIO_VOLTAGE:
> > @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = {
> >       .write_raw = ad7746_write_raw,
> >  };
> >
> > -/*
> > - * device probe and remove
> > - */
> > -
> >  static int ad7746_probe(struct i2c_client *client,
> >                       const struct i2c_device_id *id)
> >  {
> > @@ -730,11 +723,7 @@ static int ad7746_probe(struct i2c_client *client,
> >       if (ret < 0)
> >               return ret;
> >
> > -     ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > -     if (ret)
> > -             return ret;
> > -
> > -     return 0;
> > +     return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> >  }
> >
> >  static const struct i2c_device_id ad7746_id[] = {
>

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Lucas Stankus]

On Thu, May 13, 2021 at 12:51 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 12 May 2021 20:20:02 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> > <lucas.p.stankus@gmail.com> wrote:
> > >
> > > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > > channels. Forcing the number of AD7746 channels may lead to enabling more
> > > channels than what the hardware actually supports.
> > > Avoid num_channels being overwritten after first assignment.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index e03d010b2f4c..9e0da43b2871 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> > >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         else
> > >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >
> > ohh; good catch
> >
> > this falls into the category of a fix, so a Fixes tag is required;
> > this looks so old, that i did not bother tracking it before
> > 83e416f458d53  [which is 2011]
> >
> > so, maybe something like:
> >
> > Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> > scratch.")
>
> ouch.  Given I was queuing up some fixes I've added this one to the fixes-togreg
> branch of iio.git and marked it for stable.
>
> So drop this one from your v2 series with the changes requested in patch 1.
>
> Thanks,
>
> Jonathan

No problems, but I think I should've better checked the mailing list before
sending the patch, it would have avoided the noise.

Anyway, thanks for the review :)

>
> >
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> > >         if (pdata) {
> > > --
> > > 2.31.1
> > >
>

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Lucas Stankus]

On Thu, May 13, 2021 at 12:51 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 12 May 2021 20:20:02 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> > <lucas.p.stankus@gmail.com> wrote:
> > >
> > > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > > channels. Forcing the number of AD7746 channels may lead to enabling more
> > > channels than what the hardware actually supports.
> > > Avoid num_channels being overwritten after first assignment.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index e03d010b2f4c..9e0da43b2871 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> > >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         else
> > >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >
> > ohh; good catch
> >
> > this falls into the category of a fix, so a Fixes tag is required;
> > this looks so old, that i did not bother tracking it before
> > 83e416f458d53  [which is 2011]
> >
> > so, maybe something like:
> >
> > Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> > scratch.")
>
> ouch.  Given I was queuing up some fixes I've added this one to the fixes-togreg
> branch of iio.git and marked it for stable.
>
> So drop this one from your v2 series with the changes requested in patch 1.
>
> Thanks,
>
> Jonathan

No problems, but I think I should've better checked the mailing list before
sending the patch, it would have avoided the noise.

Anyway, thanks for the review :)

>
> >
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> > >         if (pdata) {
> > > --
> > > 2.31.1
> > >
>

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Lucas Stankus]

On Wed, May 12, 2021 at 2:20 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> <lucas.p.stankus@gmail.com> wrote:
> >
> > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > channels. Forcing the number of AD7746 channels may lead to enabling more
> > channels than what the hardware actually supports.
> > Avoid num_channels being overwritten after first assignment.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index e03d010b2f4c..9e0da43b2871 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         else
> >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>
> ohh; good catch
>
> this falls into the category of a fix, so a Fixes tag is required;
> this looks so old, that i did not bother tracking it before
> 83e416f458d53  [which is 2011]

As Jonathan said, this bug was already fixed and the patch will be dropped,
but thank you for the review.

This was my first bug fix in the kernel, so sorry for the absence of a
Fixes tag, I'll make sure to add one next time.


>
> so, maybe something like:
>
> Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> scratch.")
>
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         if (pdata) {
> > --
> > 2.31.1
> >

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Lucas Stankus]

On Wed, May 12, 2021 at 2:20 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> <lucas.p.stankus@gmail.com> wrote:
> >
> > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > channels. Forcing the number of AD7746 channels may lead to enabling more
> > channels than what the hardware actually supports.
> > Avoid num_channels being overwritten after first assignment.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index e03d010b2f4c..9e0da43b2871 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         else
> >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>
> ohh; good catch
>
> this falls into the category of a fix, so a Fixes tag is required;
> this looks so old, that i did not bother tracking it before
> 83e416f458d53  [which is 2011]

As Jonathan said, this bug was already fixed and the patch will be dropped,
but thank you for the review.

This was my first bug fix in the kernel, so sorry for the absence of a
Fixes tag, I'll make sure to add one next time.


>
> so, maybe something like:
>
> Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> scratch.")
>
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         if (pdata) {
> > --
> > 2.31.1
> >

Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

[Jonathan Cameron]

On Mon, 17 May 2021 21:55:20 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> On Wed, May 12, 2021 at 2:20 PM Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
> >
> > On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> > <lucas.p.stankus@gmail.com> wrote:  
> > >
> > > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > > channels. Forcing the number of AD7746 channels may lead to enabling more
> > > channels than what the hardware actually supports.
> > > Avoid num_channels being overwritten after first assignment.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index e03d010b2f4c..9e0da43b2871 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> > >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         else
> > >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);  
> >
> > ohh; good catch
> >
> > this falls into the category of a fix, so a Fixes tag is required;
> > this looks so old, that i did not bother tracking it before
> > 83e416f458d53  [which is 2011]  
> 
> As Jonathan said, this bug was already fixed and the patch will be dropped,
> but thank you for the review.
> 
> This was my first bug fix in the kernel, so sorry for the absence of a
> Fixes tag, I'll make sure to add one next time.
> 

Wasn't already fixed - I just applied this patch without PATCH 1/2
so now it is ;)

Jonathan

> 
> >
> > so, maybe something like:
> >
> > Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> > scratch.")
> >  
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> > >         if (pdata) {
> > > --
> > > 2.31.1
> > >