Received-SPF: pass (google.com: domain of linux-kernel+bounces-123190-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <4ef9f4d81f126de73e88cb221d6cc51f8bab6baf.camel@gmail.com>
Subject: Re: [PATCH][next] iio: addac: ad74115: remove redundant if statement
From: Nuno =?ISO-8859-1?Q?S=E1?= <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
Date: Thu, 28 Mar 2024 16:56:11 +0100
In-Reply-To: <2b3d418d-55ee-4ca4-b6ae-bab441a76adf@moroto.mountain>
References: <20240328112211.761618-1-colin.i.king@gmail.com>
	 <3db9a68c6f71a67d95d25886fdc708de6269adc2.camel@gmail.com>
	 <2b3d418d-55ee-4ca4-b6ae-bab441a76adf@moroto.mountain>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-03-28 at 17:14 +0300, Dan Carpenter wrote:
> On Thu, Mar 28, 2024 at 02:52:43PM +0100, Nuno S=C3=A1 wrote:
> > Hi Colin,
> >=20
> > Thanks for your patch...
> >=20
> > 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.
> > >=20
> > > Cleans up clang scan build warning:
> > > drivers/iio/addac/ad74115.c:570:3: warning: Value stored to 'i'
> > > is never read [deadcode.DeadStores]
> > >=20
> > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> > > ---
> > > =C2=A0drivers/iio/addac/ad74115.c | 3 ---
> > > =C2=A01 file changed, 3 deletions(-)
> > >=20
> > > 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 ad741=
15_state
> > > *st,
> > > unsigned int val)
> > > =C2=A0		if (val <=3D ad74115_debounce_tbl[i])
> > > =C2=A0			break;
> > > =C2=A0
> > > -	if (i =3D=3D len)
> > > -		i =3D len - 1;
> > > -
> >=20
> > 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 furt=
her
> > 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_DEBOUNC=
E_MASK and
> > the
> > possible values of val...
>=20
> Good eye.=C2=A0 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.=C2=A0 In other words the <=3D should be < =
and
> the i =3D len - 1 should just be i--.

I'm not sure about the i--. Apparently the intent was to keep the first ind=
ex
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 belo=
w.
>=20
> (The first element in the ad74115_debounce_tbl[] array is zero btw, so
> it's never going to break with i =3D=3D 0).
>=20
> regards,
> dan carpenter
>=20
> 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
> =C2=A0
> =C2=A0static int ad74115_set_comp_debounce(struct ad74115_state *st, unsi=
gned int val)
> =C2=A0{
> -	unsigned int len =3D ARRAY_SIZE(ad74115_debounce_tbl);
> =C2=A0	unsigned int i;
> =C2=A0
> -	for (i =3D 0; i < len; i++)
> -		if (val <=3D ad74115_debounce_tbl[i])
> +	for (i =3D 0; i < ARRAY_SIZE(ad74115_debounce_tbl); i++)
> +		if (ad74115_debounce_tbl[i] > val)
> =C2=A0			break;
> -
> -	if (i =3D=3D len)
> -		i =3D len - 1;
> +	i--;
> =C2=A0
> =C2=A0	return regmap_update_bits(st->regmap, AD74115_DIN_CONFIG1_REG,
> =C2=A0				=C2=A0 AD74115_DIN_DEBOUNCE_MASK,
> -				=C2=A0 FIELD_PREP(AD74115_DIN_DEBOUNCE_MASK, val));
> +				=C2=A0 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 l=
ook at the
mask and the possible values in the array, you see we can easily go off the=
 mask.

- Nuno S=C3=A1