[PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Robert Lippert]

The _L low-current mode coefficient values should reference the
datasheet rows with CL=VDD but it seems were mistakenly pulled from
the rows with CL=GND.

This causes the current/power to be reported as approximately double
the actual value when CL=GND and half the actual value when CL=VDD.

Signed-off-by: Robert Lippert <rlippert@google.com>
---
 drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 10d17fb8f283..aa052f4449a9 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -191,19 +191,19 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
 			.R = -2,
 		},
 		[PSC_CURRENT_IN] = {
-			.m = 10753,
+			.m = 5405,
 			.R = -2,
 		},
 		[PSC_CURRENT_IN_L] = {
-			.m = 5405,
+			.m = 10753,
 			.R = -2,
 		},
 		[PSC_POWER] = {
-			.m = 1204,
+			.m = 605,
 			.R = -3,
 		},
 		[PSC_POWER_L] = {
-			.m = 605,
+			.m = 1204,
 			.R = -3,
 		},
 		[PSC_TEMPERATURE] = {
@@ -222,23 +222,23 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
 			.R = -2,
 		},
 		[PSC_CURRENT_IN] = {
-			.m = 15076,
-			.b = -504,
+			.m = 7645,
+			.b = 100,
 			.R = -2,
 		},
 		[PSC_CURRENT_IN_L] = {
-			.m = 7645,
-			.b = 100,
+			.m = 15076,
+			.b = -504,
 			.R = -2,
 		},
 		[PSC_POWER] = {
-			.m = 1701,
-			.b = -4000,
+			.m = 861,
+			.b = -965,
 			.R = -3,
 		},
 		[PSC_POWER_L] = {
-			.m = 861,
-			.b = -965,
+			.m = 1701,
+			.b = -4000,
 			.R = -3,
 		},
 		[PSC_TEMPERATURE] = {
-- 
2.15.0.448.gf294e3d99a-goog

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Guenter Roeck]

On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
> The _L low-current mode coefficient values should reference the
> datasheet rows with CL=VDD but it seems were mistakenly pulled from
> the rows with CL=GND.
> 
> This causes the current/power to be reported as approximately double
> the actual value when CL=GND and half the actual value when CL=VDD.
> 

This would affect all chips supported by this driver. Hmm, and I was sure
I tested this. I'll have to dig out my hardware and confirm.

The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
to determine which current limit setting to use. Looking into the
datasheet, it looks like it also has to evaluate bit 2, and I wonder
if there is a means to determine CL if bit 2 = 0. Any idea ?
Does bit 4 report the CL pin value if bit 2 = 0 ?

Thanks,
Guenter

> Signed-off-by: Robert Lippert <rlippert@google.com>
> ---
>  drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index 10d17fb8f283..aa052f4449a9 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c
> @@ -191,19 +191,19 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
>  			.R = -2,
>  		},
>  		[PSC_CURRENT_IN] = {
> -			.m = 10753,
> +			.m = 5405,
>  			.R = -2,
>  		},
>  		[PSC_CURRENT_IN_L] = {
> -			.m = 5405,
> +			.m = 10753,
>  			.R = -2,
>  		},
>  		[PSC_POWER] = {
> -			.m = 1204,
> +			.m = 605,
>  			.R = -3,
>  		},
>  		[PSC_POWER_L] = {
> -			.m = 605,
> +			.m = 1204,
>  			.R = -3,
>  		},
>  		[PSC_TEMPERATURE] = {
> @@ -222,23 +222,23 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
>  			.R = -2,
>  		},
>  		[PSC_CURRENT_IN] = {
> -			.m = 15076,
> -			.b = -504,
> +			.m = 7645,
> +			.b = 100,
>  			.R = -2,
>  		},
>  		[PSC_CURRENT_IN_L] = {
> -			.m = 7645,
> -			.b = 100,
> +			.m = 15076,
> +			.b = -504,
>  			.R = -2,
>  		},
>  		[PSC_POWER] = {
> -			.m = 1701,
> -			.b = -4000,
> +			.m = 861,
> +			.b = -965,
>  			.R = -3,
>  		},
>  		[PSC_POWER_L] = {
> -			.m = 861,
> -			.b = -965,
> +			.m = 1701,
> +			.b = -4000,
>  			.R = -3,
>  		},
>  		[PSC_TEMPERATURE] = {
> -- 
> 2.15.0.448.gf294e3d99a-goog
> 

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Rob Lippert]

On Wed, Nov 22, 2017 at 3:28 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
> > The _L low-current mode coefficient values should reference the
> > datasheet rows with CL=VDD but it seems were mistakenly pulled from
> > the rows with CL=GND.
> >
> > This causes the current/power to be reported as approximately double
> > the actual value when CL=GND and half the actual value when CL=VDD.
> >
>
> This would affect all chips supported by this driver. Hmm, and I was sure
> I tested this. I'll have to dig out my hardware and confirm.

I'm still not 100% convinced this commit is correct as I haven't been
able to validate the measurements against an external probe yet (and
my test board uses a non-standard sense resistor which means it needs
additional massaging of the data anyhow).

>
>
> The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
> to determine which current limit setting to use. Looking into the
> datasheet, it looks like it also has to evaluate bit 2, and I wonder
> if there is a means to determine CL if bit 2 = 0. Any idea ?

On my test board CL=floating (equivalent to GND) and the value of
register 0xD9 is all zeroes.

>
> Does bit 4 report the CL pin value if bit 2 = 0 ?

I can't tell by reading the datasheet that 0xD9 bit4 will ever report
the pin value as the language is difficult to parse :)
But I don't have any hardware setup with CL=VDD to test...

-Rob

>
> Thanks,
> Guenter
>
> > Signed-off-by: Robert Lippert <rlippert@google.com>
> > ---
> >  drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> > index 10d17fb8f283..aa052f4449a9 100644
> > --- a/drivers/hwmon/pmbus/lm25066.c
> > +++ b/drivers/hwmon/pmbus/lm25066.c
> > @@ -191,19 +191,19 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
> >                       .R = -2,
> >               },
> >               [PSC_CURRENT_IN] = {
> > -                     .m = 10753,
> > +                     .m = 5405,
> >                       .R = -2,
> >               },
> >               [PSC_CURRENT_IN_L] = {
> > -                     .m = 5405,
> > +                     .m = 10753,
> >                       .R = -2,
> >               },
> >               [PSC_POWER] = {
> > -                     .m = 1204,
> > +                     .m = 605,
> >                       .R = -3,
> >               },
> >               [PSC_POWER_L] = {
> > -                     .m = 605,
> > +                     .m = 1204,
> >                       .R = -3,
> >               },
> >               [PSC_TEMPERATURE] = {
> > @@ -222,23 +222,23 @@ static struct __coeff lm25066_coeff[6][PSC_NUM_CLASSES + 2] = {
> >                       .R = -2,
> >               },
> >               [PSC_CURRENT_IN] = {
> > -                     .m = 15076,
> > -                     .b = -504,
> > +                     .m = 7645,
> > +                     .b = 100,
> >                       .R = -2,
> >               },
> >               [PSC_CURRENT_IN_L] = {
> > -                     .m = 7645,
> > -                     .b = 100,
> > +                     .m = 15076,
> > +                     .b = -504,
> >                       .R = -2,
> >               },
> >               [PSC_POWER] = {
> > -                     .m = 1701,
> > -                     .b = -4000,
> > +                     .m = 861,
> > +                     .b = -965,
> >                       .R = -3,
> >               },
> >               [PSC_POWER_L] = {
> > -                     .m = 861,
> > -                     .b = -965,
> > +                     .m = 1701,
> > +                     .b = -4000,
> >                       .R = -3,
> >               },
> >               [PSC_TEMPERATURE] = {
> > --
> > 2.15.0.448.gf294e3d99a-goog
> >

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Guenter Roeck]

Hi Rob,

On 11/22/2017 03:39 PM, Rob Lippert wrote:
> On Wed, Nov 22, 2017 at 3:28 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
>>> The _L low-current mode coefficient values should reference the
>>> datasheet rows with CL=VDD but it seems were mistakenly pulled from
>>> the rows with CL=GND.
>>>
>>> This causes the current/power to be reported as approximately double
>>> the actual value when CL=GND and half the actual value when CL=VDD.
>>>
>>
>> This would affect all chips supported by this driver. Hmm, and I was sure
>> I tested this. I'll have to dig out my hardware and confirm.
> 
> I'm still not 100% convinced this commit is correct as I haven't been
> able to validate the measurements against an external probe yet (and
> my test board uses a non-standard sense resistor which means it needs
> additional massaging of the data anyhow).
> 
>>
>>
>> The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
>> to determine which current limit setting to use. Looking into the
>> datasheet, it looks like it also has to evaluate bit 2, and I wonder
>> if there is a means to determine CL if bit 2 = 0. Any idea ?
> 
> On my test board CL=floating (equivalent to GND) and the value of
> register 0xD9 is all zeroes.
> 
Are you sure that floating is equivalent to GND ? I didn't check the
datasheet, but it is more common for chips to have an internal pull-up.

>>
>> Does bit 4 report the CL pin value if bit 2 = 0 ?
> 
> I can't tell by reading the datasheet that 0xD9 bit4 will ever report
> the pin value as the language is difficult to parse :)

Same here.

> But I don't have any hardware setup with CL=VDD to test...
> 

I do have various evaluation boards, so I should be able to do some testing.
I hope I'll get to it over the weekend.

Guenter

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Rob Lippert]

On Wed, Nov 22, 2017 at 5:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Rob,
>
> On 11/22/2017 03:39 PM, Rob Lippert wrote:
>>
>> On Wed, Nov 22, 2017 at 3:28 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>
>>> On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
>>>>
>>>> The _L low-current mode coefficient values should reference the
>>>> datasheet rows with CL=VDD but it seems were mistakenly pulled from
>>>> the rows with CL=GND.
>>>>
>>>> This causes the current/power to be reported as approximately double
>>>> the actual value when CL=GND and half the actual value when CL=VDD.
>>>>
>>>
>>> This would affect all chips supported by this driver. Hmm, and I was sure
>>> I tested this. I'll have to dig out my hardware and confirm.
>>
>>
>> I'm still not 100% convinced this commit is correct as I haven't been
>> able to validate the measurements against an external probe yet (and
>> my test board uses a non-standard sense resistor which means it needs
>> additional massaging of the data anyhow).
>>
>>>
>>>
>>> The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
>>> to determine which current limit setting to use. Looking into the
>>> datasheet, it looks like it also has to evaluate bit 2, and I wonder
>>> if there is a means to determine CL if bit 2 = 0. Any idea ?
>>
>>
>> On my test board CL=floating (equivalent to GND) and the value of
>> register 0xD9 is all zeroes.
>>
> Are you sure that floating is equivalent to GND ? I didn't check the
> datasheet, but it is more common for chips to have an internal pull-up.
>
>>>
>>> Does bit 4 report the CL pin value if bit 2 = 0 ?
>>
>>
>> I can't tell by reading the datasheet that 0xD9 bit4 will ever report
>> the pin value as the language is difficult to parse :)
>
>
> Same here.
>
>> But I don't have any hardware setup with CL=VDD to test...
>>
>
> I do have various evaluation boards, so I should be able to do some testing.
> I hope I'll get to it over the weekend.

Actually turns out my board does tie CL=VDD as recommended by the
LM5066i datasheet to improve the current/power reporting accuracy.
But the value in 0xD9 is always zero so it seems there is no way to
read this CL pin setting from the device.

I think my commit is technically correct but it seems will likely
break the readings for most boards that follow the datasheet typical
circuit which recommends CL=VDD.

Would it make sense to remove the code trying to read the CL setting
and default the coefficient values to the "low current limit" CL=VDD
setting?  (and maybe something in devtree or module param to pick the
other coefficients?)

-Rob

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Guenter Roeck]

On Mon, Nov 27, 2017 at 01:51:35PM -0800, Rob Lippert wrote:
> On Wed, Nov 22, 2017 at 5:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > Hi Rob,
> >
> > On 11/22/2017 03:39 PM, Rob Lippert wrote:
> >>
> >> On Wed, Nov 22, 2017 at 3:28 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >>>
> >>> On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
> >>>>
> >>>> The _L low-current mode coefficient values should reference the
> >>>> datasheet rows with CL=VDD but it seems were mistakenly pulled from
> >>>> the rows with CL=GND.
> >>>>
> >>>> This causes the current/power to be reported as approximately double
> >>>> the actual value when CL=GND and half the actual value when CL=VDD.
> >>>>
> >>>
> >>> This would affect all chips supported by this driver. Hmm, and I was sure
> >>> I tested this. I'll have to dig out my hardware and confirm.
> >>
> >>
> >> I'm still not 100% convinced this commit is correct as I haven't been
> >> able to validate the measurements against an external probe yet (and
> >> my test board uses a non-standard sense resistor which means it needs
> >> additional massaging of the data anyhow).
> >>
> >>>
> >>>
> >>> The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
> >>> to determine which current limit setting to use. Looking into the
> >>> datasheet, it looks like it also has to evaluate bit 2, and I wonder
> >>> if there is a means to determine CL if bit 2 = 0. Any idea ?
> >>
> >>
> >> On my test board CL=floating (equivalent to GND) and the value of
> >> register 0xD9 is all zeroes.
> >>
> > Are you sure that floating is equivalent to GND ? I didn't check the
> > datasheet, but it is more common for chips to have an internal pull-up.
> >
> >>>
> >>> Does bit 4 report the CL pin value if bit 2 = 0 ?
> >>
> >>
> >> I can't tell by reading the datasheet that 0xD9 bit4 will ever report
> >> the pin value as the language is difficult to parse :)
> >
> >
> > Same here.
> >
> >> But I don't have any hardware setup with CL=VDD to test...
> >>
> >
> > I do have various evaluation boards, so I should be able to do some testing.
> > I hope I'll get to it over the weekend.
> 
> Actually turns out my board does tie CL=VDD as recommended by the
> LM5066i datasheet to improve the current/power reporting accuracy.
> But the value in 0xD9 is always zero so it seems there is no way to
> read this CL pin setting from the device.
> 
> I think my commit is technically correct but it seems will likely
> break the readings for most boards that follow the datasheet typical
> circuit which recommends CL=VDD.
> 
> Would it make sense to remove the code trying to read the CL setting
> and default the coefficient values to the "low current limit" CL=VDD
> setting?  (and maybe something in devtree or module param to pick the
> other coefficients?)
> 

The code should also check bit 2. If bit 2 is not set, it is ok to assume
CL=VDD. We can add a devicetree property if/when needed. Question though
is still how to handle the problem for the other chips supported by the
driver; if we change the code we should fix the problem for all chips,
not just for one. Wonder if we can just swap the defines to minimize code
changes.

Unfortunately I spent the entire weekend in bed with a cold, so I didn't get
to do any tests with my eval boards :-(.

Guenter

Re: [PATCH] hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)

[Rob Lippert]

On Mon, Nov 27, 2017 at 2:08 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Nov 27, 2017 at 01:51:35PM -0800, Rob Lippert wrote:
>> On Wed, Nov 22, 2017 at 5:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > Hi Rob,
>> >
>> > On 11/22/2017 03:39 PM, Rob Lippert wrote:
>> >>
>> >> On Wed, Nov 22, 2017 at 3:28 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >>>
>> >>>
>> >>> On Wed, Nov 22, 2017 at 02:07:28PM -0800, Robert Lippert wrote:
>> >>>>
>> >>>> The _L low-current mode coefficient values should reference the
>> >>>> datasheet rows with CL=VDD but it seems were mistakenly pulled from
>> >>>> the rows with CL=GND.
>> >>>>
>> >>>> This causes the current/power to be reported as approximately double
>> >>>> the actual value when CL=GND and half the actual value when CL=VDD.
>> >>>>
>> >>>
>> >>> This would affect all chips supported by this driver. Hmm, and I was sure
>> >>> I tested this. I'll have to dig out my hardware and confirm.
>> >>
>> >>
>> >> I'm still not 100% convinced this commit is correct as I haven't been
>> >> able to validate the measurements against an external probe yet (and
>> >> my test board uses a non-standard sense resistor which means it needs
>> >> additional massaging of the data anyhow).
>> >>
>> >>>
>> >>>
>> >>> The code currently only uses bit 4 of the DEVICE_SETUP (D9h) command
>> >>> to determine which current limit setting to use. Looking into the
>> >>> datasheet, it looks like it also has to evaluate bit 2, and I wonder
>> >>> if there is a means to determine CL if bit 2 = 0. Any idea ?
>> >>
>> >>
>> >> On my test board CL=floating (equivalent to GND) and the value of
>> >> register 0xD9 is all zeroes.
>> >>
>> > Are you sure that floating is equivalent to GND ? I didn't check the
>> > datasheet, but it is more common for chips to have an internal pull-up.
>> >
>> >>>
>> >>> Does bit 4 report the CL pin value if bit 2 = 0 ?
>> >>
>> >>
>> >> I can't tell by reading the datasheet that 0xD9 bit4 will ever report
>> >> the pin value as the language is difficult to parse :)
>> >
>> >
>> > Same here.
>> >
>> >> But I don't have any hardware setup with CL=VDD to test...
>> >>
>> >
>> > I do have various evaluation boards, so I should be able to do some testing.
>> > I hope I'll get to it over the weekend.
>>
>> Actually turns out my board does tie CL=VDD as recommended by the
>> LM5066i datasheet to improve the current/power reporting accuracy.
>> But the value in 0xD9 is always zero so it seems there is no way to
>> read this CL pin setting from the device.
>>
>> I think my commit is technically correct but it seems will likely
>> break the readings for most boards that follow the datasheet typical
>> circuit which recommends CL=VDD.
>>
>> Would it make sense to remove the code trying to read the CL setting
>> and default the coefficient values to the "low current limit" CL=VDD
>> setting?  (and maybe something in devtree or module param to pick the
>> other coefficients?)
>>
>
> The code should also check bit 2. If bit 2 is not set, it is ok to assume
> CL=VDD. We can add a devicetree property if/when needed. Question though
> is still how to handle the problem for the other chips supported by the
> driver; if we change the code we should fix the problem for all chips,
> not just for one. Wonder if we can just swap the defines to minimize code
> changes.

OK sent patch "hwmon: (pmbus/lm25066) Default coefficients for low
current limit" that I think does what was discussed.

I have not checked any of the other chip variant datasheet tables or
recommended default designs to see if they all match up though...

-Rob