* [PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging @ 2021-05-11 20:53 Lucas Stankus 2021-05-11 20:54 ` [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return Lucas Stankus 2021-05-11 20:54 ` [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels Lucas Stankus 0 siblings, 2 replies; 11+ messages in thread From: Lucas Stankus @ 2021-05-11 20:53 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, gregkh Cc: linux-iio, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return 2021-05-11 20:53 [PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus @ 2021-05-11 20:54 ` Lucas Stankus 2021-05-12 7:45 ` Fabio Aiuto 2021-05-13 16:00 ` Jonathan Cameron 2021-05-11 20:54 ` [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels Lucas Stankus 1 sibling, 2 replies; 11+ messages in thread From: Lucas Stankus @ 2021-05-11 20:54 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, gregkh Cc: linux-iio, linux-staging, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return 2021-05-11 20:54 ` [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return Lucas Stankus @ 2021-05-12 7:45 ` Fabio Aiuto 2021-05-13 16:00 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: Fabio Aiuto @ 2021-05-12 7:45 UTC (permalink / raw) To: Lucas Stankus Cc: lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return 2021-05-11 20:54 ` [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return Lucas Stankus 2021-05-12 7:45 ` Fabio Aiuto @ 2021-05-13 16:00 ` Jonathan Cameron 2021-05-18 0:43 ` Lucas Stankus 1 sibling, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2021-05-13 16:00 UTC (permalink / raw) To: Lucas Stankus Cc: lars, Michael.Hennerich, gregkh, linux-iio, linux-staging, linux-kernel 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[] = { ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return 2021-05-13 16:00 ` Jonathan Cameron @ 2021-05-18 0:43 ` Lucas Stankus 0 siblings, 0 replies; 11+ messages in thread From: Lucas Stankus @ 2021-05-18 0:43 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, Michael.Hennerich, gregkh, linux-iio, linux-staging, linux-kernel, fabioaiuto83 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[] = { > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-11 20:53 [PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus 2021-05-11 20:54 ` [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return Lucas Stankus @ 2021-05-11 20:54 ` Lucas Stankus 2021-05-12 17:20 ` Alexandru Ardelean 1 sibling, 1 reply; 11+ messages in thread From: Lucas Stankus @ 2021-05-11 20:54 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, gregkh Cc: linux-iio, linux-staging, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-11 20:54 ` [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels Lucas Stankus @ 2021-05-12 17:20 ` Alexandru Ardelean 2021-05-13 15:52 ` Jonathan Cameron 2021-05-18 0:55 ` Lucas Stankus 0 siblings, 2 replies; 11+ messages in thread From: Alexandru Ardelean @ 2021-05-12 17:20 UTC (permalink / raw) To: Lucas Stankus Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron, Greg Kroah-Hartman, linux-iio, linux-staging, LKML 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-12 17:20 ` Alexandru Ardelean @ 2021-05-13 15:52 ` Jonathan Cameron 2021-05-18 0:49 ` Lucas Stankus 2021-05-18 0:55 ` Lucas Stankus 1 sibling, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2021-05-13 15:52 UTC (permalink / raw) To: Alexandru Ardelean Cc: Lucas Stankus, Lars-Peter Clausen, Hennerich, Michael, Greg Kroah-Hartman, linux-iio, linux-staging, LKML 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-13 15:52 ` Jonathan Cameron @ 2021-05-18 0:49 ` Lucas Stankus 0 siblings, 0 replies; 11+ messages in thread From: Lucas Stankus @ 2021-05-18 0:49 UTC (permalink / raw) To: Jonathan Cameron Cc: Alexandru Ardelean, Lars-Peter Clausen, Hennerich, Michael, Greg Kroah-Hartman, linux-iio, linux-staging, LKML 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 > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-12 17:20 ` Alexandru Ardelean 2021-05-13 15:52 ` Jonathan Cameron @ 2021-05-18 0:55 ` Lucas Stankus 2021-05-18 12:08 ` Jonathan Cameron 1 sibling, 1 reply; 11+ messages in thread From: Lucas Stankus @ 2021-05-18 0:55 UTC (permalink / raw) To: Alexandru Ardelean Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron, Greg Kroah-Hartman, linux-iio, linux-staging, LKML 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels 2021-05-18 0:55 ` Lucas Stankus @ 2021-05-18 12:08 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2021-05-18 12:08 UTC (permalink / raw) To: Lucas Stankus Cc: Alexandru Ardelean, Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron, Greg Kroah-Hartman, linux-iio, linux-staging, LKML 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 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-18 12:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-11 20:53 [PATCH 0/2] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus 2021-05-11 20:54 ` [PATCH 1/2] staging: iio: cdc: ad7746: clean up driver comments and probe return Lucas Stankus 2021-05-12 7:45 ` Fabio Aiuto 2021-05-13 16:00 ` Jonathan Cameron 2021-05-18 0:43 ` Lucas Stankus 2021-05-11 20:54 ` [PATCH 2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels Lucas Stankus 2021-05-12 17:20 ` Alexandru Ardelean 2021-05-13 15:52 ` Jonathan Cameron 2021-05-18 0:49 ` Lucas Stankus 2021-05-18 0:55 ` Lucas Stankus 2021-05-18 12:08 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).