Re: [PATCH v5 3/4] pmbus (core): Add virtual page config bit

[Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

On Sun, 2017-11-19 at 08:49 -0800, Guenter Roeck wrote:
> On 11/17/2017 07:26 PM, Andrew Jeffery wrote:
> > Some circumstances call for virtual pages, to expose multiple values
> > packed into an extended PMBus register in a manner non-compliant with
> > the PMBus standard. An example of this is the Maxim MAX31785 controller, which
> > extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
> > tach readings for both rotors of a dual rotor fan. This extended register
> > contains two word-sized values, one reporting the rate of the fastest rotor,
> > the other the rate of the slowest. The concept of virtual pages aids this
> > situation by mapping the page number onto the value to be selected from the
> > vectored result.
> > 
> > We should not try to set virtual pages on the device as such a page explicitly
> > doesn't exist; add a flag so we can avoid doing so.
> > 
> > Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
> > ---
> >   drivers/hwmon/pmbus/pmbus.h      |  2 ++
> >   drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++---
> >   2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index b54d7604d3ef..d39d506aa63e 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -372,6 +372,8 @@ enum pmbus_sensor_classes {
> >   #define PMBUS_HAVE_PWM12	BIT(20)
> >   #define PMBUS_HAVE_PWM34	BIT(21)
> >   
> > +#define PMBUS_PAGE_VIRTUAL	BIT(31)
> > +
> >   enum pmbus_data_format { linear = 0, direct, vid };
> >   enum vrm_version { vr11 = 0, vr12, vr13 };
> >   
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 631db88526b6..ba97230fcde7 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, int page)
> >   	int rv = 0;
> >   	int newpage;
> >   
> > -	if (page >= 0 && page != data->currpage) {
> > +	if (page < 0 || page == data->currpage)
> > +		return 0;
> > +
> > +	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
> >   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> >   		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> >   		if (newpage != page)
> >   			rv = -EIO;
> 
> This should probably be something like
> 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> 		if (rv < 0)
> 			return rv;
> 		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> 		if (newpage < 0)
> 			return newpage;
> 		if (newpage != page)
> 			return -EIO;
> 
> We can actually drop 'newpage' and just use 'rv'.
> 
> > -		else
> > -			data->currpage = page;
> >   	}
> > +
> > +	data->currpage = page;
> > +
> 
> ... otherwise currpage is set even on error.

Thanks for catching that, clearly I needed to pay more attention
resolving the conflicts when rebasing on hwmon-next. I'll address all
your points.

Cheers,

Andrew

> 
> >   	return rv;
> 
> this can then be
> 	return 0;
> 
> >   }
> >   EXPORT_SYMBOL_GPL(pmbus_set_page);
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]