[PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[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. 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[] = {
 		{
-- 
2.25.1

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[Andy Shevchenko]

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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[Wilfried Wessner]

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

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[Jonathan Cameron]

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

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[Wilfried Wessner]

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

Re: [PATCH v3] iio: ad7949: fix wrong ADC result due to incorrect bit mask

[Couret Charles-Antoine]

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