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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux