Skip to contentSkip to conversation

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

PATCH In review Robert Lippert 3 participants 7 messages Nov 22, 2017 – Nov 28, 2017 Hardware monitoring

lore
Share & tools
lore

Robert Lippert

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/hwmon/pmbus/lm25066.c +12 -12
index 10d17fb8f283..aa052f4449a9 100644
@@ -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] = {
Signature · 1 line
2.15.0.448.gf294e3d99a-goog
maintainer
#1 lore raw
Quoted 7 lines · 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.

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

Quoted 68 lines
Signed-off-by Robert Lippert <[email protected]>
---
 drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/hwmon/pmbus/lm25066.c +12 -12
index 10d17fb8f283..aa052f4449a9 100644
@@ -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] = {
Signature · 1 line
2.15.0.448.gf294e3d99a-goog
#2 lore raw
Quoted 6 lines · Guenter Roeck
Quoted 4 lines · 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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 72 lines

Thanks,
Guenter

Quoted 68 lines
Signed-off-by Robert Lippert <[email protected]>
---
 drivers/hwmon/pmbus/lm25066.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/hwmon/pmbus/lm25066.c +12 -13
index 10d17fb8f283..aa052f4449a9 100644
@@ -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

maintainer
#3 lore raw

Hi Rob,

Quoted 21 lines · Rob Lippert
Quoted 6 lines · Guenter Roeck
Quoted 4 lines · 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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines
Quoted 8 lines
Quoted 4 lines

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.

Quoted 5 lines
Quoted 2 lines

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.

Quoted 2 lines

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

#4 lore raw
Quoted 48 lines · Guenter Roeck

Hi Rob,

Quoted 25 lines · Rob Lippert
Quoted 8 lines · Guenter Roeck
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines
Quoted 9 lines
Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 7 lines

Same here.

Quoted 2 lines

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

maintainer
#5 lore raw
Quoted 62 lines · Rob Lippert
Quoted 48 lines · Guenter Roeck

Hi Rob,

Quoted 25 lines · Rob Lippert
Quoted 8 lines · Guenter Roeck
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines
Quoted 9 lines
Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 7 lines

Same here.

Quoted 2 lines

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.

Quoted 9 lines

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

#6 lore raw
Quoted 73 lines · Guenter Roeck
Quoted 62 lines · Rob Lippert
Quoted 48 lines · Guenter Roeck

Hi Rob,

Quoted 25 lines · Rob Lippert
Quoted 8 lines · Guenter Roeck
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines
Quoted 9 lines
Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 7 lines

Same here.

Quoted 2 lines

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.

Quoted 9 lines

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

Quoted 6 lines

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