[PATCH] [BUGFIX] iio: mlx96014: Error checking from positive to negative

[PATCH] [BUGFIX] iio: mlx96014: Error checking from positive to negative

[Crt Mori]

Dan Carpenter reported a static checker report and after his mail I
noticed that we actually return from function if positive value is
obtained from i2c read. This was remainder from when code was not in
separate function (which I changed during the review process).

Static checker reported
  drivers/iio/temperature/mlx90614.c:167
  mlx90614_iir_search()
    warn: this cast is a no-op
which meant that cast before negating is useless.

Also changed magic number to macro in process as that was confusing.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90614.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 3fd3ba4..78d4a15 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -72,6 +72,7 @@
 #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */
 #define MLX90614_CONST_RAW_EMISSIVITY_MAX 65535 /* max value for emissivity */
 #define MLX90614_CONST_EMISSIVITY_RESOLUTION 15259 /* 1/65535 ~ 0.000015259 */
+#define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */
 
 struct mlx90614_data {
 	struct i2c_client *client;
@@ -156,15 +157,15 @@ static inline s32 mlx90614_iir_search(const struct i2c_client *client,
 	 * changes
 	 */
 	ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG);
-	if (ret > 0)
+	if (ret < 0)
 		return ret;
 
 	/* Write changed values */
 	ret = mlx90614_write_word(client, MLX90614_CONFIG,
 			(i << MLX90614_CONFIG_IIR_SHIFT) |
-			(((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) |
-			((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) &
-			(~(u16) MLX90614_CONFIG_IIR_MASK)));
+			(((u16) ((MLX90614_CONST_FIR << MLX90614_CONFIG_FIR_SHIFT) |
+			((u16) ret & ((u16) ~MLX90614_CONFIG_FIR_MASK)))) &
+			((u16) ~MLX90614_CONFIG_IIR_MASK)));
 	return ret;
 }
 
-- 
2.1.4

Re: [PATCH] [BUGFIX] iio: mlx96014: Error checking from positive to negative

[Dan Carpenter]

On Fri, Oct 02, 2015 at 02:14:03AM -0700, Crt Mori wrote:
>  	/* Write changed values */
>  	ret = mlx90614_write_word(client, MLX90614_CONFIG,
>  			(i << MLX90614_CONFIG_IIR_SHIFT) |
> -			(((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) |
> -			((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) &
> -			(~(u16) MLX90614_CONFIG_IIR_MASK)));
> +			(((u16) ((MLX90614_CONST_FIR << MLX90614_CONFIG_FIR_SHIFT) |
> +			((u16) ret & ((u16) ~MLX90614_CONFIG_FIR_MASK)))) &
> +			((u16) ~MLX90614_CONFIG_IIR_MASK)));

Yeah.  Ok.  This works.  When I was looking at it, I got some of the
parenthesis mixed up so I simplified it in the wrong way.  It really is
an unparsable wall of gobbledigook.  Also the casts are still not needed
even for time travellers I mentioned earlier.

This could be written more simply as:

	ret &= ~MLX90614_CONFIG_FIR_MASK;
	ret &= ~MLX90614_CONFIG_IIR_MASK;
	ret |= MLX90614_CONST_FIR << MLX90614_CONFIG_FIR_SHIFT;

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

regards,
dan carpenter