iio: mlx90614: Implement filter configuration

DISCUSSION Dan Carpenter 2 participants 8 messages Oct 1, 2015 – Oct 2, 2015 IIO

Share & tools

Dan Carpenter

author

2015-10-01 22:16 UTC #0 lore raw

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

Crt Mori

2015-10-01 23:09 UTC #1 lore raw

Hello Dan Carpenter,
Thanks for checking. You found an error although not where checker pointed. See explanation inline

Quoted 6 lines · Dan Carpenter

Hello Crt Mori,
The patch 764589b688a1: "iio: mlx90614: Implement filter configuration" from Aug 17, 2015, leads to the following static checker warning:

Quoted 6 lines

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?

Quoted 9 lines

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

Quoted 4 lines

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)

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

regards,
dan carpenter

Dan Carpenter

author

2015-10-02 07:00 UTC #2 lore raw

Quoted 10 lines · Crt Mori

Hello Dan Carpenter,
Thanks for checking. You found an error although not where checker pointed. See explanation inline
Quoted 6 lines · Dan Carpenter
Hello Crt Mori,
The patch 764589b688a1: "iio: mlx90614: Implement filter configuration" from Aug 17, 2015, leads to the following static checker warning:

Quoted 8 lines

Quoted 6 lines
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.

Quoted 9 lines

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

Quoted 6 lines

Quoted 4 lines
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.

Quoted 9 lines

Quoted 6 lines
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

Crt Mori

2015-10-02 08:12 UTC #3 lore raw

Quoted 14 lines · Dan Carpenter

Quoted 13 lines · Crt Mori
Hello Dan Carpenter,
Thanks for checking. You found an error although not where checker pointed. See explanation inline
Quoted 9 lines · 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

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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.

Quoted 25 lines

Quoted 9 lines
Quoted 9 lines
   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)));
Quoted 6 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 3 lines

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

Quoted 8 lines

        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.

Quoted 40 lines

        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

Dan Carpenter

author

2015-10-02 08:32 UTC #4 lore raw

Quoted 5 lines · Crt Mori

Quoted 3 lines
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.

Quoted 1 line

I rather have it defined than let compiler decide

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

Quoted 7 lines

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

Quoted 6 lines

Quoted 2 lines
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

Crt Mori

2015-10-02 08:49 UTC #5 lore raw

Quoted 24 lines · Dan Carpenter

Quoted 5 lines · Crt Mori
Quoted 3 lines
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.
Quoted 1 line
I rather have it defined than let compiler decide
Focus on writing simple code and not making it portable for time travellers to 1980.
Quoted 10 lines
Quoted 9 lines
        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.

Quoted 9 lines

Quoted 3 lines
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.

Quoted 3 lines

regards,
dan carpenter

Dan Carpenter

author

2015-10-02 08:56 UTC #6 lore raw

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

	ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG)

regards,
dan carpenter

Crt Mori

2015-10-02 09:05 UTC #7 lore raw

Quoted 4 lines · Dan Carpenter

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

Quoted 3 lines

regards,
dan carpenter