hwmon: (pmbus/core) Add lock and unlock functions

PATCH Superseded Eddie James 2 participants 5 messages Apr 5, 2023 – Apr 12, 2023 Hardware monitoring

Share & tools

Eddie James

author

2023-04-05 14:52 UTC #0 lore raw

cover hwmon: (pmbus/core) Add lock and unlock functions

Debugfs operations may set the page number, which must be done atomically with the subsequent i2c operation. Lock the update_lock in the debugfs functions and provide a function for pmbus drivers to lock and unlock the update_lock.
Use the new lock/unlock functions in the ibm-cffps driver and make some additional improvements.

Eddie James (2):
hwmon: (pmbus/core) Add lock and unlock functions
hwmon: (pmbus/ibm-cffps) Use default debugfs attributes and lock
function

 drivers/hwmon/pmbus/ibm-cffps.c  | 273 +++++++++++++------------------
 drivers/hwmon/pmbus/pmbus.h      |   2 +
 drivers/hwmon/pmbus/pmbus_core.c |  30 ++++
 3 files changed, 150 insertions(+), 155 deletions(-)

Signature · 1 line

2.31.1

Eddie James

author

2023-04-05 14:52 UTC #1 lore raw

PATCH 1/2 hwmon: (pmbus/core) Add lock and unlock functions

Signed-off-by Eddie James <[email protected]>

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

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

index 11e84e141126..b0832a4c690d 100644
@@ -505,6 +505,8 @@ int pmbus_get_fan_rate_device(struct i2c_client *client, int page, int id,
 			      enum pmbus_fan_mode mode);
 int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
 			      enum pmbus_fan_mode mode);
+int pmbus_lock_interruptible(struct i2c_client *client);
+void pmbus_unlock(struct i2c_client *client);
 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);

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

index ca4510e4f918..04b4c65666fd 100644
@@ -3212,8 +3212,13 @@ static int pmbus_debugfs_get(void *data, u64 *val)
 {
 	int rc;
 	struct pmbus_debugfs_entry *entry = data;
+	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
 
+	rc = mutex_lock_interruptible(&pdata->update_lock);
+	if (rc)
+		return rc;
 	rc = _pmbus_read_byte_data(entry->client, entry->page, entry->reg);
+	mutex_unlock(&pdata->update_lock);
 	if (rc < 0)
 		return rc;

@@ -3230,7 +3235,11 @@ static int pmbus_debugfs_get_status(void *data, u64 *val)
 	struct pmbus_debugfs_entry *entry = data;
 	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
 
+	rc = mutex_lock_interruptible(&pdata->update_lock);
+	if (rc)
+		return rc;
 	rc = pdata->read_status(entry->client, entry->page);
+	mutex_unlock(&pdata->update_lock);
 	if (rc < 0)
 		return rc;

@@ -3246,10 +3255,15 @@ static ssize_t pmbus_debugfs_mfr_read(struct file *file, char __user *buf,
 {
 	int rc;
 	struct pmbus_debugfs_entry *entry = file->private_data;
+	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
 	char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
 
+	rc = mutex_lock_interruptible(&pdata->update_lock);
+	if (rc)
+		return rc;
 	rc = pmbus_read_block_data(entry->client, entry->page, entry->reg,
 				   data);
+	mutex_unlock(&pdata->update_lock);
 	if (rc < 0)
 		return rc;

@@ -3587,6 +3601,22 @@ struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client)
 }
 EXPORT_SYMBOL_NS_GPL(pmbus_get_debugfs_dir, PMBUS);
 
+int pmbus_lock_interruptible(struct i2c_client *client)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	return mutex_lock_interruptible(&data->update_lock);
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_lock_interruptible, PMBUS);
+
+void pmbus_unlock(struct i2c_client *client)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	mutex_unlock(&data->update_lock);
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_unlock, PMBUS);
+
 static int __init pmbus_core_init(void)
 {
 	pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);

Signature · 1 line

2.31.1

Eddie James

author

2023-04-05 14:52 UTC #2 lore raw

PATCH 2/2 hwmon: (pmbus/ibm-cffps) Use default debugfs attributes and lock function

In reply to Eddie James

Switch the driver to use the default debugfs attributes instead of ones that provide the same data under different names. Use the lock functions for the debugfs and led attributes, and simplify the input history operation by dropping the timer and lock.

Signed-off-by Eddie James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 273 ++++++++++++++------------------
 1 file changed, 118 insertions(+), 155 deletions(-)

drivers/hwmon/pmbus/ibm-cffps.c +118 ~~-155~~

index e3294a1a54bb..2d7ec00e047b 100644
@@ -18,12 +18,6 @@
 
 #include "pmbus.h"
 
-#define CFFPS_MFG_ID_CMD                        0x99
-#define CFFPS_FRU_CMD				0x9A
-#define CFFPS_PN_CMD				0x9B
-#define CFFPS_HEADER_CMD			0x9C
-#define CFFPS_SN_CMD				0x9E
-#define CFFPS_MAX_POWER_OUT_CMD			0xA7
 #define CFFPS_CCIN_CMD				0xBD
 #define CFFPS_FW_CMD				0xFA
 #define CFFPS1_FW_NUM_BYTES			4

@@ -32,7 +26,7 @@
 #define CFFPS_12VCS_VOUT_CMD			0xDE
 
 #define CFFPS_INPUT_HISTORY_CMD			0xD6
-#define CFFPS_INPUT_HISTORY_SIZE		100
+#define CFFPS_INPUT_HISTORY_SIZE		101
 
 #define CFFPS_CCIN_REVISION			GENMASK(7, 0)
 #define CFFPS_CCIN_REVISION_LEGACY		 0xde

@@ -57,13 +51,7 @@
 #define CFFPS_BLINK_RATE_MS			250
 
 enum {
-	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
-	CFFPS_DEBUGFS_MFG_ID,
-	CFFPS_DEBUGFS_FRU,
-	CFFPS_DEBUGFS_PN,
-	CFFPS_DEBUGFS_HEADER,
-	CFFPS_DEBUGFS_SN,
-	CFFPS_DEBUGFS_MAX_POWER_OUT,
+	CFFPS_DEBUGFS_MAX_POWER_OUT = 0,
 	CFFPS_DEBUGFS_CCIN,
 	CFFPS_DEBUGFS_FW,
 	CFFPS_DEBUGFS_ON_OFF_CONFIG,

@@ -72,19 +60,11 @@ enum {
 
 enum versions { cffps1, cffps2, cffps_unknown };
 
-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 {
 	enum versions version;
 	struct i2c_client *client;
 
-	struct ibm_cffps_input_history input_history;
+	u8 input_history[CFFPS_INPUT_HISTORY_SIZE];
 
 	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];

@@ -93,118 +73,98 @@ struct ibm_cffps {
 	struct led_classdev led;
 };
 
-static const struct i2c_device_id ibm_cffps_id[];
-
 #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)
+static ssize_t ibm_cffps_debugfs_read_input_history(struct file *file, 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 };
+	u8 cmd = CFFPS_INPUT_HISTORY_CMD;
+	struct ibm_cffps *psu = file->private_data;
 	struct i2c_msg msg[2] = {
 		{
 			.addr = psu->client->addr,
 			.flags = psu->client->flags,
 			.len = 1,
-			.buf = msgbuf0,
+			.buf = &cmd,
 		}, {
 			.addr = psu->client->addr,
 			.flags = psu->client->flags | I2C_M_RD,
-			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
-			.buf = msgbuf1,
+			.len = CFFPS_INPUT_HISTORY_SIZE,
+			.buf = psu->input_history,
 		},
 	};
 
 	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;
-			}
+		rc = pmbus_lock_interruptible(psu->client);
+		if (rc)
+			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;
+		rc = pmbus_set_page(psu->client, 0, 0xff);
+		if (rc) {
+			pmbus_unlock(psu->client);
+			return rc;
 		}
 
-		mutex_unlock(&psu->input_history.update_lock);
+		/*
+		 * 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);
+		pmbus_unlock(psu->client);
+		if (rc < 0)
+			return rc;
 	}
 
 	return simple_read_from_buffer(buf, count, ppos,
-				       psu->input_history.data,
-				       psu->input_history.byte_count);
+				       psu->input_history + 1,
+				       psu->input_history[0]);
 }
 
+static const struct file_operations ibm_cffps_input_history_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_debugfs_read_input_history,
+	.open = simple_open,
+};
+
 static ssize_t ibm_cffps_debugfs_read(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 + 2] = { 0 };
 
-	pmbus_set_page(psu->client, 0, 0xff);
+	rc = pmbus_lock_interruptible(psu->client);
+	if (rc)
+		return rc;
+
+	rc = pmbus_set_page(psu->client, 0, 0xff);
+	if (rc)
+		goto unlock;
 
 	switch (idx) {
-	case CFFPS_DEBUGFS_INPUT_HISTORY:
-		return ibm_cffps_read_input_history(psu, buf, count, ppos);
-	case CFFPS_DEBUGFS_MFG_ID:
-		cmd = CFFPS_MFG_ID_CMD;
-		break;
-	case CFFPS_DEBUGFS_FRU:
-		cmd = CFFPS_FRU_CMD;
-		break;
-	case CFFPS_DEBUGFS_PN:
-		cmd = CFFPS_PN_CMD;
-		break;
-	case CFFPS_DEBUGFS_HEADER:
-		cmd = CFFPS_HEADER_CMD;
-		break;
-	case CFFPS_DEBUGFS_SN:
-		cmd = CFFPS_SN_CMD;
-		break;
 	case CFFPS_DEBUGFS_MAX_POWER_OUT:
-		if (psu->version == cffps1) {
-			rc = i2c_smbus_read_word_swapped(psu->client,
-					CFFPS_MAX_POWER_OUT_CMD);
-		} else {
-			rc = i2c_smbus_read_word_data(psu->client,
-					CFFPS_MAX_POWER_OUT_CMD);
-		}
-
-		if (rc < 0)
-			return rc;
-
-		rc = snprintf(data, I2C_SMBUS_BLOCK_MAX, "%d", rc);
-		goto done;
+		if (psu->version == cffps1)
+			rc = i2c_smbus_read_word_swapped(psu->client, PMBUS_MFR_POUT_MAX);
+		else
+			rc = i2c_smbus_read_word_data(psu->client, PMBUS_MFR_POUT_MAX);
+		if (rc >= 0)
+			rc = snprintf(data, I2C_SMBUS_BLOCK_MAX, "%d", rc);
+		break;
 	case CFFPS_DEBUGFS_CCIN:
 		rc = i2c_smbus_read_word_swapped(psu->client, CFFPS_CCIN_CMD);
-		if (rc < 0)
-			return rc;
-
-		rc = snprintf(data, 5, "%04X", rc);
-		goto done;
+		if (rc >= 0)
+			rc = snprintf(data, 5, "%04X", rc);
+		break;
 	case CFFPS_DEBUGFS_FW:
 		switch (psu->version) {
 		case cffps1:
 			for (i = 0; i < CFFPS1_FW_NUM_BYTES; ++i) {
-				rc = i2c_smbus_read_byte_data(psu->client,
-							      CFFPS_FW_CMD +
-								i);
+				rc = i2c_smbus_read_byte_data(psu->client, CFFPS_FW_CMD + i);
 				if (rc < 0)
-					return rc;
+					goto unlock;
 
 				snprintf(&data[i * 2], 3, "%02X", rc);
 			}

@@ -213,11 +173,9 @@ static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf,
 			break;
 		case cffps2:
 			for (i = 0; i < CFFPS2_FW_NUM_WORDS; ++i) {
-				rc = i2c_smbus_read_word_data(psu->client,
-							      CFFPS_FW_CMD +
-								i);
+				rc = i2c_smbus_read_word_data(psu->client, CFFPS_FW_CMD + i);
 				if (rc < 0)
-					return rc;
+					goto unlock;
 
 				snprintf(&data[i * 4], 5, "%04X", rc);
 			}

@@ -225,29 +183,27 @@ static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf,
 			rc = i * 4;
 			break;
 		default:
-			return -EOPNOTSUPP;
+			rc = -EOPNOTSUPP;
+			break;
 		}
-		goto done;
+		break;
 	case CFFPS_DEBUGFS_ON_OFF_CONFIG:
-		rc = i2c_smbus_read_byte_data(psu->client,
-					      PMBUS_ON_OFF_CONFIG);
-		if (rc < 0)
-			return rc;
-
-		rc = snprintf(data, 3, "%02x", rc);
-		goto done;
+		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_ON_OFF_CONFIG);
+		if (rc >= 0)
+			rc = snprintf(data, 3, "%02x", rc);
+		break;
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
+		break;
 	}
 
-	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+unlock:
+	pmbus_unlock(psu->client);
 	if (rc < 0)
 		return rc;
 
-done:
 	data[rc] = '\n';
 	rc += 2;
-
 	return simple_read_from_buffer(buf, count, ppos, data, rc);
 }

@@ -263,14 +219,22 @@ static ssize_t ibm_cffps_debugfs_write(struct file *file,
 
 	switch (idx) {
 	case CFFPS_DEBUGFS_ON_OFF_CONFIG:
-		pmbus_set_page(psu->client, 0, 0xff);
-
 		rc = simple_write_to_buffer(&data, 1, ppos, buf, count);
 		if (rc <= 0)
 			return rc;
 
-		rc = i2c_smbus_write_byte_data(psu->client,
-					       PMBUS_ON_OFF_CONFIG, data);
+		rc = pmbus_lock_interruptible(psu->client);
+		if (rc)
+			return rc;
+
+		rc = pmbus_set_page(psu->client, 0, 0xff);
+		if (rc) {
+			pmbus_unlock(psu->client);
+			return rc;
+		}
+
+		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_ON_OFF_CONFIG, data);
+		pmbus_unlock(psu->client);
 		if (rc)
 			return rc;

@@ -396,10 +360,19 @@ static int ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
 	dev_dbg(&psu->client->dev, "LED brightness set: %d. Command: %d.\n",
 		brightness, next_led_state);
 
-	pmbus_set_page(psu->client, 0, 0xff);
+	rc = pmbus_lock_interruptible(psu->client);
+	if (rc)
+		return rc;
+
+	rc = pmbus_set_page(psu->client, 0, 0xff);
+	if (rc) {
+		pmbus_unlock(psu->client);
+		return rc;
+	}
 
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
 				       next_led_state);
+	pmbus_unlock(psu->client);
 	if (rc < 0)
 		return rc;

@@ -418,10 +391,19 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
 
 	dev_dbg(&psu->client->dev, "LED blink set.\n");
 
-	pmbus_set_page(psu->client, 0, 0xff);
+	rc = pmbus_lock_interruptible(psu->client);
+	if (rc)
+		return rc;
+
+	rc = pmbus_set_page(psu->client, 0, 0xff);
+	if (rc) {
+		pmbus_unlock(psu->client);
+		return rc;
+	}
 
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
 				       CFFPS_LED_BLINK);
+	pmbus_unlock(psu->client);
 	if (rc < 0)
 		return rc;

@@ -474,9 +456,11 @@ static struct pmbus_driver_info ibm_cffps_info[] = {
 			PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
 			PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
 			PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_VMON,
-		.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
-			PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
-			PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT,
+		.func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+			PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+			PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+			PMBUS_HAVE_STATUS_TEMP,
 		.read_byte_data = ibm_cffps_read_byte_data,
 		.read_word_data = ibm_cffps_read_word_data,
 	},

@@ -486,12 +470,19 @@ static struct pmbus_platform_data ibm_cffps_pdata = {
 	.flags = PMBUS_SKIP_STATUS_CHECK | PMBUS_NO_CAPABILITY,
 };
 
+static const struct i2c_device_id ibm_cffps_id[] = {
+	{ "ibm_cffps1", cffps1 },
+	{ "ibm_cffps2", cffps2 },
+	{ "ibm_cffps", cffps_unknown },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
+
 static int ibm_cffps_probe(struct i2c_client *client)
 {
 	int i, rc;
 	enum versions vs = cffps_unknown;
 	struct dentry *debugfs;
-	struct dentry *ibm_cffps_dir;
 	struct ibm_cffps *psu;
 	const void *md = of_device_get_match_data(&client->dev);
 	const struct i2c_device_id *id;

@@ -560,8 +551,6 @@ static int ibm_cffps_probe(struct i2c_client *client)
 
 	psu->version = vs;
 	psu->client = client;
-	mutex_init(&psu->input_history.update_lock);
-	psu->input_history.last_update = jiffies - HZ;
 
 	ibm_cffps_create_led_class(psu);

@@ -570,55 +559,29 @@ static int ibm_cffps_probe(struct i2c_client *client)
 	if (!debugfs)
 		return 0;
 
-	ibm_cffps_dir = debugfs_create_dir(client->name, debugfs);
-	if (!ibm_cffps_dir)
-		return 0;
-
 	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
 		psu->debugfs_entries[i] = i;
 
-	debugfs_create_file("input_history", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_INPUT_HISTORY],
-			    &ibm_cffps_fops);
-	debugfs_create_file("mfg_id", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_MFG_ID],
-			    &ibm_cffps_fops);
-	debugfs_create_file("fru", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_FRU],
-			    &ibm_cffps_fops);
-	debugfs_create_file("part_number", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_PN],
-			    &ibm_cffps_fops);
-	debugfs_create_file("header", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_HEADER],
-			    &ibm_cffps_fops);
-	debugfs_create_file("serial_number", 0444, ibm_cffps_dir,
-			    &psu->debugfs_entries[CFFPS_DEBUGFS_SN],
-			    &ibm_cffps_fops);
-	debugfs_create_file("max_power_out", 0444, ibm_cffps_dir,
+	debugfs_create_file("input_history", 0444, debugfs, psu, &ibm_cffps_input_history_fops);
+	debugfs_create_file("max_power_out", 0444, debugfs,
 			    &psu->debugfs_entries[CFFPS_DEBUGFS_MAX_POWER_OUT],
 			    &ibm_cffps_fops);
-	debugfs_create_file("ccin", 0444, ibm_cffps_dir,
+	debugfs_create_file("ccin", 0444, debugfs,
 			    &psu->debugfs_entries[CFFPS_DEBUGFS_CCIN],
 			    &ibm_cffps_fops);
-	debugfs_create_file("fw_version", 0444, ibm_cffps_dir,
+	debugfs_create_file("fw_version", 0444, debugfs,
 			    &psu->debugfs_entries[CFFPS_DEBUGFS_FW],
 			    &ibm_cffps_fops);
-	debugfs_create_file("on_off_config", 0644, ibm_cffps_dir,
+	debugfs_create_file("on_off_config", 0644, debugfs,
 			    &psu->debugfs_entries[CFFPS_DEBUGFS_ON_OFF_CONFIG],
 			    &ibm_cffps_fops);
 
+	/* For compatibility with users of the old naming scheme. */
+	debugfs_create_symlink(client->name, debugfs, ".");
+
 	return 0;
 }
 
-static const struct i2c_device_id ibm_cffps_id[] = {
-	{ "ibm_cffps1", cffps1 },
-	{ "ibm_cffps2", cffps2 },
-	{ "ibm_cffps", cffps_unknown },
-	{}
-};
-MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
-
 static const struct of_device_id ibm_cffps_of_match[] = {
 	{
 		.compatible = "ibm,cffps1",

Signature · 1 line

2.31.1

Guenter Roeck

maintainer

2023-04-07 16:10 UTC #3 lore raw

Re: [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Use default debugfs attributes and lock function

Quoted 16 lines · Eddie James

Switch the driver to use the default debugfs attributes instead of ones that provide the same data under different names. Use the lock functions for the debugfs and led attributes, and simplify the input history operation by dropping the timer and lock.
Signed-off-by Eddie James <[email protected]>
---
 drivers/hwmon/pmbus/ibm-cffps.c | 273 ++++++++++++++------------------
 1 file changed, 118 insertions(+), 155 deletions(-)
drivers/hwmon/pmbus/ibm-cffps.c +0 -0
index e3294a1a54bb..2d7ec00e047b 100644
@@ -18,12 +18,6 @@
 

[ ... ]

Quoted 29 lines

drivers/hwmon/pmbus/ibm-cffps.c +9 ~~-10~~

@@ -225,29 +183,27 @@ static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf,
 			rc = i * 4;
 			break;
 		default:
-			return -EOPNOTSUPP;
+			rc = -EOPNOTSUPP;
+			break;
 		}
-		goto done;
+		break;
 	case CFFPS_DEBUGFS_ON_OFF_CONFIG:
-		rc = i2c_smbus_read_byte_data(psu->client,
-					      PMBUS_ON_OFF_CONFIG);
-		if (rc < 0)
-			return rc;
-
-		rc = snprintf(data, 3, "%02x", rc);
-		goto done;
+		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_ON_OFF_CONFIG);
+		if (rc >= 0)
+			rc = snprintf(data, 3, "%02x", rc);
+		break;
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
+		break;
 	}

drivers/hwmon/pmbus/ibm-cffps.c static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf, · line 250

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +2 -3

-	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+unlock:
+	pmbus_unlock(psu->client);
 	if (rc < 0)
 		return rc;
 
-done:
 	data[rc] = '\n';
 	rc += 2;
-

I hate to say (repeat) that, but please refrain from whitespace changes.

Guenter

Quoted 2 lines

drivers/hwmon/pmbus/ibm-cffps.c +0 -0
 	return simple_read_from_buffer(buf, count, ppos, data, rc);
 }

Eddie James

author

2023-04-12 15:25 UTC #4 lore raw

Re: [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Use default debugfs attributes and lock function

Quoted 53 lines · Guenter Roeck

Quoted 16 lines · Eddie James

Switch the driver to use the default debugfs attributes instead of ones that provide the same data under different names. Use the lock functions for the debugfs and led attributes, and simplify the input history operation by dropping the timer and lock.
Signed-off-by Eddie James <[email protected]>
---
  drivers/hwmon/pmbus/ibm-cffps.c | 273 ++++++++++++++------------------
  1 file changed, 118 insertions(+), 155 deletions(-)
drivers/hwmon/pmbus/ibm-cffps.c +0 -0
index e3294a1a54bb..2d7ec00e047b 100644
@@ -18,12 +18,6 @@
 

[ ... ]

Quoted 35 lines

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

@@ -225,29 +183,27 @@ static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf,
  			rc = i * 4;
  			break;
  		default:
-			return -EOPNOTSUPP;
+			rc = -EOPNOTSUPP;
+			break;
  		}
-		goto done;
+		break;
  	case CFFPS_DEBUGFS_ON_OFF_CONFIG:
-		rc = i2c_smbus_read_byte_data(psu->client,
-					      PMBUS_ON_OFF_CONFIG);
-		if (rc < 0)
-			return rc;
-
-		rc = snprintf(data, 3, "%02x", rc);
-		goto done;
+		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_ON_OFF_CONFIG);
+		if (rc >= 0)
+			rc = snprintf(data, 3, "%02x", rc);
+		break;
  	default:
-		return -EINVAL;
+		rc = -EINVAL;
+		break;
  	}
 
-	rc = i2c_smbus_read_block_data(psu->client, cmd, data);
+unlock:
+	pmbus_unlock(psu->client);
  	if (rc < 0)
  		return rc;

Quoted 5 lines

drivers/hwmon/pmbus/ibm-cffps.c static ssize_t ibm_cffps_debugfs_read(struct file *file, char __user *buf, · line 250
Quoted 4 lines
drivers/hwmon/pmbus/ibm-cffps.c +0 -2
-done:
  	data[rc] = '\n';
  	rc += 2;
-
I hate to say (repeat) that, but please refrain from whitespace changes.

Sure, sorry.

Thanks,

Eddie

Quoted 5 lines

Guenter
Quoted 2 lines
drivers/hwmon/pmbus/ibm-cffps.c +0 -0
  	return simple_read_from_buffer(buf, count, ppos, data, rc);
  }