re: iio: mlx90614: Implement filter configuration

re: iio: mlx90614: Implement filter configuration

[Dan Carpenter]

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)
   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)

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.

But why are we ANDing it with "ret" which is a negative error code???
I think there is some other typo here beyond the extra casts.

   168          return ret;

regards,
dan carpenter

Re: iio: mlx90614: Implement filter configuration

[Crt Mori]

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?
>    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)
>
> 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. Also macros in theory are not
u16 and I like doing
bitwise operations on known (equal) datatypes.
>
> But why are we ANDing it with "ret" which is a negative error code???
This is a bug which you found.
There should not be negative ret in this case because of above if
statement (line 160). There
should be a result of currently read CONFIG register in there and
because we need to set
two bits with two masks as well as maintain all other values unchanged
there is that bitwise operation.

In first part it shifts IIR value in position, then it shifts fixed
0x7 value in FIR position (since we
have put a fixed FIR value - see table in commit message), then it
applies negative FIR and IIR mask
to current register values so that other bits remain unchanged.
Because sensor offers also changes to
FIR, I would like to keep it this way in case someone decides he/she
needs to change FIR as well.

> I think there is some other typo here beyond the extra casts.
>
>    168          return ret;
Why is this a typo? Function does return negative value if write fails
so we should pass it back up.

>
> regards,
> dan carpenter

Re: iio: mlx90614: Implement filter configuration

[Dan Carpenter]

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

Re: iio: mlx90614: Implement filter configuration

[Crt Mori]

On 2 October 2015 at 09:00, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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.
>

OK, will prepare bugfix in few hours.

>> >    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.
>

OK, seems like I am one of the people thinking that. Will follow your
advice and cast afterwards.

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

Like I said they are on most processors, but might not be on others. I
rather have it
defined than let compiler decide

>         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.

That macro is for mask and it is just coincidence that they are the
same. I would like to keep the
way it is in case someone wants to change FIR values as well.

>
>         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
>

Re: iio: mlx90614: Implement filter configuration

[Dan Carpenter]

On Fri, Oct 02, 2015 at 10:12:01AM +0200, Crt Mori wrote:
> > It looks a little more readable without any casts and it works exactly
> > the same since the casts were just for decoration.
> >
> 
> Like I said they are on most processors, but might not be on others.

And monkeys *might* fly out of my butt.

> I rather have it defined than let compiler decide

Focus on writing simple code and not making it portable for time
travellers to 1980.

> 
> >         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.
> 
> That macro is for mask and it is just coincidence that they are the
> same. I would like to keep the
> way it is in case someone wants to change FIR values as well.

Did you read the rest of the email?  This code make no sense.  We know
that it has never been tested since the earlier ret test was inverted so
it was unreachable.  Why are you defending something that is so clearly
wrong?  Anyway get rid of the magic number at least.

regards,
dan carpenter

Re: iio: mlx90614: Implement filter configuration

[Crt Mori]

On 2 October 2015 at 10:32, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Oct 02, 2015 at 10:12:01AM +0200, Crt Mori wrote:
>> > It looks a little more readable without any casts and it works exactly
>> > the same since the casts were just for decoration.
>> >
>>
>> Like I said they are on most processors, but might not be on others.
>
> And monkeys *might* fly out of my butt.
>
>> I rather have it defined than let compiler decide
>
> Focus on writing simple code and not making it portable for time
> travellers to 1980.
>
>>
>> >         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.
>>
>> That macro is for mask and it is just coincidence that they are the
>> same. I would like to keep the
>> way it is in case someone wants to change FIR values as well.
>
> Did you read the rest of the email?  This code make no sense.  We know
> that it has never been tested since the earlier ret test was inverted so
> it was unreachable.  Why are you defending something that is so clearly
> wrong?  Anyway get rid of the magic number at least.
>

Actually that part of code was extensively tested on BeagleBone Black
with sensor
attached - the ret value was artifact of reviewing process. I also
wrote unit tests for
that function to be certain it works as intended. Before this I had:
if( ret >=0)
{
        /* Write changed values */
}
but it was decided that it goes too deep so I changed it to
if (ret > 0)
        return;
/* Write changed values */
which was wrong.

I actually still have the first version in my local branch, so all
other tests in last month
passed on that old if statement.


> regards,
> dan carpenter
>

Re: iio: mlx90614: Implement filter configuration

[Dan Carpenter]

Ok.  Hm...  Then what are the possible positive values of:

	ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG)

regards,
dan carpenter

Re: iio: mlx90614: Implement filter configuration

[Crt Mori]

On 2 October 2015 at 10:56, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Ok.  Hm...  Then what are the possible positive values of:
>
>         ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG)
>

16 bit Config register value, which has some bits fixed to maintain
calibration (14...11;7;3).

> regards,
> dan carpenter
>