| Message ID | 20240328112211.761618-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [next] iio: addac: ad74115: remove redundant if statement | expand |
Hi Colin, Thanks for your patch... On Thu, 2024-03-28 at 11:22 +0000, Colin Ian King wrote: > The if statement is redundant because the variable i being > assigned in the statement is never read afterwards. Remove it. > > Cleans up clang scan build warning: > drivers/iio/addac/ad74115.c:570:3: warning: Value stored to 'i' > is never read [deadcode.DeadStores] > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > drivers/iio/addac/ad74115.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c > index e6bc5eb3788d..d31d4adb017e 100644 > --- a/drivers/iio/addac/ad74115.c > +++ b/drivers/iio/addac/ad74115.c > @@ -566,9 +566,6 @@ static int ad74115_set_comp_debounce(struct ad74115_state *st, > unsigned int val) > if (val <= ad74115_debounce_tbl[i]) > break; > > - if (i == len) > - i = len - 1; > - Hmm, this change is clearly good but I think we're actually missing the proper fix in here. I'm being lazy and not checking the datasheet and Cosmin can further comment. But I'm fairly sure that the intent of the code is actually to use i in the call to regmap_update_bits(). I mean if we look at the mask AD74115_DIN_DEBOUNCE_MASK and the possible values of val... - Nuno Sá
On Thu, Mar 28, 2024 at 02:52:43PM +0100, Nuno Sá wrote: > Hi Colin, > > Thanks for your patch... > > On Thu, 2024-03-28 at 11:22 +0000, Colin Ian King wrote: > > The if statement is redundant because the variable i being > > assigned in the statement is never read afterwards. Remove it. > > > > Cleans up clang scan build warning: > > drivers/iio/addac/ad74115.c:570:3: warning: Value stored to 'i' > > is never read [deadcode.DeadStores] > > > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > > --- > > drivers/iio/addac/ad74115.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c > > index e6bc5eb3788d..d31d4adb017e 100644 > > --- a/drivers/iio/addac/ad74115.c > > +++ b/drivers/iio/addac/ad74115.c > > @@ -566,9 +566,6 @@ static int ad74115_set_comp_debounce(struct ad74115_state *st, > > unsigned int val) > > if (val <= ad74115_debounce_tbl[i]) > > break; > > > > - if (i == len) > > - i = len - 1; > > - > > Hmm, this change is clearly good but I think we're actually missing the proper fix in > here. I'm being lazy and not checking the datasheet and Cosmin can further comment. > But I'm fairly sure that the intent of the code is actually to use i in the call to > regmap_update_bits(). I mean if we look at the mask AD74115_DIN_DEBOUNCE_MASK and the > possible values of val... Good eye. I haven't looked at the datasheet either, but logically I would think you go until you hit something that is too high and then step back to the valid valies. In other words the <= should be < and the i = len - 1 should just be i--. (The first element in the ad74115_debounce_tbl[] array is zero btw, so it's never going to break with i == 0). regards, dan carpenter diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c index e6bc5eb3788d..8d484cefe5ff 100644 --- a/drivers/iio/addac/ad74115.c +++ b/drivers/iio/addac/ad74115.c @@ -559,19 +559,16 @@ static void ad74115_gpio_set(struct gpio_chip *gc, unsigned int offset, int valu static int ad74115_set_comp_debounce(struct ad74115_state *st, unsigned int val) { - unsigned int len = ARRAY_SIZE(ad74115_debounce_tbl); unsigned int i; - for (i = 0; i < len; i++) - if (val <= ad74115_debounce_tbl[i]) + for (i = 0; i < ARRAY_SIZE(ad74115_debounce_tbl); i++) + if (ad74115_debounce_tbl[i] > val) break; - - if (i == len) - i = len - 1; + i--; return regmap_update_bits(st->regmap, AD74115_DIN_CONFIG1_REG, AD74115_DIN_DEBOUNCE_MASK, - FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, val)); + FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, ad74115_debounce_tbl[i])); } static int ad74115_comp_gpio_get_direction(struct gpio_chip *chip,
On Thu, 2024-03-28 at 17:14 +0300, Dan Carpenter wrote: > On Thu, Mar 28, 2024 at 02:52:43PM +0100, Nuno Sá wrote: > > Hi Colin, > > > > Thanks for your patch... > > > > On Thu, 2024-03-28 at 11:22 +0000, Colin Ian King wrote: > > > The if statement is redundant because the variable i being > > > assigned in the statement is never read afterwards. Remove it. > > > > > > Cleans up clang scan build warning: > > > drivers/iio/addac/ad74115.c:570:3: warning: Value stored to 'i' > > > is never read [deadcode.DeadStores] > > > > > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > > > --- > > > drivers/iio/addac/ad74115.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c > > > index e6bc5eb3788d..d31d4adb017e 100644 > > > --- a/drivers/iio/addac/ad74115.c > > > +++ b/drivers/iio/addac/ad74115.c > > > @@ -566,9 +566,6 @@ static int ad74115_set_comp_debounce(struct ad74115_state > > > *st, > > > unsigned int val) > > > if (val <= ad74115_debounce_tbl[i]) > > > break; > > > > > > - if (i == len) > > > - i = len - 1; > > > - > > > > Hmm, this change is clearly good but I think we're actually missing the proper > > fix in > > here. I'm being lazy and not checking the datasheet and Cosmin can further > > comment. > > But I'm fairly sure that the intent of the code is actually to use i in the call > > to > > regmap_update_bits(). I mean if we look at the mask AD74115_DIN_DEBOUNCE_MASK and > > the > > possible values of val... > > Good eye. I haven't looked at the datasheet either, but logically I > would think you go until you hit something that is too high and then > step back to the valid valies. In other words the <= should be < and > the i = len - 1 should just be i--. I'm not sure about the i--. Apparently the intent was to keep the first index with bigger debounce than val and only step back if we go beyond the array but I'll leave that for Cosmin. The most important to me is something else. See below. > > (The first element in the ad74115_debounce_tbl[] array is zero btw, so > it's never going to break with i == 0). > > regards, > dan carpenter > > diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c > index e6bc5eb3788d..8d484cefe5ff 100644 > --- a/drivers/iio/addac/ad74115.c > +++ b/drivers/iio/addac/ad74115.c > @@ -559,19 +559,16 @@ static void ad74115_gpio_set(struct gpio_chip *gc, unsigned > int offset, int valu > > static int ad74115_set_comp_debounce(struct ad74115_state *st, unsigned int val) > { > - unsigned int len = ARRAY_SIZE(ad74115_debounce_tbl); > unsigned int i; > > - for (i = 0; i < len; i++) > - if (val <= ad74115_debounce_tbl[i]) > + for (i = 0; i < ARRAY_SIZE(ad74115_debounce_tbl); i++) > + if (ad74115_debounce_tbl[i] > val) > break; > - > - if (i == len) > - i = len - 1; > + i--; > > return regmap_update_bits(st->regmap, AD74115_DIN_CONFIG1_REG, > AD74115_DIN_DEBOUNCE_MASK, > - FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, val)); > + FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, > ad74115_debounce_tbl[i])); In here, I think we want FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, i). If you look at the mask and the possible values in the array, you see we can easily go off the mask. - Nuno Sá
diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c index e6bc5eb3788d..d31d4adb017e 100644 --- a/drivers/iio/addac/ad74115.c +++ b/drivers/iio/addac/ad74115.c @@ -566,9 +566,6 @@ static int ad74115_set_comp_debounce(struct ad74115_state *st, unsigned int val) if (val <= ad74115_debounce_tbl[i]) break; - if (i == len) - i = len - 1; - return regmap_update_bits(st->regmap, AD74115_DIN_CONFIG1_REG, AD74115_DIN_DEBOUNCE_MASK, FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, val));
The if statement is redundant because the variable i being assigned in the statement is never read afterwards. Remove it. Cleans up clang scan build warning: drivers/iio/addac/ad74115.c:570:3: warning: Value stored to 'i' is never read [deadcode.DeadStores] Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/iio/addac/ad74115.c | 3 --- 1 file changed, 3 deletions(-)