* [PATCH][next] iio: addac: ad74115: remove redundant if statement @ 2024-03-28 11:22 Colin Ian King 2024-03-28 13:52 ` Nuno Sá 0 siblings, 1 reply; 4+ messages in thread From: Colin Ian King @ 2024-03-28 11:22 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Cosmin Tanislav, Jonathan Cameron, linux-iio Cc: kernel-janitors, linux-kernel 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; - return regmap_update_bits(st->regmap, AD74115_DIN_CONFIG1_REG, AD74115_DIN_DEBOUNCE_MASK, FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, val)); -- 2.39.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement 2024-03-28 11:22 [PATCH][next] iio: addac: ad74115: remove redundant if statement Colin Ian King @ 2024-03-28 13:52 ` Nuno Sá 2024-03-28 14:14 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Nuno Sá @ 2024-03-28 13:52 UTC (permalink / raw) To: Colin Ian King, Lars-Peter Clausen, Michael Hennerich, Cosmin Tanislav, Jonathan Cameron, linux-iio Cc: kernel-janitors, linux-kernel 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á ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement 2024-03-28 13:52 ` Nuno Sá @ 2024-03-28 14:14 ` Dan Carpenter 2024-03-28 15:56 ` Nuno Sá 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2024-03-28 14:14 UTC (permalink / raw) To: Nuno Sá Cc: Colin Ian King, Lars-Peter Clausen, Michael Hennerich, Cosmin Tanislav, Jonathan Cameron, linux-iio, kernel-janitors, linux-kernel 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, ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement 2024-03-28 14:14 ` Dan Carpenter @ 2024-03-28 15:56 ` Nuno Sá 0 siblings, 0 replies; 4+ messages in thread From: Nuno Sá @ 2024-03-28 15:56 UTC (permalink / raw) To: Dan Carpenter Cc: Colin Ian King, Lars-Peter Clausen, Michael Hennerich, Cosmin Tanislav, Jonathan Cameron, linux-iio, kernel-janitors, linux-kernel 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á ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-03-28 15:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-28 11:22 [PATCH][next] iio: addac: ad74115: remove redundant if statement Colin Ian King 2024-03-28 13:52 ` Nuno Sá 2024-03-28 14:14 ` Dan Carpenter 2024-03-28 15:56 ` Nuno Sá
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox