diff mbox series

[8/9] iio: adc: ad7606: fix oversampling gpio array

Message ID 20240618-cleanup-ad7606-v1-8-f1854d5c779d@baylibre.com (mailing list archive)
State Not Applicable
Headers show
Series iio: adc: ad7606: Improvements | expand

Commit Message

Guillaume Stols June 18, 2024, 2:02 p.m. UTC
gpiod_set_array_value was misused here: the implementation relied on the
assumption that an unsigned long was required for each gpio, while the
function expects a bit array stored in "as much unsigned long as needed
for storing one bit per GPIO", i.e it is using a bit field.

Fixes: d2a415c86c6b ("iio: adc: ad7606: Add support for AD7606B ADC")
Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 4 ++--
 drivers/iio/adc/ad7606_spi.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron June 23, 2024, 3:45 p.m. UTC | #1
On Tue, 18 Jun 2024 14:02:40 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> gpiod_set_array_value was misused here: the implementation relied on the
> assumption that an unsigned long was required for each gpio, while the
> function expects a bit array stored in "as much unsigned long as needed
> for storing one bit per GPIO", i.e it is using a bit field.
> 
> Fixes: d2a415c86c6b ("iio: adc: ad7606: Add support for AD7606B ADC")
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
Always drag fixes to the start of a series.  Probably doesn't matter
in this case but we want it to be obvious there are no necessary precursors
in this series for anyone backporting.

What is the user visible outcome of this bug?  Superficially the numbers
all end up the same I think even though the code is clearly working
mostly by luck.  So might not warrant a fixes tag?


> ---
>  drivers/iio/adc/ad7606.c     | 4 ++--
>  drivers/iio/adc/ad7606_spi.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index e3426287edf6..502344e019e0 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -235,9 +235,9 @@ static int ad7606_write_os_hw(struct iio_dev *indio_dev, int val)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  	DECLARE_BITMAP(values, 3);
>  
> -	values[0] = val;
> +	values[0] = val & GENMASK(2, 0);
>  
> -	gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
> +	gpiod_set_array_value(st->gpio_os->ndescs, st->gpio_os->desc,
>  			      st->gpio_os->info, values);
>  
>  	/* AD7616 requires a reset to update value */
> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> index 263a778bcf25..287a0591533b 100644
> --- a/drivers/iio/adc/ad7606_spi.c
> +++ b/drivers/iio/adc/ad7606_spi.c
> @@ -249,8 +249,9 @@ static int ad7616_sw_mode_config(struct iio_dev *indio_dev)
>  static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> -	unsigned long os[3] = {1};
> +	DECLARE_BITMAP(os, 3);
>  
> +	bitmap_fill(os, 3);
>  	/*
>  	 * Software mode is enabled when all three oversampling
>  	 * pins are set to high. If oversampling gpios are defined
> @@ -258,7 +259,7 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
>  	 * otherwise, they must be hardwired to VDD
>  	 */
>  	if (st->gpio_os) {
> -		gpiod_set_array_value(ARRAY_SIZE(os),
> +		gpiod_set_array_value(st->gpio_os->ndescs,
>  				      st->gpio_os->desc, st->gpio_os->info, os);
>  	}
>  	/* OS of 128 and 256 are available only in software mode */
>
Guillaume Stols June 24, 2024, 3:08 p.m. UTC | #2
Resend, previous mail was erroneously sent in HTML. I apologize for the 
spamming.

On 6/23/24 17:45, Jonathan Cameron wrote:

 > On Tue, 18 Jun 2024 14:02:40 +0000
 > Guillaume Stols <gstols@baylibre.com> wrote:
 >> gpiod_set_array_value was misused here: the implementation relied on the
 >> assumption that an unsigned long was required for each gpio, while the
 >> function expects a bit array stored in "as much unsigned long as needed
 >> for storing one bit per GPIO", i.e it is using a bit field.
 >>
 >> Fixes: d2a415c86c6b ("iio: adc: ad7606: Add support for AD7606B ADC")
 >> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
 > Always drag fixes to the start of a series.  Probably doesn't matter
 > in this case but we want it to be obvious there are no necessary 
precursors
 > in this series for anyone backporting.

OK will do this change in the next version.

 >
 > What is the user visible outcome of this bug?  Superficially the numbers
 > all end up the same I think even though the code is clearly working
 > mostly by luck.  So might not warrant a fixes tag?

This is leading into some issues I should maybe have better documented 
in the commit message.

See below

 >
 >> ---
 >>   drivers/iio/adc/ad7606.c     | 4 ++--
 >>   drivers/iio/adc/ad7606_spi.c | 5 +++--
 >>   2 files changed, 5 insertions(+), 4 deletions(-)
 >>
 >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
 >> index e3426287edf6..502344e019e0 100644
 >> --- a/drivers/iio/adc/ad7606.c
 >> +++ b/drivers/iio/adc/ad7606.c
 >> @@ -235,9 +235,9 @@ static int ad7606_write_os_hw(struct iio_dev 
*indio_dev, int val)
 >>       struct ad7606_state *st = iio_priv(indio_dev);
 >>       DECLARE_BITMAP(values, 3);
 >>   -    values[0] = val;
 >> +    values[0] = val & GENMASK(2, 0);
 >>   -    gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
 >> +    gpiod_set_array_value(st->gpio_os->ndescs, st->gpio_os->desc,
 >>                     st->gpio_os->info, values);

ARRAY_SIZE(values) is 1 because DECLARE_BITMAP will declare a dimension 
1 unsigned long array (more than enough for 3 bits !).
We want to set 3 bits in gpiod_set_array_value, thus the first parameter 
should be 3, not 1.

 >>         /* AD7616 requires a reset to update value */
 >> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
 >> index 263a778bcf25..287a0591533b 100644
 >> --- a/drivers/iio/adc/ad7606_spi.c
 >> +++ b/drivers/iio/adc/ad7606_spi.c
 >> @@ -249,8 +249,9 @@ static int ad7616_sw_mode_config(struct iio_dev 
*indio_dev)
 >>   static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
 >>   {
 >>       struct ad7606_state *st = iio_priv(indio_dev);
 >> -    unsigned long os[3] = {1};
 >> +    DECLARE_BITMAP(os, 3);
 >>   +    bitmap_fill(os, 3);

Here we need 3 bits set HIGH in one unsigned long (i.e 0x07) and we get 
3 times 0x01 instead.

Thus, it will not switch to software mode if OS pins are not hardwired 
(which is I must admit, rather unlikely).

 >>       /*
 >>        * Software mode is enabled when all three oversampling
 >>        * pins are set to high. If oversampling gpios are defined
 >> @@ -258,7 +259,7 @@ static int ad7606B_sw_mode_config(struct iio_dev 
*indio_dev)
 >>        * otherwise, they must be hardwired to VDD
 >>        */
 >>       if (st->gpio_os) {
 >> -        gpiod_set_array_value(ARRAY_SIZE(os),
 >> +        gpiod_set_array_value(st->gpio_os->ndescs,
 >>                         st->gpio_os->desc, st->gpio_os->info, os);
 >>       }
 >>       /* OS of 128 and 256 are available only in software mode */
 >>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index e3426287edf6..502344e019e0 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -235,9 +235,9 @@  static int ad7606_write_os_hw(struct iio_dev *indio_dev, int val)
 	struct ad7606_state *st = iio_priv(indio_dev);
 	DECLARE_BITMAP(values, 3);
 
-	values[0] = val;
+	values[0] = val & GENMASK(2, 0);
 
-	gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
+	gpiod_set_array_value(st->gpio_os->ndescs, st->gpio_os->desc,
 			      st->gpio_os->info, values);
 
 	/* AD7616 requires a reset to update value */
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 263a778bcf25..287a0591533b 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -249,8 +249,9 @@  static int ad7616_sw_mode_config(struct iio_dev *indio_dev)
 static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
-	unsigned long os[3] = {1};
+	DECLARE_BITMAP(os, 3);
 
+	bitmap_fill(os, 3);
 	/*
 	 * Software mode is enabled when all three oversampling
 	 * pins are set to high. If oversampling gpios are defined
@@ -258,7 +259,7 @@  static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
 	 * otherwise, they must be hardwired to VDD
 	 */
 	if (st->gpio_os) {
-		gpiod_set_array_value(ARRAY_SIZE(os),
+		gpiod_set_array_value(st->gpio_os->ndescs,
 				      st->gpio_os->desc, st->gpio_os->info, os);
 	}
 	/* OS of 128 and 256 are available only in software mode */