[PATCH 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase

[PATCH 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase

[Grant Peltier]

The patch series adds debugfs features for 2nd generation Renesas digital
multiphase voltage regulators. The new debugfs endpoints allow users to
utilize features of the VR that are not compatible with sysfs.

The series contains 2 patches:
 - patch #1 adds the new debugfs features to the isl68137 driver
 - patch #2 adds documentation for the new debugfs endpoints

Grant Peltier (2):
  hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
  docs: hwmon: Update driver documentation for debugfs

 Documentation/hwmon/isl68137.rst |  34 ++-
 drivers/hwmon/pmbus/isl68137.c   | 490 ++++++++++++++++++++++++++++++-
 2 files changed, 520 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Grant Peltier]

Add debugfs endpoints to support features of 2nd generation Renesas
digital multiphase voltage regulators that are not compatible with
sysfs.

The read_black_box endpoint allows users to read the contents of a
RAM segment used to record fault conditions within Gen 2 devices.

The write_config endpoint allows users to write configuration hex
files to Gen 2 devices which modify how they operate.

Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
 drivers/hwmon/pmbus/isl68137.c | 490 ++++++++++++++++++++++++++++++++-
 1 file changed, 487 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
index 0c622711ef7e..24af016729a3 100644
--- a/drivers/hwmon/pmbus/isl68137.c
+++ b/drivers/hwmon/pmbus/isl68137.c
@@ -7,10 +7,12 @@
  *
  */
 
+#include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/string.h>
@@ -18,8 +20,22 @@
 
 #include "pmbus.h"
 
-#define ISL68137_VOUT_AVS	0x30
-#define RAA_DMPVR2_READ_VMON	0xc8
+#define ISL68137_VOUT_AVS		0x30
+#define RAA_DMPVR2_DMA_FIX		0xc5
+#define RAA_DMPVR2_DMA_SEQ		0xc6
+#define RAA_DMPVR2_DMA_ADDR		0xc7
+#define RAA_DMPVR2_READ_VMON		0xc8
+#define RAA_DMPVR2_BB_BASE_ADDR		0xef80
+#define RAA_DMPVR2_BB_WSIZE		4
+#define RAA_DMPVR2_BB_WCNT		32
+#define RAA_DMPVR2_BB_BUF_SIZE		288
+#define RAA_DMPVR2_NVM_CNT_ADDR		0x00c2
+#define RAA_DMPVR2_PRGM_STATUS_ADDR	0x0707
+#define RAA_DMPVR2_BANK0_STATUS_ADDR	0x0709
+#define RAA_DMPVR2_BANK1_STATUS_ADDR	0x070a
+#define RAA_DMPVR2_CFG_MAX_SLOT		16
+#define RAA_DMPVR2_CFG_HEAD_LEN		290
+#define RAA_DMPVR2_CFG_SLOT_LEN		358
 
 enum chips {
 	isl68137,
@@ -71,6 +87,21 @@ enum variants {
 	raa_dmpvr2_hv,
 };
 
+enum {
+	RAA_DMPVR2_DEBUGFS_CFG_W = 0,
+	RAA_DMPVR2_DEBUGFS_BB_R,
+	RAA_DMPVR2_DEBUGFS_NUM_ENTRIES,
+};
+
+struct raa_dmpvr2_ctrl {
+	enum chips part;
+	struct i2c_client *client;
+	int debugfs_entries[RAA_DMPVR2_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_ctrl(x, y) container_of((x), struct raa_dmpvr2_ctrl, \
+				   debugfs_entries[(y)])
+
 static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
 					     int page,
 					     char *buf)
@@ -157,6 +188,425 @@ static const struct attribute_group *isl68137_attribute_groups[] = {
 	NULL,
 };
 
+/**
+ * Non-standard SMBus read function to account for I2C controllers that
+ * do not support SMBus block reads. Reads 5 bytes from client (length + 4 data
+ * bytes)
+ */
+static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
+			    unsigned char *data)
+{
+	int status;
+	unsigned char msgbuf[1];
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = client->flags,
+			.len = 1,
+			.buf = msgbuf,
+		},
+		{
+			.addr = client->addr,
+			.flags = client->flags | I2C_M_RD,
+			.len = 5,
+			.buf = data,
+		},
+	};
+
+	msgbuf[0] = command;
+	status = i2c_transfer(client->adapter, msg, 2);
+	if (status != 2)
+		return status;
+	return 0;
+}
+
+/**
+ * Helper function required since linux SMBus implementation does not currently
+ * (v5.6) support the SMBus 3.0 "Read 32" protocol
+ */
+static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
+				   unsigned char *data)
+{
+	int status;
+	unsigned char msgbuf[1];
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = client->flags,
+			.len = 1,
+			.buf = msgbuf,
+		},
+		{
+			.addr = client->addr,
+			.flags = client->flags | I2C_M_RD,
+			.len = 4,
+			.buf = data,
+		},
+	};
+
+	msgbuf[0] = command;
+	status = i2c_transfer(client->adapter, msg, 2);
+	if (status != 2)
+		return status;
+	return 0;
+}
+
+/**
+ * Helper function required since linux SMBus implementation does not currently
+ * (v5.6) support the SMBus 3.0 "Write 32" protocol
+ */
+static s32 raa_dmpvr2_smbus_write32(const struct i2c_client *client,
+				    u8 command, u32 value)
+{
+	int status;
+	unsigned char msgbuf[5];
+	struct i2c_msg msg[1] = {
+		{
+			.addr = client->addr,
+			.flags = client->flags,
+			.len = 5,
+			.buf = msgbuf,
+		},
+	};
+	msgbuf[0] = command;
+	msgbuf[1] = value & 0x0FF;
+	msgbuf[2] = (value >> 8) & 0x0FF;
+	msgbuf[3] = (value >> 16) & 0x0FF;
+	msgbuf[4] = (value >> 24) &  0x0FF;
+	status = i2c_transfer(client->adapter, msg, 1);
+	if (status != 1)
+		return status;
+	return 0;
+}
+
+static ssize_t raa_dmpvr2_read_black_box(struct raa_dmpvr2_ctrl *ctrl,
+					 char __user *buf, size_t len,
+					 loff_t *ppos)
+{
+	int i, j;
+	u16 addr = RAA_DMPVR2_BB_BASE_ADDR;
+	unsigned char data[RAA_DMPVR2_BB_WSIZE] = { 0 };
+	char out[RAA_DMPVR2_BB_BUF_SIZE] = { 0 };
+	char *optr = out;
+
+	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR, addr);
+	for (i = 0; i < RAA_DMPVR2_BB_WCNT; i++) {
+		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ,
+					data);
+		for (j = 0; j < RAA_DMPVR2_BB_WSIZE; j++)
+			optr += snprintf(optr, sizeof(out), "%02X", data[j]);
+		*optr = '\n';
+		optr++;
+	}
+
+	return simple_read_from_buffer(buf, len, ppos, &out, sizeof(out));
+}
+
+struct raa_dmpvr_cfg_cmd {
+	u8 cmd;
+	u8 len;
+	u8 data[4];
+};
+
+struct raa_dmpvr2_cfg {
+	u8 dev_id[4];
+	u8 dev_rev[4];
+	int slot_cnt;
+	int cmd_cnt;
+	struct raa_dmpvr_cfg_cmd *cmds;
+	u8 crc[4];
+};
+
+/**
+ * Helper function to handle hex string to byte conversions
+ */
+static int raa_dmpvr2_hextou8(char *buf, u8 *res)
+{
+	char s[3];
+
+	s[0] = buf[0];
+	s[1] = buf[1];
+	s[2] = '\0';
+	return kstrtou8(s, 16, res);
+}
+
+static int raa_dmpvr2_parse_cfg(char *buf, struct raa_dmpvr2_cfg *cfg)
+{
+	const int lsta = 2;
+	const int csta = 6;
+	const int dsta = 8;
+	char *cptr, *line;
+	int lcnt = 1;
+	int ccnt = 0;
+	int i, j, ret;
+	u8 b;
+
+	// Ensure there is enough memory for the file
+	for (i = 0; i < strlen(buf); i++) {
+		if (buf[i] == '\n' && i < strlen(buf)-2) {
+			lcnt++;
+			if (buf[i+1] != '4' || buf[i+2] != '9')
+				ccnt++;
+		}
+	}
+	cfg->slot_cnt = (lcnt-RAA_DMPVR2_CFG_HEAD_LEN)/RAA_DMPVR2_CFG_SLOT_LEN;
+	if (cfg->slot_cnt < 1 || cfg->slot_cnt > RAA_DMPVR2_CFG_MAX_SLOT)
+		return -EINVAL;
+	cfg->cmd_cnt = ccnt;
+	cfg->cmds = kmalloc_array(ccnt, sizeof(struct raa_dmpvr_cfg_cmd),
+				  GFP_KERNEL);
+	if (!cfg->cmds)
+		return -ENOMEM;
+
+	// Parse header
+	for (i = 0; i < 2; i++) {
+		line = strsep(&buf, "\n");
+		cptr = line + dsta;
+		for (j = 3; j >= 0; j--) {
+			ret = raa_dmpvr2_hextou8(cptr, &b);
+			if (ret)
+				goto parse_err;
+			if (i == 0)
+				cfg->dev_id[j] = b;
+			else
+				cfg->dev_rev[j] = b;
+			cptr += 2;
+		}
+	}
+
+	// Parse cmds
+	i = 0;
+	while (line != NULL && strlen(line) > (dsta + 2)) {
+		if (strncmp(line, "49", 2) != 0) {
+			ret = raa_dmpvr2_hextou8(line+lsta, &b);
+			if (ret)
+				goto parse_err;
+			cfg->cmds[i].len = b - 3;
+			ret = raa_dmpvr2_hextou8(line+csta, &b);
+			if (ret)
+				goto parse_err;
+			cfg->cmds[i].cmd = b;
+			for (j = 0; j < cfg->cmds[i].len; j++) {
+				cptr = line + dsta + (2 * j);
+				ret = raa_dmpvr2_hextou8(cptr, &b);
+				if (ret)
+					goto parse_err;
+				cfg->cmds[i].data[j] = b;
+			}
+			i++;
+		}
+		line = strsep(&buf, "\n");
+	}
+	return 0;
+
+parse_err:
+	kfree(cfg->cmds);
+	return ret;
+}
+
+static int raa_dmpvr2_verify_device(struct raa_dmpvr2_ctrl *ctrl,
+				    struct raa_dmpvr2_cfg *cfg)
+{
+	u8 dev_id[5];
+	u8 dev_rev[5];
+	int status;
+
+	status = raa_smbus_read40(ctrl->client, PMBUS_IC_DEVICE_ID, dev_id);
+	if (status)
+		return status;
+
+	status = raa_smbus_read40(ctrl->client, PMBUS_IC_DEVICE_REV, dev_rev);
+	if (status)
+		return status;
+
+	return memcmp(cfg->dev_id, dev_id+1, 4)
+		| memcmp(cfg->dev_rev+3, dev_rev+4, 1);
+}
+
+static int raa_dmpvr2_check_cfg(struct raa_dmpvr2_ctrl *ctrl,
+				struct raa_dmpvr2_cfg *cfg)
+{
+	u8 data[4];
+
+	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+				  RAA_DMPVR2_NVM_CNT_ADDR);
+	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ, data);
+	if (cfg->slot_cnt > data[0])
+		return -EINVAL;
+	return 0;
+}
+
+static int raa_dmpvr2_send_cfg(struct raa_dmpvr2_ctrl *ctrl,
+			       struct raa_dmpvr2_cfg *cfg)
+{
+	int i, status;
+	u16 word;
+	u32 dbl_word;
+
+	for (i = 0; i < cfg->cmd_cnt; i++) {
+		if (cfg->cmds[i].len == 2) {
+			word = cfg->cmds[i].data[0]
+				| (cfg->cmds[i].data[1] << 8);
+			status = i2c_smbus_write_word_data(ctrl->client,
+							   cfg->cmds[i].cmd,
+							   word);
+			if (status < 0)
+				return status;
+		} else if (cfg->cmds[i].len == 4) {
+			dbl_word = cfg->cmds[i].data[0]
+				| (cfg->cmds[i].data[1] << 8)
+				| (cfg->cmds[i].data[2] << 16)
+				| (cfg->cmds[i].data[3] << 24);
+			status = raa_dmpvr2_smbus_write32(ctrl->client,
+							  cfg->cmds[i].cmd,
+							  dbl_word);
+			if (status < 0)
+				return status;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
+				       struct raa_dmpvr2_cfg *cfg)
+{
+	u8 data[4] = {0};
+	u8 data1[4];
+	u8 *dptr;
+	unsigned long start;
+	int i, j, status;
+
+	// Check programmer status
+	start = jiffies;
+	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+				  RAA_DMPVR2_PRGM_STATUS_ADDR);
+	while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
+		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
+					data);
+	}
+	if (data[0] != 1)
+		return -ETIME;
+
+	// Check bank statuses
+	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+				  RAA_DMPVR2_BANK0_STATUS_ADDR);
+	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data);
+	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+				  RAA_DMPVR2_BANK1_STATUS_ADDR);
+	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data1);
+
+	for (i = 0; i < cfg->slot_cnt; i++) {
+		if (i < 8) {
+			j = i;
+			dptr = data;
+		} else {
+			j = i - 8;
+			dptr = data1;
+		}
+		status = (dptr[j/2] >> (4 * (j % 2))) & 0x0F;
+		if (status != 1)
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static ssize_t raa_dmpvr2_write_cfg(struct raa_dmpvr2_ctrl *ctrl,
+				    const char __user *buf, size_t len,
+				    loff_t *ppos)
+{
+	char *cbuf;
+	int ret, status;
+	struct raa_dmpvr2_cfg *cfg;
+
+	cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	cbuf = kmalloc(len, GFP_KERNEL);
+	if (!cbuf) {
+		status = -ENOMEM;
+		goto buf_err;
+	}
+	ret = simple_write_to_buffer(cbuf, len, ppos, buf, len);
+
+	// Parse file
+	status = raa_dmpvr2_parse_cfg(cbuf, cfg);
+	if (status)
+		goto parse_err;
+	// Verify device and file IDs/revs match
+	status = raa_dmpvr2_verify_device(ctrl, cfg);
+	if (status)
+		goto dev_err;
+	// Verify enough of NVM slots available
+	status = raa_dmpvr2_check_cfg(ctrl, cfg);
+	if (status)
+		goto dev_err;
+	// Write CFG to device
+	status = raa_dmpvr2_send_cfg(ctrl, cfg);
+	if (status)
+		goto dev_err;
+	// Verify programming success
+	status = raa_dmpvr2_cfg_write_result(ctrl, cfg);
+	if (status)
+		goto dev_err;
+	// Free memory
+	kfree(cbuf);
+	kfree(cfg->cmds);
+	kfree(cfg);
+	return ret;
+
+	// Handle Errors
+dev_err:
+	kfree(cfg->cmds);
+parse_err:
+	kfree(cbuf);
+buf_err:
+	kfree(cfg);
+	return status;
+}
+
+static ssize_t raa_dmpvr2_debugfs_read(struct file *file, char __user *buf,
+				       size_t len, loff_t *ppos)
+{
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
+
+	switch (idx) {
+	case RAA_DMPVR2_DEBUGFS_BB_R:
+		return raa_dmpvr2_read_black_box(ctrl, buf, len, ppos);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t raa_dmpvr2_debugfs_write(struct file *file,
+					const char __user *buf, size_t len,
+					loff_t *ppos)
+{
+	int *idxp = file->private_data;
+	int idx = *idxp;
+	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
+
+	switch (idx) {
+	case RAA_DMPVR2_DEBUGFS_CFG_W:
+		return raa_dmpvr2_write_cfg(ctrl, buf, len, ppos);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct file_operations raa_dmpvr2_debugfs_fops = {
+	.llseek = noop_llseek,
+	.read = raa_dmpvr2_debugfs_read,
+	.write = raa_dmpvr2_debugfs_write,
+	.open = simple_open,
+};
+
 static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
 				     int phase, int reg)
 {
@@ -220,7 +670,11 @@ static struct pmbus_driver_info raa_dmpvr_info = {
 static int isl68137_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
+	int i, ret;
 	struct pmbus_driver_info *info;
+	struct dentry *debugfs;
+	struct dentry *debug_dir;
+	struct raa_dmpvr2_ctrl *ctrl;
 
 	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -262,7 +716,37 @@ static int isl68137_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	return pmbus_do_probe(client, id, info);
+	// No debugfs features for Gen 1
+	if (id->driver_data == raa_dmpvr1_2rail)
+		return pmbus_do_probe(client, id, info);
+
+	ret = pmbus_do_probe(client, id, info);
+	if (ret != 0)
+		return ret;
+
+	ctrl = devm_kzalloc(&client->dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return 0;
+
+	ctrl->client = client;
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return 0;
+
+	debug_dir = debugfs_create_dir(client->name, debugfs);
+	if (!debug_dir)
+		return 0;
+
+	for (i = 0; i < RAA_DMPVR2_DEBUGFS_NUM_ENTRIES; i++)
+		ctrl->debugfs_entries[i] = i;
+
+	debugfs_create_file("write_config", 0220, debug_dir,
+			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_CFG_W],
+			    &raa_dmpvr2_debugfs_fops);
+	debugfs_create_file("read_black_box", 0440, debug_dir,
+			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_BB_R],
+			    &raa_dmpvr2_debugfs_fops);
+	return 0;
 }
 
 static const struct i2c_device_id raa_dmpvr_id[] = {
-- 
2.20.1

[PATCH 2/2] docs: hwmon: Update driver documentation for debugfs

[Grant Peltier]

Add usage details for the two new debugfs endpoints for 2nd Gen devices:
read_black_box and write_config

Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
 Documentation/hwmon/isl68137.rst | 34 +++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/isl68137.rst b/Documentation/hwmon/isl68137.rst
index 0e71b22047f8..c5f7b2981db6 100644
--- a/Documentation/hwmon/isl68137.rst
+++ b/Documentation/hwmon/isl68137.rst
@@ -423,7 +423,8 @@ Beyond the normal sysfs pmbus attributes, the driver exposes a control attribute
 for the ISL68137.
 
 For 2nd generation Renesas digital multiphase voltage regulators, only the
-normal sysfs pmbus attributes are supported.
+normal sysfs pmbus attributes are supported. However, 2nd generation devices do
+have additional debugfs features which are detailed below.
 
 ISL68137 sysfs attributes
 -------------------------
@@ -603,3 +604,34 @@ temp[1-7]_crit_alarm	Chip temperature critical high alarm
 temp[1-7]_max		Maximum temperature
 temp[1-7]_max_alarm	Chip temperature high alarm
 ======================= ==========================================
+
+debugfs features (Gen 2 ONLY)
+-----------------------------
+
+If you have debugfs enabled, two file endpoints will be created under
+`/sys/kernel/debug/pmbus/<device-id>` when you instantiate a 2nd generation
+device: `read_black_box` and `write_config`.
+
+`read_black_box` is a read-only file. When read, the driver will attempt to read
+the contents of the device's fault recording RAM segment called "Black Box". The
+RAM segment is then printed to the file as hex formatted strings. For easier
+consumption, the 128-byte segment is broken up into 32 4-byte little-endian
+lines (i.e. each line starts with the least significant byte). See your device's
+datasheet for information regarding how to parse the data.
+
+.. warning::
+    When using `write_config`, only use hex files as they are exported from the
+    Renesas Power Navigator application:
+    https://www.renesas.com/us/en/products/power-management/digital-power/powernavigator.html
+
+`write_config` is a write-only file. This endpoint expects the contents of a
+configuration hex file specific to your device exactly as it is exported from
+the Renesas Power Navigator application. When this endpoint is written to, the
+driver will attempt to write the contents of the file down to one or more of the
+device's NVM slots and will return a standard error code if any part of the
+process fails. Be careful when using this feature as writing an unstable
+configuration to your device may may cause it to misbehave. Each device also has
+a fixed number of NVM writes allowed. Consult the datasheet for your device for
+more information about NVM configuration options. Finally, note that you will
+have to power cycle your device in order for NVM configuration changes to take
+effect.
-- 
2.20.1

Re: [PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Guenter Roeck]

On Mon, Apr 20, 2020 at 02:03:36PM -0500, Grant Peltier wrote:
> Add debugfs endpoints to support features of 2nd generation Renesas
> digital multiphase voltage regulators that are not compatible with
> sysfs.
> 
> The read_black_box endpoint allows users to read the contents of a
> RAM segment used to record fault conditions within Gen 2 devices.
> 
> The write_config endpoint allows users to write configuration hex
> files to Gen 2 devices which modify how they operate.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> ---
>  drivers/hwmon/pmbus/isl68137.c | 490 ++++++++++++++++++++++++++++++++-
>  1 file changed, 487 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
> index 0c622711ef7e..24af016729a3 100644
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -7,10 +7,12 @@
>   *
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/err.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/string.h>
> @@ -18,8 +20,22 @@
>  
>  #include "pmbus.h"
>  
> -#define ISL68137_VOUT_AVS	0x30
> -#define RAA_DMPVR2_READ_VMON	0xc8
> +#define ISL68137_VOUT_AVS		0x30
> +#define RAA_DMPVR2_DMA_FIX		0xc5
> +#define RAA_DMPVR2_DMA_SEQ		0xc6
> +#define RAA_DMPVR2_DMA_ADDR		0xc7
> +#define RAA_DMPVR2_READ_VMON		0xc8
> +#define RAA_DMPVR2_BB_BASE_ADDR		0xef80
> +#define RAA_DMPVR2_BB_WSIZE		4
> +#define RAA_DMPVR2_BB_WCNT		32
> +#define RAA_DMPVR2_BB_BUF_SIZE		288
> +#define RAA_DMPVR2_NVM_CNT_ADDR		0x00c2
> +#define RAA_DMPVR2_PRGM_STATUS_ADDR	0x0707
> +#define RAA_DMPVR2_BANK0_STATUS_ADDR	0x0709
> +#define RAA_DMPVR2_BANK1_STATUS_ADDR	0x070a
> +#define RAA_DMPVR2_CFG_MAX_SLOT		16
> +#define RAA_DMPVR2_CFG_HEAD_LEN		290
> +#define RAA_DMPVR2_CFG_SLOT_LEN		358
>  
>  enum chips {
>  	isl68137,
> @@ -71,6 +87,21 @@ enum variants {
>  	raa_dmpvr2_hv,
>  };
>  
> +enum {
> +	RAA_DMPVR2_DEBUGFS_CFG_W = 0,
> +	RAA_DMPVR2_DEBUGFS_BB_R,
> +	RAA_DMPVR2_DEBUGFS_NUM_ENTRIES,
> +};
> +
> +struct raa_dmpvr2_ctrl {
> +	enum chips part;
> +	struct i2c_client *client;
> +	int debugfs_entries[RAA_DMPVR2_DEBUGFS_NUM_ENTRIES];
> +};
> +
> +#define to_ctrl(x, y) container_of((x), struct raa_dmpvr2_ctrl, \
> +				   debugfs_entries[(y)])
> +
>  static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
>  					     int page,
>  					     char *buf)
> @@ -157,6 +188,425 @@ static const struct attribute_group *isl68137_attribute_groups[] = {
>  	NULL,
>  };
>  
> +/**
> + * Non-standard SMBus read function to account for I2C controllers that
> + * do not support SMBus block reads. Reads 5 bytes from client (length + 4 data
> + * bytes)
> + */

Normally this is emulated for such controllers. I don't recall seeing
such a need before. The code below duplicates similar code in
i2c_smbus_xfer_emulated(), which is much more sophisticated.
Are you sure this is needed ? Can you point me to an affected
controller ?

> +static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
> +			    unsigned char *data)
> +{
> +	int status;
> +	unsigned char msgbuf[1];
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +			.len = 1,
> +			.buf = msgbuf,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags | I2C_M_RD,
> +			.len = 5,
> +			.buf = data,
> +		},
> +	};
> +
> +	msgbuf[0] = command;
> +	status = i2c_transfer(client->adapter, msg, 2);
> +	if (status != 2)
> +		return status;

i2c_transfer() can return 1 if only one of the two messages was sent.

> +	return 0;
> +}
> +
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Read 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
> +				   unsigned char *data)
> +{
> +	int status;
> +	unsigned char msgbuf[1];
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +			.len = 1,
> +			.buf = msgbuf,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags | I2C_M_RD,
> +			.len = 4,
> +			.buf = data,
> +		},
> +	};
> +
> +	msgbuf[0] = command;
> +	status = i2c_transfer(client->adapter, msg, 2);
> +	if (status != 2)
> +		return status;
> +	return 0;
> +}

Maybe it would be worthwhile to consider implementing it ?

> +
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Write 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_write32(const struct i2c_client *client,
> +				    u8 command, u32 value)
> +{
> +	int status;
> +	unsigned char msgbuf[5];
> +	struct i2c_msg msg[1] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +			.len = 5,
> +			.buf = msgbuf,
> +		},
> +	};
> +	msgbuf[0] = command;
> +	msgbuf[1] = value & 0x0FF;
> +	msgbuf[2] = (value >> 8) & 0x0FF;
> +	msgbuf[3] = (value >> 16) & 0x0FF;
> +	msgbuf[4] = (value >> 24) &  0x0FF;
> +	status = i2c_transfer(client->adapter, msg, 1);
> +	if (status != 1)
> +		return status;
> +	return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_read_black_box(struct raa_dmpvr2_ctrl *ctrl,
> +					 char __user *buf, size_t len,
> +					 loff_t *ppos)
> +{
> +	int i, j;
> +	u16 addr = RAA_DMPVR2_BB_BASE_ADDR;
> +	unsigned char data[RAA_DMPVR2_BB_WSIZE] = { 0 };
> +	char out[RAA_DMPVR2_BB_BUF_SIZE] = { 0 };
> +	char *optr = out;
> +
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR, addr);
> +	for (i = 0; i < RAA_DMPVR2_BB_WCNT; i++) {
> +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ,
> +					data);
> +		for (j = 0; j < RAA_DMPVR2_BB_WSIZE; j++)
> +			optr += snprintf(optr, sizeof(out), "%02X", data[j]);
> +		*optr = '\n';
> +		optr++;
> +	}
> +
> +	return simple_read_from_buffer(buf, len, ppos, &out, sizeof(out));
> +}
> +
> +struct raa_dmpvr_cfg_cmd {
> +	u8 cmd;
> +	u8 len;
> +	u8 data[4];
> +};
> +
> +struct raa_dmpvr2_cfg {
> +	u8 dev_id[4];
> +	u8 dev_rev[4];
> +	int slot_cnt;
> +	int cmd_cnt;
> +	struct raa_dmpvr_cfg_cmd *cmds;
> +	u8 crc[4];
> +};
> +
> +/**
> + * Helper function to handle hex string to byte conversions
> + */
> +static int raa_dmpvr2_hextou8(char *buf, u8 *res)
> +{
> +	char s[3];
> +
> +	s[0] = buf[0];
> +	s[1] = buf[1];
> +	s[2] = '\0';
> +	return kstrtou8(s, 16, res);
> +}
> +
> +static int raa_dmpvr2_parse_cfg(char *buf, struct raa_dmpvr2_cfg *cfg)
> +{
> +	const int lsta = 2;
> +	const int csta = 6;
> +	const int dsta = 8;
> +	char *cptr, *line;
> +	int lcnt = 1;
> +	int ccnt = 0;
> +	int i, j, ret;
> +	u8 b;
> +
> +	// Ensure there is enough memory for the file
> +	for (i = 0; i < strlen(buf); i++) {
> +		if (buf[i] == '\n' && i < strlen(buf)-2) {
> +			lcnt++;
> +			if (buf[i+1] != '4' || buf[i+2] != '9')
> +				ccnt++;
> +		}
> +	}
> +	cfg->slot_cnt = (lcnt-RAA_DMPVR2_CFG_HEAD_LEN)/RAA_DMPVR2_CFG_SLOT_LEN;
> +	if (cfg->slot_cnt < 1 || cfg->slot_cnt > RAA_DMPVR2_CFG_MAX_SLOT)
> +		return -EINVAL;
> +	cfg->cmd_cnt = ccnt;
> +	cfg->cmds = kmalloc_array(ccnt, sizeof(struct raa_dmpvr_cfg_cmd),
> +				  GFP_KERNEL);
> +	if (!cfg->cmds)
> +		return -ENOMEM;
> +
> +	// Parse header
> +	for (i = 0; i < 2; i++) {
> +		line = strsep(&buf, "\n");
> +		cptr = line + dsta;
> +		for (j = 3; j >= 0; j--) {
> +			ret = raa_dmpvr2_hextou8(cptr, &b);
> +			if (ret)
> +				goto parse_err;
> +			if (i == 0)
> +				cfg->dev_id[j] = b;
> +			else
> +				cfg->dev_rev[j] = b;
> +			cptr += 2;
> +		}
> +	}
> +
> +	// Parse cmds
> +	i = 0;
> +	while (line != NULL && strlen(line) > (dsta + 2)) {
> +		if (strncmp(line, "49", 2) != 0) {
> +			ret = raa_dmpvr2_hextou8(line+lsta, &b);
> +			if (ret)
> +				goto parse_err;
> +			cfg->cmds[i].len = b - 3;
> +			ret = raa_dmpvr2_hextou8(line+csta, &b);
> +			if (ret)
> +				goto parse_err;
> +			cfg->cmds[i].cmd = b;
> +			for (j = 0; j < cfg->cmds[i].len; j++) {
> +				cptr = line + dsta + (2 * j);
> +				ret = raa_dmpvr2_hextou8(cptr, &b);
> +				if (ret)
> +					goto parse_err;
> +				cfg->cmds[i].data[j] = b;
> +			}
> +			i++;
> +		}
> +		line = strsep(&buf, "\n");
> +	}
> +	return 0;
> +
> +parse_err:
> +	kfree(cfg->cmds);
> +	return ret;
> +}
> +
> +static int raa_dmpvr2_verify_device(struct raa_dmpvr2_ctrl *ctrl,
> +				    struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 dev_id[5];
> +	u8 dev_rev[5];
> +	int status;
> +
> +	status = raa_smbus_read40(ctrl->client, PMBUS_IC_DEVICE_ID, dev_id);
> +	if (status)
> +		return status;
> +
> +	status = raa_smbus_read40(ctrl->client, PMBUS_IC_DEVICE_REV, dev_rev);
> +	if (status)
> +		return status;
> +
> +	return memcmp(cfg->dev_id, dev_id+1, 4)
> +		| memcmp(cfg->dev_rev+3, dev_rev+4, 1);
> +}
> +
> +static int raa_dmpvr2_check_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +				struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 data[4];
> +
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_NVM_CNT_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ, data);
> +	if (cfg->slot_cnt > data[0])
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int raa_dmpvr2_send_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +			       struct raa_dmpvr2_cfg *cfg)
> +{
> +	int i, status;
> +	u16 word;
> +	u32 dbl_word;
> +
> +	for (i = 0; i < cfg->cmd_cnt; i++) {
> +		if (cfg->cmds[i].len == 2) {
> +			word = cfg->cmds[i].data[0]
> +				| (cfg->cmds[i].data[1] << 8);
> +			status = i2c_smbus_write_word_data(ctrl->client,
> +							   cfg->cmds[i].cmd,
> +							   word);
> +			if (status < 0)
> +				return status;
> +		} else if (cfg->cmds[i].len == 4) {
> +			dbl_word = cfg->cmds[i].data[0]
> +				| (cfg->cmds[i].data[1] << 8)
> +				| (cfg->cmds[i].data[2] << 16)
> +				| (cfg->cmds[i].data[3] << 24);
> +			status = raa_dmpvr2_smbus_write32(ctrl->client,
> +							  cfg->cmds[i].cmd,
> +							  dbl_word);
> +			if (status < 0)
> +				return status;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> +				       struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 data[4] = {0};
> +	u8 data1[4];
> +	u8 *dptr;
> +	unsigned long start;
> +	int i, j, status;
> +
> +	// Check programmer status
> +	start = jiffies;
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_PRGM_STATUS_ADDR);
> +	while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> +					data);
> +	}
> +	if (data[0] != 1)
> +		return -ETIME;
> +
> +	// Check bank statuses
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_BANK0_STATUS_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data);
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_BANK1_STATUS_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data1);
> +
> +	for (i = 0; i < cfg->slot_cnt; i++) {
> +		if (i < 8) {
> +			j = i;
> +			dptr = data;
> +		} else {
> +			j = i - 8;
> +			dptr = data1;
> +		}
> +		status = (dptr[j/2] >> (4 * (j % 2))) & 0x0F;
> +		if (status != 1)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_write_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +				    const char __user *buf, size_t len,
> +				    loff_t *ppos)
> +{
> +	char *cbuf;
> +	int ret, status;
> +	struct raa_dmpvr2_cfg *cfg;
> +
> +	cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	cbuf = kmalloc(len, GFP_KERNEL);
> +	if (!cbuf) {
> +		status = -ENOMEM;
> +		goto buf_err;
> +	}
> +	ret = simple_write_to_buffer(cbuf, len, ppos, buf, len);
> +
> +	// Parse file
> +	status = raa_dmpvr2_parse_cfg(cbuf, cfg);
> +	if (status)
> +		goto parse_err;
> +	// Verify device and file IDs/revs match
> +	status = raa_dmpvr2_verify_device(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Verify enough of NVM slots available
> +	status = raa_dmpvr2_check_cfg(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Write CFG to device
> +	status = raa_dmpvr2_send_cfg(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Verify programming success
> +	status = raa_dmpvr2_cfg_write_result(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Free memory
> +	kfree(cbuf);
> +	kfree(cfg->cmds);
> +	kfree(cfg);
> +	return ret;
> +
> +	// Handle Errors
> +dev_err:
> +	kfree(cfg->cmds);
> +parse_err:
> +	kfree(cbuf);
> +buf_err:
> +	kfree(cfg);
> +	return status;
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_read(struct file *file, char __user *buf,
> +				       size_t len, loff_t *ppos)
> +{
> +	int *idxp = file->private_data;
> +	int idx = *idxp;
> +	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> +	switch (idx) {
> +	case RAA_DMPVR2_DEBUGFS_BB_R:
> +		return raa_dmpvr2_read_black_box(ctrl, buf, len, ppos);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_write(struct file *file,
> +					const char __user *buf, size_t len,
> +					loff_t *ppos)
> +{
> +	int *idxp = file->private_data;
> +	int idx = *idxp;
> +	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> +	switch (idx) {
> +	case RAA_DMPVR2_DEBUGFS_CFG_W:
> +		return raa_dmpvr2_write_cfg(ctrl, buf, len, ppos);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct file_operations raa_dmpvr2_debugfs_fops = {
> +	.llseek = noop_llseek,
> +	.read = raa_dmpvr2_debugfs_read,
> +	.write = raa_dmpvr2_debugfs_write,
> +	.open = simple_open,
> +};
> +
>  static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
>  				     int phase, int reg)
>  {
> @@ -220,7 +670,11 @@ static struct pmbus_driver_info raa_dmpvr_info = {
>  static int isl68137_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> +	int i, ret;
>  	struct pmbus_driver_info *info;
> +	struct dentry *debugfs;
> +	struct dentry *debug_dir;
> +	struct raa_dmpvr2_ctrl *ctrl;
>  
>  	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -262,7 +716,37 @@ static int isl68137_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>  
> -	return pmbus_do_probe(client, id, info);
> +	// No debugfs features for Gen 1
> +	if (id->driver_data == raa_dmpvr1_2rail)
> +		return pmbus_do_probe(client, id, info);
> +
> +	ret = pmbus_do_probe(client, id, info);
> +	if (ret != 0)
> +		return ret;
> +
> +	ctrl = devm_kzalloc(&client->dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return 0;
> +
> +	ctrl->client = client;
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return 0;
> +
> +	debug_dir = debugfs_create_dir(client->name, debugfs);
> +	if (!debug_dir)
> +		return 0;
> +
> +	for (i = 0; i < RAA_DMPVR2_DEBUGFS_NUM_ENTRIES; i++)
> +		ctrl->debugfs_entries[i] = i;
> +
> +	debugfs_create_file("write_config", 0220, debug_dir,
> +			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_CFG_W],
> +			    &raa_dmpvr2_debugfs_fops);
> +	debugfs_create_file("read_black_box", 0440, debug_dir,
> +			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_BB_R],
> +			    &raa_dmpvr2_debugfs_fops);
> +	return 0;
>  }
>  
>  static const struct i2c_device_id raa_dmpvr_id[] = {
> -- 
> 2.20.1
> 

Re: [PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Grant Peltier]

On Tue, Apr 21, 2020 at 11:58:51AM -0700, Guenter Roeck wrote:
> Normally this is emulated for such controllers. I don't recall seeing
> such a need before. The code below duplicates similar code in
> i2c_smbus_xfer_emulated(), which is much more sophisticated.
> Are you sure this is needed ? Can you point me to an affected
> controller ?
> 
> > +static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
> > +			    unsigned char *data)
> > +{
> > +	int status;
> > +	unsigned char msgbuf[1];
> > +	struct i2c_msg msg[2] = {
> > +		{
> > +			.addr = client->addr,
> > +			.flags = client->flags,
> > +			.len = 1,
> > +			.buf = msgbuf,
> > +		},
> > +		{
> > +			.addr = client->addr,
> > +			.flags = client->flags | I2C_M_RD,
> > +			.len = 5,
> > +			.buf = data,
> > +		},
> > +	};
> > +
> > +	msgbuf[0] = command;
> > +	status = i2c_transfer(client->adapter, msg, 2);
> > +	if (status != 2)
> > +		return status;
> 
> i2c_transfer() can return 1 if only one of the two messages was sent.
> 
> > +	return 0;
> > +}
I have been using BCM2835 for most of my testing. I originally tried using
i2c_smbus_read_block_data() but that was returning errors. However, from your
email, I went back and tried i2c_smbus_read_i2c_block_data() and that appears
to be working so I will switch to that instead.

> > +
> > +/**
> > + * Helper function required since linux SMBus implementation does not currently
> > + * (v5.6) support the SMBus 3.0 "Read 32" protocol
> > + */
> > +static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
> > +				   unsigned char *data)
> > +{
> > +	int status;
> > +	unsigned char msgbuf[1];
> > +	struct i2c_msg msg[2] = {
> > +		{
> > +			.addr = client->addr,
> > +			.flags = client->flags,
> > +			.len = 1,
> > +			.buf = msgbuf,
> > +		},
> > +		{
> > +			.addr = client->addr,
> > +			.flags = client->flags | I2C_M_RD,
> > +			.len = 4,
> > +			.buf = data,
> > +		},
> > +	};
> > +
> > +	msgbuf[0] = command;
> > +	status = i2c_transfer(client->adapter, msg, 2);
> > +	if (status != 2)
> > +		return status;
> > +	return 0;
> > +}
> 
> Maybe it would be worthwhile to consider implementing it ?
> 
I could add these functions to i2c-core-smbus instead if that is desired.
However, I am unsure if it would be proper to only partially update the SMBus
implementation to match the 3.0 spec. Is there perhaps a better forum to
discuss adding all of the 3.0 changes?

Re: [PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Guenter Roeck]

On Tue, Apr 21, 2020 at 04:49:17PM -0500, Grant Peltier wrote:
> On Tue, Apr 21, 2020 at 11:58:51AM -0700, Guenter Roeck wrote:
> > Normally this is emulated for such controllers. I don't recall seeing
> > such a need before. The code below duplicates similar code in
> > i2c_smbus_xfer_emulated(), which is much more sophisticated.
> > Are you sure this is needed ? Can you point me to an affected
> > controller ?
> > 
> > > +static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
> > > +			    unsigned char *data)
> > > +{
> > > +	int status;
> > > +	unsigned char msgbuf[1];
> > > +	struct i2c_msg msg[2] = {
> > > +		{
> > > +			.addr = client->addr,
> > > +			.flags = client->flags,
> > > +			.len = 1,
> > > +			.buf = msgbuf,
> > > +		},
> > > +		{
> > > +			.addr = client->addr,
> > > +			.flags = client->flags | I2C_M_RD,
> > > +			.len = 5,
> > > +			.buf = data,
> > > +		},
> > > +	};
> > > +
> > > +	msgbuf[0] = command;
> > > +	status = i2c_transfer(client->adapter, msg, 2);
> > > +	if (status != 2)
> > > +		return status;
> > 
> > i2c_transfer() can return 1 if only one of the two messages was sent.
> > 
> > > +	return 0;
> > > +}
> I have been using BCM2835 for most of my testing. I originally tried using
> i2c_smbus_read_block_data() but that was returning errors. However, from your
> email, I went back and tried i2c_smbus_read_i2c_block_data() and that appears
> to be working so I will switch to that instead.
> 
This is odd, since the function should do a SMBus block read. Did you pass a
buffer that was too small, by any chance ?

> > > +
> > > +/**
> > > + * Helper function required since linux SMBus implementation does not currently
> > > + * (v5.6) support the SMBus 3.0 "Read 32" protocol
> > > + */
> > > +static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
> > > +				   unsigned char *data)
> > > +{
> > > +	int status;
> > > +	unsigned char msgbuf[1];
> > > +	struct i2c_msg msg[2] = {
> > > +		{
> > > +			.addr = client->addr,
> > > +			.flags = client->flags,
> > > +			.len = 1,
> > > +			.buf = msgbuf,
> > > +		},
> > > +		{
> > > +			.addr = client->addr,
> > > +			.flags = client->flags | I2C_M_RD,
> > > +			.len = 4,
> > > +			.buf = data,
> > > +		},
> > > +	};
> > > +
> > > +	msgbuf[0] = command;
> > > +	status = i2c_transfer(client->adapter, msg, 2);
> > > +	if (status != 2)
> > > +		return status;
> > > +	return 0;
> > > +}
> > 
> > Maybe it would be worthwhile to consider implementing it ?
> > 
> I could add these functions to i2c-core-smbus instead if that is desired.
> However, I am unsure if it would be proper to only partially update the SMBus
> implementation to match the 3.0 spec. Is there perhaps a better forum to
> discuss adding all of the 3.0 changes?
> 
Never mind, just keep it local. I don't like it too much, but getting patches
accepted into the i2c core might take several releases, and it would be unfair
to impose that on you.

Guenter

Re: [PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Grant Peltier]

On Tue, Apr 21, 2020 at 07:04:04PM -0700, Guenter Roeck wrote:
> On Tue, Apr 21, 2020 at 04:49:17PM -0500, Grant Peltier wrote:
> > On Tue, Apr 21, 2020 at 11:58:51AM -0700, Guenter Roeck wrote:
> > > Normally this is emulated for such controllers. I don't recall seeing
> > > such a need before. The code below duplicates similar code in
> > > i2c_smbus_xfer_emulated(), which is much more sophisticated.
> > > Are you sure this is needed ? Can you point me to an affected
> > > controller ?
> > > 
> > > > +static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
> > > > +			    unsigned char *data)
> > > > +{
> > > > +	int status;
> > > > +	unsigned char msgbuf[1];
> > > > +	struct i2c_msg msg[2] = {
> > > > +		{
> > > > +			.addr = client->addr,
> > > > +			.flags = client->flags,
> > > > +			.len = 1,
> > > > +			.buf = msgbuf,
> > > > +		},
> > > > +		{
> > > > +			.addr = client->addr,
> > > > +			.flags = client->flags | I2C_M_RD,
> > > > +			.len = 5,
> > > > +			.buf = data,
> > > > +		},
> > > > +	};
> > > > +
> > > > +	msgbuf[0] = command;
> > > > +	status = i2c_transfer(client->adapter, msg, 2);
> > > > +	if (status != 2)
> > > > +		return status;
> > > 
> > > i2c_transfer() can return 1 if only one of the two messages was sent.
> > > 
> > > > +	return 0;
> > > > +}
> > I have been using BCM2835 for most of my testing. I originally tried using
> > i2c_smbus_read_block_data() but that was returning errors. However, from your
> > email, I went back and tried i2c_smbus_read_i2c_block_data() and that appears
> > to be working so I will switch to that instead.
> > 
 
> This is odd, since the function should do a SMBus block read. Did you pass a
> buffer that was too small, by any chance ?

I tried with a variety of buffer sizes from 4 (data size) to 32 (max block size)
and it always returned an error. I believe that i2c_smbus_read_block_data()
attempts to do a legitimate SMBus block read which depends on the controller
interpretting and reading the correct number of bytes as signaled from the
client. This is in line with the docstring for the function and the fact that
the BCM2835 responds with false (0) from i2c_check_functionality() for
I2C_FUNC_SMBUS_READ_BLOCK_DATA. On the other hand,
i2c_smbus_read_i2c_block_data() appears to do a fixed length read similar to
the function that I wrote above.

Thanks,
Grant