| Message ID | 09e65d3a235febfc4c3ee172b573ba8c9cde94b8.1620766020.git.lucas.p.stankus@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: cdc: ad7746: initial effort to move out of staging | expand |
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 >
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 > >
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 > > > >
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 > >
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 > > >
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) {
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(-)