Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase'
@ 2020-05-07 17:44 Guenter Roeck
  2020-05-07 20:19 ` Alex Qiu
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-05-07 17:44 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Alex Qiu

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 */
 };
 
 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase'
  2020-05-07 17:44 [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase' Guenter Roeck
@ 2020-05-07 20:19 ` Alex Qiu
  2020-05-07 20:55   ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Alex Qiu @ 2020-05-07 20:19 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Hardware Monitoring, Jean Delvare

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase'
  2020-05-07 20:19 ` Alex Qiu
@ 2020-05-07 20:55   ` Guenter Roeck
  0 siblings, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2020-05-07 20:55 UTC (permalink / raw)
  To: Alex Qiu; +Cc: Hardware Monitoring, Jean Delvare

On Thu, May 07, 2020 at 01:19:58PM -0700, Alex Qiu wrote:
> 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?

The driver uses hardcoded 0xff in various places. We could replace it
with defines, but that should be a separate patch.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 17:44 [PATCH] hwmon: (pmbus) Improve initialization of 'currpage' and 'currphase' Guenter Roeck
2020-05-07 20:19 ` Alex Qiu
2020-05-07 20:55   ` Guenter Roeck

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git