Re: [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase'

[Alex Qiu <xqiu@google.com>]

Hi Guenter,

LGTM; minor nits:

On Thu, May 7, 2020 at 10:44 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> The 'currpage' and 'currphase' variables in struct pmbus_data are used by
> the PMBus core to determine if the phase or page value has changed. Both
> are initialized with values which are never expected to be set in the code
> to ensure that the first page/phase write operation is actually performed.
>
> This is not well explained and occasionally causes confusion. Change the
> type of both variables to s16 and initialize with -1 to ensure that the
> initial value never matches a requested value, and clarify that this
> value means "unknown/unset".
>
> Cc: Alex Qiu <xqiu@google.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 8d321bf7d15b..a420877ba533 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -109,8 +109,8 @@ struct pmbus_data {
>         bool has_status_word;           /* device uses STATUS_WORD register */
>         int (*read_status)(struct i2c_client *client, int page);
>
> -       u8 currpage;
> -       u8 currphase;   /* current phase, 0xff for all */
> +       s16 currpage;   /* current page, -1 for unknown/unset */
> +       s16 currphase;  /* current phase, 0xff for all, -1 for unknown/unset */
I'm not sure if assigning macros for -1 and 0xff would be a good idea?
>  };
>
>  struct pmbus_debugfs_entry {
> @@ -2529,8 +2529,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>         if (pdata)
>                 data->flags = pdata->flags;
>         data->info = info;
> -       data->currpage = 0xff;
> -       data->currphase = 0xfe;
> +       data->currpage = -1;
> +       data->currphase = -1;
>
>         ret = pmbus_init_common(client, data, info);
>         if (ret < 0)
> --
> 2.17.1
>

Thx!

- Alex Qiu