[RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Eddie James]

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

Hi Guenter,

I'm looking for some feedback for some extensions to the pmbus core. We're
looking for some additional functionality, particularly with STATUS_WORD and
obtaining raw status data.

The first two patches enable the use of the STATUS_WORD register in the pmbus
core. This allows the use of more default alarm/fault attributes for default
pmbus sensors by allowing the use of the higher byte status bits.

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 (e.g. STATUS_INPUT and STATUS_TEMPERATURE). I
realize this is not really "normal" for hwmon or pmbus. These are potentially
very useful in hardware diagnostic situations where it might be impossible
to tell the origin of a failure from a simple alarm or fault bit set. We really
want to access the status registers, and for a multi-page pmbus device, this is
pretty tricky from userspace.

Please let me know your thoughts,
Thanks,

Edward A. James (3):
  drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
    default
  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 1/3] drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by default

[Eddie James]

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

Pmbus core always reads byte data from the status register, even if
configured to use STATUS_WORD. Use a function pointer so we always do
either byte or word access depending on which register we're trying to
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 f1eff6b..8511aba 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -113,7 +113,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;
 };
@@ -324,7 +325,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))
@@ -348,6 +349,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);
@@ -393,9 +424,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];
 
@@ -1051,8 +1081,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,
@@ -1729,6 +1758,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)
 {
@@ -1736,16 +1775,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 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 8511aba..3b53fa7 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)
@@ -421,11 +421,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];
 
@@ -718,6 +721,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.
@@ -746,12 +761,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;
 
@@ -890,7 +905,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;
@@ -906,7 +921,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);
 }
@@ -992,7 +1007,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 */
@@ -1337,6 +1352,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),
 	}, {
@@ -1421,6 +1437,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 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 3b53fa7..00ce145 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -86,6 +86,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;
@@ -851,6 +859,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) {
@@ -983,6 +1001,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.
  */
@@ -1105,6 +1142,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;
 }
 
@@ -1693,6 +1740,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

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Guenter Roeck]

On 08/07/2017 08:25 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Hi Guenter,
> 
> I'm looking for some feedback for some extensions to the pmbus core. We're
> looking for some additional functionality, particularly with STATUS_WORD and
> obtaining raw status data.
> 
> The first two patches enable the use of the STATUS_WORD register in the pmbus
> core. This allows the use of more default alarm/fault attributes for default
> pmbus sensors by allowing the use of the higher byte status bits.
> 
> 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 (e.g. STATUS_INPUT and STATUS_TEMPERATURE). I
> realize this is not really "normal" for hwmon or pmbus. These are potentially
> very useful in hardware diagnostic situations where it might be impossible
> to tell the origin of a failure from a simple alarm or fault bit set. We really
> want to access the status registers, and for a multi-page pmbus device, this is
> pretty tricky from userspace.
> 
> Please let me know your thoughts,
> Thanks,

I don't mind providing such data with debugfs, for example, but I don't see
the point in providing it as part of the ABI. Which, in part, since it requires
a lot of thought on my side, is part of the reason why I didn't provide
feedback to your earlier patches yet. Sorry, I've been exceptionally busy
lately, and non-standard requests tend to end up at the end of the queue :-(.

Any reason why debugfs is not sufficient and/or acceptable for your use case ?
You _are_ talking about diagnostic situations, which seems to be an exact fit
for debugfs.

Guenter

> 
> Edward A. James (3):
>    drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
>      default
>    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(-)
> 

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Eddie James]

On 08/07/2017 11:00 PM, Guenter Roeck wrote:
> On 08/07/2017 08:25 PM, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Hi Guenter,
>>
>> I'm looking for some feedback for some extensions to the pmbus core. 
>> We're
>> looking for some additional functionality, particularly with 
>> STATUS_WORD and
>> obtaining raw status data.
>>
>> The first two patches enable the use of the STATUS_WORD register in 
>> the pmbus
>> core. This allows the use of more default alarm/fault attributes for 
>> default
>> pmbus sensors by allowing the use of the higher byte status bits.
>>
>> 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 (e.g. STATUS_INPUT and 
>> STATUS_TEMPERATURE). I
>> realize this is not really "normal" for hwmon or pmbus. These are 
>> potentially
>> very useful in hardware diagnostic situations where it might be 
>> impossible
>> to tell the origin of a failure from a simple alarm or fault bit set. 
>> We really
>> want to access the status registers, and for a multi-page pmbus 
>> device, this is
>> pretty tricky from userspace.
>>
>> Please let me know your thoughts,
>> Thanks,
>
> I don't mind providing such data with debugfs, for example, but I 
> don't see
> the point in providing it as part of the ABI. Which, in part, since it 
> requires
> a lot of thought on my side, is part of the reason why I didn't provide
> feedback to your earlier patches yet. Sorry, I've been exceptionally busy
> lately, and non-standard requests tend to end up at the end of the 
> queue :-(.

No problem! Thanks for the quick response on this.

>
> Any reason why debugfs is not sufficient and/or acceptable for your 
> use case ?
> You _are_ talking about diagnostic situations, which seems to be an 
> exact fit
> for debugfs.

Agreed, great idea, I think debugfs will work perfectly. I probably 
should have thought of that sooner...

How about the first two patches in the series? They are unrelated to 
adding any attributes. Mainly I would
like to have the PB_STATUS_INPUT bit available to trigger the default 
boolean alarm attribute, as our hardware doesn't support any limits.

Thanks again,
Eddie

>
> Guenter
>
>>
>> Edward A. James (3):
>>    drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
>>      default
>>    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(-)
>>
>

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Christopher Bostic]

On 8/8/17 11:12 AM, Eddie James wrote:
>
>
> On 08/07/2017 11:00 PM, Guenter Roeck wrote:
>> On 08/07/2017 08:25 PM, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>>>
>>> Hi Guenter,
>>>
>>> I'm looking for some feedback for some extensions to the pmbus core. 
>>> We're
>>> looking for some additional functionality, particularly with 
>>> STATUS_WORD and
>>> obtaining raw status data.
>>>
>>> The first two patches enable the use of the STATUS_WORD register in 
>>> the pmbus
>>> core. This allows the use of more default alarm/fault attributes for 
>>> default
>>> pmbus sensors by allowing the use of the higher byte status bits.
>>>
>>> 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 (e.g. STATUS_INPUT and 
>>> STATUS_TEMPERATURE). I
>>> realize this is not really "normal" for hwmon or pmbus. These are 
>>> potentially
>>> very useful in hardware diagnostic situations where it might be 
>>> impossible
>>> to tell the origin of a failure from a simple alarm or fault bit 
>>> set. We really
>>> want to access the status registers, and for a multi-page pmbus 
>>> device, this is
>>> pretty tricky from userspace.
>>>
>>> Please let me know your thoughts,
>>> Thanks,
>>
>> I don't mind providing such data with debugfs, for example, but I 
>> don't see
>> the point in providing it as part of the ABI. Which, in part, since 
>> it requires
>> a lot of thought on my side, is part of the reason why I didn't provide
>> feedback to your earlier patches yet. Sorry, I've been exceptionally 
>> busy
>> lately, and non-standard requests tend to end up at the end of the 
>> queue :-(.
Hi Guenter,

In this case we're pulling fail data out of the hardware to diagnose 
what happened, so just wanted to verify that debugfs is still ok. I was 
assuming it was for internal kernel debugging and not hardware fault 
logging.

Thanks,
Chris


>
> No problem! Thanks for the quick response on this.
>
>>
>> Any reason why debugfs is not sufficient and/or acceptable for your 
>> use case ?
>> You _are_ talking about diagnostic situations, which seems to be an 
>> exact fit
>> for debugfs.
>
> Agreed, great idea, I think debugfs will work perfectly. I probably 
> should have thought of that sooner...
>
> How about the first two patches in the series? They are unrelated to 
> adding any attributes. Mainly I would
> like to have the PB_STATUS_INPUT bit available to trigger the default 
> boolean alarm attribute, as our hardware doesn't support any limits.
>
> Thanks again,
> Eddie
>
>>
>> Guenter
>>
>>>
>>> Edward A. James (3):
>>>    drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
>>>      default
>>>    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(-)
>>>
>>
>

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Guenter Roeck]

On Tue, Aug 08, 2017 at 02:26:15PM -0500, Christopher Bostic wrote:
> 
> 
> On 8/8/17 11:12 AM, Eddie James wrote:
> >
> >
> >On 08/07/2017 11:00 PM, Guenter Roeck wrote:
> >>On 08/07/2017 08:25 PM, Eddie James wrote:
> >>>From: "Edward A. James" <eajames@us.ibm.com>
> >>>
> >>>Hi Guenter,
> >>>
> >>>I'm looking for some feedback for some extensions to the pmbus core.
> >>>We're
> >>>looking for some additional functionality, particularly with
> >>>STATUS_WORD and
> >>>obtaining raw status data.
> >>>
> >>>The first two patches enable the use of the STATUS_WORD register in
> >>>the pmbus
> >>>core. This allows the use of more default alarm/fault attributes for
> >>>default
> >>>pmbus sensors by allowing the use of the higher byte status bits.
> >>>
> >>>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 (e.g. STATUS_INPUT and
> >>>STATUS_TEMPERATURE). I
> >>>realize this is not really "normal" for hwmon or pmbus. These are
> >>>potentially
> >>>very useful in hardware diagnostic situations where it might be
> >>>impossible
> >>>to tell the origin of a failure from a simple alarm or fault bit set.
> >>>We really
> >>>want to access the status registers, and for a multi-page pmbus
> >>>device, this is
> >>>pretty tricky from userspace.
> >>>
> >>>Please let me know your thoughts,
> >>>Thanks,
> >>
> >>I don't mind providing such data with debugfs, for example, but I don't
> >>see
> >>the point in providing it as part of the ABI. Which, in part, since it
> >>requires
> >>a lot of thought on my side, is part of the reason why I didn't provide
> >>feedback to your earlier patches yet. Sorry, I've been exceptionally
> >>busy
> >>lately, and non-standard requests tend to end up at the end of the queue
> >>:-(.
> Hi Guenter,
> 
> In this case we're pulling fail data out of the hardware to diagnose what
> happened, so just wanted to verify that debugfs is still ok. I was assuming
> it was for internal kernel debugging and not hardware fault logging.
> 
News to me, but who am I to know. Do you have a reference for that ?

debugfs is often used to display register contents. Are you sure it is even
possible to clearly distinguish kernel debugging from hardware fault logging
(or, rather, in this case from displaying raw status register values) ?

Anyway, debugfs is fine with me.

Guenter

> Thanks,
> Chris
> 
> 
> >
> >No problem! Thanks for the quick response on this.
> >
> >>
> >>Any reason why debugfs is not sufficient and/or acceptable for your use
> >>case ?
> >>You _are_ talking about diagnostic situations, which seems to be an
> >>exact fit
> >>for debugfs.
> >
> >Agreed, great idea, I think debugfs will work perfectly. I probably should
> >have thought of that sooner...
> >
> >How about the first two patches in the series? They are unrelated to
> >adding any attributes. Mainly I would
> >like to have the PB_STATUS_INPUT bit available to trigger the default
> >boolean alarm attribute, as our hardware doesn't support any limits.
> >
> >Thanks again,
> >Eddie
> >
> >>
> >>Guenter
> >>
> >>>
> >>>Edward A. James (3):
> >>>   drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
> >>>     default
> >>>   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(-)
> >>>
> >>
> >
> 

Re: [RFC, 1/3] drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by default

[Guenter Roeck]

On Mon, Aug 07, 2017 at 10:25:46PM -0500, eajames@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Pmbus core always reads byte data from the status register, even if
> configured to use STATUS_WORD. Use a function pointer so we always do
> either byte or word access depending on which register we're trying to
> 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 f1eff6b..8511aba 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -113,7 +113,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;
>  };
> @@ -324,7 +325,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))
> @@ -348,6 +349,36 @@ static bool pmbus_check_register(struct i2c_client *client,
>  	return rv >= 0;
>  }
>  
> +/* Check specified page status register accessibility. Need a separate

I am not in favor of networking-style multi-line comments.

> + * 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;

If you want to use bool, please use bool.

> +
> +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);
> @@ -393,9 +424,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);

This doesn't really work anymore, since the status register now has to hold 16
bit. Sure, the next patch tries to fix that, but that leaves a 1-patch gap.

Changing the width of data->status to 16 bit results in a "waste" of a whopping
32 * 6 + 2 or around 200 bytes. I don't think that warrants the complexity you
are introducing with the next patch. Are you sure that adds less than 200 bytes
of code ?)

>  			for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
>  				struct _pmbus_status *s = &pmbus_status[j];
>  
> @@ -1051,8 +1081,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,
> @@ -1729,6 +1758,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)
>  {
> @@ -1736,16 +1775,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;
>  		}

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Guenter Roeck]

On Tue, Aug 08, 2017 at 11:12:19AM -0500, Eddie James wrote:
> 
> 
> On 08/07/2017 11:00 PM, Guenter Roeck wrote:
> >On 08/07/2017 08:25 PM, Eddie James wrote:
> >>From: "Edward A. James" <eajames@us.ibm.com>
> >>
> >>Hi Guenter,
> >>
> >>I'm looking for some feedback for some extensions to the pmbus core.
> >>We're
> >>looking for some additional functionality, particularly with STATUS_WORD
> >>and
> >>obtaining raw status data.
> >>
> >>The first two patches enable the use of the STATUS_WORD register in the
> >>pmbus
> >>core. This allows the use of more default alarm/fault attributes for
> >>default
> >>pmbus sensors by allowing the use of the higher byte status bits.
> >>
> >>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 (e.g. STATUS_INPUT and
> >>STATUS_TEMPERATURE). I
> >>realize this is not really "normal" for hwmon or pmbus. These are
> >>potentially
> >>very useful in hardware diagnostic situations where it might be
> >>impossible
> >>to tell the origin of a failure from a simple alarm or fault bit set. We
> >>really
> >>want to access the status registers, and for a multi-page pmbus device,
> >>this is
> >>pretty tricky from userspace.
> >>
> >>Please let me know your thoughts,
> >>Thanks,
> >
> >I don't mind providing such data with debugfs, for example, but I don't
> >see
> >the point in providing it as part of the ABI. Which, in part, since it
> >requires
> >a lot of thought on my side, is part of the reason why I didn't provide
> >feedback to your earlier patches yet. Sorry, I've been exceptionally busy
> >lately, and non-standard requests tend to end up at the end of the queue
> >:-(.
> 
> No problem! Thanks for the quick response on this.
> 
> >
> >Any reason why debugfs is not sufficient and/or acceptable for your use
> >case ?
> >You _are_ talking about diagnostic situations, which seems to be an exact
> >fit
> >for debugfs.
> 
> Agreed, great idea, I think debugfs will work perfectly. I probably should
> have thought of that sooner...
> 
> How about the first two patches in the series? They are unrelated to adding
> any attributes. Mainly I would
> like to have the PB_STATUS_INPUT bit available to trigger the default
> boolean alarm attribute, as our hardware doesn't support any limits.
> 
Main problem with it is that patch 2 assumes that the word status register
and thus PB_STATUS_INPUT is available, which is not always the case.
On affected chips, the code will now generate the input attributes even
if those are not really supported.

Guenter

> Thanks again,
> Eddie
> 
> >
> >Guenter
> >
> >>
> >>Edward A. James (3):
> >>   drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
> >>     default
> >>   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(-)
> >>
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC,2/3] drivers/hmwon/pmbus: store STATUS_WORD in status registers

[Guenter Roeck]

On Mon, Aug 07, 2017 at 10:25:47PM -0500, eajames@linux.vnet.ibm.com wrote:
> 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 8511aba..3b53fa7 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)
> @@ -421,11 +421,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;
> +

As mentioned before, I don't think the "waste" of some 200 bytes of memory
warrants the added complexity here.

>  			for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
>  				struct _pmbus_status *s = &pmbus_status[j];
>  
> @@ -718,6 +721,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;
> +}

... and much less here. How much additional code does this generate ?
How much execution time does it add to save some 200 bytes of data ?

> +
>  /*
>   * Return boolean calculated from converted data.
>   * <index> defines a status register index and mask.
> @@ -746,12 +761,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;
>  
> @@ -890,7 +905,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;
> @@ -906,7 +921,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);
>  }
> @@ -992,7 +1007,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 */
> @@ -1337,6 +1352,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,

The problem with this is that it assumes that the generic status bit is always
available, which is not the case. You would have to add more conditionals
when generating the attributes: An attribute must only be generated based
on the generic status if the generic status bit is available. It is not
available if (gbit & 0xff00) and there is no STATUS_WORD register.

>  		.limit = iin_limit_attrs,
>  		.nlimit = ARRAY_SIZE(iin_limit_attrs),
>  	}, {
> @@ -1421,6 +1437,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),
>  	}, {

Re: [RFC,3/3] drivers/hwmon/pmbus: Add sensor status to pmbus attributes

[Guenter Roeck]

On Mon, Aug 07, 2017 at 10:25:48PM -0500, eajames@linux.vnet.ibm.com wrote:
> 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>

NACK, sorry. As mentioned separately, please consider adding debugfs
support instead.

Thanks,
Guenter

> ---
>  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 3b53fa7..00ce145 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -86,6 +86,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;
> @@ -851,6 +859,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) {
> @@ -983,6 +1001,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.
>   */
> @@ -1105,6 +1142,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;
>  }
>  
> @@ -1693,6 +1740,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++;
>  		}