Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Mar 2022 23:32:17 -0300
From:   Marcelo Schmitt <marcelo.schmitt1@gmail.com>
To:     "Colin King (gmail)" <colin.i.king@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>, linux-iio@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: iio:adc:ad7280a: Move out of staging
Message-ID: <YiLLsQmnK+f6hjvi@marsc.168.1.7>
References: <69aeef03-69ba-07f4-2506-31e481f86c76@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <69aeef03-69ba-07f4-2506-31e481f86c76@gmail.com>
Precedence: bulk

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