diff mbox

[v3,1/3] hwmon: ltc2990: refactor value conversion

Message ID 1499056140-6064-1-git-send-email-tom.levens@cern.ch (mailing list archive)
State Accepted
Headers show

Commit Message

Tom Levens July 3, 2017, 4:28 a.m. UTC
Conversion from raw values to signed integers has been refactored using
bitops.h. This also fixes a bug where negative temperatures were
converted incorrectly.

Signed-off-by: Tom Levens <tom.levens@cern.ch>
---
 drivers/hwmon/ltc2990.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

Comments

Guenter Roeck July 8, 2017, 3:45 p.m. UTC | #1
On Mon, Jul 03, 2017 at 06:28:58AM +0200, Tom Levens wrote:
> Conversion from raw values to signed integers has been refactored using
> bitops.h. This also fixes a bug where negative temperatures were
> converted incorrectly.
> 

Documentation/process/submitting-patches.rst, chapter 2: "Solve only one
problem per patch.".

I can understand the urge to merge two sets of changes here. However,
that creates a problem for me: The fix should be applied to stable kernels,
but not the refactoring.

Please split the patch in two, one for the bug fix and one for the
refactoring.

Thanks,
Guenter

> Signed-off-by: Tom Levens <tom.levens@cern.ch>
> ---
>  drivers/hwmon/ltc2990.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> index 8f8fe05..e320d21 100644
> --- a/drivers/hwmon/ltc2990.c
> +++ b/drivers/hwmon/ltc2990.c
> @@ -11,6 +11,7 @@
>   * the chip's internal temperature and Vcc power supply voltage.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/err.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -34,15 +35,6 @@
>  #define LTC2990_CONTROL_MODE_CURRENT	0x06
>  #define LTC2990_CONTROL_MODE_VOLTAGE	0x07
>  
> -/* convert raw register value to sign-extended integer in 16-bit range */
> -static int ltc2990_voltage_to_int(int raw)
> -{
> -	if (raw & BIT(14))
> -		return -(0x4000 - (raw & 0x3FFF)) << 2;
> -	else
> -		return (raw & 0x3FFF) << 2;
> -}
> -
>  /* Return the converted value from the given register in uV or mC */
>  static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>  {
> @@ -55,18 +47,16 @@ static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>  	switch (reg) {
>  	case LTC2990_TINT_MSB:
>  		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
> -		val = (val & 0x1FFF) << 3;
> -		*result = (val * 1000) >> 7;
> +		*result = sign_extend32(val, 12) * 1000 / 16;
>  		break;
>  	case LTC2990_V1_MSB:
>  	case LTC2990_V3_MSB:
>  		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
> -		*result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
> +		*result = sign_extend32(val, 14) * 1942 / 100;
>  		break;
>  	case LTC2990_VCC_MSB:
>  		/* Vcc, 305.18μV/LSB, 2.5V offset */
> -		*result = (ltc2990_voltage_to_int(val) * 30518 /
> -			   (4 * 100 * 1000)) + 2500;
> +		*result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
>  		break;
>  	default:
>  		return -EINVAL; /* won't happen, keep compiler happy */
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tom Levens July 11, 2017, 10:03 p.m. UTC | #2
On Sat, 8 Jul 2017, Guenter Roeck wrote:

> On Mon, Jul 03, 2017 at 06:28:58AM +0200, Tom Levens wrote:
>> Conversion from raw values to signed integers has been refactored using
>> bitops.h. This also fixes a bug where negative temperatures were
>> converted incorrectly.
>>
>
> Documentation/process/submitting-patches.rst, chapter 2: "Solve only one
> problem per patch.".
>
> I can understand the urge to merge two sets of changes here. However,
> that creates a problem for me: The fix should be applied to stable kernels,
> but not the refactoring.
>
> Please split the patch in two, one for the bug fix and one for the
> refactoring.

In reality the refactoring *is* the fix for this bug. I am not sure how to 
to fix it simply without using the bitops macro. Of course, I could fix 
only the temperature conversion in one patch and refactor the other two in 
a separate one if you prefer?

Cheers,

> Thanks,
> Guenter
>
>> Signed-off-by: Tom Levens <tom.levens@cern.ch>
>> ---
>>  drivers/hwmon/ltc2990.c | 18 ++++--------------
>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>> index 8f8fe05..e320d21 100644
>> --- a/drivers/hwmon/ltc2990.c
>> +++ b/drivers/hwmon/ltc2990.c
>> @@ -11,6 +11,7 @@
>>   * the chip's internal temperature and Vcc power supply voltage.
>>   */
>>
>> +#include <linux/bitops.h>
>>  #include <linux/err.h>
>>  #include <linux/hwmon.h>
>>  #include <linux/hwmon-sysfs.h>
>> @@ -34,15 +35,6 @@
>>  #define LTC2990_CONTROL_MODE_CURRENT	0x06
>>  #define LTC2990_CONTROL_MODE_VOLTAGE	0x07
>>
>> -/* convert raw register value to sign-extended integer in 16-bit range */
>> -static int ltc2990_voltage_to_int(int raw)
>> -{
>> -	if (raw & BIT(14))
>> -		return -(0x4000 - (raw & 0x3FFF)) << 2;
>> -	else
>> -		return (raw & 0x3FFF) << 2;
>> -}
>> -
>>  /* Return the converted value from the given register in uV or mC */
>>  static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>  {
>> @@ -55,18 +47,16 @@ static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>  	switch (reg) {
>>  	case LTC2990_TINT_MSB:
>>  		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
>> -		val = (val & 0x1FFF) << 3;
>> -		*result = (val * 1000) >> 7;
>> +		*result = sign_extend32(val, 12) * 1000 / 16;
>>  		break;
>>  	case LTC2990_V1_MSB:
>>  	case LTC2990_V3_MSB:
>>  		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
>> -		*result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
>> +		*result = sign_extend32(val, 14) * 1942 / 100;
>>  		break;
>>  	case LTC2990_VCC_MSB:
>>  		/* Vcc, 305.18μV/LSB, 2.5V offset */
>> -		*result = (ltc2990_voltage_to_int(val) * 30518 /
>> -			   (4 * 100 * 1000)) + 2500;
>> +		*result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
>>  		break;
>>  	default:
>>  		return -EINVAL; /* won't happen, keep compiler happy */
>
>
Guenter Roeck July 12, 2017, 12:39 a.m. UTC | #3
On 07/11/2017 03:03 PM, Tom Levens wrote:
> 
> 
> On Sat, 8 Jul 2017, Guenter Roeck wrote:
> 
>> On Mon, Jul 03, 2017 at 06:28:58AM +0200, Tom Levens wrote:
>>> Conversion from raw values to signed integers has been refactored using
>>> bitops.h. This also fixes a bug where negative temperatures were
>>> converted incorrectly.
>>>
>>
>> Documentation/process/submitting-patches.rst, chapter 2: "Solve only one
>> problem per patch.".
>>
>> I can understand the urge to merge two sets of changes here. However,
>> that creates a problem for me: The fix should be applied to stable kernels,
>> but not the refactoring.
>>
>> Please split the patch in two, one for the bug fix and one for the
>> refactoring.
> 
> In reality the refactoring *is* the fix for this bug. I am not sure how to to fix it simply without using the bitops macro. Of course, I could fix only the temperature conversion in one patch and refactor the other two in a separate one if you prefer?
> 

If so, the subject and description are reversed. The subject should then be
something like "Fix incorrect conversion of negative temperatures",
and the description should explain that this is accomplished by using
existing API functions (then you can add "while at it, refactor voltage
conversions as well").

Thanks,
Guenter

> Cheers,
> 
>> Thanks,
>> Guenter
>>
>>> Signed-off-by: Tom Levens <tom.levens@cern.ch>
>>> ---
>>>  drivers/hwmon/ltc2990.c | 18 ++++--------------
>>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>>> index 8f8fe05..e320d21 100644
>>> --- a/drivers/hwmon/ltc2990.c
>>> +++ b/drivers/hwmon/ltc2990.c
>>> @@ -11,6 +11,7 @@
>>>   * the chip's internal temperature and Vcc power supply voltage.
>>>   */
>>>
>>> +#include <linux/bitops.h>
>>>  #include <linux/err.h>
>>>  #include <linux/hwmon.h>
>>>  #include <linux/hwmon-sysfs.h>
>>> @@ -34,15 +35,6 @@
>>>  #define LTC2990_CONTROL_MODE_CURRENT    0x06
>>>  #define LTC2990_CONTROL_MODE_VOLTAGE    0x07
>>>
>>> -/* convert raw register value to sign-extended integer in 16-bit range */
>>> -static int ltc2990_voltage_to_int(int raw)
>>> -{
>>> -    if (raw & BIT(14))
>>> -        return -(0x4000 - (raw & 0x3FFF)) << 2;
>>> -    else
>>> -        return (raw & 0x3FFF) << 2;
>>> -}
>>> -
>>>  /* Return the converted value from the given register in uV or mC */
>>>  static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>>  {
>>> @@ -55,18 +47,16 @@ static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
>>>      switch (reg) {
>>>      case LTC2990_TINT_MSB:
>>>          /* internal temp, 0.0625 degrees/LSB, 13-bit  */
>>> -        val = (val & 0x1FFF) << 3;
>>> -        *result = (val * 1000) >> 7;
>>> +        *result = sign_extend32(val, 12) * 1000 / 16;
>>>          break;
>>>      case LTC2990_V1_MSB:
>>>      case LTC2990_V3_MSB:
>>>           /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
>>> -        *result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
>>> +        *result = sign_extend32(val, 14) * 1942 / 100;
>>>          break;
>>>      case LTC2990_VCC_MSB:
>>>          /* Vcc, 305.18μV/LSB, 2.5V offset */
>>> -        *result = (ltc2990_voltage_to_int(val) * 30518 /
>>> -               (4 * 100 * 1000)) + 2500;
>>> +        *result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
>>>          break;
>>>      default:
>>>          return -EINVAL; /* won't happen, keep compiler happy */
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
index 8f8fe05..e320d21 100644
--- a/drivers/hwmon/ltc2990.c
+++ b/drivers/hwmon/ltc2990.c
@@ -11,6 +11,7 @@ 
  * the chip's internal temperature and Vcc power supply voltage.
  */
 
+#include <linux/bitops.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -34,15 +35,6 @@ 
 #define LTC2990_CONTROL_MODE_CURRENT	0x06
 #define LTC2990_CONTROL_MODE_VOLTAGE	0x07
 
-/* convert raw register value to sign-extended integer in 16-bit range */
-static int ltc2990_voltage_to_int(int raw)
-{
-	if (raw & BIT(14))
-		return -(0x4000 - (raw & 0x3FFF)) << 2;
-	else
-		return (raw & 0x3FFF) << 2;
-}
-
 /* Return the converted value from the given register in uV or mC */
 static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
 {
@@ -55,18 +47,16 @@  static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
 	switch (reg) {
 	case LTC2990_TINT_MSB:
 		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
-		val = (val & 0x1FFF) << 3;
-		*result = (val * 1000) >> 7;
+		*result = sign_extend32(val, 12) * 1000 / 16;
 		break;
 	case LTC2990_V1_MSB:
 	case LTC2990_V3_MSB:
 		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
-		*result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
+		*result = sign_extend32(val, 14) * 1942 / 100;
 		break;
 	case LTC2990_VCC_MSB:
 		/* Vcc, 305.18μV/LSB, 2.5V offset */
-		*result = (ltc2990_voltage_to_int(val) * 30518 /
-			   (4 * 100 * 1000)) + 2500;
+		*result = sign_extend32(val, 14) * 30518 / (100 * 1000) + 2500;
 		break;
 	default:
 		return -EINVAL; /* won't happen, keep compiler happy */