[RFC linux dev-4.10 0/3] drivers/hwmon/pmbus: core extensions

[RFC linux dev-4.10 0/3] drivers/hwmon/pmbus: core extensions

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This series adds some functionality to the pmbus core.

The first two patches are fixes to enable the use of the STATUS_WORD register
which was completely unused in existing pmbus. This allows the use of more
default alarm/fault attributes for default pmbus sensors.

The third patch adds "status" attributes to each class of hwmon sensor created
by pmbus. For example, in1_status and temp1_status. These will display the
associated raw status register (STATUS_INPUT and STATUS_TEMPERATURE). These
will probably be essential for the UCD driver, and certainly very convenient
for the power supply driver.

An alternative I considered was breaking all the status registers out into
bits, but some bits seem to be device-dependent and therefore couldn't be named
correctly or at all in the pmbus core. This would also really make the hwmon
directory messy, unless you have a separate directory for that, either in the
device directory or under the hwmon directory (not sure if that's even
possible).

Edward A. James (3):
  drivers/hwmon/pmbus: Use STATUS_WORD by default and do correct access
  drivers/hmwon/pmbus: store STATUS_WORD in status registers
  drivers/hwmon/pmbus: Add sensor status to pmbus attributes

 drivers/hwmon/pmbus/pmbus_core.c | 153 +++++++++++++++++++++++++++++++++------
 1 file changed, 130 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[RFC linux dev-4.10 1/3] drivers/hwmon/pmbus: Use STATUS_WORD by default and do correct access

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Pmbus core always read byte data from the status register, even if
configured to use STATUS_WORD. Use a function pointer so we always do
the correct byte/word access. Also switch to use STATUS_WORD by default.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 69 +++++++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 3c8bcbd..b293682 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -114,7 +114,8 @@ struct pmbus_data {
 	 * so we keep them all together.
 	 */
 	u8 status[PB_NUM_STATUS_REG];
-	u8 status_register;
+
+	int (*read_status)(struct i2c_client *client, int page);
 
 	u8 currpage;
 };
@@ -460,7 +461,7 @@ static int pmbus_check_status_cml(struct i2c_client *client)
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	int status, status2;
 
-	status = _pmbus_read_byte_data(client, -1, data->status_register);
+	status = data->read_status(client, -1);
 	if (status < 0 || (status & PB_STATUS_CML)) {
 		status2 = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
 		if (status2 < 0 || (status2 & PB_CML_FAULT_INVALID_COMMAND))
@@ -484,6 +485,36 @@ static bool pmbus_check_register(struct i2c_client *client,
 	return rv >= 0;
 }
 
+/* Check specified page status register accessibility. Need a separate
+ * function rather than the two above so we can use the correct status
+ * register, and we can optimize out the second status register read.
+ */
+static bool pmbus_check_status_register(struct i2c_client *client, int page)
+{
+	int status, rc = 0;
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	status = data->read_status(client, page);
+	if (status < 0)
+		goto out;
+
+	if (!(data->flags & PMBUS_SKIP_STATUS_CHECK)) {
+		if (status & PB_STATUS_CML) {
+			status = _pmbus_read_byte_data(client, -1,
+						       PMBUS_STATUS_CML);
+			if (status < 0 ||
+			    (status & PB_CML_FAULT_INVALID_COMMAND))
+				goto out;
+		}
+	}
+
+	rc = 1;
+
+out:
+	pmbus_clear_fault_page(client, -1);
+	return rc;
+}
+
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg)
 {
 	return pmbus_check_register(client, _pmbus_read_byte_data, page, reg);
@@ -529,9 +560,8 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
 		int i, j;
 
 		for (i = 0; i < info->pages; i++) {
-			data->status[PB_STATUS_BASE + i]
-			    = _pmbus_read_byte_data(client, i,
-						    data->status_register);
+			data->status[PB_STATUS_BASE + i] =
+				data->read_status(client, i);
 			for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
 				struct _pmbus_status *s = &pmbus_status[j];
 
@@ -1199,8 +1229,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 		 * the generic status register for this page is accessible.
 		 */
 		if (!ret && attr->gbit &&
-		    pmbus_check_byte_register(client, page,
-					      data->status_register)) {
+		    pmbus_check_status_register(client, page)) {
 			ret = pmbus_add_boolean(data, name, "alarm", index,
 						NULL, NULL,
 						PB_STATUS_BASE + page,
@@ -1921,6 +1950,16 @@ static int pmbus_identify_common(struct i2c_client *client,
 	return 0;
 }
 
+static int pmbus_read_status_word(struct i2c_client *client, int page)
+{
+	return _pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
+}
+
+static int pmbus_read_status_byte(struct i2c_client *client, int page)
+{
+	return _pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
+}
+
 static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 			     struct pmbus_driver_info *info)
 {
@@ -1928,16 +1967,16 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	int page, ret;
 
 	/*
-	 * Some PMBus chips don't support PMBUS_STATUS_BYTE, so try
-	 * to use PMBUS_STATUS_WORD instead if that is the case.
+	 * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
+	 * to use PMBUS_STATUS_BYTE instead if that is the case.
 	 * Bail out if both registers are not supported.
 	 */
-	data->status_register = PMBUS_STATUS_BYTE;
-	ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
-	if (ret < 0 || ret == 0xff) {
-		data->status_register = PMBUS_STATUS_WORD;
-		ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
-		if (ret < 0 || ret == 0xffff) {
+	data->read_status = pmbus_read_status_word;
+	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
+	if (ret < 0 || ret == 0xffff) {
+		data->read_status = pmbus_read_status_byte;
+		ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
+		if (ret < 0 || ret == 0xff) {
 			dev_err(dev, "PMBus status register not found\n");
 			return -ENODEV;
 		}
-- 
1.8.3.1

[RFC linux dev-4.10 2/3] drivers/hmwon/pmbus: store STATUS_WORD in status registers

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Higher byte of the status register wasn't available for boolean alarms.
Store the STATUS_WORD register if it's available, and read it out when
queried by boolean attributes.

The method of storing and retrieving the status reg is a bit hacky but
I couldn't work out another way without doubling the storage requirement
of every pmbus device or tearing up more of the pmbus core code.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index b293682..fc88cae 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -43,7 +43,7 @@
  * Index into status register array, per status register group
  */
 #define PB_STATUS_BASE		0
-#define PB_STATUS_VOUT_BASE	(PB_STATUS_BASE + PMBUS_PAGES)
+#define PB_STATUS_VOUT_BASE	(PB_STATUS_BASE + (PMBUS_PAGES * 2))
 #define PB_STATUS_IOUT_BASE	(PB_STATUS_VOUT_BASE + PMBUS_PAGES)
 #define PB_STATUS_FAN_BASE	(PB_STATUS_IOUT_BASE + PMBUS_PAGES)
 #define PB_STATUS_FAN34_BASE	(PB_STATUS_FAN_BASE + PMBUS_PAGES)
@@ -557,11 +557,14 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
 
 	mutex_lock(&data->update_lock);
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		int i, j;
+		int i, j, status;
 
 		for (i = 0; i < info->pages; i++) {
-			data->status[PB_STATUS_BASE + i] =
-				data->read_status(client, i);
+			status = data->read_status(client, i);
+			data->status[PB_STATUS_BASE + (i * 2)] = status;
+			data->status[PB_STATUS_BASE + (i * 2) + 1] =
+				status >> 8;
+
 			for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
 				struct _pmbus_status *s = &pmbus_status[j];
 
@@ -860,6 +863,18 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
 	return regval;
 }
 
+static int pmbus_get_status(struct pmbus_data *data, int reg)
+{
+	int ret;
+
+	if (reg < PB_STATUS_BASE + PMBUS_PAGES)
+		ret = data->status[reg * 2] | data->status[(reg * 2) + 1] << 8;
+	else
+		ret = data->status[reg];
+
+	return ret;
+}
+
 /*
  * Return boolean calculated from converted data.
  * <index> defines a status register index and mask.
@@ -888,12 +903,12 @@ static int pmbus_get_boolean(struct pmbus_data *data, struct pmbus_boolean *b,
 {
 	struct pmbus_sensor *s1 = b->s1;
 	struct pmbus_sensor *s2 = b->s2;
-	u16 reg = (index >> 8) & 0xffff;
-	u8 mask = index & 0xff;
+	u16 reg = index >> 16;
+	u16 mask = index & 0xffff;
 	int ret, status;
 	u8 regval;
 
-	status = data->status[reg];
+	status = pmbus_get_status(data, reg);
 	if (status < 0)
 		return status;
 
@@ -1032,7 +1047,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
 			     const char *name, const char *type, int seq,
 			     struct pmbus_sensor *s1,
 			     struct pmbus_sensor *s2,
-			     u16 reg, u8 mask)
+			     u16 reg, u16 mask)
 {
 	struct pmbus_boolean *boolean;
 	struct sensor_device_attribute *a;
@@ -1048,7 +1063,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
 	boolean->s1 = s1;
 	boolean->s2 = s2;
 	pmbus_attr_init(a, boolean->name, S_IRUGO, pmbus_show_boolean, NULL,
-			(reg << 8) | mask);
+			(reg << 16) | mask);
 
 	return pmbus_add_attribute(data, &a->dev_attr.attr);
 }
@@ -1140,7 +1155,7 @@ struct pmbus_limit_attr {
  */
 struct pmbus_sensor_attr {
 	u16 reg;			/* sensor register */
-	u8 gbit;			/* generic status bit */
+	u16 gbit;			/* generic status bit */
 	u8 nlimit;			/* # of limit registers */
 	enum pmbus_sensor_classes class;/* sensor class */
 	const char *label;		/* sensor label */
@@ -1485,6 +1500,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_IIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = iin_limit_attrs,
 		.nlimit = ARRAY_SIZE(iin_limit_attrs),
 	}, {
@@ -1569,6 +1585,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_PIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = pin_limit_attrs,
 		.nlimit = ARRAY_SIZE(pin_limit_attrs),
 	}, {
-- 
1.8.3.1

[RFC linux dev-4.10 3/3] drivers/hwmon/pmbus: Add sensor status to pmbus attributes

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Extend the pmbus core to provide status registers for attributes.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 51 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index fc88cae..d319f94 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -87,6 +87,14 @@ struct pmbus_label {
 #define to_pmbus_label(_attr) \
 	container_of(_attr, struct pmbus_label, attribute)
 
+struct pmbus_status {
+	char name[PMBUS_NAME_SIZE];	/* sysfs status name */
+	struct device_attribute attribute;
+	int reg;
+};
+#define to_pmbus_status(_attr) \
+	container_of(_attr, struct pmbus_status, attribute)
+
 struct pmbus_data {
 	struct device *dev;
 	struct device *hwmon_dev;
@@ -993,6 +1001,16 @@ static ssize_t pmbus_show_label(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
 }
 
+static ssize_t pmbus_show_status(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct pmbus_status *status = to_pmbus_status(da);
+	struct pmbus_data *data = pmbus_update_device(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%02x\n",
+			pmbus_get_status(data, status->reg) & 0xFF);
+}
+
 static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
@@ -1131,6 +1149,25 @@ static int pmbus_add_label(struct pmbus_data *data,
 	return pmbus_add_attribute(data, &a->attr);
 }
 
+static int pmbus_add_status(struct pmbus_data *data, const char *name, int seq,
+			    int reg)
+{
+	struct pmbus_status *status;
+	struct device_attribute *a;
+
+	status = devm_kzalloc(data->dev, sizeof(*status), GFP_KERNEL);
+	if (!status)
+		return -ENOMEM;
+
+	a = &status->attribute;
+
+	snprintf(status->name, sizeof(status->name), "%s%d_status", name, seq);
+	status->reg = reg;
+
+	pmbus_dev_attr_init(a, status->name, S_IRUGO, pmbus_show_status, NULL);
+	return pmbus_add_attribute(data, &a->attr);
+}
+
 /*
  * Search for attributes. Allocate sensors, booleans, and labels as needed.
  */
@@ -1253,6 +1290,16 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 				return ret;
 		}
 	}
+	if (attr->sbase > 0 && attr->sbase < PB_STATUS_VMON_BASE) {
+		/* Add "status" attribute for physical status registers to dump
+		 * the contents. This is pretty useful for hardware diagnostic
+		 * purposes, as an alarm or fault bit doesn't necessarily
+		 * provide all the info available.
+		 */
+		ret = pmbus_add_status(data, name, index, attr->sbase + page);
+		if (ret)
+			return ret;
+	}
 	return 0;
 }
 
@@ -1885,6 +1932,10 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 					PB_FAN_FAN1_FAULT >> (f & 1));
 				if (ret)
 					return ret;
+				ret = pmbus_add_status(data, "fan", index,
+						       base);
+				if (ret)
+					return ret;
 			}
 			index++;
 		}
-- 
1.8.3.1