lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 29 Jun 2010 14:40:59 +0100
From:	Jonathan Cameron <kernel@...23.retrosnub.co.uk>
To:	Guenter Roeck <guenter.roeck@...csson.com>
CC:	Jean Delvare <khali@...ux-fr.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Ira W. Snyder" <iws@...o.caltech.edu>,
	"Darrick J. Wong" <djwong@...ibm.com>,
	"Ben Dooks (embedded platforms)" <ben-linux@...ff.org>,
	Hans de Goede <hdegoede@...hat.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Crane Cai <crane.cai@....com>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Ralf Baechle <ralf@...ux-mips.org>, lm-sensors@...sensors.org,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH/RFC 2/4] hwmon: PMBus device driver

On 06/28/10 22:56, Guenter Roeck wrote:
Hi Guenter,

A few questions and inital comments...
Ouch the pmbus spec is tricky to follow!

> Signed-off-by: Guenter Roeck <guenter.roeck@...csson.com>
> ---
>  drivers/hwmon/Kconfig  |   12 +
>  drivers/hwmon/Makefile |    1 +
>  drivers/hwmon/pmbus.c  | 1227 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/pmbus.h  |  209 ++++++++
>  4 files changed, 1449 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/pmbus.c
>  create mode 100644 drivers/hwmon/pmbus.h
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e19cf8e..8d53cf7 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -702,6 +702,18 @@ config SENSORS_PCF8591
>  	  These devices are hard to detect and rarely found on mainstream
>  	  hardware.  If unsure, say N.
>  
> +config SENSORS_PMBUS
> +	tristate "PMBus devices"
> +	depends on I2C && EXPERIMENTAL
> +	default n
> +	help
> +	  If you say yes here you get hardware monitoring support for various
> +	  PMBus devices, including but not limited to BMR45x, LTC2978, MAX16064,
> +	  MAX8688, and UCD92xx.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called pmbus.
> +
>  config SENSORS_SHT15
>  	tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
>  	depends on GENERIC_GPIO
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 2138ceb..88b043e 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> +obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
>  obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
> diff --git a/drivers/hwmon/pmbus.c b/drivers/hwmon/pmbus.c
> new file mode 100644
> index 0000000..418ee2c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus.c
> @@ -0,0 +1,1227 @@
> +/*
> + * Hardware monitoring driver for PMBus devices
> + *
> + * Copyright (C) 2010 Ericsson AB.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/delay.h>
> +#include "pmbus.h"
> +
> +#define PMBUS_SENSORS	64
> +#define PMBUS_BOOLEANS	64
> +#define PMBUS_LABELS	32
> +#define PMBUS_NUM_ATTR	(PMBUS_BOOLEANS + PMBUS_SENSORS + PMBUS_LABELS)
> +#define PMBUS_PAGES	8
> +
> +static int pages;
> +module_param(pages, int, 0);
> +MODULE_PARM_DESC(pages, "Number of sensor pages");
Why is this a module parameter?  If you do it like this
you will be overriding page count for all devices in the system.
Is that the intent?

> +
> +enum chips { ltc2978, max16064, max8688, pmbus, ucd9220, ucd9240 };
> +
> +enum pmbus_sensor_classes {
> +	PSC_VOLTAGE = 0,
> +	PSC_TEMPERATURE,
> +	PSC_CURRENT,
> +	PSC_POWER,

Perhaps name this PSC_MAX_LABEL or similar to indicate it is just here to
allow the number of possible classes to be established.
> +	SENSOR_CLASSES
> +};
> +
> +struct pmbus_config {
> +	int pages;		/* Total number of pages (= output sensors) */
> +	bool direct;		/* true if device uses direct data format */
> +	/*
> +	 * Support one set of coefficients for each sensor type
> +	 * Used for chips providing data in direct mode.
> +	 */
> +	int m[SENSOR_CLASSES];	/* mantissa for direct data format */
> +	int b[SENSOR_CLASSES];	/* offset */
> +	int R[SENSOR_CLASSES];	/* exponent */
> +};
> +
Can we name these something to do with PB to cut down on chance of name
clashes.
> +#define HAVE_STATUS_VOUT	(1<<0)
> +#define HAVE_STATUS_IOUT	(1<<1)
> +#define HAVE_STATUS_INPUT	(1<<2)
> +#define HAVE_STATUS_TEMP	(1<<3)
> +
> +#define PB_STATUS_BASE		0
> +#define PB_STATUS_VOUT_BASE	(PB_STATUS_BASE + PMBUS_PAGES)
> +#define PB_STATUS_IOUT_BASE	(PB_STATUS_VOUT_BASE + PMBUS_PAGES)
> +#define PB_STATUS_INPUT_BASE	(PB_STATUS_IOUT_BASE + PMBUS_PAGES)
> +#define PB_STATUS_TEMP_BASE	(PB_STATUS_INPUT_BASE + 1)
> +
> +struct pmbus_sensor {
> +	char name[I2C_NAME_SIZE];	/* sysfs sensor name */
> +	u8 page;			/* page number */
> +	u8 reg;				/* register */
> +	enum pmbus_sensor_classes class;/* sensor class */
> +	bool update;			/* runtime sensor update needed */
> +};
> +
What are the next two for?
> +struct pmbus_boolean {
> +	char name[I2C_NAME_SIZE];	/* sysfs boolean name */
> +};
> +
> +struct pmbus_label {
> +	char name[I2C_NAME_SIZE];	/* sysfs label name */
> +	char label[I2C_NAME_SIZE];	/* label */
> +};
> +
> +struct pmbus_data {
> +	struct device *hwmon_dev;
> +	enum chips type;
> +
> +	bool direct;
> +	int pages;
> +
> +	/* Coefficients, for chips providing data in direct mode  */
> +	int m[SENSOR_CLASSES];	/* mantissa for direct data format */
> +	int b[SENSOR_CLASSES];	/* offset */
> +	int R[SENSOR_CLASSES];	/* exponent */
> +
Any chance of getting rid of one of the following? The 'attributes'
array is must an array of pointers to elements within sensor_devices_attributes.
It would prevent easy usage of sysfs_create_group so maybe it is better
to just have the redunancy here.

> +	struct sensor_device_attribute
> +	  sensor_device_attributes[PMBUS_NUM_ATTR];
> +	struct attribute *attributes[PMBUS_NUM_ATTR];
> +	int num_attributes;
> +	struct attribute_group group;
> +
> +	int num_sensors;
> +	struct pmbus_sensor sensors[PMBUS_SENSORS];
> +	int num_booleans;
> +	struct pmbus_boolean booleans[PMBUS_BOOLEANS];
> +	int num_labels;
Can you comment on what these labels are for?
> +	struct pmbus_label labels[PMBUS_LABELS];
> +
> +	struct mutex update_lock;
> +	bool valid;
> +	unsigned long last_updated;	/* in jiffies */
> +	u8 status_bits;

What is the theoretical maximum number of pages?
Is it the 8 you have above?  A quick look at the rev 1 spec
suggests 0-1F so 32.  That is going to make for some big
arrays of fixed size whatever the chip in question needs.

Could you do some further grouping into suitable structures?
Perhaps put sensor_data in the pmbus_sensor struct?
Can status also be moved into such a struct?  I'm a little
unclear how it is assocated with the various sensors inputs?

After such grouping, perhaps the allocation can be made dynamic.
> +	/*
> +	 * status, status_vout, status_iout are paged.
> +	 * status_input and status_temp are unpaged.
> +	 */
> +	u8 status[PMBUS_PAGES * 3 + 2];
> +	u16 sensor_data[PMBUS_SENSORS];
> +
> +	u8 currpage;
> +};
> +

Do these need to be complete or do only some devices support direct mode
reads?
> +static const struct pmbus_config pmbus_config[] = {
> +	[pmbus] = {
> +		.pages = 1,
> +		},
> +	[ltc2978] = {
> +		.pages = 8,
> +		},
> +	[max16064] = {
> +		.pages = 4,
> +		.direct = true,
> +		.m[PSC_VOLTAGE] = 19995,
> +		.b[PSC_VOLTAGE] = 0,
> +		.R[PSC_VOLTAGE] = -1,
> +		.m[PSC_TEMPERATURE] = -7612,
> +		.b[PSC_TEMPERATURE] = 335,
> +		.R[PSC_TEMPERATURE] = -3,
> +		},
> +	[max8688] = {
> +		.pages = 1,
> +		.direct = true,
> +		.m[PSC_VOLTAGE] = 19995,
> +		.b[PSC_VOLTAGE] = 0,
> +		.R[PSC_VOLTAGE] = -1,
> +		.m[PSC_TEMPERATURE] = -7612,
> +		.b[PSC_TEMPERATURE] = 335,
> +		.R[PSC_TEMPERATURE] = -3,
> +		.m[PSC_CURRENT] = 23109,
> +		.b[PSC_CURRENT] = 0,
> +		.R[PSC_CURRENT] = -2,
> +		},
> +	[ucd9220] = {
> +		.pages = 2,
> +		},
> +	[ucd9240] = {
> +		.pages = 4,
> +		},
> +};
> +
> +static int pmbus_set_page(struct i2c_client *client, u8 page)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	int rv = 0;
> +
> +	if (page != data->currpage) {
> +		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +		data->currpage = page;
> +	}
> +	return rv;
> +}
> +
> +static int pmbus_write_byte(struct i2c_client *client, u8 page, u8 value)
> +{
> +	if (pmbus_set_page(client, page) < 0)
> +		return -1;
> +
> +	return i2c_smbus_write_byte(client, value);
> +}
> +
> +static int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg,
> +				 u16 word)
> +{
> +	if (pmbus_set_page(client, page) < 0)
> +		return -1;
> +
> +	return i2c_smbus_write_word_data(client, reg, word);
> +}
> +
> +static int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)
> +{
> +	if (pmbus_set_page(client, page) < 0)
> +		return -1;
> +
> +	return i2c_smbus_read_word_data(client, reg);
> +}
> +
> +static int pmbus_read_byte_data(struct i2c_client *client, u8 page, u8 reg)
> +{
> +	if (pmbus_set_page(client, page) < 0)
> +		return -1;
> +
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static void pmbus_clear_faults(struct i2c_client *client)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	for (i = 0; i < data->pages; i++)
> +		pmbus_write_byte(client, i, PMBUS_CLEAR_FAULTS);
> +}
> +
> +static bool pmbus_check_byte_register(struct i2c_client *client, int page,
> +				      int reg)
> +{
Roll everything into one line.

return (pmbus_read_byte_data(client, page, reg) >= 0);
> +	int rv;
> +
> +	rv = pmbus_read_byte_data(client, page, reg);
> +	return (rv >= 0);
> +}
> +
> +static bool pmbus_check_word_register(struct i2c_client *client, int page,
> +				      int reg)
> +{
Another one liner.
> +	int rv;
> +
> +	rv = pmbus_read_word_data(client, page, reg);
> +	return (rv >= 0);
> +}
> +
> +static struct pmbus_data *pmbus_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	struct pmbus_data *ret = data;
> +
> +	mutex_lock(&data->update_lock);
> +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> +		int i, val;
> +
> +		for (i = 0; i < data->pages; i++) {
> +			val = pmbus_read_byte_data(client, i,
> +						   PMBUS_STATUS_BYTE);
> +			if (val < 0) {
> +				ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			data->status[PB_STATUS_BASE + i] = val;
> +		}
> +		if (data->status_bits & HAVE_STATUS_VOUT)
> +			for (i = 0; i < data->pages; i++) {
> +				val = pmbus_read_byte_data(client, i,
> +							   PMBUS_STATUS_VOUT);
> +				if (val < 0) {
> +					ret = ERR_PTR(val);
> +					goto abort;
> +				}
> +				data->status[PB_STATUS_VOUT_BASE + i] = val;
> +			}
> +		if (data->status_bits & HAVE_STATUS_IOUT)
> +			for (i = 0; i < data->pages; i++) {
> +				val = pmbus_read_byte_data(client, i,
> +							   PMBUS_STATUS_IOUT);
> +				if (val < 0) {
> +					ret = ERR_PTR(val);
> +					goto abort;
> +				}
> +				data->status[PB_STATUS_IOUT_BASE + i] = val;
> +			}
> +		if (data->status_bits & HAVE_STATUS_INPUT) {
> +			val = pmbus_read_byte_data(client, 0,
> +						   PMBUS_STATUS_INPUT);
> +			if (val < 0) {
> +				ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			data->status[PB_STATUS_INPUT_BASE] = val;
> +		}
> +		if (data->status_bits & HAVE_STATUS_TEMP) {
> +			val = pmbus_read_byte_data(client, 0,
> +						   PMBUS_STATUS_TEMPERATURE);
> +			if (val < 0) {
> +				ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			data->status[PB_STATUS_TEMP_BASE] = val;
> +		}
> +		for (i = 0; i < data->num_sensors; i++) {
> +			struct pmbus_sensor *sensor = &data->sensors[i];
> +
> +			if (!data->valid || sensor->update) {
> +				val = pmbus_read_word_data(client, sensor->page,
> +							   sensor->reg);
> +				if (val < 0) {
> +					ret = ERR_PTR(val);
> +					goto abort;
> +				}
> +				data->sensor_data[i] = val;
> +			}
> +		}
> +		pmbus_clear_faults(client);
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +abort:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +/*
> + * Convert linear sensor values to milli- or micro-units
> + * depending on sensor type.
> + */
> +static long pmbus_reg2data_linear(struct pmbus_data *data,
> +				  enum pmbus_sensor_classes class, u16 adc)
> +{
> +	s16 exponent, mantissa;
> +	long val;
> +
> +	exponent = adc >> 11;
> +	mantissa = adc & 0x07ff;
> +
> +	if (exponent > 0x0f)
> +		exponent |= 0xffe0;	/* sign extend exponent */
> +	if (mantissa > 0x03ff)
> +		mantissa |= 0xf800;	/* sign extend mantissa */
> +
> +	/* scale result to milli-units */
> +	val = mantissa * 1000L;
> +
> +	/* scale result to micro-units for power sensors */
> +	if (class == PSC_POWER)
> +		val = val * 1000L;
> +
> +	if (exponent > 0)
> +		val <<= exponent;
Don't need this check...
val >>= 0 is same as val.
> +	else if (exponent < 0)
> +		val >>= -exponent;
> +
> +	return val;
> +}
> +
> +/*
> + * Convert direct sensor values to milli- or micro-units
> + * depending on sensor type.
> + */
> +static long pmbus_reg2data_direct(struct pmbus_data *data,
> +				  enum pmbus_sensor_classes class, u16 adc)
> +{
> +	long val = (s16) adc;
> +	long m, b, R;
> +
> +	m = data->m[class];
> +	b = data->b[class];
> +	R = data->R[class];
> +
> +	if (m == 0)
> +		return 0;
> +
> +	/* X = 1/m * (Y * 10^-R - b) */
> +	R = -R + 3;		/* scale result to milli-units */
> +	b *= 1000;		/* subtract milli-units */
> +
> +	/* scale result to micro-units for power sensors */
> +	if (class == PSC_POWER) {
> +		R += 3;
> +		b *= 1000;
> +	}
> +
> +	while (R > 0) {
> +		val *= 10;
> +		R--;
> +	}
> +	while (R < 0) {
> +		val = DIV_ROUND_CLOSEST(val, 10);
> +		R++;
> +	}
> +
> +	return (val - b) / m;
> +}
> +
> +static long pmbus_reg2data(struct pmbus_data *data,
> +			   enum pmbus_sensor_classes class, u16 adc)
> +{
> +	long val;
> +
> +	if (data->direct)
> +		val = pmbus_reg2data_direct(data, class, adc);
> +	else
> +		val = pmbus_reg2data_linear(data, class, adc);
> +
> +	return val;
> +}
> +
> +#define MAX_MANTISSA    (1023 * 1000)
> +#define MIN_MANTISSA    (511 * 1000)
> +
> +static u16 pmbus_data2reg_linear(struct pmbus_data *data,
> +				 enum pmbus_sensor_classes class, long val)
> +{
> +	s16 exponent = 0, mantissa = 0;
> +	bool negative = false;
> +
> +	/* simple case */
> +	if (val == 0)
> +		return 0;
> +
> +	if (val < 0) {
> +		negative = true;
> +		val = -val;
> +	}
> +
> +	/* Power is in uW. Convert to mW before converting. */
> +	if (class == PSC_POWER)
> +		val = DIV_ROUND_CLOSEST(val, 1000L);
> +
> +	/* Reduce large mantissa until it fits into 10 bit */
> +	while (val >= MAX_MANTISSA && exponent < 15) {
> +		exponent++;
> +		val >>= 1;
> +	}
> +	/* Increase small mantissa to improve precision */
> +	while (val < MIN_MANTISSA && exponent > -15) {
> +		exponent--;
> +		val <<= 1;
> +	}
> +
> +	/* Convert mantissa from milli-units to units */
> +	mantissa = DIV_ROUND_CLOSEST(val, 1000);
> +
> +	/* Ensure that resulting number is within range */
> +	if (mantissa > 0x3ff)
> +		mantissa = 0x3ff;
> +
> +	/* restore sign */
> +	if (negative)
> +		mantissa = -mantissa;
> +
> +	/* Convert to 5 bit exponent, 11 bit mantissa */
> +	return (mantissa & 0x7ff) | ((exponent << 11) & 0xf800);
> +}
> +
> +static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> +				 enum pmbus_sensor_classes class, long val)
> +{
> +	long m, b, R;
> +
> +	m = data->m[class];
> +	b = data->b[class];
> +	R = data->R[class];
> +
> +	/* Power is in uW. Adjust R. */
> +	if (class == PSC_POWER)
> +		R -= 3;
> +
> +	/* Calculate Y = (m * X + b) * 10^R */
> +	R -= 3;		/* Adjust R for data in milli-units */
> +	val *= m;
> +	val += b * 1000;
> +
> +	while (R > 0) {
> +		val *= 10;
> +		R--;
> +	}
> +	while (R < 0) {
> +		val = DIV_ROUND_CLOSEST(val, 10);
> +		R++;
> +	}
> +
> +	return val;
> +}
> +
> +static u16 pmbus_data2reg(struct pmbus_data *data,
> +			  enum pmbus_sensor_classes class, long val)
> +{
> +	u16 reg;
> +
> +	if (data->direct)
> +		reg = pmbus_data2reg_direct(data, class, val);
> +	else
> +		reg = pmbus_data2reg_linear(data, class, val);
> +
> +	return reg;
> +}
> +
> +/* Return converted value from given object */
> +static long pmbus_get_sensor(struct pmbus_data *data, int index)
> +{
> +	return pmbus_reg2data(data, data->sensors[index].class,
> +			      data->sensor_data[index]);
> +}
> +
> +/*
> + * Return boolean calculated from converted data.
> + * <index> defines a status register index and mask, and optionally
> + * two sensor indexes.
> + * The upper half-word references the two sensors,
> + * the lower half word references status register and mask.
> + * The function returns true if (status[reg] & mask) is true and,
> + * if specified, if v1 >= v2.
> + * To determine if an object exceeds upper limits, specify <v, limit>.
> + * To determine if an object exceeds lower limits, specify <limit, v>.
> + */
> +static long pmbus_get_boolean(struct pmbus_data *data, int index)
> +{
> +	u8 s1 = (index >> 24) & 0xff;
> +	u8 s2 = (index >> 16) & 0xff;
> +	u8 reg = (index >> 8) & 0xff;
> +	u8 mask = index & 0xff;
> +	int rv = 0;
> +	u8 regval = data->status[reg] & mask;
> +
> +	if (!s1 && !s2)
> +		rv = !!regval;
> +	else {
> +		int v1, v2;
> +
> +		v1 = pmbus_reg2data(data, data->sensors[s1].class,
> +				    data->sensor_data[s1]);
> +		v2 = pmbus_reg2data(data, data->sensors[s2].class,
> +				    data->sensor_data[s2]);
> +		rv = !!(regval && v1 >= v2);
> +	}
> +	return rv;
> +}
> +
> +#define PMBUS_SHOW_INT(what) \
> +	static ssize_t pmbus_show_##what(struct device *dev, \
> +					 struct device_attribute *da, \
> +					 char *buf) \
> +{ \
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da); \
> +	struct pmbus_data *data = pmbus_update_device(dev); \
> +	long val; \
> +	if (IS_ERR(data)) \
> +		return PTR_ERR(data); \
> +	val = pmbus_get_##what(data, attr->index); \
> +	return snprintf(buf, PAGE_SIZE, "%ld\n", val); \
> +}
> +
> +PMBUS_SHOW_INT(sensor);
> +PMBUS_SHOW_INT(boolean);
> +
> +static ssize_t pmbus_set_sensor(struct device *dev,
> +				struct device_attribute *devattr,
> +				const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	struct pmbus_sensor *sensor = &data->sensors[attr->index];
> +	ssize_t rv = count;
> +	long val = 0;
> +	int ret;
> +	u16 reg;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	reg = pmbus_data2reg(data, sensor->class, val);
> +	ret = pmbus_write_word_data(client, sensor->page, sensor->reg, reg);
> +	if (ret < 0)
> +		rv = ret;
> +	else
> +		data->sensor_data[attr->index] = reg;
> +	mutex_unlock(&data->update_lock);
> +	return rv;
> +}
> +
> +static ssize_t pmbus_show_label(struct device *dev,
> +				struct device_attribute *da,
> +				char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			data->labels[attr->index].label);
> +}
> +
> +#define PMBUS_ADD_ATTR(data, _name, _idx, _mode, _show, _set)		\
> +do {									\
> +	struct sensor_device_attribute *a				\
> +		= &data->sensor_device_attributes[data->num_attributes];\
> +	BUG_ON(data->num_attributes >=PMBUS_NUM_ATTR);			\
> +	a->dev_attr.attr.name = _name;					\
> +	a->dev_attr.attr.mode = _mode;					\
> +	a->dev_attr.show = _show;					\
> +	a->dev_attr.store = _set;					\
> +	a->index = _idx;						\
> +	data->attributes[data->num_attributes] = &a->dev_attr.attr;	\
> +	data->num_attributes++;						\
> +} while (0)
> +
> +#define PMBUS_ADD_GET_ATTR(data, _name, _type, _idx)			\
> +	PMBUS_ADD_ATTR(data, _name, _idx, S_IRUGO,			\
> +		       pmbus_show_##_type,  NULL)
> +
> +#define PMBUS_ADD_SET_ATTR(data, _name, _type, _idx)			\
> +	PMBUS_ADD_ATTR(data, _name, _idx, S_IWUSR | S_IRUGO,		\
> +		       pmbus_show_##_type, pmbus_set_##_type)
> +
> +static void pmbus_add_boolean(struct pmbus_data *data,
> +			      const char *name, const char *type, int seq,
> +			      int val)
> +{
> +	struct pmbus_boolean *boolean;
> +
> +	BUG_ON(data->num_booleans >= PMBUS_BOOLEANS);
> +
> +	boolean = &data->booleans[data->num_booleans];
> +
> +	snprintf(boolean->name, sizeof(boolean->name), "%s%d_%s",
> +		 name, seq, type);
> +	PMBUS_ADD_GET_ATTR(data, boolean->name, boolean, val);
> +	data->num_booleans++;
> +}
> +
> +static void pmbus_add_boolean_reg(struct pmbus_data *data,
> +				  const char *name, const char *type,
> +				  int seq, int reg, int bit)
> +{
> +	pmbus_add_boolean(data, name, type, seq,
> +			  (reg << 8) | bit);
> +}
> +
> +static void pmbus_add_boolean_cmp(struct pmbus_data *data,
> +				  const char *name, const char *type,
> +				  int seq, int i1, int i2, int reg,
> +				  int mask)
> +{
> +       pmbus_add_boolean(data, name, type, seq,
> +			 (i1 << 24) | (i2 << 16) | (reg << 8) | mask);
> +}
> +
> +static void pmbus_add_sensor(struct pmbus_data *data,
> +			     const char *name, const char *type, int seq,
> +			     int page, int reg, enum pmbus_sensor_classes class,
> +			     bool update)
> +{
> +	struct pmbus_sensor *sensor;
> +
> +	BUG_ON(data->num_sensors >= PMBUS_SENSORS);
> +
> +	sensor = &data->sensors[data->num_sensors];
> +	snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> +		 name, seq, type);
> +	sensor->page = page;
> +	sensor->reg = reg;
> +	sensor->class = class;
> +	sensor->update = update;
> +	if (update)
> +		PMBUS_ADD_GET_ATTR(data, sensor->name, sensor,
> +				   data->num_sensors);
> +	else
> +		PMBUS_ADD_SET_ATTR(data, sensor->name, sensor,
> +				   data->num_sensors);
> +	data->num_sensors++;
> +}
> +
> +static void pmbus_add_label(struct pmbus_data *data,
> +			    const char *name, int seq,
> +			    const char *lstring, int index)
> +{
> +	struct pmbus_label *label;
> +
> +	BUG_ON(data->num_labels >= PMBUS_LABELS);
> +
> +	label = &data->labels[data->num_labels];
> +	snprintf(label->name, sizeof(label->name), "%s%d_label",
> +		 name, seq);
> +	if (!index)
> +		strncpy(label->label, lstring, sizeof(label->label) - 1);
> +	else
> +		snprintf(label->label, sizeof(label->label), "%s%d", lstring,
> +			 index);
> +
> +	PMBUS_ADD_GET_ATTR(data, label->name, label, data->num_labels);
> +	data->num_labels++;
> +}
> +
> +static int pmbus_temp_sensors[] = {
> +	PMBUS_READ_TEMPERATURE_1,
> +	PMBUS_READ_TEMPERATURE_2,
> +	PMBUS_READ_TEMPERATURE_3
> +};
> +
> +static int pmbus_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct pmbus_data *data;
> +	int i, in_index, ret;
> +	int i0, i1;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA
> +				     | I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
This is an odd way of doing this.  Assign ret only if (!data) rather
than here.
> +	ret = -ENOMEM;
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		goto out;
> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->update_lock);
> +
Why not use the name rather than the id?
> +	dev_dbg(&client->dev, "PMBus device type %d", (int)id->driver_data);
> +
> +	data->type = id->driver_data;

Perhaps neater to put a pointer directly to pmbus_config in pmbus_data
structure and use that?
> +	data->direct = pmbus_config[data->type].direct;
> +	data->pages = pmbus_config[data->type].pages;
Please justify this parameter (may be elsewhere!) as I'm unconvinced it is
a good idea when you may well have multiple pmbus devices in a system?
> +	if (pages > 0)
> +		data->pages = pages;
> +
Again, if one just has a pointer to the pmbus_config hten no need to copy
these across.
> +	if (data->direct)
> +		for (i = 0; i < SENSOR_CLASSES; i++) {
> +			data->m[i] = pmbus_config[data->type].m[i];
> +			data->b[i] = pmbus_config[data->type].b[i];
> +			data->R[i] = pmbus_config[data->type].R[i];
> +		}
> +
> +	if (data->type == pmbus) {
> +		/*
> +		 * We could try to determine supported options here.
> +		 * However, it appears that hardly any chips support
> +		 * the CAPABILITY, QUERY, or COEFFICIENTS commands.
> +		 * Thus, we just use default basic settings.
> +		 */
> +	}
> +
> +	/*
> +	 * Bail out if the number of pages is bad, or if more than
> +	 * one page was configured and the chip has no PAGE register.
> +	 */
  I think you only need to run this if you have the pages module parameter
specified (or there is a bug in your driver / someone is lying about what
is wired up, neither of which should be explicitly handled)
> +	if (data->pages > PMBUS_PAGES ||
> +	    (data->pages > 1 && !pmbus_check_byte_register(client, 0,
> +							   PMBUS_PAGE))) {
> +		dev_err(&client->dev, "%d pages but no PAGE register",
> +			data->pages);
> +		ret = -EINVAL;
> +		goto out_data;
> +	}
> +
> +	/*
> +	 * Bail out if status register or PMBus revision register
> +	 * does not exist.
> +	 */
Again, doesn't this just mean you don't have a pmbus device?  If this driver's
probe is running you should be sure you have one.
> +	if (!pmbus_check_byte_register(client, 0, PMBUS_STATUS_BYTE)
> +	    || !pmbus_check_byte_register(client, 0, PMBUS_REVISION)) {
> +		ret = -ENODEV;
> +		goto out_data;
> +	}
> +
> +	/*
> +	 * Identify supported status registers
> +	 */
> +	if (pmbus_check_byte_register(client, 0, PMBUS_STATUS_VOUT))
> +		data->status_bits |= HAVE_STATUS_VOUT;
> +	if (pmbus_check_byte_register(client, 0, PMBUS_STATUS_IOUT))
> +		data->status_bits |= HAVE_STATUS_IOUT;
> +	if (pmbus_check_byte_register(client, 0, PMBUS_STATUS_INPUT))
> +		data->status_bits |= HAVE_STATUS_INPUT;
> +	if (pmbus_check_byte_register(client, 0, PMBUS_STATUS_TEMPERATURE))
> +		data->status_bits |= HAVE_STATUS_TEMP;
> +
> +	/*
> +	 * Input voltage sensors
> +	 */
> +	in_index = 1;
> +	if (pmbus_check_word_register(client, 0, PMBUS_READ_VIN)) {
> +		bool have_fault = false;
> +
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "in", in_index, "vin", 0);
> +		pmbus_add_sensor(data, "in", "input", in_index,
> +				 0, PMBUS_READ_VIN, PSC_VOLTAGE, true);
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_VIN_UV_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "min", in_index,
> +					 0, PMBUS_VIN_UV_WARN_LIMIT,
> +					 PSC_VOLTAGE, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT)
> +				pmbus_add_boolean_reg(data, "in", "min_alarm",
> +						      in_index,
> +						      PB_STATUS_INPUT_BASE,
> +						      PB_VOLTAGE_UV_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_VIN_UV_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "lcrit", in_index,
> +					 0, PMBUS_VIN_UV_FAULT_LIMIT,
> +					 PSC_VOLTAGE, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT)
> +				have_fault = true;
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_VIN_OV_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "max", in_index,
> +					 0, PMBUS_VIN_OV_WARN_LIMIT,
> +					 PSC_VOLTAGE, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT)
> +				pmbus_add_boolean_reg(data, "in", "max_alarm",
> +						      in_index,
> +						      PB_STATUS_INPUT_BASE,
> +						      PB_VOLTAGE_OV_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_VIN_OV_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "crit", in_index,
> +					 0, PMBUS_VIN_OV_FAULT_LIMIT,
> +					 PSC_VOLTAGE, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT)
> +				have_fault = true;
> +		}
> +		if (have_fault) {
> +			pmbus_add_boolean_reg(data, "in", "fault",
> +					      in_index,
> +					      PB_STATUS_INPUT_BASE,
> +					      PB_VOLTAGE_UV_FAULT
> +					      | PB_VOLTAGE_OV_FAULT);
> +		}
> +		in_index++;
> +	}
> +	if (pmbus_check_word_register(client, 0, PMBUS_READ_VCAP)) {
> +		pmbus_add_label(data, "in", in_index, "vcap", 0);
> +		pmbus_add_sensor(data, "in", "input", in_index, 0,
> +				 PMBUS_READ_VCAP, PSC_VOLTAGE, true);
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Output voltage sensors
> +	 */
> +	for (i = 0; i < data->pages; i++) {
> +		bool have_fault = false;
> +
> +		if (!pmbus_check_word_register(client, i, PMBUS_READ_VOUT))
> +			break;
> +
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "in", in_index, "vout", i+1);
> +		pmbus_add_sensor(data, "in", "input", in_index, i,
> +				 PMBUS_READ_VOUT, PSC_VOLTAGE, true);
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_VOUT_UV_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "min", in_index, i,
> +					 PMBUS_VOUT_UV_WARN_LIMIT, PSC_VOLTAGE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_VOUT)
> +				pmbus_add_boolean_reg(data, "in", "min_alarm",
> +						      in_index,
> +						      PB_STATUS_VOUT_BASE + i,
> +						      PB_VOLTAGE_UV_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_VOUT_UV_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "lcrit", in_index, i,
> +					 PMBUS_VOUT_UV_FAULT_LIMIT, PSC_VOLTAGE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_VOUT)
> +				have_fault = true;
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_VOUT_OV_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "max", in_index, i,
> +					 PMBUS_VOUT_OV_WARN_LIMIT, PSC_VOLTAGE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_VOUT) {
> +				pmbus_add_boolean_reg(data, "in", "max_alarm",
> +						      in_index,
> +						      PB_STATUS_VOUT_BASE + i,
> +						      PB_VOLTAGE_OV_WARNING);
> +			}
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_VOUT_OV_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "in", "crit", in_index, i,
> +					 PMBUS_VOUT_OV_FAULT_LIMIT, PSC_VOLTAGE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_VOUT)
> +				have_fault = true;
> +		}
> +		if (have_fault) {
> +			pmbus_add_boolean_reg(data, "in", "fault",
> +					      in_index,
> +					      PB_STATUS_VOUT_BASE + i,
> +					      PB_VOLTAGE_UV_FAULT
> +					      | PB_VOLTAGE_OV_FAULT);
> +		}
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Current sensors
> +	 */
> +
> +	/*
> +	 * Input current sensors
> +	 */
> +	in_index = 1;
> +	if (pmbus_check_word_register(client, 0, PMBUS_READ_IIN)) {
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "curr", in_index, "iin", 0);
> +		pmbus_add_sensor(data, "curr", "input", in_index,
> +				 0, PMBUS_READ_IIN, PSC_CURRENT, true);
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_IIN_OC_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "curr", "max", in_index,
> +					 0, PMBUS_IIN_OC_WARN_LIMIT,
> +					 PSC_CURRENT, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT) {
> +				pmbus_add_boolean_reg(data, "curr", "alarm",
> +						      in_index,
> +						      PB_STATUS_INPUT_BASE,
> +						      PB_IIN_OC_WARNING);
> +			}
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_IIN_OC_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "curr", "crit", in_index,
> +					 0, PMBUS_IIN_OC_FAULT_LIMIT,
> +					 PSC_CURRENT, false);
> +			if (data->status_bits & HAVE_STATUS_INPUT) {
> +				pmbus_add_boolean_reg(data, "curr", "fault",
> +						      in_index,
> +						      PB_STATUS_INPUT_BASE,
> +						      PB_IIN_OC_FAULT);
> +			}
> +		}
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Output Current sensors
> +	 */
> +	for (i = 0; i < data->pages; i++) {
> +		bool have_fault = false;
> +
> +		if (!pmbus_check_word_register(client, i, PMBUS_READ_IOUT))
> +			break;
> +
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "curr", in_index, "iout", i+1);
> +		pmbus_add_sensor(data, "curr", "input", in_index, i,
> +				 PMBUS_READ_IOUT, PSC_CURRENT, true);
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_IOUT_OC_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "curr", "max", in_index, i,
> +					 PMBUS_IOUT_OC_WARN_LIMIT, PSC_CURRENT,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_IOUT)
> +				pmbus_add_boolean_reg(data, "curr", "alarm",
> +						      in_index,
> +						      PB_STATUS_IOUT_BASE + i,
> +						      PB_IOUT_OC_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_IOUT_OC_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "curr", "crit", in_index, i,
> +					 PMBUS_IOUT_OC_FAULT_LIMIT, PSC_CURRENT,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_IOUT)
> +				have_fault = true;
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_IOUT_UC_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "curr", "lcrit", in_index, i,
> +					 PMBUS_IOUT_UC_FAULT_LIMIT, PSC_CURRENT,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_IOUT)
> +				have_fault = true;
> +		}
> +		if (have_fault) {
> +			pmbus_add_boolean_reg(data, "curr", "fault",
> +					      in_index,
> +					      PB_STATUS_IOUT_BASE + i,
> +					      PB_IOUT_UC_FAULT
> +					      | PB_IOUT_OC_FAULT);
> +		}
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Power sensors
> +	 */
> +	/*
> +	 * Input Power sensors
> +	 */
> +	in_index = 1;
> +	if (pmbus_check_word_register(client, 0, PMBUS_READ_PIN)) {
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "power", in_index, "pin", 0);
> +		pmbus_add_sensor(data, "power", "input", in_index,
> +				 0, PMBUS_READ_PIN, PSC_POWER, true);
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_PIN_OP_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "power", "max", in_index,
> +					 0, PMBUS_PIN_OP_WARN_LIMIT, PSC_POWER,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_INPUT)
> +				pmbus_add_boolean_reg(data, "power", "alarm",
> +						      in_index,
> +						      PB_STATUS_INPUT_BASE,
> +						      PB_PIN_OP_WARNING);
> +		}
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Output Power sensors
> +	 */
> +	for (i = 0; i < data->pages; i++) {
> +		bool need_alarm = false;
> +
> +		if (!pmbus_check_word_register(client, i, PMBUS_READ_POUT))
> +			break;
> +
> +		i0 = data->num_sensors;
> +		pmbus_add_label(data, "power", in_index, "pout", i+1);
> +		pmbus_add_sensor(data, "power", "input", in_index, i,
> +				 PMBUS_READ_POUT, PSC_POWER, true);
> +		/*
> +		 * Per hwmon sysfs API, power_cap is to be used to limit output
> +		 * power.
> +		 * We have two registers related to maximum output power,
> +		 * PMBUS_POUT_MAX and PMBUS_POUT_OP_WARN_LIMIT.
> +		 * PMBUS_POUT_MAX matches the powerX_cap attribute definition.
> +		 * There is no attribute in the API to match
> +		 * PMBUS_POUT_OP_WARN_LIMIT. We use powerX_max for now.
> +		 */
> +		if (pmbus_check_word_register(client, i, PMBUS_POUT_MAX)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "power", "cap", in_index, i,
> +					 PMBUS_POUT_MAX, PSC_POWER, false);
> +			need_alarm = true;
> +		}
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_POUT_OP_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "power", "max", in_index, i,
> +					 PMBUS_POUT_OP_WARN_LIMIT, PSC_POWER,
> +					 false);
> +			need_alarm = true;
> +		}
> +		if (need_alarm && (data->status_bits & HAVE_STATUS_IOUT))
> +			pmbus_add_boolean_reg(data, "power", "alarm",
> +					      in_index,
> +					      PB_STATUS_IOUT_BASE,
> +					      PB_POUT_OP_WARNING
> +					      | PB_POWER_LIMITING);
> +
> +		if (pmbus_check_word_register(client, i,
> +					      PMBUS_POUT_OP_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "power", "crit", in_index, i,
> +					 PMBUS_POUT_OP_FAULT_LIMIT, PSC_POWER,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_IOUT)
> +				pmbus_add_boolean_reg(data, "power", "fault",
> +						      in_index,
> +						      PB_STATUS_IOUT_BASE,
> +						      PB_POUT_OP_FAULT);
> +		}
> +		in_index++;
> +	}
> +
> +	/*
> +	 * Temperature sensors
> +	 */
> +	in_index = 1;
> +	for (i = 0; i < ARRAY_SIZE(pmbus_temp_sensors); i++) {
> +		if (!pmbus_check_word_register(client, 0,
> +					       pmbus_temp_sensors[i]))
> +			break;
> +
> +		i0 = data->num_sensors;
> +		pmbus_add_sensor(data, "temp", "input", in_index, 0,
> +				 pmbus_temp_sensors[i], PSC_TEMPERATURE, true);
> +
> +		/*
> +		 * PMBus provides only one status register for all temperature
> +		 * sensors. Thus, we can not use the status register to
> +		 * determine which of the sensors actually caused an alarm
> +		 * or fault. Always compare the current temperature against
> +		 * the limit registers to determine warning or fault conditions.
> +		 */
> +		if (pmbus_check_word_register(client, 0, PMBUS_UT_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "temp", "min", in_index, 0,
> +					 PMBUS_UT_WARN_LIMIT, PSC_TEMPERATURE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_TEMP)
> +				pmbus_add_boolean_cmp(data, "temp", "min_alarm",
> +						      in_index, i1, i0,
> +						      PB_STATUS_TEMP_BASE,
> +						      PB_TEMP_UT_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_UT_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "temp", "lcrit", in_index, 0,
> +					 PMBUS_UT_FAULT_LIMIT, PSC_TEMPERATURE,
> +					 false);
> +		}
> +		if (pmbus_check_word_register(client, 0, PMBUS_OT_WARN_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "temp", "max", in_index, 0,
> +					 PMBUS_OT_WARN_LIMIT, PSC_TEMPERATURE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_TEMP)
> +				pmbus_add_boolean_cmp(data, "temp", "max_alarm",
> +						      in_index, i0, i1,
> +						      PB_STATUS_TEMP_BASE,
> +						      PB_TEMP_OT_WARNING);
> +		}
> +		if (pmbus_check_word_register(client, 0,
> +					      PMBUS_OT_FAULT_LIMIT)) {
> +			i1 = data->num_sensors;
> +			pmbus_add_sensor(data, "temp", "crit", in_index, 0,
> +					 PMBUS_OT_FAULT_LIMIT, PSC_TEMPERATURE,
> +					 false);
> +			if (data->status_bits & HAVE_STATUS_TEMP)
> +				pmbus_add_boolean_cmp(data, "temp",
> +						      "crit_alarm",
> +						      in_index, i0, i1,
> +						      PB_STATUS_TEMP_BASE,
> +						      PB_TEMP_OT_FAULT);
> +		}
> +		in_index++;
> +	}
> +
> +	pmbus_clear_faults(client);
> +
> +	ret = -ENODEV;
> +	/* Register sysfs hooks */
> +	data->group.attrs = data->attributes;
> +	ret = sysfs_create_group(&client->dev.kobj, &data->group);
> +	if (ret)
> +		goto out_data;
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		ret = PTR_ERR(data->hwmon_dev);
> +		goto out_hwmon_device_register;
> +	}
> +	return 0;
> +out_hwmon_device_register:
> +	sysfs_remove_group(&client->dev.kobj, &data->group);
> +out_data:
> +	kfree(data);
> +out:
> +	return ret;
> +}
> +
> +static int pmbus_remove(struct i2c_client *client)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&client->dev.kobj, &data->group);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id pmbus_id[] = {
> +	{"bmr45x", pmbus},
> +	{"ltc2978", ltc2978},
> +	{"max16064", max16064},
> +	{"max8688", max8688},
> +	{"pmbus", pmbus},
> +	{"ucd921x", pmbus},
> +	{"ucd9220", ucd9220},
> +	{"ucd9240", ucd9240},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver pmbus_driver = {
> +	.driver = {
> +		.name = "pmbus",
> +		},
> +	.probe = pmbus_probe,
> +	.remove = pmbus_remove,
> +	.id_table = pmbus_id,
> +};
> +
> +static int __init pmbus_init(void)
> +{
> +	return i2c_add_driver(&pmbus_driver);
> +}
> +
> +static void __exit pmbus_exit(void)
> +{
> +	i2c_del_driver(&pmbus_driver);
> +}
> +
> +MODULE_AUTHOR("Guenter Roeck");
> +MODULE_DESCRIPTION("PMBus driver");
> +MODULE_LICENSE("GPL");
> +module_init(pmbus_init);
> +module_exit(pmbus_exit);
> diff --git a/drivers/hwmon/pmbus.h b/drivers/hwmon/pmbus.h
> new file mode 100644
> index 0000000..2a8a027
> --- /dev/null
> +++ b/drivers/hwmon/pmbus.h
> @@ -0,0 +1,209 @@
> +/*
> + * pmbus.h - Common defines and structures for PMBus devices
> + *
> + * Copyright (c) 2010 Ericsson AB.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef PMBUS_H
> +#define PMBUS_H
> +
> +/*
> + * Registers
> + */
> +#define PMBUS_PAGE			0x00
> +#define PMBUS_OPERATION			0x01
> +#define PMBUS_ON_OFF_CONFIG		0x02
> +#define PMBUS_CLEAR_FAULTS		0x03
> +#define PMBUS_PHASE			0x04
> +
> +#define PMBUS_CAPABILITY		0x19
> +#define PMBUS_QUERY			0x1A
> +
> +#define PMBUS_VOUT_MODE			0x20
> +#define PMBUS_VOUT_COMMAND		0x21
> +#define PMBUS_VOUT_TRIM			0x22
> +#define PMBUS_VOUT_CAL_OFFSET		0x23
> +#define PMBUS_VOUT_MAX			0x24
> +#define PMBUS_VOUT_MARGIN_HIGH		0x25
> +#define PMBUS_VOUT_MARGIN_LOW		0x26
> +#define PMBUS_VOUT_TRANSITION_RATE	0x27
> +#define PMBUS_VOUT_DROOP		0x28
> +#define PMBUS_VOUT_SCALE_LOOP		0x29
> +#define PMBUS_VOUT_SCALE_MONITOR	0x2A
> +
> +#define PMBUS_COEFFICIENTS		0x30
> +#define PMBUS_POUT_MAX			0x31
> +
> +#define PMBUS_VOUT_OV_FAULT_LIMIT	0x40
> +#define PMBUS_VOUT_OV_FAULT_RESPONSE	0x41
> +#define PMBUS_VOUT_OV_WARN_LIMIT	0x42
> +#define PMBUS_VOUT_UV_WARN_LIMIT	0x43
> +#define PMBUS_VOUT_UV_FAULT_LIMIT	0x44
> +#define PMBUS_VOUT_UV_FAULT_RESPONSE	0x45
> +#define PMBUS_IOUT_OC_FAULT_LIMIT	0x46
> +#define PMBUS_IOUT_OC_FAULT_RESPONSE	0x47
> +#define PMBUS_IOUT_OC_LV_FAULT_LIMIT	0x48
> +#define PMBUS_IOUT_OC_LV_FAULT_RESPONSE	0x49
> +#define PMBUS_IOUT_OC_WARN_LIMIT	0x4A
> +#define PMBUS_IOUT_UC_FAULT_LIMIT	0x4B
> +#define PMBUS_IOUT_UC_FAULT_RESPONSE	0x4C
> +
> +#define PMBUS_OT_FAULT_LIMIT		0x4F
> +#define PMBUS_OT_FAULT_RESPONSE		0x50
> +#define PMBUS_OT_WARN_LIMIT		0x51
> +#define PMBUS_UT_WARN_LIMIT		0x52
> +#define PMBUS_UT_FAULT_LIMIT		0x53
> +#define PMBUS_UT_FAULT_RESPONSE		0x54
> +#define PMBUS_VIN_OV_FAULT_LIMIT	0x55
> +#define PMBUS_VIN_OV_FAULT_RESPONSE	0x56
> +#define PMBUS_VIN_OV_WARN_LIMIT		0x57
> +#define PMBUS_VIN_UV_WARN_LIMIT		0x58
> +#define PMBUS_VIN_UV_FAULT_LIMIT	0x59
> +
> +#define PMBUS_IIN_OC_FAULT_LIMIT	0x5B
> +#define PMBUS_IIN_OC_WARN_LIMIT		0x5D
> +
> +#define PMBUS_POUT_OP_FAULT_LIMIT	0x68
> +#define PMBUS_POUT_OP_WARN_LIMIT	0x6A
> +#define PMBUS_PIN_OP_WARN_LIMIT		0x6B
> +
> +#define PMBUS_STATUS_BYTE		0x78
> +#define PMBUS_STATUS_WORD		0x79
> +#define PMBUS_STATUS_VOUT		0x7A
> +#define PMBUS_STATUS_IOUT		0x7B
> +#define PMBUS_STATUS_INPUT		0x7C
> +#define PMBUS_STATUS_TEMPERATURE	0x7D
> +#define PMBUS_STATUS_CML		0x7E
> +#define PMBUS_STATUS_OTHER		0x7F
> +#define PMBUS_STATUS_MFR_SPECIFIC	0x80
> +#define PMBUS_STATUS_FANS_1_2		0x81
> +#define PMBUS_STATUS_FANS_3_4		0x82
> +
> +#define PMBUS_READ_VIN			0x88
> +#define PMBUS_READ_IIN			0x89
> +#define PMBUS_READ_VCAP			0x8A
> +#define PMBUS_READ_VOUT			0x8B
> +#define PMBUS_READ_IOUT			0x8C
> +#define PMBUS_READ_TEMPERATURE_1	0x8D
> +#define PMBUS_READ_TEMPERATURE_2	0x8E
> +#define PMBUS_READ_TEMPERATURE_3	0x8F
> +#define PMBUS_READ_FAN_SPEED_1		0x90
> +#define PMBUS_READ_FAN_SPEED_2		0x91
> +#define PMBUS_READ_FAN_SPEED_3		0x92
> +#define PMBUS_READ_FAN_SPEED_4		0x93
> +#define PMBUS_READ_DUTY_CYCLE		0x94
> +#define PMBUS_READ_FREQUENCY		0x95
> +#define PMBUS_READ_POUT			0x96
> +#define PMBUS_READ_PIN			0x97
> +
> +#define PMBUS_REVISION			0x98
> +#define PMBUS_MFR_ID			0x99
> +#define PMBUS_MFR_MODEL			0x9A
> +#define PMBUS_MFR_REVISION		0x9B
> +#define PMBUS_MFR_LOCATION		0x9C
> +#define PMBUS_MFR_DATE			0x9D
> +#define PMBUS_MFR_SERIAL		0x9E
> +
> +#define LTC2978_MFR_SPECIAL_ID		0xE7
> +
> +/*
> + * CAPABILITY
> + */
> +#define PB_CAPABILITY_SMBALERT		(1<<4)
> +#define PB_CAPABILITY_ERROR_CHECK	(1<<7)
> +
> +/*
> + * VOUT_MODE
> + */
> +#define PB_VOUT_MODE_MODE_MASK		0xe0
> +#define PB_VOUT_MODE_PARAM_MASK		0x1f
> +
> +#define PB_VOUT_MODE_LINEAR		0x00
> +#define PB_VOUT_MODE_VID		0x20
> +#define PB_VOUT_MODE_DIRECT		0x40
> +
> +/*
> + * STATUS_BYTE, STATUS_WORD (lower)
> + */
> +#define PB_STATUS_NONE_ABOVE		(1<<0)
> +#define PB_STATUS_CML			(1<<1)
> +#define PB_STATUS_TEMPERATURE		(1<<2)
> +#define PB_STATUS_VIN_UV		(1<<3)
> +#define PB_STATUS_IOUT_OC		(1<<4)
> +#define PB_STATUS_VOUT_OV		(1<<5)
> +#define PB_STATUS_OFF			(1<<6)
> +#define PB_STATUS_BUSY			(1<<7)
> +
> +/*
> + * STATUS_WORD (upper)
> + */
> +#define PB_STATUS_UNKNOWN		(1<<8)
> +#define PB_STATUS_OTHER			(1<<9)
> +#define PB_STATUS_FANS			(1<<10)
> +#define PB_STATUS_POWER_GOOD_N		(1<<11)
> +#define PB_STATUS_WORD_MFR		(1<<12)
> +#define PB_STATUS_INPUT			(1<<13)
> +#define PB_STATUS_IOUT_POUT		(1<<14)
> +#define PB_STATUS_VOUT			(1<<15)
> +
> +/*
> + * STATUS_IOUT
> + */
> +#define PB_POUT_OP_WARNING		(1<<0)
> +#define PB_POUT_OP_FAULT		(1<<1)
> +#define PB_POWER_LIMITING		(1<<2)
> +#define PB_CURRENT_SHARE_FAULT		(1<<3)
> +#define PB_IOUT_UC_FAULT		(1<<4)
> +#define PB_IOUT_OC_WARNING		(1<<5)
> +#define PB_IOUT_OC_LV_FAULT		(1<<6)
> +#define PB_IOUT_OC_FAULT		(1<<7)
> +
> +/*
> + * STATUS_VOUT, STATUS_INPUT
> + */
> +#define PB_VOLTAGE_UV_FAULT		(1<<4)
> +#define PB_VOLTAGE_UV_WARNING		(1<<5)
> +#define PB_VOLTAGE_OV_WARNING		(1<<6)
> +#define PB_VOLTAGE_OV_FAULT		(1<<7)
> +
> +/*
> + * STATUS_INPUT
> + */
> +#define PB_PIN_OP_WARNING		(1<<0)
> +#define PB_IIN_OC_WARNING		(1<<1)
> +#define PB_IIN_OC_FAULT			(1<<2)
> +
> +/*
> + * STATUS_TEMPERATURE
> + */
> +#define PB_TEMP_UT_FAULT		(1<<4)
> +#define PB_TEMP_UT_WARNING		(1<<5)
> +#define PB_TEMP_OT_WARNING		(1<<6)
> +#define PB_TEMP_OT_FAULT		(1<<7)
> +
> +/*
> + * CML_FAULT_STATUS
> + */
> +#define	PB_CML_FAULT_OTHER_MEM_LOGIC	(1<<0)
> +#define	PB_CML_FAULT_OTHER_COMM		(1<<1)
> +#define	PB_CML_FAULT_PROCESSOR		(1<<3)
> +#define	PB_CML_FAULT_MEMORY		(1<<4)
> +#define	PB_CML_FAULT_PACKET_ERROR	(1<<5)
> +#define	PB_CML_FAULT_INVALID_DATA	(1<<6)
> +#define	PB_CML_FAULT_INVALID_COMMAND	(1<<7)
> +
> +#endif /* PB_H */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ