linux-kernel - Re: [PATCH] hwmon: (pmbus/lm25066) Default coefficients for low current limit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171128220043.GF10144@roeck-us.net>
Date:   Tue, 28 Nov 2017 14:00:44 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Rob Lippert <rlippert@...gle.com>
Cc:     Robert Lippert <roblip@...il.com>, linux-hwmon@...r.kernel.org,
        jdelvare@...e.com, linux-kernel@...r.kernel.org,
        Xo Wang <xow@...gle.com>
Subject: Re: [PATCH] hwmon: (pmbus/lm25066) Default coefficients for low
 current limit

On Tue, Nov 28, 2017 at 10:39:10AM -0800, Rob Lippert wrote:
> On Mon, Nov 27, 2017 at 8:26 PM, Guenter Roeck <linux@...ck-us.net> wrote:
> > On 11/27/2017 04:28 PM, Robert Lippert wrote:
> >>
> >> 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 <rlippert@...gle.com>
> >
> >
> > 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.
> 
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

> -Rob
> 
> >
> > Thanks,
> > Guenter
> >
> >
> >> ---
> >>   drivers/hwmon/pmbus/lm25066.c | 19 +++++++++++++------
> >>   1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> >> index aa052f4449a9..f6ae7b4cf705 100644
> >> --- a/drivers/hwmon/pmbus/lm25066.c
> >> +++ b/drivers/hwmon/pmbus/lm25066.c
> >> @@ -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);
> >>
> >