Re: iio:adc:ad7280a: Move out of staging

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 02/24, Colin King (gmail) wrote:
> Hi,
> 
> Static analysis with clang scan picked up a potential issue with
> drivers/iio/adc/ad7280a.c in function ad7280a_write_thresh, the analysis is
> as follows:
> 
>         switch (chan->type) {
>         case IIO_VOLTAGE:
>                 value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
>                 value = clamp(value, 0L, 0xFFL);
> 
>                 ^^
> Note: variable value is being assigned a value
> 
>                 switch (dir) {
>                 case IIO_EV_DIR_RISING:
>                         addr = AD7280A_CELL_OVERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->cell_threshhigh = value;
> 
> ..and value is being used here ^^
> 
>                         break;
>                 case IIO_EV_DIR_FALLING:
>                         addr = AD7280A_CELL_UNDERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->cell_threshlow = value;
> 
> and value is being used here ^^
> 
>                         break;
>                 default:
>                         ret = -EINVAL;
>                         goto err_unlock;
>                 }
>                 break;
> 
> However for the IIO_TEMP case:
> 
>         case IIO_TEMP:
>                 value = (val * 10) / 196; /* LSB 19.6mV */
>                 value = clamp(value, 0L, 0xFFL);
> 
>                 ^^
> Note: variable value is being assigned a value
> 
>                 switch (dir) {
>                 case IIO_EV_DIR_RISING:
>                         addr = AD7280A_AUX_ADC_OVERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->aux_threshhigh = val;
>                                              ^^
> But val is being used here rather than value
> 
>                         break;
>                 case IIO_EV_DIR_FALLING:
>                         addr = AD7280A_AUX_ADC_UNDERVOLTAGE_REG;
>                         ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
>                                            1, val);
>                         if (ret)
>                                 break;
>                         st->aux_threshlow = val;
>                                             ^^
> and val us being used here rather than value too
> 
> 
> So for the IIO_TEMP case either the assignment to value is redundant or the
> setting of st->aux_threshhigh or st->auxthreashlow is incorrect.

Good catch. This has really slipped through my review.
It's impressive how we can discuss changes at one specific part of the
code and miss inconsistencies a few lines below it.

Thanks,
Marcelo

> 
> Colin