Re: iio: mlx90614: Implement filter configuration

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Oct 01, 2015 at 04:09:02PM -0700, Crt Mori wrote:
> Hello Dan Carpenter,
> Thanks for checking. You found an error although not where checker pointed.
> See explanation inline
> On 2 October 2015 at 00:16, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Hello Crt Mori,
> >
> > The patch 764589b688a1: "iio: mlx90614: Implement filter
> > configuration" from Aug 17, 2015, leads to the following static
> > checker warning:
> >
> >         drivers/iio/temperature/mlx90614.c:167 mlx90614_iir_search()
> >         warn: this cast is a no-op
> >
> > drivers/iio/temperature/mlx90614.c
> >    158          ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG);
> >    159          if (ret > 0)
> Here is the error: it should be if (ret < 0) . Do you want to send a
> bugfix patch?

Nah.  Just give me a reported-by tag, please.

> >    160                  return ret;
> >    161
> >    162          /* Write changed values */
> >    163          ret = mlx90614_write_word(client, MLX90614_CONFIG,
> >    164                          (i << MLX90614_CONFIG_IIR_SHIFT) |
> >    165                          (((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) |
> >    166                          ((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) &
> >    167                          (~(u16) MLX90614_CONFIG_IIR_MASK)));
> >
> > Quite a few of these casts make no sense.  It's not clear what was
> > intended.
> >
> >         (~(u16) MLX90614_CONFIG_IIR_MASK)
>  (~((u16) MLX90614_CONFIG_IIR_MASK)) does not seem better, or it does? This was
> intended (so to cast value to 16 bit, then negate it)

Because of type promotion what actually happens is we cast it to u16
then we cast it to int then we negate it.

> >
> > So we take int 0x7 and cast it to u16, then because of type promotion
> > we convert it to int and do a bitwise negate.  The static checker
> > warning is because often that means (u16)~MLX90614_CONFIG_IIR_MASK is
> > intended instead.  In this case it looks like we could just remove the
> > cast with no harm done.
> True, we could remove the cast, but I wanted to make sure that u16
> data type is used
> since ret and i are not that datatypes.

Because of type promotion int type is used not u16.  The u16 casts are
misleading no-ops because people think that u16 data type is used.
Hence the static checker warning.

It looks a little more readable without any casts and it works exactly
the same since the casts were just for decoration.

	ret = mlx90614_write_word(client, MLX90614_CONFIG,
				  (i << MLX90614_CONFIG_IIR_SHIFT) |
				  ((0x7 << MLX90614_CONFIG_FIR_SHIFT) |
				   (ret & ~MLX90614_CONFIG_FIR_MASK) &
				   (~MLX90614_CONFIG_IIR_MASK)));

The 0x7 is a magic number.  I would think it would be a named _MASK
macro.  Ah, we do have a macro for that.

	ret = mlx90614_write_word(client, MLX90614_CONFIG,
				  (i << MLX90614_CONFIG_IIR_SHIFT) |
				  (MLX90614_CONFIG_FIR_MASK |
				   (ret & ~MLX90614_CONFIG_FIR_MASK) &
				   (~MLX90614_CONFIG_IIR_MASK)));

The MLX90614_CONFIG_FIR_MASK and ~MLX90614_CONFIG_FIR_MASK can be
simplified.

	ret = mlx90614_write_word(client, MLX90614_CONFIG,
				  (i << MLX90614_CONFIG_IIR_SHIFT) |
				  (ret | MLX90614_CONFIG_FIR_MASK &
				   ~MLX90614_CONFIG_IIR_MASK));

Once we have simplified the code, then it looks buggy to me.  I suspect
that it is supposed to look like:

	ret = mlx90614_write_word(client, MLX90614_CONFIG,
				  (i << MLX90614_CONFIG_IIR_SHIFT) |
				  (ret << MLX90614_CONFIG_FIR_SHIFT);

Or possibly like:

	ret = mlx90614_write_word(client, MLX90614_CONFIG,
				  (i << MLX90614_CONFIG_IIR_SHIFT) |
				  (ret & MLX90614_CONFIG_FIR_MASK);

Keep in mind that:

	ret & MLX90614_CONFIG_FIR_MASK

And
	ret & MLX90614_CONFIG_FIR_MASK & ~MLX90614_CONFIG_IIR_MASK

are equivalent.  But the first one is simpler.

regards,
dan carpenter