| Message ID | 20240618-cleanup-ad7606-v1-8-f1854d5c779d@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | iio: adc: ad7606: Improvements | expand |
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 */ >
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 --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 */
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(-)