Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1524495 > unrolled thread
| Started by | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| First post | 2016-11-17 18:10 +0100 |
| Last post | 2016-11-18 15:20 +0100 |
| Articles | 4 — 1 participant |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion Guenter Roeck <linux@roeck-us.net> - 2016-11-17 18:10 +0100
Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion Guenter Roeck <linux@roeck-us.net> - 2016-11-17 18:10 +0100
Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion Guenter Roeck <linux@roeck-us.net> - 2016-11-18 15:10 +0100
Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion Guenter Roeck <linux@roeck-us.net> - 2016-11-18 15:20 +0100
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-11-17 18:10 +0100 |
| Subject | Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion |
| Message-ID | <sEyme-3v8-73@gated-at.bofh.it> |
Hi Tom,
On 11/17/2016 04:10 AM, Tom Levens wrote:
> Conversion from raw values to signed integers has been refactored using
> the macros in bitops.h.
>
Please also mention that this fixes a bug in negative temperature conversions.
> Signed-off-by: Tom Levens <tom.levens@cern.ch>
> ---
> drivers/hwmon/ltc2990.c | 27 ++++++++++-----------------
> 1 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> index 8f8fe05..0ec4102 100644
> --- a/drivers/hwmon/ltc2990.c
> +++ b/drivers/hwmon/ltc2990.c
> @@ -9,8 +9,12 @@
> * This driver assumes the chip is wired as a dual current monitor, and
> * reports the voltage drop across two series resistors. It also reports
> * the chip's internal temperature and Vcc power supply voltage.
> + *
> + * Value conversion refactored
> + * by Tom Levens <tom.levens@cern.ch>
Kind of unusual to do that for minor changes like this. Imagine if everyone would do that.
The commit log is what gives you credit.
> */
>
> +#include <linux/bitops.h>
> #include <linux/err.h>
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> @@ -34,19 +38,10 @@
> #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)
> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
> {
> - int val;
> + s32 val;
Please just leave the variable type alone. it is also used for the return value
from i2c_smbus_read_word_swapped(), which is an int, and changing it to s32 doesn't
really make the code better.
Can you send me a register map for the chip ? I would like to write a module test.
Thanks,
Guenter
[toc] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-11-17 18:10 +0100 |
| Message-ID | <sEyme-3v8-91@gated-at.bofh.it> |
| In reply to | #1524495 |
On 11/17/2016 08:23 AM, Tom Levens wrote:
> Hi Guenter,
>
> Thanks for taking the time to review the patch.
>
> On Thu, 17 Nov 2016, Guenter Roeck wrote:
>
>> Hi Tom,
>>
>> On 11/17/2016 04:10 AM, Tom Levens wrote:
>>> Conversion from raw values to signed integers has been refactored using
>>> the macros in bitops.h.
>>>
>> Please also mention that this fixes a bug in negative temperature conversions.
>
> Yes, of course, I will include the information in v3.
>
>>
>>> Signed-off-by: Tom Levens <tom.levens@cern.ch>
>>> ---
>>> drivers/hwmon/ltc2990.c | 27 ++++++++++-----------------
>>> 1 files changed, 10 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>>> index 8f8fe05..0ec4102 100644
>>> --- a/drivers/hwmon/ltc2990.c
>>> +++ b/drivers/hwmon/ltc2990.c
>>> @@ -9,8 +9,12 @@
>>> * This driver assumes the chip is wired as a dual current monitor, and
>>> * reports the voltage drop across two series resistors. It also reports
>>> * the chip's internal temperature and Vcc power supply voltage.
>>> + *
>>> + * Value conversion refactored
>>> + * by Tom Levens <tom.levens@cern.ch>
>>
>> Kind of unusual to do that for minor changes like this. Imagine if everyone would do that.
>> The commit log is what gives you credit.
>
> Good point, thanks for the hint. I will remove it from v3.
>
>>> */
>>>
>>> +#include <linux/bitops.h>
>>> #include <linux/err.h>
>>> #include <linux/hwmon.h>
>>> #include <linux/hwmon-sysfs.h>
>>> @@ -34,19 +38,10 @@
>>> #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)
>>> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 *result)
>>> {
>>> - int val;
>>> + s32 val;
>>
>> Please just leave the variable type alone. it is also used for the return value
>> from i2c_smbus_read_word_swapped(), which is an int, and changing it to s32 doesn't really make the code better.
>
> According to i2c.h the return type for i2c_smbus_read_word_swapped() is s32, which is why I modified it here. But it could be changed back if you think it is better to leave it as an int.
>
Ah, ok. Good to know. Please leave it anyway, reason being that there is no real
reason to change it. Effectively those are just whitespace changes (unlike the rest,
which is part bug fix, part cleanup).
>> Can you send me a register map for the chip ? I would like to write a module test.
>
> Here is an example register dump:
I meant the output of i2cdump (something like "i2cdump -y -f <bus> <i2c-address> w").
Thanks,
Guenter
>
> 00 00 00 00
> 01 90 07 d0
> 2c cd 7d 80
> 7c 29 20 00
>
> The expected values in this case are:
>
> in0_input 5000
> in1_input 610
> in2_input 3500
> in3_input -195
> in4_input -299
> temp1_input 25000
> temp2_input 125000
> temp3_input -40000
> curr1_input 38840
> curr2_input -12428
>
> Testing with lltc,mode set to <5>, <6> and <7> should give you all measurements.
>
>> Thanks,
>> Guenter
>
> Cheers,
>
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-11-18 15:10 +0100 |
| Message-ID | <sES1z-8aS-13@gated-at.bofh.it> |
| In reply to | #1524497 |
On 11/18/2016 12:18 AM, Tom Levens wrote:
> Hi Guenter,
>
> On Thu, 17 Nov 2016, Guenter Roeck wrote:
>
>> On 11/17/2016 08:23 AM, Tom Levens wrote:
>>> Hi Guenter,
>>>
>>> Thanks for taking the time to review the patch.
>>>
>>> On Thu, 17 Nov 2016, Guenter Roeck wrote:
>>>
>>> > Hi Tom,
>>> > > On 11/17/2016 04:10 AM, Tom Levens wrote:
>>> > > Conversion from raw values to signed integers has been refactored > > using
>>> > > the macros in bitops.h.
>>> > > > Please also mention that this fixes a bug in negative temperature > conversions.
>>>
>>> Yes, of course, I will include the information in v3.
>>>
>>> > > > Signed-off-by: Tom Levens <tom.levens@cern.ch>
>>> > > ---
>>> > > drivers/hwmon/ltc2990.c | 27 ++++++++++-----------------
>>> > > 1 files changed, 10 insertions(+), 17 deletions(-)
>>> > > > > diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>>> > > index 8f8fe05..0ec4102 100644
>>> > > --- a/drivers/hwmon/ltc2990.c
>>> > > +++ b/drivers/hwmon/ltc2990.c
>>> > > @@ -9,8 +9,12 @@
>>> > > * This driver assumes the chip is wired as a dual current monitor, > > and
>>> > > * reports the voltage drop across two series resistors. It also > > reports
>>> > > * the chip's internal temperature and Vcc power supply voltage.
>>> > > + *
>>> > > + * Value conversion refactored
>>> > > + * by Tom Levens <tom.levens@cern.ch>
>>> > > Kind of unusual to do that for minor changes like this. Imagine if > everyone would do that.
>>> > The commit log is what gives you credit.
>>>
>>> Good point, thanks for the hint. I will remove it from v3.
>>>
>>> > > */
>>> > > > > +#include <linux/bitops.h>
>>> > > #include <linux/err.h>
>>> > > #include <linux/hwmon.h>
>>> > > #include <linux/hwmon-sysfs.h>
>>> > > @@ -34,19 +38,10 @@
>>> > > #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)
>>> > > +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, s32 > > *result)
>>> > > {
>>> > > - int val;
>>> > > + s32 val;
>>> > > Please just leave the variable type alone. it is also used for the > return value
>>> > from i2c_smbus_read_word_swapped(), which is an int, and changing it to > s32 doesn't really make the code better.
>>>
>>> According to i2c.h the return type for i2c_smbus_read_word_swapped() is
>>> s32, which is why I modified it here. But it could be changed back if you
>>> think it is better to leave it as an int.
>>>
>> Ah, ok. Good to know. Please leave it anyway, reason being that there is no real
>> reason to change it. Effectively those are just whitespace changes (unlike the rest,
>> which is part bug fix, part cleanup).
>>
>>> > Can you send me a register map for the chip ? I would like to write a > module test.
>>>
>>> Here is an example register dump:
>>
>> I meant the output of i2cdump (something like "i2cdump -y -f <bus> <i2c-address> w").
>>
>
> The register map wraps at 0x0F, so I only sent you the first 16 bytes. But the fully expanded form is:
>
> 0 1 2 3 4 5 6 7 8 9 a b c d e f
> 00: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 10: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 20: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 30: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 40: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 50: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 60: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 70: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 80: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> 90: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> a0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> b0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> c0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> d0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> e0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
> f0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00
Sorry, I wasn't clear. The chip uses 16-bit registers, so the
"w" in the command would be important to report the entire
register content, not just the first 8 bit of each register.
Thanks,
Guenter
>
> Cheers,
>
>> Thanks,
>> Guenter
>>
>>>
>>> 00 00 00 00
>>> 01 90 07 d0
>>> 2c cd 7d 80
>>> 7c 29 20 00
>>>
>>> The expected values in this case are:
>>>
>>> in0_input 5000
>>> in1_input 610
>>> in2_input 3500
>>> in3_input -195
>>> in4_input -299
>>> temp1_input 25000
>>> temp2_input 125000
>>> temp3_input -40000
>>> curr1_input 38840
>>> curr2_input -12428
>>>
>>> Testing with lltc,mode set to <5>, <6> and <7> should give you all
>>> measurements.
>>>
>>> > Thanks,
>>> > Guenter
>>>
>>> Cheers,
>>>
>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-11-18 15:20 +0100 |
| Message-ID | <sESbg-8gx-37@gated-at.bofh.it> |
| In reply to | #1525355 |
On 11/18/2016 06:09 AM, Guenter Roeck wrote: >> >> The register map wraps at 0x0F, so I only sent you the first 16 bytes. But the fully expanded form is: >> >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 10: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 20: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 30: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 40: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 50: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 60: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 70: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 80: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> 90: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> a0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> b0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> c0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> d0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> e0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 >> f0: 00 00 00 00 01 90 07 d0 2c cd 7d 80 7c 29 20 00 > > Sorry, I wasn't clear. The chip uses 16-bit registers, so the > "w" in the command would be important to report the entire > register content, not just the first 8 bit of each register. > Too early, sorry. the above is fine. Guenter
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web