iio: ad7949: fix wrong ADC result due to incorrect bit mask
Subsystems (2)
PATCH v3 1/1 iio: ad7949: fix wrong ADC result due to incorrect bit mask
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].
---
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(-)
drivers/iio/adc/ad7949.c +1 -1
@@ -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[] = { {
Signature · 1 line
2.25.1
Quoted 11 lines · Wilfried Wessner
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. TheGENMASK()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,
Quoted 30 lines
Signed-off-by Wilfried Wessner <[email protected]>---
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(-)drivers/iio/adc/ad7949.c +1
-2@@ -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
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 15 lines · Andy Shevchenko
Quoted 11 lines · Wilfried Wessner
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. TheGENMASK()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")
Quoted 5 lines
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:
Please advise, thank you!
Best Regards,
Willi
Quoted 39 lines
And FWIW,
Reviewed-by Andy Shevchenko <[email protected]>Quoted 28 lines
Signed-off-by Wilfried Wessner <[email protected]>---
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(-)drivers/iio/adc/ad7949.c +1
-2@@ -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[] = { { --Quoted 2 lines
2.25.1
--
With Best Regards,
Andy Shevchenko
Quoted 4 lines · Wilfried Wessner
Quoted 2 lines · Andy Shevchenko
On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner
<[email protected]> wrote:
...
Quoted 8 lines
Quoted 5 lines
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.
Quoted 3 lines
And if so, should I add the tags:
Reviewed-by Andy Shevchenko <[email protected]>Reviewed-by Charles-Antoine Couret <[email protected]>
If resend, yes, you need to add them.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 18 lines · On Thu, 11 Feb 2021 21:24:04 +0200 Andy Shevchenko <[email protected]>…
Quoted 4 lines · Wilfried Wessner
Quoted 2 lines · Andy Shevchenko
On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner
<[email protected]> wrote:...
Quoted 8 lines
Quoted 5 lines
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.
Quoted 9 lines
Depends on you. Jonothan usually processes the queue during weekends, so no hurry.
Quoted 3 lines
And if so, should I add the tags:
Reviewed-by Andy Shevchenko <[email protected]>Reviewed-by Charles-Antoine Couret <[email protected]>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
Quoted 33 lines · Jonathan Cameron
Quoted 18 lines · On Thu, 11 Feb 2021 21:24:04 +0200 Andy Shevchenko <[email protected]>…
Quoted 4 lines · Wilfried Wessner
Quoted 2 lines · Andy Shevchenko
On Mon, Feb 8, 2021 at 4:27 PM Wilfried Wessner
<[email protected]> wrote:...
Quoted 8 lines
Quoted 5 lines
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.
Quoted 9 lines
Depends on you. Jonothan usually processes the queue during weekends, so no hurry.
Quoted 3 lines
And if so, should I add the tags:
Reviewed-by Andy Shevchenko <[email protected]>Reviewed-by Charles-Antoine Couret <[email protected]>If resend, yes, you need to add them.
Applied with the minor spacing and title tweak Andy suggested
Quoted 9 lines
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
Le 08/02/2021 à 15:27, Wilfried Wessner a écrit :
Quoted 27 lines
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. TheGENMASK()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 <[email protected]>---
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(-)drivers/iio/adc/ad7949.c +0
-0
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 97
Quoted 8 lines
drivers/iio/adc/ad7949.c +1
-1@@ -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. :)
Regards,
Charles-Antoine Couret