hwmon: (pmbus/lm25066) Default coefficients for low current limit
PATCH 1/1 hwmon: (pmbus/lm25066) Default coefficients for low current limit
There seems to be no way to detect the value of the CL/GAIN pin from the device using PMBus.
Low current mode seems to be recommended (from LM5066 datasheet) for improved current reporting accuracy in a typical design.
Assume the device is in low current mode unless the register override bit is set and indicates high current limit mode.
--- drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/hwmon/pmbus/lm25066.c +13 -6
@@ -43,6 +43,7 @@ enum chips { lm25056, lm25063, lm25066, lm5064, lm5066, lm5066i };#define LM25066_READ_AVG_IIN 0xde#define LM25066_READ_AVG_PIN 0xdf +#define LM25066_DEV_SETUP_CL_CONFIG BIT(2)/* Use pin or SMBus values */ #define LM25066_DEV_SETUP_CL BIT(4)/* Current limit */ /* LM25056 only */
@@ -526,16 +527,22 @@ static int lm25066_probe(struct i2c_client *client, info->R[PSC_VOLTAGE_OUT ] = coeff[PSC_VOLTAGE_OUT ].R; info->R[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].R; info->R[PSC_POWER ] = coeff[PSC_POWER ].R; -if (config &LM25066_DEV_SETUP_CL ) { - info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; - info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; - info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; - info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; - }else { + +/* +* Assume chip is in low current limit/gain mode by default unless +* the register override is asserted AND indicates high limit mode. +*/ +if ((config &LM25066_DEV_SETUP_CL_CONFIG ) && + (config &LM25066_DEV_SETUP_CL )) { info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].m; info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].b; info->m[PSC_POWER ] = coeff[PSC_POWER ].m; info->b[PSC_POWER ] = coeff[PSC_POWER ].b; + }else { + info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; + info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; + info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; + info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; }return pmbus_do_probe (client, id, info);
Signature · 1 line
2.15.0.417.g466bffb3ac-goog
Guenter Roeck
maintainerQuoted 10 lines · Robert Lippert
There seems to be no way to detect the value of the CL/GAIN pin from the device using PMBus.
Low current mode seems to be recommended (from LM5066 datasheet) for improved current reporting accuracy in a typical design.
Assume the device is in low current mode unless the register override bit is set and indicates high current limit mode.
Signed-off-by Robert Lippert <[email protected]>
Applied (silently fixed continuation line alignment).
Looking at the most recent version of various datasheets for the chips supported by the driver, looks like TI changed the coefficients (again). Not the first time. Time to go through all chips and update coefficients. Might be worthwhile to do that at least for the chips(s) which are of interest for you.
Thanks,
Guenter
Quoted 46 lines
--- drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)drivers/hwmon/pmbus/lm25066.c +13
-6@@ -43,6 +43,7 @@ enum chips { lm25056, lm25063, lm25066, lm5064, lm5066, lm5066i };#define LM25066_READ_AVG_IIN 0xde#define LM25066_READ_AVG_PIN 0xdf +#define LM25066_DEV_SETUP_CL_CONFIG BIT(2)/* Use pin or SMBus values */ #define LM25066_DEV_SETUP_CL BIT(4)/* Current limit */ /* LM25056 only */ @@ -526,16 +527,22 @@ static int lm25066_probe(struct i2c_client *client, info->R[PSC_VOLTAGE_OUT ] = coeff[PSC_VOLTAGE_OUT ].R; info->R[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].R; info->R[PSC_POWER ] = coeff[PSC_POWER ].R; -if (config &LM25066_DEV_SETUP_CL ) { - info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; - info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; - info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; - info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; - }else { + +/* +* Assume chip is in low current limit/gain mode by default unless +* the register override is asserted AND indicates high limit mode. +*/ +if ((config &LM25066_DEV_SETUP_CL_CONFIG ) && + (config &LM25066_DEV_SETUP_CL )) { info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].m; info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].b; info->m[PSC_POWER ] = coeff[PSC_POWER ].m; info->b[PSC_POWER ] = coeff[PSC_POWER ].b; + }else { + info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; + info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; + info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; + info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; }return pmbus_do_probe (client, id, info);
Quoted 14 lines · Guenter Roeck
Quoted 11 lines · Robert Lippert
There seems to be no way to detect the value of the CL/GAIN pin from the device using PMBus.
Low current mode seems to be recommended (from LM5066 datasheet) for improved current reporting accuracy in a typical design.
Assume the device is in low current mode unless the register override bit is set and indicates high current limit mode.
Signed-off-by Robert Lippert <[email protected]>
Quoted 8 lines
Applied (silently fixed continuation line alignment).
Looking at the most recent version of various datasheets for the chips supported
by the driver, looks like TI changed the coefficients (again). Not the first time.
Time to go through all chips and update coefficients. Might be worthwhile to do that at least for the chips(s) which are of interest for you.
This patch also needs commit "hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)" to maintain the existing reporting behavior for LM5066 devices atleast.
(as now it will be pulling from the _L values by default)
I spot checked the coeff values for lm25056 with the online datasheet and they also seem to be swapped although it is a little hard to be sure since the meaning of GAIN input seems to be opposite of CL input:
GAIN=0 is low current and GAIN=1 is high current.
-Rob
Quoted 52 lines
Thanks,
GuenterQuoted 46 lines
--- drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)drivers/hwmon/pmbus/lm25066.c +4
-6@@ -43,6 +43,7 @@ enum chips { lm25056, lm25063, lm25066, lm5064, lm5066, lm5066i };#define LM25066_READ_AVG_IIN 0xde#define LM25066_READ_AVG_PIN 0xdf +#define LM25066_DEV_SETUP_CL_CONFIG BIT(2) /* Use pin or SMBus values*/ #define LM25066_DEV_SETUP_CL BIT(4)/* Current limit */ /* LM25056 only */ @@ -526,16 +527,22 @@ static int lm25066_probe(struct i2c_client *client, info->R[PSC_VOLTAGE_OUT ] = coeff[PSC_VOLTAGE_OUT ].R; info->R[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].R; info->R[PSC_POWER ] = coeff[PSC_POWER ].R; -if (config &LM25066_DEV_SETUP_CL ) { - info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; - info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; - info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; - info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; - }else { + +/* +* Assume chip is in low current limit/gain mode by default unless +* the register override is asserted AND indicates high limit mode.
drivers/hwmon/pmbus/lm25066.c +8
-0+*/ +if ((config &LM25066_DEV_SETUP_CL_CONFIG ) && + (config &LM25066_DEV_SETUP_CL )) { info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].m; info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].b; info->m[PSC_POWER ] = coeff[PSC_POWER ].m; info->b[PSC_POWER ] = coeff[PSC_POWER ].b; + }else { + info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; + info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; + info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; + info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; }return pmbus_do_probe (client, id, info);
Guenter Roeck
maintainerQuoted 29 lines · Rob Lippert
Quoted 14 lines · Guenter Roeck
Quoted 11 lines · Robert Lippert
There seems to be no way to detect the value of the CL/GAIN pin from the device using PMBus.
Low current mode seems to be recommended (from LM5066 datasheet) for improved current reporting accuracy in a typical design.
Assume the device is in low current mode unless the register override bit is set and indicates high current limit mode.
Signed-off-by Robert Lippert <[email protected]>Quoted 8 lines
Applied (silently fixed continuation line alignment).
Looking at the most recent version of various datasheets for the chips supported
by the driver, looks like TI changed the coefficients (again). Not the first time.
Time to go through all chips and update coefficients. Might be worthwhile to do that at least for the chips(s) which are of interest for you.This patch also needs commit "hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)" to maintain the existing reporting behavior for LM5066 devices atleast.
(as now it will be pulling from the _L values by default)
Quoted 5 lines
I spot checked the coeff values for lm25056 with the online datasheet and they also seem to be swapped although it is a little hard to be sure since the meaning of GAIN input seems to be opposite of CL input:
GAIN=0 is low current and GAIN=1 is high current.
Guess I'll have to test that with real HW first after all before sending it all upstream. I don't want to apply "hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)" as-is because the problem affects all chips supported by the driver (or at least several), not just one.
Guenter
Quoted 54 lines
-Rob
Quoted 52 lines
Thanks,
GuenterQuoted 46 lines
--- drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)drivers/hwmon/pmbus/lm25066.c +4
-6@@ -43,6 +43,7 @@ enum chips { lm25056, lm25063, lm25066, lm5064, lm5066, lm5066i };#define LM25066_READ_AVG_IIN 0xde#define LM25066_READ_AVG_PIN 0xdf +#define LM25066_DEV_SETUP_CL_CONFIG BIT(2) /* Use pin or SMBus values*/ #define LM25066_DEV_SETUP_CL BIT(4)/* Current limit */ /* LM25056 only */ @@ -526,16 +527,22 @@ static int lm25066_probe(struct i2c_client *client, info->R[PSC_VOLTAGE_OUT ] = coeff[PSC_VOLTAGE_OUT ].R; info->R[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].R; info->R[PSC_POWER ] = coeff[PSC_POWER ].R; -if (config &LM25066_DEV_SETUP_CL ) { - info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; - info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; - info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; - info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; - }else { + +/* +* Assume chip is in low current limit/gain mode by default unless +* the register override is asserted AND indicates high limit mode.
drivers/hwmon/pmbus/lm25066.c +8
-0+*/ +if ((config &LM25066_DEV_SETUP_CL_CONFIG ) && + (config &LM25066_DEV_SETUP_CL )) { info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].m; info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].b; info->m[PSC_POWER ] = coeff[PSC_POWER ].m; info->b[PSC_POWER ] = coeff[PSC_POWER ].b; + }else { + info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; + info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; + info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; + info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; }return pmbus_do_probe (client, id, info);
Guenter Roeck
maintainerQuoted 77 lines · Rob Lippert
Quoted 14 lines · Guenter Roeck
Quoted 11 lines · Robert Lippert
There seems to be no way to detect the value of the CL/GAIN pin from the device using PMBus.
Low current mode seems to be recommended (from LM5066 datasheet) for improved current reporting accuracy in a typical design.
Assume the device is in low current mode unless the register override bit is set and indicates high current limit mode.
Signed-off-by Robert Lippert <[email protected]>Quoted 8 lines
Applied (silently fixed continuation line alignment).
Looking at the most recent version of various datasheets for the chips supported
by the driver, looks like TI changed the coefficients (again). Not the first time.
Time to go through all chips and update coefficients. Might be worthwhile to do that at least for the chips(s) which are of interest for you.This patch also needs commit "hwmon: (pmbus/lm25066) Swap low/high current coefficients for LM5066(i)" to maintain the existing reporting behavior for LM5066 devices atleast.
(as now it will be pulling from the _L values by default)I spot checked the coeff values for lm25056 with the online datasheet and they also seem to be swapped although it is a little hard to be sure since the meaning of GAIN input seems to be opposite of CL input:
GAIN=0 is low current and GAIN=1 is high current.-Rob
Quoted 41 lines
Thanks,
GuenterQuoted 36 lines
--- drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)drivers/hwmon/pmbus/lm25066.c +4
-6@@ -43,6 +43,7 @@ enum chips { lm25056, lm25063, lm25066, lm5064, lm5066, lm5066i };#define LM25066_READ_AVG_IIN 0xde#define LM25066_READ_AVG_PIN 0xdf +#define LM25066_DEV_SETUP_CL_CONFIG BIT(2) /* Use pin or SMBus values*/ #define LM25066_DEV_SETUP_CL BIT(4)/* Current limit */ /* LM25056 only */ @@ -526,16 +527,22 @@ static int lm25066_probe(struct i2c_client *client, info->R[PSC_VOLTAGE_OUT ] = coeff[PSC_VOLTAGE_OUT ].R; info->R[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].R; info->R[PSC_POWER ] = coeff[PSC_POWER ].R; -if (config &LM25066_DEV_SETUP_CL ) { - info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; - info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; - info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; - info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; - }else { + +/* +* Assume chip is in low current limit/gain mode by default unless +* the register override is asserted AND indicates high limit mode.
drivers/hwmon/pmbus/lm25066.c +3
-0+*/ +if ((config &LM25066_DEV_SETUP_CL_CONFIG ) && + (config &LM25066_DEV_SETUP_CL )) { info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].m; info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN ].b;
Quoted 8 lines
Quoted 8 lines
drivers/hwmon/pmbus/lm25066.c static int lm25066_probe(struct i2c_client *client, · line 546
Quoted 8 lines
info->m[PSC_POWER] = coeff[PSC_POWER].m; info->b[PSC_POWER] = coeff[PSC_POWER].b;drivers/hwmon/pmbus/lm25066.c +5
-0+ }else { + info->m[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].m; + info->b[PSC_CURRENT_IN ] = coeff[PSC_CURRENT_IN_L ].b; + info->m[PSC_POWER ] = coeff[PSC_POWER_L ].m; + info->b[PSC_POWER ] = coeff[PSC_POWER_L ].b; }
After looking into it more closely, turns out this is wrong.
The _L defines were supposed to be used if bit 4 is set, not if it is unset. It wasn't supposed to mean "low" but "CL/GAIN=1" (ok, looking at it with 20/20 hindsight, that seems to be odd, but it is consistent with the code).
So the code should be something like
if ((config & LM25066_DEV_SETUP_CL_CONFIG) &&
(config & LM25066_DEV_SETUP_CL)) {
info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].m;
info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].b;
info->m[PSC_POWER] = coeff[PSC_POWER_L].m;
info->b[PSC_POWER] = coeff[PSC_POWER_L].b;
} else {
info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].m;
info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].b;
info->m[PSC_POWER] = coeff[PSC_POWER].m;
info->b[PSC_POWER] = coeff[PSC_POWER].b;
}Only that by itself is insufficient because not all chips support bit 2 (at least lm25056 doesn't). We can work around that with something like
if (data->id == lm25056) config |= LM25066_DEV_SETUP_CL_CONFIG;
Can you do that ?
[ LM25063 doesn't have the bit either, but it looks like TI acquired
National before LM25063 shipped, and the chip was never actually released. Dropping that will be a separate patch. ]
Thanks,
Guenter