hwmon (pmbus): cffps: Add debugfs entries

PATCH Superseded Eddie James 2 participants 7 messages Dec 7, 2017 Hardware monitoring

lore

Share & tools

lore

Eddie James

author

2017-12-07 19:50 UTC #0 lore raw

cover hwmon (pmbus): cffps: Add debugfs entries

From: "Edward A. James" <[email protected]>

This series enables debugfs entries in the IBM common form factor power supply driver. In order to avoid multiple debugfs directories for the same driver, the first patch in the series allows a pmbus client driver to query pmbus core for it's debugfs directory.

The second patch in the series adds debugfs files for a number of attributes of the power supply, including FRU (field replaceable unit) number, firmware version, part number, serial number, and CCIN. There is also an entry for the psu "input history" in binary format. This data represents previous 10 minutes of power supply data.

Edward A. James (2):
hwmon: (pmbus): Export pmbus device debugfs directory entry hwmon (pmbus): cffps: Add debugfs entries

 drivers/hwmon/pmbus/ibm-cffps.c  | 199 ++++++++++++++++++++++++++++++++++++++-
 drivers/hwmon/pmbus/pmbus.h      |   2 +
 drivers/hwmon/pmbus/pmbus_core.c |   7 ++
 3 files changed, 207 insertions(+), 1 deletion(-)

Signature · 1 line

1.8.3.1

Eddie James

author

2017-12-07 19:50 UTC #1 lore raw

PATCH 1/2 hwmon: (pmbus): Export pmbus device debugfs directory entry

From: "Edward A. James" <[email protected]>

Pmbus client drivers, if they want to use debugfs, should use the same root directory as the pmbus debugfs entries are using. Therefore, export the device dentry for the pmbus client.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/pmbus/pmbus.h      | 2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 7 +++++++
 2 files changed, 9 insertions(+)

drivers/hwmon/pmbus/pmbus.h +2 -0

index d39d506..1d24397 100644
@@ -461,4 +461,6 @@ int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
 			      enum pmbus_fan_mode mode);
 int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		     u8 config, u8 mask, u16 command);
+struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client);
+
 #endif /* PMBUS_H */

drivers/hwmon/pmbus/pmbus_core.c +7 -0

index 99ab39f..9abd382 100644
@@ -2381,6 +2381,13 @@ int pmbus_do_remove(struct i2c_client *client)
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);
 
+struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	return data->debugfs;
+}
+
 static int __init pmbus_core_init(void)
 {
 	pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);

Signature · 1 line

1.8.3.1

Guenter Roeck

maintainer

2017-12-07 20:42 UTC #2 lore raw

Re: [PATCH 1/2] hwmon: (pmbus): Export pmbus device debugfs directory entry

Quoted 28 lines · Eddie James

From: "Edward A. James" <[email protected]>
Pmbus client drivers, if they want to use debugfs, should use the same root directory as the pmbus debugfs entries are using. Therefore, export the device dentry for the pmbus client.
Signed-off-by Edward A. James <[email protected]>
---
 drivers/hwmon/pmbus/pmbus.h      | 2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 7 +++++++
 2 files changed, 9 insertions(+)
drivers/hwmon/pmbus/pmbus.h +2 -0
index d39d506..1d24397 100644
@@ -461,4 +461,6 @@ int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
 			      enum pmbus_fan_mode mode);
 int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		     u8 config, u8 mask, u16 command);
+struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client);
+
 #endif /* PMBUS_H */
drivers/hwmon/pmbus/pmbus_core.c +0 -0
index 99ab39f..9abd382 100644

drivers/hwmon/pmbus/pmbus_core.c int pmbus_do_remove(struct i2c_client *client) · line 2389

Quoted 9 lines

drivers/hwmon/pmbus/pmbus_core.c +6 -0

@@ -2381,6 +2381,13 @@ int pmbus_do_remove(struct i2c_client *client)
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);
 
+struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	return data->debugfs;
+}

EXPORT_SYMBOL_GPL(pmbus_get_debugfs_dir);

Quoted 11 lines

drivers/hwmon/pmbus/pmbus_core.c +1 -0

+
 static int __init pmbus_core_init(void)
 {
 	pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eddie James

author

2017-12-07 19:50 UTC #3 lore raw

PATCH 2/2 hwmon (pmbus): cffps: Add debugfs entries

In reply to Eddie James

From: "Edward A. James" <[email protected]>

Add debugfs entries for additional power supply data, including part number, serial number, FRU number, firmware revision, ccin, and the input history of the power supply. The input history is 10 minutes of input power data in the form of twenty 30-second packets. Each packet contains average and maximum power for that 30 second period.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 199 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)

drivers/hwmon/pmbus/ibm-cffps.c +198 -1

index cb56da6..6d19399 100644
@@ -8,12 +8,26 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 
 #include "pmbus.h"
 
+#define CFFPS_FRU_CMD				0x9A
+#define CFFPS_PN_CMD				0x9B
+#define CFFPS_SN_CMD				0x9E
+#define CFFPS_CCIN_CMD				0xBD
+#define CFFPS_FW_CMD_START			0xFA
+#define CFFPS_FW_NUM_BYTES			4
+
+#define CFFPS_INPUT_HISTORY_CMD			0xD6
+#define CFFPS_INPUT_HISTORY_SIZE		100
+
 /* STATUS_MFR_SPECIFIC bits */
 #define CFFPS_MFR_FAN_FAULT			BIT(0)
 #define CFFPS_MFR_THERMAL_FAULT			BIT(1)

@@ -24,6 +38,144 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+enum {
+	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
+	CFFPS_DEBUGFS_FRU,
+	CFFPS_DEBUGFS_PN,
+	CFFPS_DEBUGFS_SN,
+	CFFPS_DEBUGFS_CCIN,
+	CFFPS_DEBUGFS_FW,
+	CFFPS_DEBUGFS_NUM_ENTRIES
+};
+
+struct ibm_cffps_input_history {
+	struct mutex update_lock;
+	unsigned long last_update;
+
+	u8 byte_count;
+	u8 data[CFFPS_INPUT_HISTORY_SIZE];
+};
+
+struct ibm_cffps {
+	struct i2c_client *client;
+
+	struct ibm_cffps_input_history input_history;
+
+	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])
+
+static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
+{
+	int rc;
+	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
+	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
+	struct i2c_msg msg[2] = {
+		{
+			.addr = psu->client->addr,
+			.flags = psu->client->flags,
+			.len = 1,
+			.buf = msgbuf0,
+		}, {
+			.addr = psu->client->addr,
+			.flags = psu->client->flags | I2C_M_RD,
+			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
+			.buf = msgbuf1,
+		},
+	};
+
+	if (!*ppos) {
+		mutex_lock(&psu->input_history.update_lock);
+		if (time_after(jiffies, psu->input_history.last_update + HZ)) {
+			/*
+			 * Use a raw i2c transfer, since we need more bytes
+			 * than Linux I2C supports through smbus xfr (only 32).
+			 */
+			rc = i2c_transfer(psu->client->adapter, msg, 2);
+			if (rc < 0) {
+				mutex_unlock(&psu->input_history.update_lock);
+				return rc;
+			}
+
+			psu->input_history.byte_count = msgbuf1[0];
+			memcpy(psu->input_history.data, &msgbuf1[1],
+			       CFFPS_INPUT_HISTORY_SIZE);
+			psu->input_history.last_update = jiffies;
+		}
+
+		mutex_unlock(&psu->input_history.update_lock);
+	}
+
+	return simple_read_from_buffer(buf, count, ppos,
+				       psu->input_history.data,
+				       psu->input_history.byte_count);
+}
+
+static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	u8 cmd;
+	int i, rc;
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct ibm_cffps *psu = to_psu(idxp, idx);
+	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+
+	switch (idx) {
+	case CFFPS_DEBUGFS_INPUT_HISTORY:
+		return ibm_cffps_read_input_history(psu, buf, count, ppos);
+	case CFFPS_DEBUGFS_FRU:
+		cmd = CFFPS_FRU_CMD;
+		break;
+	case CFFPS_DEBUGFS_PN:
+		cmd = CFFPS_PN_CMD;
+		break;
+	case CFFPS_DEBUGFS_SN:
+		cmd = CFFPS_SN_CMD;
+		break;
+	case CFFPS_DEBUGFS_CCIN:
+		rc = i2c_smbus_read_word_data(psu->client, CFFPS_CCIN_CMD);
+		if (rc < 0)
+			return rc;
+
+		rc = snprintf(data, 5, "%04X", rc);
+		goto done;
+	case CFFPS_DEBUGFS_FW:
+		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
+			rc = i2c_smbus_read_byte_data(psu->client,
+						      CFFPS_FW_CMD_START + i);
+			if (rc < 0)
+				return rc;
+
+			snprintf(&data[i * 2], 3, "%02X", rc);
+		}
+
+		rc = i * 2;
+		goto done;
+	default:
+		return -EINVAL;
+	}
+
+	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+	if (rc < 0)
+		return rc;
+
+done:
+	data[rc] = '\n';
+	rc += 2;
+
+	return simple_read_from_buffer(buf, count, ppos, data, rc);
+}
+
+static const struct file_operations ibm_cffps_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_debugfs_op,
+	.open = simple_open,
+};
+
 static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
 				    int reg)
 {

@@ -119,7 +271,52 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
-	return pmbus_do_probe(client, id, &ibm_cffps_info);
+	int i, rc;
+	struct dentry *debugfs;
+	struct ibm_cffps *psu;
+
+	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
+	if (rc)
+		return rc;
+
+	/* Don't fail the probe if we can't create debugfs */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	if (jiffies > HZ)
+		/* time_after wouldn't succeed with last_update = 0 in test */
+		psu->input_history.last_update = jiffies - HZ;
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return 0;
+
+	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
+		psu->debugfs_entries[i] = i;
+
+	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_INPUT_HISTORY],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fru", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FRU],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_pn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_PN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_sn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_SN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_ccin", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_CCIN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fw", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FW],
+			    &ibm_cffps_fops);
+
+	return 0;
 }
 
 static const struct i2c_device_id ibm_cffps_id[] = {

Signature · 1 line

1.8.3.1

Guenter Roeck

maintainer

2017-12-07 20:53 UTC #4 lore raw

Quoted 201 lines · Eddie James

From: "Edward A. James" <[email protected]>

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 199 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)

drivers/hwmon/pmbus/ibm-cffps.c +159 -1

index cb56da6..6d19399 100644
@@ -8,12 +8,26 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 
 #include "pmbus.h"
 
+#define CFFPS_FRU_CMD				0x9A
+#define CFFPS_PN_CMD				0x9B
+#define CFFPS_SN_CMD				0x9E
+#define CFFPS_CCIN_CMD				0xBD
+#define CFFPS_FW_CMD_START			0xFA
+#define CFFPS_FW_NUM_BYTES			4
+
+#define CFFPS_INPUT_HISTORY_CMD			0xD6
+#define CFFPS_INPUT_HISTORY_SIZE		100
+
 /* STATUS_MFR_SPECIFIC bits */
 #define CFFPS_MFR_FAN_FAULT			BIT(0)
 #define CFFPS_MFR_THERMAL_FAULT			BIT(1)

@@ -24,6 +38,144 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+enum {
+	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
+	CFFPS_DEBUGFS_FRU,
+	CFFPS_DEBUGFS_PN,
+	CFFPS_DEBUGFS_SN,
+	CFFPS_DEBUGFS_CCIN,
+	CFFPS_DEBUGFS_FW,
+	CFFPS_DEBUGFS_NUM_ENTRIES
+};
+
+struct ibm_cffps_input_history {
+	struct mutex update_lock;
+	unsigned long last_update;
+
+	u8 byte_count;
+	u8 data[CFFPS_INPUT_HISTORY_SIZE];
+};
+
+struct ibm_cffps {
+	struct i2c_client *client;
+
+	struct ibm_cffps_input_history input_history;
+
+	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])
+
+static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
+{
+	int rc;
+	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
+	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
+	struct i2c_msg msg[2] = {
+		{
+			.addr = psu->client->addr,
+			.flags = psu->client->flags,
+			.len = 1,
+			.buf = msgbuf0,
+		}, {
+			.addr = psu->client->addr,
+			.flags = psu->client->flags | I2C_M_RD,
+			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
+			.buf = msgbuf1,
+		},
+	};
+
+	if (!*ppos) {
+		mutex_lock(&psu->input_history.update_lock);
+		if (time_after(jiffies, psu->input_history.last_update + HZ)) {
+			/*
+			 * Use a raw i2c transfer, since we need more bytes
+			 * than Linux I2C supports through smbus xfr (only 32).
+			 */
+			rc = i2c_transfer(psu->client->adapter, msg, 2);
+			if (rc < 0) {
+				mutex_unlock(&psu->input_history.update_lock);
+				return rc;
+			}
+
+			psu->input_history.byte_count = msgbuf1[0];
+			memcpy(psu->input_history.data, &msgbuf1[1],
+			       CFFPS_INPUT_HISTORY_SIZE);
+			psu->input_history.last_update = jiffies;
+		}
+
+		mutex_unlock(&psu->input_history.update_lock);
+	}
+
+	return simple_read_from_buffer(buf, count, ppos,
+				       psu->input_history.data,
+				       psu->input_history.byte_count);
+}
+
+static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	u8 cmd;
+	int i, rc;
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct ibm_cffps *psu = to_psu(idxp, idx);
+	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+
+	switch (idx) {
+	case CFFPS_DEBUGFS_INPUT_HISTORY:
+		return ibm_cffps_read_input_history(psu, buf, count, ppos);
+	case CFFPS_DEBUGFS_FRU:
+		cmd = CFFPS_FRU_CMD;
+		break;
+	case CFFPS_DEBUGFS_PN:
+		cmd = CFFPS_PN_CMD;
+		break;
+	case CFFPS_DEBUGFS_SN:
+		cmd = CFFPS_SN_CMD;
+		break;
+	case CFFPS_DEBUGFS_CCIN:
+		rc = i2c_smbus_read_word_data(psu->client, CFFPS_CCIN_CMD);
+		if (rc < 0)
+			return rc;
+
+		rc = snprintf(data, 5, "%04X", rc);
+		goto done;
+	case CFFPS_DEBUGFS_FW:
+		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
+			rc = i2c_smbus_read_byte_data(psu->client,
+						      CFFPS_FW_CMD_START + i);
+			if (rc < 0)
+				return rc;
+
+			snprintf(&data[i * 2], 3, "%02X", rc);
+		}
+
+		rc = i * 2;
+		goto done;
+	default:
+		return -EINVAL;
+	}
+
+	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+	if (rc < 0)
+		return rc;
+
+done:
+	data[rc] = '\n';
+	rc += 2;
+
+	return simple_read_from_buffer(buf, count, ppos, data, rc);
+}
+
+static const struct file_operations ibm_cffps_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_debugfs_op,
+	.open = simple_open,
+};
+
 static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
 				    int reg)
 {

@@ -119,7 +271,52 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
-	return pmbus_do_probe(client, id, &ibm_cffps_info);
+	int i, rc;
+	struct dentry *debugfs;
+	struct ibm_cffps *psu;
+
+	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
+	if (rc)
+		return rc;

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 290

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +10 -0

+
+	/* Don't fail the probe if we can't create debugfs */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	if (jiffies > HZ)
+		/* time_after wouldn't succeed with last_update = 0 in test */

... but you are setting it to 'jiffies' elsewhere, so it can still end up being 0 at some point, especially since 'jiffies' starts with a large number and wraps a few minutes after boot. Not sure I understand the problem, actually. Can you elaborate ?

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 296

Quoted 6 lines

drivers/hwmon/pmbus/ibm-cffps.c +6 -0

+		psu->input_history.last_update = jiffies - HZ;
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return 0;
+

It might be better to do that first, to avoid allocating memory unnecessarily. That memory would only be freed when the driver is unloaded.

Quoted 11 lines

drivers/hwmon/pmbus/ibm-cffps.c +11 -0

+	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
+		psu->debugfs_entries[i] = i;
+
+	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_INPUT_HISTORY],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fru", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FRU],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_pn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_PN],

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 317

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +10 -0

+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_sn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_SN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_ccin", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_CCIN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fw", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FW],
+			    &ibm_cffps_fops);

Can you put those into their own subdirectory ?

	.../ibm_cffps/input_history
	.../ibm_cffps/fru

and so on. Also, consider using ibm_cffps1 (or client->name) to align with the the hwmon 'name' attribute.

Thanks,
Guenter

Quoted 8 lines

drivers/hwmon/pmbus/ibm-cffps.c +2 -0
+
+	return 0;
 }
 
 static const struct i2c_device_id ibm_cffps_id[] = {
Signature · 1 line
1.8.3.1

Eddie James

author

2017-12-07 22:19 UTC #5 lore raw

Quoted 208 lines · Guenter Roeck

Quoted 207 lines · Eddie James

From: "Edward A. James" <[email protected]>

Signed-off-by Edward A. James <[email protected]>

---
  drivers/hwmon/pmbus/ibm-cffps.c | 199 +++++++++++++++++++++++++++++++++++++++-
  1 file changed, 198 insertions(+), 1 deletion(-)

drivers/hwmon/pmbus/ibm-cffps.c +165 -1

index cb56da6..6d19399 100644
@@ -8,12 +8,26 @@
   */
 
  #include <linux/bitops.h>
+#include <linux/debugfs.h>
  #include <linux/device.h>
+#include <linux/fs.h>
  #include <linux/i2c.h>
+#include <linux/jiffies.h>
  #include <linux/module.h>
+#include <linux/mutex.h>
 
  #include "pmbus.h"
 
+#define CFFPS_FRU_CMD				0x9A
+#define CFFPS_PN_CMD				0x9B
+#define CFFPS_SN_CMD				0x9E
+#define CFFPS_CCIN_CMD				0xBD
+#define CFFPS_FW_CMD_START			0xFA
+#define CFFPS_FW_NUM_BYTES			4
+
+#define CFFPS_INPUT_HISTORY_CMD			0xD6
+#define CFFPS_INPUT_HISTORY_SIZE		100
+
  /* STATUS_MFR_SPECIFIC bits */
  #define CFFPS_MFR_FAN_FAULT			BIT(0)
  #define CFFPS_MFR_THERMAL_FAULT			BIT(1)

@@ -24,6 +38,144 @@
  #define CFFPS_MFR_VAUX_FAULT			BIT(6)
  #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+enum {
+	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
+	CFFPS_DEBUGFS_FRU,
+	CFFPS_DEBUGFS_PN,
+	CFFPS_DEBUGFS_SN,
+	CFFPS_DEBUGFS_CCIN,
+	CFFPS_DEBUGFS_FW,
+	CFFPS_DEBUGFS_NUM_ENTRIES
+};
+
+struct ibm_cffps_input_history {
+	struct mutex update_lock;
+	unsigned long last_update;
+
+	u8 byte_count;
+	u8 data[CFFPS_INPUT_HISTORY_SIZE];
+};
+
+struct ibm_cffps {
+	struct i2c_client *client;
+
+	struct ibm_cffps_input_history input_history;
+
+	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])
+
+static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
+{
+	int rc;
+	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
+	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
+	struct i2c_msg msg[2] = {
+		{
+			.addr = psu->client->addr,
+			.flags = psu->client->flags,
+			.len = 1,
+			.buf = msgbuf0,
+		}, {
+			.addr = psu->client->addr,
+			.flags = psu->client->flags | I2C_M_RD,
+			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
+			.buf = msgbuf1,
+		},
+	};
+
+	if (!*ppos) {
+		mutex_lock(&psu->input_history.update_lock);
+		if (time_after(jiffies, psu->input_history.last_update + HZ)) {
+			/*
+			 * Use a raw i2c transfer, since we need more bytes
+			 * than Linux I2C supports through smbus xfr (only 32).
+			 */
+			rc = i2c_transfer(psu->client->adapter, msg, 2);
+			if (rc < 0) {
+				mutex_unlock(&psu->input_history.update_lock);
+				return rc;
+			}
+
+			psu->input_history.byte_count = msgbuf1[0];
+			memcpy(psu->input_history.data, &msgbuf1[1],
+			       CFFPS_INPUT_HISTORY_SIZE);
+			psu->input_history.last_update = jiffies;
+		}
+
+		mutex_unlock(&psu->input_history.update_lock);
+	}
+
+	return simple_read_from_buffer(buf, count, ppos,
+				       psu->input_history.data,
+				       psu->input_history.byte_count);
+}
+
+static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	u8 cmd;
+	int i, rc;
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct ibm_cffps *psu = to_psu(idxp, idx);
+	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+
+	switch (idx) {
+	case CFFPS_DEBUGFS_INPUT_HISTORY:
+		return ibm_cffps_read_input_history(psu, buf, count, ppos);
+	case CFFPS_DEBUGFS_FRU:
+		cmd = CFFPS_FRU_CMD;
+		break;
+	case CFFPS_DEBUGFS_PN:
+		cmd = CFFPS_PN_CMD;
+		break;
+	case CFFPS_DEBUGFS_SN:
+		cmd = CFFPS_SN_CMD;
+		break;
+	case CFFPS_DEBUGFS_CCIN:
+		rc = i2c_smbus_read_word_data(psu->client, CFFPS_CCIN_CMD);
+		if (rc < 0)
+			return rc;
+
+		rc = snprintf(data, 5, "%04X", rc);
+		goto done;
+	case CFFPS_DEBUGFS_FW:
+		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
+			rc = i2c_smbus_read_byte_data(psu->client,
+						      CFFPS_FW_CMD_START + i);
+			if (rc < 0)
+				return rc;
+
+			snprintf(&data[i * 2], 3, "%02X", rc);
+		}
+
+		rc = i * 2;
+		goto done;
+	default:
+		return -EINVAL;
+	}
+
+	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+	if (rc < 0)
+		return rc;
+
+done:
+	data[rc] = '\n';
+	rc += 2;
+
+	return simple_read_from_buffer(buf, count, ppos, data, rc);
+}
+
+static const struct file_operations ibm_cffps_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_debugfs_op,
+	.open = simple_open,
+};
+
  static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
  				    int reg)
  {

@@ -119,7 +271,52 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
  static int ibm_cffps_probe(struct i2c_client *client,
  			   const struct i2c_device_id *id)
  {
-	return pmbus_do_probe(client, id, &ibm_cffps_info);
+	int i, rc;
+	struct dentry *debugfs;
+	struct ibm_cffps *psu;
+
+	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
+	if (rc)
+		return rc;
+
+	/* Don't fail the probe if we can't create debugfs */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+

Quoted 8 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 290
Quoted 4 lines
drivers/hwmon/pmbus/ibm-cffps.c +4 -0
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	if (jiffies > HZ)
+		/* time_after wouldn't succeed with last_update = 0 in test */
... but you are setting it to 'jiffies' elsewhere, so it can still end up being 0 at some point, especially since 'jiffies' starts with a large number and wraps a few minutes after boot. Not sure I understand the problem, actually. Can you elaborate ?

Yea, not sure I understand it either, but basically, if we don't set last_update to jiffies - HZ or similar at start up, the check above for time_after(jiffies, last_upate + HZ) doesn't succeed soon after the boot. This doesn't make any sense to me, unless time_after is broken, but setting last_update to something relative to jiffies during probe fixes it.

[ 279.480000] failed time_after with jiffies[-2052] last_update[0] [ 302.050000] succeeded time_after with jiffies[205] last_update[0]

tested with this (should have used %u instead of %d, but you get the idea):

drivers/hwmon/pmbus/ibm-cffps.c +0 -0

index 6d19399..e5cb52a 100644
@@ -90,6 +90,7 @@ static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,

     if (!*ppos) {
         mutex_lock(&psu->input_history.update_lock);
         if (time_after(jiffies, psu->input_history.last_update + HZ)) { +printk("succeeded time_after with jiffies[%d] last_update[%d]\n", jiffies, psu->input_history.last_update); /*
             * Use a raw i2c transfer, since we need more bytes
             * than Linux I2C supports through smbus xfr (only 32).
@@ -105,6 +106,8 @@ static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu, CFFPS_INPUT_HISTORY_SIZE);
             psu->input_history.last_update = jiffies;
         }
+        else
+printk("failed time_after with jiffies[%d] last_update[%d]\n", jiffies, psu->input_history.last_update);

mutex_unlock(&psu->input_history.update_lock);
}
@@ -286,9 +289,6 @@ static int ibm_cffps_probe(struct i2c_client *client,

     psu->client = client; mutex_init(&psu->input_history.update_lock);
-    if (jiffies > HZ)
-        /* time_after wouldn't succeed with last_update = 0 in test */ -        psu->input_history.last_update = jiffies - HZ;

debugfs = pmbus_get_debugfs_dir(client);
if (!debugfs)

Quoted 9 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 296
Quoted 7 lines
drivers/hwmon/pmbus/ibm-cffps.c +6 -0
+		psu->input_history.last_update = jiffies - HZ;
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return 0;
+
It might be better to do that first, to avoid allocating memory unnecessarily. That memory would only be freed when the driver is unloaded.

True.

Quoted 24 lines

Quoted 12 lines

drivers/hwmon/pmbus/ibm-cffps.c +11 -0

+	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
+		psu->debugfs_entries[i] = i;
+
+	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_INPUT_HISTORY],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fru", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FRU],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_pn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_PN],

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 317

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +10 -0

+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_sn", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_SN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_ccin", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_CCIN],
+			    &ibm_cffps_fops);
+	debugfs_create_file("ibm_cffps_fw", 0444, debugfs,
+			    &psu->debugfs_entries[CFFPS_DEBUGFS_FW],
+			    &ibm_cffps_fops);

Can you put those into their own subdirectory ?

Quoted 5 lines

	.../ibm_cffps/input_history
	.../ibm_cffps/fru
and so on. Also, consider using ibm_cffps1 (or client->name) to align with the the hwmon 'name' attribute.

Good idea.

Thanks,
Eddie

Quoted 12 lines

Thanks,
Guenter
Quoted 8 lines
drivers/hwmon/pmbus/ibm-cffps.c +2 -0
+
+	return 0;
  }
 
  static const struct i2c_device_id ibm_cffps_id[] = {
Signature · 1 line
1.8.3.1

Guenter Roeck

maintainer

2017-12-07 23:18 UTC #6 lore raw

Quoted 216 lines · Eddie James

Quoted 204 lines · Guenter Roeck

Quoted 203 lines · Eddie James

From: "Edward A. James" <[email protected]>

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 199 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)

drivers/hwmon/pmbus/ibm-cffps.c +165 -1

index cb56da6..6d19399 100644
@@ -8,12 +8,26 @@
  */
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include "pmbus.h"
+#define CFFPS_FRU_CMD				0x9A
+#define CFFPS_PN_CMD				0x9B
+#define CFFPS_SN_CMD				0x9E
+#define CFFPS_CCIN_CMD				0xBD
+#define CFFPS_FW_CMD_START			0xFA
+#define CFFPS_FW_NUM_BYTES			4
+
+#define CFFPS_INPUT_HISTORY_CMD			0xD6
+#define CFFPS_INPUT_HISTORY_SIZE		100
+
 /* STATUS_MFR_SPECIFIC bits */
 #define CFFPS_MFR_FAN_FAULT			BIT(0)
 #define CFFPS_MFR_THERMAL_FAULT			BIT(1)

@@ -24,6 +38,144 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
+enum {
+	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
+	CFFPS_DEBUGFS_FRU,
+	CFFPS_DEBUGFS_PN,
+	CFFPS_DEBUGFS_SN,
+	CFFPS_DEBUGFS_CCIN,
+	CFFPS_DEBUGFS_FW,
+	CFFPS_DEBUGFS_NUM_ENTRIES
+};
+
+struct ibm_cffps_input_history {
+	struct mutex update_lock;
+	unsigned long last_update;
+
+	u8 byte_count;
+	u8 data[CFFPS_INPUT_HISTORY_SIZE];
+};
+
+struct ibm_cffps {
+	struct i2c_client *client;
+
+	struct ibm_cffps_input_history input_history;
+
+	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])
+
+static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
+{
+	int rc;
+	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
+	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
+	struct i2c_msg msg[2] = {
+		{
+			.addr = psu->client->addr,
+			.flags = psu->client->flags,
+			.len = 1,
+			.buf = msgbuf0,
+		}, {
+			.addr = psu->client->addr,
+			.flags = psu->client->flags | I2C_M_RD,
+			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
+			.buf = msgbuf1,
+		},
+	};
+
+	if (!*ppos) {
+		mutex_lock(&psu->input_history.update_lock);
+		if (time_after(jiffies, psu->input_history.last_update + HZ)) {
+			/*
+			 * Use a raw i2c transfer, since we need more bytes
+			 * than Linux I2C supports through smbus xfr (only 32).
+			 */
+			rc = i2c_transfer(psu->client->adapter, msg, 2);
+			if (rc < 0) {
+				mutex_unlock(&psu->input_history.update_lock);
+				return rc;
+			}
+
+			psu->input_history.byte_count = msgbuf1[0];
+			memcpy(psu->input_history.data, &msgbuf1[1],
+			       CFFPS_INPUT_HISTORY_SIZE);
+			psu->input_history.last_update = jiffies;
+		}
+
+		mutex_unlock(&psu->input_history.update_lock);
+	}
+
+	return simple_read_from_buffer(buf, count, ppos,
+				       psu->input_history.data,
+				       psu->input_history.byte_count);
+}
+
+static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	u8 cmd;
+	int i, rc;
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct ibm_cffps *psu = to_psu(idxp, idx);
+	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+
+	switch (idx) {
+	case CFFPS_DEBUGFS_INPUT_HISTORY:
+		return ibm_cffps_read_input_history(psu, buf, count, ppos);
+	case CFFPS_DEBUGFS_FRU:
+		cmd = CFFPS_FRU_CMD;
+		break;
+	case CFFPS_DEBUGFS_PN:
+		cmd = CFFPS_PN_CMD;
+		break;
+	case CFFPS_DEBUGFS_SN:
+		cmd = CFFPS_SN_CMD;
+		break;
+	case CFFPS_DEBUGFS_CCIN:
+		rc = i2c_smbus_read_word_data(psu->client, CFFPS_CCIN_CMD);
+		if (rc < 0)
+			return rc;
+
+		rc = snprintf(data, 5, "%04X", rc);
+		goto done;
+	case CFFPS_DEBUGFS_FW:
+		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
+			rc = i2c_smbus_read_byte_data(psu->client,
+						      CFFPS_FW_CMD_START + i);
+			if (rc < 0)
+				return rc;
+
+			snprintf(&data[i * 2], 3, "%02X", rc);
+		}
+
+		rc = i * 2;
+		goto done;
+	default:
+		return -EINVAL;
+	}
+
+	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+	if (rc < 0)
+		return rc;
+
+done:
+	data[rc] = '\n';
+	rc += 2;
+
+	return simple_read_from_buffer(buf, count, ppos, data, rc);
+}
+
+static const struct file_operations ibm_cffps_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_debugfs_op,
+	.open = simple_open,
+};
+
 static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
 				    int reg)
 {

@@ -119,7 +271,52 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
-	return pmbus_do_probe(client, id, &ibm_cffps_info);
+	int i, rc;
+	struct dentry *debugfs;
+	struct ibm_cffps *psu;
+
+	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
+	if (rc)
+		return rc;
+
+	/* Don't fail the probe if we can't create debugfs */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+

Quoted 8 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_read_word_data(struct i2c_client *client, int page, · line 290
Quoted 4 lines
drivers/hwmon/pmbus/ibm-cffps.c +4 -0
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	if (jiffies > HZ)
+		/* time_after wouldn't succeed with last_update = 0 in test */
... but you are setting it to 'jiffies' elsewhere, so it can still end up being 0 at some point, especially since 'jiffies' starts with a large number and wraps a few minutes after boot. Not sure I understand the problem, actually. Can you elaborate ?

Quoted 6 lines

Yea, not sure I understand it either, but basically, if we don't set last_update to jiffies - HZ or similar at start up, the check above for time_after(jiffies, last_upate + HZ) doesn't succeed soon after the boot. This doesn't make any sense to me, unless time_after is broken, but setting last_update to something relative to jiffies during probe fixes it.

That is easy; just set it unconditionally to jiffies - HZ. The 'jiffies > HZ' check is wrong; if jiffies happens to be < HZ, you'd still want to set the value to jiffies - HZ (and it will be somewhere between ULONG_MAX-HZ+1 .. ULONG_MAX).

Problem is that jiffies doesn't start with 0 but with something large. That is why drivers typically have a 'valid' flag in addition to the jiffies check.

Thanks,
Guenter