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

[Jonathan Cameron <jic23@kernel.org>]

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 */
>