[PATCH v2] hwmon: (amc6821) sign extension temperature

[Matt Weber]

From: Jared Bents 

Converts the unsigned temperature values from the i2c read
to be sign extended as defined in the datasheet so that
negative temperatures are properly read.

Signed-off-by: Jared Bents 
Signed-off-by: Matt Weber 
---
v1 -> v2
 - checkpatch cleanup, removed space between cast
   and function call.  truncated line over 80chars

---
 drivers/hwmon/amc6821.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 12e851a..eb53e0b 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -188,7 +188,8 @@ static struct amc6821_data *amc6821_update_device(struct 
device *dev)
!data->valid) {
 
for (i = 0; i < TEMP_IDX_LEN; i++)
-   data->temp[i] = i2c_smbus_read_byte_data(client,
+   data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
+   client,
temp_reg[i]);
 
data->stat1 = i2c_smbus_read_byte_data(client,
-- 
1.9.1

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

Re: [PATCH] hwmon: (amc6821) sign extension temperature

[Guenter Roeck]

On Fri, Nov 18, 2016 at 03:28:30PM -0600, Matthew Weber wrote:
> Guenter,
> 
> On Thu, Nov 17, 2016 at 4:18 PM, Guenter Roeck  wrote:
> >
> > On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote:
> > > On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck  wrote:
> > > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> > > >> From: Jared Bents 
> > > >>
> > > >> Converts the unsigned temperature values from the i2c read
> > > >> to be sign extended as defined in the datasheet so that
> > > >> negative temperatures are properly read.
> > > >>
> > > >> Signed-off-by: Jared Bents 
> > > >> Signed-off-by: Matt Weber 
> > > >> ---
> > > >>  drivers/hwmon/amc6821.c | 2 +-
> > > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > > >> index 12e851a..d7368f7 100644
> > > >> --- a/drivers/hwmon/amc6821.c
> > > >> +++ b/drivers/hwmon/amc6821.c
> > > >> @@ -188,7 +188,7 @@ static struct amc6821_data 
> > > >> *amc6821_update_device(struct device *dev)
> > > >>   !data->valid) {
> > > >>
> > > >>   for (i = 0; i < TEMP_IDX_LEN; i++)
> > > >> - data->temp[i] = i2c_smbus_read_byte_data(client,
> > > >> + data->temp[i] = (int8_t) 
> > > >> i2c_smbus_read_byte_data(client,
> > > >
> > > > How does that fix anything ? The only difference is that errors 
> > > > reported from
> > > > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no 
> > > > longer
> > > > as negative numbers.  I don't see how a value of 0xff read from the 
> > > > chip would
> > > > now be reported as -1 degrees C; it should be reported as 255 degrees C 
> > > > as it
> > > > was all along. What am I missing here ?
> > > >
> > > > A real fix would be to actually check for errors either here or in the 
> > > > show
> > > > function (temp[] < 0 indicates a transfer error), and to use 
> > > > sign_extend()
> > > > to convert the 8-bit reading to a signed value.
> > > >
> > > > Guenter
> > > >
> > >
> > > As stated in the patch note, the amc6821 uses signed numbers for the
> > > temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the
> > > temperature range table in the amc6821 datasheet.  This change does
> > > not break any error checking when reading the temperature over the i2c
> > > bus in this driver as this driver does not do any error checking for
> > > the temperature reads to begin with.  There are error checks in the
> > > driver but they are for some i2c writes and a few i2c reads for
> > > configuration settings.  None of those error checks are affected.
> > > Without this patch, the temperatures that are displayed in /sys/class
> > > when below 0 Deg C are 255 Deg C to 128 Deg C.  With the patch, the
> > > temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as
> > > expected.
> > >
> > Ah, yes, it is "int8_t", which is extended to a negative number.
> > Sorry, I somehow read it as unsigned.
> >
> > Please run your patch through checkpatch --strict, fix what it reports,
> > and resubmit.
> 
> 
> I assume we fix errors but wanted to check on warnings as it looks
> like this file has a lot.
> 

That is not a reason to introduce new warnings and check messages
in a new patch. I did not ask you to fix all the checkpatch problems
in this file, only the ones reported in your patch.

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

Re: [PATCH] hwmon: (amc6821) sign extension temperature

[Matthew Weber]

Guenter,

On Thu, Nov 17, 2016 at 4:18 PM, Guenter Roeck  wrote:
>
> On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote:
> > On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck  wrote:
> > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> > >> From: Jared Bents 
> > >>
> > >> Converts the unsigned temperature values from the i2c read
> > >> to be sign extended as defined in the datasheet so that
> > >> negative temperatures are properly read.
> > >>
> > >> Signed-off-by: Jared Bents 
> > >> Signed-off-by: Matt Weber 
> > >> ---
> > >>  drivers/hwmon/amc6821.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > >> index 12e851a..d7368f7 100644
> > >> --- a/drivers/hwmon/amc6821.c
> > >> +++ b/drivers/hwmon/amc6821.c
> > >> @@ -188,7 +188,7 @@ static struct amc6821_data 
> > >> *amc6821_update_device(struct device *dev)
> > >>   !data->valid) {
> > >>
> > >>   for (i = 0; i < TEMP_IDX_LEN; i++)
> > >> - data->temp[i] = i2c_smbus_read_byte_data(client,
> > >> + data->temp[i] = (int8_t) 
> > >> i2c_smbus_read_byte_data(client,
> > >
> > > How does that fix anything ? The only difference is that errors reported 
> > > from
> > > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no 
> > > longer
> > > as negative numbers.  I don't see how a value of 0xff read from the chip 
> > > would
> > > now be reported as -1 degrees C; it should be reported as 255 degrees C 
> > > as it
> > > was all along. What am I missing here ?
> > >
> > > A real fix would be to actually check for errors either here or in the 
> > > show
> > > function (temp[] < 0 indicates a transfer error), and to use sign_extend()
> > > to convert the 8-bit reading to a signed value.
> > >
> > > Guenter
> > >
> >
> > As stated in the patch note, the amc6821 uses signed numbers for the
> > temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the
> > temperature range table in the amc6821 datasheet.  This change does
> > not break any error checking when reading the temperature over the i2c
> > bus in this driver as this driver does not do any error checking for
> > the temperature reads to begin with.  There are error checks in the
> > driver but they are for some i2c writes and a few i2c reads for
> > configuration settings.  None of those error checks are affected.
> > Without this patch, the temperatures that are displayed in /sys/class
> > when below 0 Deg C are 255 Deg C to 128 Deg C.  With the patch, the
> > temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as
> > expected.
> >
> Ah, yes, it is "int8_t", which is extended to a negative number.
> Sorry, I somehow read it as unsigned.
>
> Please run your patch through checkpatch --strict, fix what it reports,
> and resubmit.


I assume we fix errors but wanted to check on warnings as it looks
like this file has a lot.

-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / Security Systems and Software / Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.we...@corp.rockwellcollins.com.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] hwmon: ltc2990: add devicetree binding

[Tom Levens]

Hi Rob,

Thanks for taking the time to look at this patch.

On Fri, 18 Nov 2016, Rob Herring wrote:

On Thu, Nov 17, 2016 at 01:10:15PM +0100, Tom Levens wrote:

Add a devicetree binding for the ltc2990 hwmon driver.

Signed-off-by: Tom Levens 
---
 .../devicetree/bindings/hwmon/ltc2990.txt  |   28 
 1 files changed, 28 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2990.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ltc2990.txt 
b/Documentation/devicetree/bindings/hwmon/ltc2990.txt
new file mode 100644
index 000..e4040e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ltc2990.txt
@@ -0,0 +1,28 @@
+ltc2990
+
+Required properties:
+- compatible: Must be "lltc,ltc2990"
+- reg: I2C slave address
+
+Optional properties:
+- lltc,mode:


What determines the mode? If this is something a user will want to
control, then it should be a sysfs attr rather than DT prop. If the
board design dictates then DT is the right place.


It is based on the external connections of the chip. So, I would say that 
it is board specific.



'mode' is a bit vague, 'lltc,meas-mode' perhaps.


Sure thing.

There is also one question which came up in the thread for the patch 3 of 
this patchset. The full mode for this chip is actually made of two logical 
values which are written to the bits 4..3 and 2..0 of the register. This 
version of the patch only configures one of the values, but for the next 
version we would like to configure both. While combining them into a 
single integer value would be possible, they are defined as two values in 
the datasheet. Therefore, I propose using an array, such as:


lltc,meas-mode = <7 3>;

This would set the mode[2..0]=7 and mode[4..3]=3.

What do you think of this? Or should this be split into two properties? 
However, in this case I struggle come up with names for the properties. 
The datasheet, helpfully, calls both the fields "mode" and there is no 
clear difference in their function as both control which measurements are 
available.


Thanks,


+A  Sets the chip's measurement mode, defaults to <6> if unset.
+
+   The following measurements are available in each mode:
+
+   0: V1, V2, TR2
+   1: V1-V2, TR2
+   2: V1-V2, V3, V4
+   3: TR1, V3, V4
+   4: TR1, V3-V4
+   5: TR1, TR2
+   6: V1-V2, V3-V4
+   7: V1, V2, V3, V4
+
+Example:
+
+ltc2990@4c {
+   compatible = "lltc,ltc2990";
+   reg = <0x4c>;
+   lltc,mode = <7>;
+};
--
1.7.1




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

Re: [PATCH v2 2/3] hwmon: ltc2990: add devicetree binding

[Rob Herring]

On Thu, Nov 17, 2016 at 01:10:15PM +0100, Tom Levens wrote:
> Add a devicetree binding for the ltc2990 hwmon driver.
> 
> Signed-off-by: Tom Levens 
> ---
>  .../devicetree/bindings/hwmon/ltc2990.txt  |   28 
> 
>  1 files changed, 28 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2990.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2990.txt 
> b/Documentation/devicetree/bindings/hwmon/ltc2990.txt
> new file mode 100644
> index 000..e4040e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ltc2990.txt
> @@ -0,0 +1,28 @@
> +ltc2990
> +
> +Required properties:
> +- compatible: Must be "lltc,ltc2990"
> +- reg: I2C slave address
> +
> +Optional properties:
> +- lltc,mode:

What determines the mode? If this is something a user will want to 
control, then it should be a sysfs attr rather than DT prop. If the 
board design dictates then DT is the right place.

'mode' is a bit vague, 'lltc,meas-mode' perhaps.

> + Sets the chip's measurement mode, defaults to <6> if unset.
> +
> + The following measurements are available in each mode:
> +
> + 0: V1, V2, TR2
> + 1: V1-V2, TR2
> + 2: V1-V2, V3, V4
> + 3: TR1, V3, V4
> + 4: TR1, V3-V4
> + 5: TR1, TR2
> + 6: V1-V2, V3-V4
> + 7: V1, V2, V3, V4
> +
> +Example:
> +
> +ltc2990@4c {
> + compatible = "lltc,ltc2990";
> + reg = <0x4c>;
> + lltc,mode = <7>;
> +};
> -- 
> 1.7.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion

[Guenter Roeck]

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

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

Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes

[Guenter Roeck]

On 11/18/2016 04:23 AM, Tom Levens wrote:



On Fri, 18 Nov 2016, Guenter Roeck wrote:


On Thu, Nov 17, 2016 at 11:25:30PM +, Tom Levens wrote:

On 17 Nov 2016, at 22:54, Guenter Roeck  wrote:

On Thu, Nov 17, 2016 at 08:52:12PM +0100, Mike Looijmans wrote:

On 17-11-2016 19:56, Guenter Roeck wrote:

On Thu, Nov 17, 2016 at 06:40:17PM +0100, Mike Looijmans wrote:

On 17-11-16 17:56, Guenter Roeck wrote:

On 11/17/2016 04:10 AM, Tom Levens wrote:

Updated version of the ltc2990 driver which supports all measurement
modes available in the chip. The mode can be set through a devicetree
attribute.



[ ... ]



static int ltc2990_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
   int ret;
   struct device *hwmon_dev;
+struct ltc2990_data *data;
+struct device_node *of_node = i2c->dev.of_node;

   if (!i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_WORD_DATA))
   return -ENODEV;

-/* Setup continuous mode, current monitor */
+data = devm_kzalloc(>dev, sizeof(struct ltc2990_data),
GFP_KERNEL);
+if (unlikely(!data))
+return -ENOMEM;
+data->i2c = i2c;
+
+if (!of_node || of_property_read_u32(of_node, "lltc,mode",
>mode))
+data->mode = LTC2990_CONTROL_MODE_DEFAULT;


Iam arguing with myself if we should still do this or if we should read
the mode
from the chip instead if it isn't provided (after all, it may have been
initialized
by the BIOS/ROMMON).


I think the mode should be explicitly set, without default. There's no way
to tell whether the BIOS or bootloader has really set it up or whether the
chip is just reporting whatever it happened to default to. And given the
chip's function, it's unlikely a bootloader would want to initialize it.


Unlikely but possible. Even if we all agree that the chip should be configured
by the driver, I don't like imposing that view on everyone else.


My advice would be to make it a required property. If not set, display an
error and bail out.


It is not that easy, unfortunately. It also has to work on a non-devicetree
system. I would not object to making the property mandatory, but we would
still need to provide non-DT support.

My "use case" for taking the current mode from the chip if not specified
is that it would enable me to run a module test with all modes. I consider
this extremely valuable.


Good point.

The chip defaults to measuring internal temperature only, and the mode
defaults to "0".

Choosing a mode that doesn't match the actual circuitry could be bad for the
chip or the board (though unlikely, it'll probably just be useless) since it
will actively drive some of the inputs in the temperature modes (which is
default for V3/V4 pins).

Instead of failing, one could choose to set the default mode to "7", which
just measures the 4 voltages, which would be a harmless mode in all cases.

As a way to let a bootloader set things up, I think it would be a good check
to see if CONTROL register bits 4:3 are set. If "00", the chip is not
acquiring data at all, and probably needs configuration still. In that case,
the mode must be provided by the devicetree (or the default "7").
If bits 4:3 are "11", it has already been set up to measure its inputs, and
it's okay to continue doing just that and use the current value of 2:0
register as default mode (if the devicetree didn't specify any mode at all).



At first glance, agreed, though by default b[3:4] are 00, and only the
internal temperature is measured. Actually, the 5 mode bits are all
relevant to determine what is measured. Maybe it would be better to take
all 5 bits into account instead of blindly setting b[34]:=11 and a specific
setting of b[0:2]. Sure, that would make the mode table a bit larger,
but then ltc2990_attrs_ena[] could be made an u16 array, and a table size
of 64 bytes would not be that bad.


I would tend to agree that it should be possible to configure all 5 mode
bits through the devicetree. What I would propose is as follows.

If a devicetree node exists, the mode parameter(s?) are required and the
chip is initialised.

If a devicetree node doesn't exist, it is assumed that the chip has
already been configured (by the BIOS, etc). The mode is read from the
chip to set the visibility of the sysfs attributes. In the worst case, where the
chip has not been configured by another source, it would only be possible
to measure the internal temperature -- but I think this is an acceptable
limitation.


SGTM.


The only case that this does not cover is if the device tree node
exists but the chip is expected to be configured by some other source.
Maybe I am wrong, but I would not expect this to be a terribly common
situation.

What do you think?


I would not bother about this case. Just make the mode property mandatory.


What do you think about making the devicetree property a list of two integers? 
Something like

lltc,mode = <7 3>;

which would set 

Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion

[Guenter Roeck]

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 
> >   ---
> >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 
> >  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 
> >#include 
> >#include 
> >#include 
> >   @@ -34,19 +38,10 @@
> >#define LTC2990_CONTROL_MODE_CURRENT0x06
> >#define LTC2990_CONTROL_MODE_VOLTAGE0x07
> > > >   -/* 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   
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_input610
 in2_input3500
 in3_input-195
 in4_input-299
 temp1_input 25000
 temp2_input 125000
 temp3_input-4
 curr1_input38840
 curr2_input-12428

 Testing with lltc,mode set to <5>, <6> and <7> should give you all
 measurements.

>  Thanks,
>  Guenter

 Cheers,








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

Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes

[Tom Levens]

On Fri, 18 Nov 2016, Guenter Roeck wrote:


On Thu, Nov 17, 2016 at 11:25:30PM +, Tom Levens wrote:

On 17 Nov 2016, at 22:54, Guenter Roeck  wrote:

On Thu, Nov 17, 2016 at 08:52:12PM +0100, Mike Looijmans wrote:

On 17-11-2016 19:56, Guenter Roeck wrote:

On Thu, Nov 17, 2016 at 06:40:17PM +0100, Mike Looijmans wrote:

On 17-11-16 17:56, Guenter Roeck wrote:

On 11/17/2016 04:10 AM, Tom Levens wrote:

Updated version of the ltc2990 driver which supports all measurement
modes available in the chip. The mode can be set through a devicetree
attribute.



[ ... ]



static int ltc2990_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
   int ret;
   struct device *hwmon_dev;
+struct ltc2990_data *data;
+struct device_node *of_node = i2c->dev.of_node;

   if (!i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_WORD_DATA))
   return -ENODEV;

-/* Setup continuous mode, current monitor */
+data = devm_kzalloc(>dev, sizeof(struct ltc2990_data),
GFP_KERNEL);
+if (unlikely(!data))
+return -ENOMEM;
+data->i2c = i2c;
+
+if (!of_node || of_property_read_u32(of_node, "lltc,mode",
>mode))
+data->mode = LTC2990_CONTROL_MODE_DEFAULT;


Iam arguing with myself if we should still do this or if we should read
the mode
from the chip instead if it isn't provided (after all, it may have been
initialized
by the BIOS/ROMMON).


I think the mode should be explicitly set, without default. There's no way
to tell whether the BIOS or bootloader has really set it up or whether the
chip is just reporting whatever it happened to default to. And given the
chip's function, it's unlikely a bootloader would want to initialize it.


Unlikely but possible. Even if we all agree that the chip should be configured
by the driver, I don't like imposing that view on everyone else.


My advice would be to make it a required property. If not set, display an
error and bail out.


It is not that easy, unfortunately. It also has to work on a non-devicetree
system. I would not object to making the property mandatory, but we would
still need to provide non-DT support.

My "use case" for taking the current mode from the chip if not specified
is that it would enable me to run a module test with all modes. I consider
this extremely valuable.


Good point.

The chip defaults to measuring internal temperature only, and the mode
defaults to "0".

Choosing a mode that doesn't match the actual circuitry could be bad for the
chip or the board (though unlikely, it'll probably just be useless) since it
will actively drive some of the inputs in the temperature modes (which is
default for V3/V4 pins).

Instead of failing, one could choose to set the default mode to "7", which
just measures the 4 voltages, which would be a harmless mode in all cases.

As a way to let a bootloader set things up, I think it would be a good check
to see if CONTROL register bits 4:3 are set. If "00", the chip is not
acquiring data at all, and probably needs configuration still. In that case,
the mode must be provided by the devicetree (or the default "7").
If bits 4:3 are "11", it has already been set up to measure its inputs, and
it's okay to continue doing just that and use the current value of 2:0
register as default mode (if the devicetree didn't specify any mode at all).



At first glance, agreed, though by default b[3:4] are 00, and only the
internal temperature is measured. Actually, the 5 mode bits are all
relevant to determine what is measured. Maybe it would be better to take
all 5 bits into account instead of blindly setting b[34]:=11 and a specific
setting of b[0:2]. Sure, that would make the mode table a bit larger,
but then ltc2990_attrs_ena[] could be made an u16 array, and a table size
of 64 bytes would not be that bad.


I would tend to agree that it should be possible to configure all 5 mode
bits through the devicetree. What I would propose is as follows.

If a devicetree node exists, the mode parameter(s?) are required and the
chip is initialised.

If a devicetree node doesn't exist, it is assumed that the chip has
already been configured (by the BIOS, etc). The mode is read from the
chip to set the visibility of the sysfs attributes. In the worst case, where the
chip has not been configured by another source, it would only be possible
to measure the internal temperature -- but I think this is an acceptable
limitation.


SGTM.


The only case that this does not cover is if the device tree node
exists but the chip is expected to be configured by some other source.
Maybe I am wrong, but I would not expect this to be a terribly common
situation.

What do you think?


I would not bother about this case. Just make the mode property mandatory.


What do you think about making the devicetree property a list of two 
integers? Something like


lltc,mode = <7 3>;

which would set mode[2..0]=7 and mode[4..3]=3.

To me, this 

Re: [PATCH v2 1/3] hwmon: ltc2990: refactor value conversion

[Tom Levens]

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 

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

> >#include 
> >#include 
> >#include 
> >   @@ -34,19 +38,10 @@
> >#define LTC2990_CONTROL_MODE_CURRENT0x06
> >#define LTC2990_CONTROL_MODE_VOLTAGE0x07
> > 
> >   -/* 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  
 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

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_input610
 in2_input3500
 in3_input-195
 in4_input-299
 temp1_input 25000
 temp2_input 125000
 temp3_input-4
 curr1_input38840
 curr2_input-12428

 Testing with lltc,mode set to <5>, <6> and <7> should give you all
 measurements.

>  Thanks,
>  Guenter

 Cheers,





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