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
