| Message ID | 20171122220728.17861-1-rlippert@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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] = {
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(-)