[PATCH 1/2] drivers/hwmon/ad7314.c: Do proper sign extension

[PATCH 1/2] drivers/hwmon/ad7314.c: Do proper sign extension

[Rasmus Villemoes]

The comment above (data << 2) >> 2 explains what the intention is: To
use bit 13 of the 14-bit value data as the sign bit. However, this
doesn't work due to C's promotion rules. data has type s16, but data
<< 2 has type int. To get sign extension, that expression would have
to be cast back to an s16 before being shifted (at which point C's
promotion rules would then kick in again and promote the left operand
to int). As it stands, both expressions are no-ops for any value of
data.

Avoid these subtleties by using the existing API for
this. sign_extend32 works equally well for 8 and 16 bit types.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/hwmon/ad7314.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
index f4f9b219bf16..11955467fc0f 100644
--- a/drivers/hwmon/ad7314.c
+++ b/drivers/hwmon/ad7314.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/bitops.h>
 
 /*
  * AD7314 temperature masks
@@ -67,7 +68,7 @@ static ssize_t ad7314_show_temperature(struct device *dev,
 	switch (spi_get_device_id(chip->spi_dev)->driver_data) {
 	case ad7314:
 		data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
-		data = (data << 6) >> 6;
+		data = sign_extend32(data, 9);
 
 		return sprintf(buf, "%d\n", 250 * data);
 	case adt7301:
@@ -78,7 +79,7 @@ static ssize_t ad7314_show_temperature(struct device *dev,
 		 * register.  1lsb - 31.25 milli degrees centigrade
 		 */
 		data = ret & ADT7301_TEMP_MASK;
-		data = (data << 2) >> 2;
+		data = sign_extend32(data, 13);
 
 		return sprintf(buf, "%d\n",
 			       DIV_ROUND_CLOSEST(data * 3125, 100));
-- 
2.1.3

[PATCH 2/2] drivers/hwmon/adc128d818.c: Do proper sign extension

[Rasmus Villemoes]

data->temp[index] has type s16. Because of C's promotion rules,
(data->temp[index] << 7) >> 7 is exactly the same as
data->temp[index]. The intention was to use bit 8 as a sign bit, so do
that using the existing API.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/hwmon/adc128d818.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 0625e50d7a6e..ad2b47e40345 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -27,6 +27,7 @@
 #include <linux/err.h>
 #include <linux/regulator/consumer.h>
 #include <linux/mutex.h>
+#include <linux/bitops.h>
 
 /* Addresses to scan
  * The chip also supports addresses 0x35..0x37. Don't scan those addresses
@@ -189,7 +190,7 @@ static ssize_t adc128_show_temp(struct device *dev,
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
-	temp = (data->temp[index] << 7) >> 7;	/* sign extend */
+	temp = sign_extend32(data->temp[index], 8);
 	return sprintf(buf, "%d\n", temp * 500);/* 0.5 degrees C resolution */
 }
 
-- 
2.1.3

Re: [PATCH 1/2] drivers/hwmon/ad7314.c: Do proper sign extension

[Guenter Roeck]

On 01/22/2015 02:44 PM, Rasmus Villemoes wrote:
> The comment above (data << 2) >> 2 explains what the intention is: To
> use bit 13 of the 14-bit value data as the sign bit. However, this
> doesn't work due to C's promotion rules. data has type s16, but data
> << 2 has type int. To get sign extension, that expression would have
> to be cast back to an s16 before being shifted (at which point C's
> promotion rules would then kick in again and promote the left operand
> to int). As it stands, both expressions are no-ops for any value of
> data.
>
> Avoid these subtleties by using the existing API for
> this. sign_extend32 works equally well for 8 and 16 bit types.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---

Good catch. Applied.

Thanks,
Guenter

Re: [PATCH 2/2] drivers/hwmon/adc128d818.c: Do proper sign extension

[Guenter Roeck]

On 01/22/2015 02:44 PM, Rasmus Villemoes wrote:
> data->temp[index] has type s16. Because of C's promotion rules,
> (data->temp[index] << 7) >> 7 is exactly the same as
> data->temp[index]. The intention was to use bit 8 as a sign bit, so do
> that using the existing API.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---

Applied.

Thanks,
Guenter