Hi Guenter,
LGTM; minor nits:
On Thu, May 7, 2020 at 10:44 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> 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
