Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Colin Ian King <colin.i.king@gmail.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>,
	Cosmin Tanislav <cosmin.tanislav@analog.com>,
	Jonathan Cameron <jic23@kernel.org>,
	linux-iio@vger.kernel.org, kernel-janitors@vger.kernel.org,
	 linux-kernel@vger.kernel.org
Subject: Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement
Date: Thu, 28 Mar 2024 16:56:11 +0100	[thread overview]
Message-ID: <4ef9f4d81f126de73e88cb221d6cc51f8bab6baf.camel@gmail.com> (raw)
In-Reply-To: <2b3d418d-55ee-4ca4-b6ae-bab441a76adf@moroto.mountain>

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á