| Message ID | 20210208142705.GA51260@ubuntu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask | expand |
On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > > Fixes a wrong bit mask used for the ADC's result, which was caused by an > improper usage of the GENMASK() macro. The bits higher than ADC's > resolution are undefined and if not masked out correctly, a wrong result > can be given. The GENMASK() macro indexing is zero based, so the mask has > to go from [resolution - 1 , 0]. > Fixes: 7f40e0614317f ("iio:adc:ad7949: Add AD7949 ADC driver family") > Shouldn't be blank like here, but I think Jonathan can fix when applying. Jonathan, can you also amend the subject (I totally forgot about subsubsystem prefix)? Should be like: "iio: adc: ad7949: fix wrong results due to incorrect bit mask" And FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > --- > > The issue was found in combination of an AD7682 ADC with an ARM based > iMX7-CPU. The SPI line was analyzed with a logic analyzer and a > discrepancy between applied voltage level and the ADC reported value > in user space was observed. Digging into the driver code revealed an > improper mask used for the ADC-result. > > > drivers/iio/adc/ad7949.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index 5d597e5050f6..1b4b3203e428 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > int ret; > int i; > int bits_per_word = ad7949_adc->resolution; > - int mask = GENMASK(ad7949_adc->resolution, 0); > + int mask = GENMASK(ad7949_adc->resolution - 1, 0); > struct spi_message msg; > struct spi_transfer tx[] = { > { > -- > 2.25.1 >
Le 08/02/2021 à 15:27, Wilfried Wessner a écrit : > Fixes a wrong bit mask used for the ADC's result, which was caused by an > improper usage of the GENMASK() macro. The bits higher than ADC's > resolution are undefined and if not masked out correctly, a wrong result > can be given. The GENMASK() macro indexing is zero based, so the mask has > to go from [resolution - 1 , 0]. > > Fixes: 7f40e0614317f ("iio:adc:ad7949: Add AD7949 ADC driver family") > > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > --- > > The issue was found in combination of an AD7682 ADC with an ARM based > iMX7-CPU. The SPI line was analyzed with a logic analyzer and a > discrepancy between applied voltage level and the ADC reported value > in user space was observed. Digging into the driver code revealed an > improper mask used for the ADC-result. > > > drivers/iio/adc/ad7949.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index 5d597e5050f6..1b4b3203e428 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > int ret; > int i; > int bits_per_word = ad7949_adc->resolution; > - int mask = GENMASK(ad7949_adc->resolution, 0); > + int mask = GENMASK(ad7949_adc->resolution - 1, 0); > struct spi_message msg; > struct spi_transfer tx[] = { > { Hi, Thank you for the fix. :) Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> Regards, Charles-Antoine Couret
On Mon, Feb 8, 2021 at 5:06 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner > <wilfried.wessner@gmail.com> wrote: > > > > Fixes a wrong bit mask used for the ADC's result, which was caused by an > > improper usage of the GENMASK() macro. The bits higher than ADC's > > resolution are undefined and if not masked out correctly, a wrong result > > can be given. The GENMASK() macro indexing is zero based, so the mask has > > to go from [resolution - 1 , 0]. > > > > Fixes: 7f40e0614317f ("iio:adc:ad7949: Add AD7949 ADC driver family") > > > > > Shouldn't be blank like here, but I think Jonathan can fix when applying. > Jonathan, can you also amend the subject (I totally forgot about > subsubsystem prefix)? > Should be like: > "iio: adc: ad7949: fix wrong results due to incorrect bit mask" Should I send a v4 with the changes proposed by Andy? It would change the subject. And if so, should I add the tags: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> Please advise, thank you! Best Regards, Willi > > And FWIW, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > > > --- > > > > The issue was found in combination of an AD7682 ADC with an ARM based > > iMX7-CPU. The SPI line was analyzed with a logic analyzer and a > > discrepancy between applied voltage level and the ADC reported value > > in user space was observed. Digging into the driver code revealed an > > improper mask used for the ADC-result. > > > > > > drivers/iio/adc/ad7949.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > index 5d597e5050f6..1b4b3203e428 100644 > > --- a/drivers/iio/adc/ad7949.c > > +++ b/drivers/iio/adc/ad7949.c > > @@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > int ret; > > int i; > > int bits_per_word = ad7949_adc->resolution; > > - int mask = GENMASK(ad7949_adc->resolution, 0); > > + int mask = GENMASK(ad7949_adc->resolution - 1, 0); > > struct spi_message msg; > > struct spi_transfer tx[] = { > > { > > -- > > 2.25.1 > > > > > -- > With Best Regards, > Andy Shevchenko
On Thu, Feb 11, 2021 at 8:42 PM Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > On Mon, Feb 8, 2021 at 5:06 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner > > <wilfried.wessner@gmail.com> wrote: ... > > Shouldn't be blank like here, but I think Jonathan can fix when applying. > > Jonathan, can you also amend the subject (I totally forgot about > > subsubsystem prefix)? > > Should be like: > > "iio: adc: ad7949: fix wrong results due to incorrect bit mask" > > Should I send a v4 with the changes proposed by Andy? > It would change the subject. Depends on you. Jonothan usually processes the queue during weekends, so no hurry. > And if so, should I add the tags: > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> If resend, yes, you need to add them.
On Thu, 11 Feb 2021 21:24:04 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Feb 11, 2021 at 8:42 PM Wilfried Wessner > <wilfried.wessner@gmail.com> wrote: > > On Mon, Feb 8, 2021 at 5:06 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner > > > <wilfried.wessner@gmail.com> wrote: > > ... > > > > Shouldn't be blank like here, but I think Jonathan can fix when applying. > > > Jonathan, can you also amend the subject (I totally forgot about > > > subsubsystem prefix)? > > > Should be like: > > > "iio: adc: ad7949: fix wrong results due to incorrect bit mask" > > > > Should I send a v4 with the changes proposed by Andy? > > It would change the subject. > > Depends on you. Jonothan usually processes the queue during weekends, > so no hurry. > > > And if so, should I add the tags: > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> > > If resend, yes, you need to add them. > Applied with the minor spacing and title tweak Andy suggested Applied to the fixes-togreg branch of iio.git. Given this has been there a while, I'm going to wait until after the merge window to send this upstream. So it will be a few weeks. Thanks, Jonathan
On Fri, Feb 12, 2021 at 8:05 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 11 Feb 2021 21:24:04 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Thu, Feb 11, 2021 at 8:42 PM Wilfried Wessner > > <wilfried.wessner@gmail.com> wrote: > > > On Mon, Feb 8, 2021 at 5:06 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner > > > > <wilfried.wessner@gmail.com> wrote: > > > > ... > > > > > > Shouldn't be blank like here, but I think Jonathan can fix when applying. > > > > Jonathan, can you also amend the subject (I totally forgot about > > > > subsubsystem prefix)? > > > > Should be like: > > > > "iio: adc: ad7949: fix wrong results due to incorrect bit mask" > > > > > > Should I send a v4 with the changes proposed by Andy? > > > It would change the subject. > > > > Depends on you. Jonothan usually processes the queue during weekends, > > so no hurry. > > > > > And if so, should I add the tags: > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> > > > > If resend, yes, you need to add them. > > > Applied with the minor spacing and title tweak Andy suggested > > Applied to the fixes-togreg branch of iio.git. Given this has been > there a while, I'm going to wait until after the merge window to > send this upstream. So it will be a few weeks. > > Thanks, > > Jonathan > > Great, thank you! Best regards, Willi
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c index 5d597e5050f6..1b4b3203e428 100644 --- a/drivers/iio/adc/ad7949.c +++ b/drivers/iio/adc/ad7949.c @@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, int ret; int i; int bits_per_word = ad7949_adc->resolution; - int mask = GENMASK(ad7949_adc->resolution, 0); + int mask = GENMASK(ad7949_adc->resolution - 1, 0); struct spi_message msg; struct spi_transfer tx[] = { {
Fixes a wrong bit mask used for the ADC's result, which was caused by an improper usage of the GENMASK() macro. The bits higher than ADC's resolution are undefined and if not masked out correctly, a wrong result can be given. The GENMASK() macro indexing is zero based, so the mask has to go from [resolution - 1 , 0]. Fixes: 7f40e0614317f ("iio:adc:ad7949: Add AD7949 ADC driver family") Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> --- The issue was found in combination of an AD7682 ADC with an ARM based iMX7-CPU. The SPI line was analyzed with a logic analyzer and a discrepancy between applied voltage level and the ADC reported value in user space was observed. Digging into the driver code revealed an improper mask used for the ADC-result. drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)