Re: [PATCH 1/2] hwmon: (pmbus/max16601) Determine and use number of populated phases

[Alex Qiu <xqiu@google.com>]

Nit: Would it be better to specify what P and N mean upfront in the doc?

- Alex Qiu

On Mon, Jan 25, 2021 at 10:53 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> The MAX16601 can report the number of populated phases. Use this
> information to only create sysfs attributes for populated phases.
>
> Cc: Alex Qiu <xqiu@google.com>
> Cc: Ugur Usug <Ugur.Usug@maximintegrated.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Alex Qiu <xqiu@google.com>
> ---
>  Documentation/hwmon/max16601.rst | 98 ++++++++++----------------------
>  drivers/hwmon/pmbus/max16601.c   | 17 +++++-
>  2 files changed, 45 insertions(+), 70 deletions(-)
>
> diff --git a/Documentation/hwmon/max16601.rst b/Documentation/hwmon/max16601.rst
> index 346e74674c51..93d25dfa028e 100644
> --- a/Documentation/hwmon/max16601.rst
> +++ b/Documentation/hwmon/max16601.rst
> @@ -60,75 +60,35 @@ curr1_input         VCORE input current, derived from duty cycle and output
>  curr1_max              Maximum input current.
>  curr1_max_alarm                Current high alarm.
>
> -curr2_label            "iin1.0"
> -curr2_input            VCORE phase 0 input current.
> -
> -curr3_label            "iin1.1"
> -curr3_input            VCORE phase 1 input current.
> -
> -curr4_label            "iin1.2"
> -curr4_input            VCORE phase 2 input current.
> -
> -curr5_label            "iin1.3"
> -curr5_input            VCORE phase 3 input current.
> -
> -curr6_label            "iin1.4"
> -curr6_input            VCORE phase 4 input current.
> -
> -curr7_label            "iin1.5"
> -curr7_input            VCORE phase 5 input current.
> -
> -curr8_label            "iin1.6"
> -curr8_input            VCORE phase 6 input current.
> -
> -curr9_label            "iin1.7"
> -curr9_input            VCORE phase 7 input current.
> -
> -curr10_label           "iin2"
> -curr10_input           VCORE input current, derived from sensor element.
> -
> -curr11_label           "iin3"
> -curr11_input           VSA input current.
> -
> -curr12_label           "iout1"
> -curr12_input           VCORE output current.
> -curr12_crit            Critical output current.
> -curr12_crit_alarm      Output current critical alarm.
> -curr12_max             Maximum output current.
> -curr12_max_alarm       Output current high alarm.
> -
> -curr13_label           "iout1.0"
> -curr13_input           VCORE phase 0 output current.
> -
> -curr14_label           "iout1.1"
> -curr14_input           VCORE phase 1 output current.
> -
> -curr15_label           "iout1.2"
> -curr15_input           VCORE phase 2 output current.
> -
> -curr16_label           "iout1.3"
> -curr16_input           VCORE phase 3 output current.
> -
> -curr17_label           "iout1.4"
> -curr17_input           VCORE phase 4 output current.
> -
> -curr18_label           "iout1.5"
> -curr18_input           VCORE phase 5 output current.
> -
> -curr19_label           "iout1.6"
> -curr19_input           VCORE phase 6 output current.
> -
> -curr20_label           "iout1.7"
> -curr20_input           VCORE phase 7 output current.
> -
> -curr21_label           "iout3"
> -curr21_input           VSA output current.
> -curr21_highest         Historical maximum VSA output current.
> -curr21_reset_history   Write any value to reset curr21_highest.
> -curr21_crit            Critical output current.
> -curr21_crit_alarm      Output current critical alarm.
> -curr21_max             Maximum output current.
> -curr21_max_alarm       Output current high alarm.
> +curr[P+2]_label                "iin1.P"
> +curr[P+2]_input                VCORE phase P input current.
> +
> +curr[N+2]_label                "iin2"
> +curr[N+2]_input                VCORE input current, derived from sensor element.
> +                       'N' is the number of enabled/populated phases.
> +
> +curr[N+3]_label                "iin3"
> +curr[N+3]_input                VSA input current.
> +
> +curr[N+4]_label                "iout1"
> +curr[N+4]_input                VCORE output current.
> +curr[N+4]_crit         Critical output current.
> +curr[N+4]_crit_alarm   Output current critical alarm.
> +curr[N+4]_max          Maximum output current.
> +curr[N+4]_max_alarm    Output current high alarm.
> +
> +curr[N+P+5]_label      "iout1.P"
> +curr[N+P+5]_input      VCORE phase P output current.
> +
> +curr[2*N+5]_label      "iout3"
> +curr[2*N+5]_input      VSA output current.
> +curr[2*N+5]_highest    Historical maximum VSA output current.
> +curr[2*N+5]_reset_history
> +                       Write any value to reset curr21_highest.
> +curr[2*N+5]_crit       Critical output current.
> +curr[2*N+5]_crit_alarm Output current critical alarm.
> +curr[2*N+5]_max                Maximum output current.
> +curr[2*N+5]_max_alarm  Output current high alarm.
>
>  power1_label           "pin1"
>  power1_input           Input power, derived from duty cycle and output current.
> diff --git a/drivers/hwmon/pmbus/max16601.c b/drivers/hwmon/pmbus/max16601.c
> index a960b86e72d2..efe6da3bc8d0 100644
> --- a/drivers/hwmon/pmbus/max16601.c
> +++ b/drivers/hwmon/pmbus/max16601.c
> @@ -31,6 +31,7 @@
>
>  #include "pmbus.h"
>
> +#define REG_DEFAULT_NUM_POP    0xc4
>  #define REG_SETPT_DVID         0xd1
>  #define  DAC_10MV_MODE         BIT(4)
>  #define REG_IOUT_AVG_PK                0xee
> @@ -40,6 +41,8 @@
>  #define  CORE_RAIL_INDICATOR   BIT(7)
>  #define REG_PHASE_REPORTING    0xf4
>
> +#define MAX16601_NUM_PHASES    8
> +
>  struct max16601_data {
>         struct pmbus_driver_info info;
>         struct i2c_client *vsa;
> @@ -195,6 +198,18 @@ static int max16601_identify(struct i2c_client *client,
>         else
>                 info->vrm_version[0] = vr12;
>
> +       reg = i2c_smbus_read_byte_data(client, REG_DEFAULT_NUM_POP);
> +       if (reg < 0)
> +               return reg;
> +
> +       /*
> +        * If REG_DEFAULT_NUM_POP returns 0, we don't know how many phases
> +        * are populated. Stick with the default in that case.
> +        */
> +       reg &= 0x0f;
> +       if (reg && reg <= MAX16601_NUM_PHASES)
> +               info->phases[0] = reg;
> +
>         return 0;
>  }
>
> @@ -216,7 +231,7 @@ static struct pmbus_driver_info max16601_info = {
>         .func[2] = PMBUS_HAVE_IIN | PMBUS_HAVE_STATUS_INPUT |
>                 PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>                 PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_PAGE_VIRTUAL,
> -       .phases[0] = 8,
> +       .phases[0] = MAX16601_NUM_PHASES,
>         .pfunc[0] = PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP,
>         .pfunc[1] = PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT,
>         .pfunc[2] = PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP,
> --
> 2.17.1
>