[PATCH v7 0/2] Add support for q50sn12072 and q54sn120a1

[PATCH v7 0/2] Add support for q50sn12072 and q54sn120a1

[Brian Chiang]

The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
module from Delta Power Modules.

The Q50SN12072, quarter brick, single output 12V. This product provides up
to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
efficiency up to 98.3%@54Vin.

The Q54SN120A1, quarter brick, single output 12V. This product provides up
to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
efficiency up to 98.1%@54Vin.

Add support for them to q54sj108a2 driver.

Signed-off-by: Jack Cheng <Cheng.JackHY@inventec.com>
Signed-off-by: Brian Chiang <chiang.brian@inventec.com>

Changes in v7:
- Restore the PMBUS_WRITE_PROTECT register to its original state
- Fix potential denial of service by storing user input berfore
  acquiring lock
- Fix use-after-free risk by moving pmbus_do_probe() after
  devm_kzalloc() 
- Link to v6: https://lore.kernel.org/r/20260427-add-support-for-q50sn12072-and-q54sn120a1-v6-0-ffa77bfa65e7@inventec.com

Changes in v6:
- Add lock to debugfs read/write handlers
- Remove the incorrect Co-developed-by tag in cover letter patch reported by checkpatch.pl
- Link to v5: https://lore.kernel.org/r/20260422-add-support-for-q50sn12072-and-q54sn120a1-v5-0-b8fb87262868@inventec.com

Changes in v5:
- Rebase from 6.19 to 7.0-rc7 for review
- Link to v4: https://lore.kernel.org/r/20260414-add-support-for-q50sn12072-and-q54sn120a1-v4-0-b81eaea49df1@inventec.com

Changes in v4:
- Add null terminator to prevent comparison of uninitialized data which
takes place when ret is shorter than strlen(mid->name)
- Link to v3: https://lore.kernel.org/r/20260402-add-support-for-q50sn12072-and-q54sn120a1-v3-0-67a5184e93b8@inventec.com

Changes in v3:
- Fix MFR_MODEL detection by using strncasecmp prefix match, without the strict length equality
- Move blackbox_read_offset debugfs entry inside the q54sj108a2-only guard block
- Sort the debugfs entries by the order of PMBus register addresses 
- Link to v2: https://lore.kernel.org/r/20260326-add-support-for-q50sn12072-and-q54sn120a1-v2-0-77bc77eedc76@inventec.com

Changes in v2:
- Drop Q50SN12072_DEBUGFS_VOUT_COMMAND debugfs entry
- Add .format[PSC_VOLTAGE_OUT] = linear explicitly to all three chip
  entries for consistency
- Replace hardcoded MFR_MODEL check (ret != 14 || strncmp("Q54SJ108A2"))
  with a loop over q54sj108a2_id[] using strncasecmp to support all
  three chip variants dynamically
- Remove of_device_get_match_data()/i2c_match_id() early chip_id path;
  derive chip_id exclusively from MFR_MODEL hardware read
- Remove unused .data fields from of_device_id entries
- Guard store_default, blackbox_erase, blackbox_set_offset, blackbox_read,
  and flash_key debugfs entries under psu->chip == q54sj108a2 check
- Add dev_notice() when configured device name differs from detected model
- Update MODULE_DESCRIPTION to "PMBus driver for Delta Q54SJ108A2 and
  compatibles"
- Fix commit message typo: "Q54SN12072" -> "Q50SN12072"
- Link to v1: https://lore.kernel.org/r/20250701-add-support-for-q50sn12072-and-q54sn120a1-v1-0-c387baf928cb@inventec.com

---
Jack Cheng (2):
      dt-bindings: trivial: Add q50sn12072 and q54sn120a1 support
      hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

 .../devicetree/bindings/trivial-devices.yaml       |   4 +
 drivers/hwmon/pmbus/q54sj108a2.c                   | 238 +++++++++++++--------
 2 files changed, 151 insertions(+), 91 deletions(-)
---
base-commit: 591cd656a1bf5ea94a222af5ef2ee76df029c1d2
change-id: 20250701-add-support-for-q50sn12072-and-q54sn120a1-a9c299e6d81d

Best regards,
-- 
Brian Chiang <chiang.brian@inventec.com>

[PATCH v7 1/2] dt-bindings: trivial: Add q50sn12072 and q54sn120a1 support

[Brian Chiang]

From: Jack Cheng <cheng.jackhy@inventec.com>

Add support for the Delta Electronics q50sn12072 and q54sn120a1
1/4 Brick DC/DC Regulated Power Modules.

Signed-off-by: Jack Cheng <cheng.jackhy@inventec.com>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index a482aeadcd44..d4b78154df82 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -96,7 +96,11 @@ properties:
             # Delta Electronics DPS920AB 920W 54V Power Supply
           - delta,dps920ab
             # 1/4 Brick DC/DC Regulated Power Module
+          - delta,q50sn12072
+            # 1/4 Brick DC/DC Regulated Power Module
           - delta,q54sj108a2
+            # 1/4 Brick DC/DC Regulated Power Module
+          - delta,q54sn120a1
             # Devantech SRF02 ultrasonic ranger in I2C mode
           - devantech,srf02
             # Devantech SRF08 ultrasonic ranger

-- 
2.43.0

[PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[Brian Chiang]

From: Jack Cheng <cheng.jackhy@inventec.com>

The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
module from Delta Power Modules.

The Q50SN12072, quarter brick, single output 12V. This product provides up
to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
efficiency up to 98.3%@54Vin.

The Q54SN120A1, quarter brick, single output 12V. This product provides up
to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
efficiency up to 98.1%@54Vin.

Add support for them to q54sj108a2 driver.

Signed-off-by: Jack Cheng <cheng.jackhy@inventec.com>
Co-developed-by: Brian Chiang <chiang.brian@inventec.com>
Signed-off-by: Brian Chiang <chiang.brian@inventec.com>
---
 drivers/hwmon/pmbus/q54sj108a2.c | 238 ++++++++++++++++++++++++---------------
 1 file changed, 147 insertions(+), 91 deletions(-)

diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
index d5d60a9af8c5..50539555a74e 100644
--- a/drivers/hwmon/pmbus/q54sj108a2.c
+++ b/drivers/hwmon/pmbus/q54sj108a2.c
@@ -22,7 +22,9 @@
 #define PMBUS_FLASH_KEY_WRITE		0xEC
 
 enum chips {
-	q54sj108a2
+	q50sn12072,
+	q54sj108a2,
+	q54sn120a1
 };
 
 enum {
@@ -55,10 +57,24 @@ struct q54sj108a2_data {
 #define to_psu(x, y) container_of((x), struct q54sj108a2_data, debugfs_entries[(y)])
 
 static struct pmbus_driver_info q54sj108a2_info[] = {
-	[q54sj108a2] = {
+	[q50sn12072] = {
 		.pages = 1,
+		/* Source : Delta Q50SN12072 */
+		.format[PSC_VOLTAGE_OUT] = linear,
+		.format[PSC_TEMPERATURE] = linear,
+		.format[PSC_VOLTAGE_IN] = linear,
+		.format[PSC_CURRENT_OUT] = linear,
 
+		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
+		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
+		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_POUT,
+	},
+	[q54sj108a2] = {
+		.pages = 1,
 		/* Source : Delta Q54SJ108A2 */
+		.format[PSC_VOLTAGE_OUT] = linear,
 		.format[PSC_TEMPERATURE] = linear,
 		.format[PSC_VOLTAGE_IN] = linear,
 		.format[PSC_CURRENT_OUT] = linear,
@@ -69,6 +85,20 @@ static struct pmbus_driver_info q54sj108a2_info[] = {
 		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
 		PMBUS_HAVE_STATUS_INPUT,
 	},
+	[q54sn120a1] = {
+		.pages = 1,
+		/* Source : Delta Q54SN120A1 */
+		.format[PSC_VOLTAGE_OUT] = linear,
+		.format[PSC_TEMPERATURE] = linear,
+		.format[PSC_VOLTAGE_IN] = linear,
+		.format[PSC_CURRENT_OUT] = linear,
+
+		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
+		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
+		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_POUT,
+	},
 };
 
 static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
@@ -83,73 +113,77 @@ static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
 	char *out = data;
 	char *res;
 
+	rc = pmbus_lock_interruptible(psu->client);
+	if (rc)
+		return rc;
+
 	switch (idx) {
 	case Q54SJ108A2_DEBUGFS_OPERATION:
 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_OPERATION);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_WRITEPROTECT:
 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_WRITE_PROTECT);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_VOUT_OV_FAULT_RESPONSE);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_IOUT_OC_FAULT_RESPONSE);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_PMBUS_VERSION:
 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_REVISION);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_MFR_ID:
 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_ID, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 		break;
 	case Q54SJ108A2_DEBUGFS_MFR_MODEL:
 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_MODEL, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 		break;
 	case Q54SJ108A2_DEBUGFS_MFR_REVISION:
 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_REVISION, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 		break;
 	case Q54SJ108A2_DEBUGFS_MFR_LOCATION:
 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_LOCATION, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 		break;
 	case Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET:
 		rc = i2c_smbus_read_byte_data(psu->client, READ_HISTORY_EVENT_NUMBER);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = snprintf(data, 3, "%02x", rc);
 		break;
 	case Q54SJ108A2_DEBUGFS_BLACKBOX_READ:
 		rc = i2c_smbus_read_block_data(psu->client, READ_HISTORY_EVENTS, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		res = bin2hex(data_char, data, rc);
 		rc = res - data_char;
@@ -158,16 +192,22 @@ static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
 	case Q54SJ108A2_DEBUGFS_FLASH_KEY:
 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_FLASH_KEY_WRITE, data);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		res = bin2hex(data_char, data, rc);
 		rc = res - data_char;
 		out = data_char;
 		break;
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
+		goto unlock;
 	}
 
+unlock:
+	pmbus_unlock(psu->client);
+	if (rc < 0)
+		return rc;
+
 	out[rc] = '\n';
 	rc += 2;
 
@@ -183,27 +223,51 @@ static ssize_t q54sj108a2_debugfs_write(struct file *file, const char __user *bu
 	int *idxp = file->private_data;
 	int idx = *idxp;
 	struct q54sj108a2_data *psu = to_psu(idxp, idx);
+	int original_wp;
+	int rc_restore;
 
-	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
-	if (rc)
-		return rc;
-
+	/*
+	 * Parse user input before acquiring the PMBus lock. Since calling
+	 * kstrtou8_from_user() while holding pmbus_lock_interruptible()
+	 * may introduce a denial of service risk.
+	 */
 	switch (idx) {
 	case Q54SJ108A2_DEBUGFS_OPERATION:
+	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
+	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
+	case Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET:
 		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
 		if (rc < 0)
 			return rc;
+		break;
+	case Q54SJ108A2_DEBUGFS_CLEARFAULT:
+	case Q54SJ108A2_DEBUGFS_STOREDEFAULT:
+	case Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE:
+		break;
+	default:
+		return -EINVAL;
+	}
 
-		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_OPERATION, dst_data);
-		if (rc < 0)
-			return rc;
+	rc = pmbus_lock_interruptible(psu->client);
+	if (rc)
+		return rc;
+
+	original_wp = i2c_smbus_read_byte_data(psu->client, PMBUS_WRITE_PROTECT);
+	if (original_wp < 0) {
+		rc = original_wp;
+		goto unlock;
+	}
+
+	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
+	if (rc < 0)
+		goto unlock;
 
+	switch (idx) {
+	case Q54SJ108A2_DEBUGFS_OPERATION:
+		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_OPERATION, dst_data);
 		break;
 	case Q54SJ108A2_DEBUGFS_CLEARFAULT:
 		rc = i2c_smbus_write_byte(psu->client, PMBUS_CLEAR_FAULTS);
-		if (rc < 0)
-			return rc;
-
 		break;
 	case Q54SJ108A2_DEBUGFS_STOREDEFAULT:
 		flash_key[0] = 0x7E;
@@ -212,52 +276,35 @@ static ssize_t q54sj108a2_debugfs_write(struct file *file, const char __user *bu
 		flash_key[3] = 0x42;
 		rc = i2c_smbus_write_block_data(psu->client, PMBUS_FLASH_KEY_WRITE, 4, flash_key);
 		if (rc < 0)
-			return rc;
-
+			break;
 		rc = i2c_smbus_write_byte(psu->client, STORE_DEFAULT_ALL);
-		if (rc < 0)
-			return rc;
-
 		break;
 	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
-		if (rc < 0)
-			return rc;
-
 		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_VOUT_OV_FAULT_RESPONSE, dst_data);
-		if (rc < 0)
-			return rc;
-
 		break;
 	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
-		if (rc < 0)
-			return rc;
-
 		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_IOUT_OC_FAULT_RESPONSE, dst_data);
-		if (rc < 0)
-			return rc;
-
 		break;
 	case Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE:
 		rc = i2c_smbus_write_byte(psu->client, ERASE_BLACKBOX_DATA);
-		if (rc < 0)
-			return rc;
-
 		break;
 	case Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET:
-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
-		if (rc < 0)
-			return rc;
-
 		rc = i2c_smbus_write_byte_data(psu->client, SET_HISTORY_EVENT_OFFSET, dst_data);
-		if (rc < 0)
-			return rc;
-
 		break;
-	default:
-		return -EINVAL;
 	}
+	/*
+	 * Always restore WRITE_PROTECT and preserve the original error in
+	 * rc; only surface the restore failure if the operation itself was
+	 * successful.
+	 */
+	rc_restore = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, original_wp);
+	if (rc_restore < 0 && rc >= 0)
+		rc = rc_restore;
+
+unlock:
+	pmbus_unlock(psu->client);
+	if (rc < 0)
+		return rc;
 
 	return count;
 }
@@ -270,7 +317,9 @@ static const struct file_operations q54sj108a2_fops = {
 };
 
 static const struct i2c_device_id q54sj108a2_id[] = {
+	{ "q50sn12072", q50sn12072 },
 	{ "q54sj108a2", q54sj108a2 },
+	{ "q54sn120a1", q54sn120a1 },
 	{ },
 };
 
@@ -280,6 +329,7 @@ static int q54sj108a2_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+	const struct i2c_device_id *mid;
 	enum chips chip_id;
 	int ret, i;
 	struct dentry *debugfs;
@@ -292,14 +342,9 @@ static int q54sj108a2_probe(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_BLOCK_DATA))
 		return -ENODEV;
 
-	if (client->dev.of_node)
-		chip_id = (enum chips)(unsigned long)of_device_get_match_data(dev);
-	else
-		chip_id = i2c_match_id(q54sj108a2_id, client)->driver_data;
-
 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
 	if (ret < 0) {
-		dev_err(&client->dev, "Failed to read Manufacturer ID\n");
+		dev_err(dev, "Failed to read Manufacturer ID\n");
 		return ret;
 	}
 	if (ret != 6 || strncmp(buf, "DELTA", 5)) {
@@ -308,19 +353,25 @@ static int q54sj108a2_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	/*
-	 * The chips support reading PMBUS_MFR_MODEL.
-	 */
 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
 	if (ret < 0) {
 		dev_err(dev, "Failed to read Manufacturer Model\n");
 		return ret;
 	}
-	if (ret != 14 || strncmp(buf, "Q54SJ108A2", 10)) {
-		buf[ret] = '\0';
+	buf[ret] = '\0';
+	for (mid = q54sj108a2_id; mid->name[0]; mid++) {
+		if (!strncasecmp(mid->name, buf, strlen(mid->name)))
+			break;
+	}
+	if (!mid->name[0]) {
 		dev_err(dev, "Unsupported Manufacturer Model '%s'\n", buf);
 		return -ENODEV;
 	}
+	chip_id = mid->driver_data;
+
+	if (strcmp(client->name, mid->name) != 0)
+		dev_notice(dev, "Device mismatch: Configured %s, detected %s\n",
+			   client->name, mid->name);
 
 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_REVISION, buf);
 	if (ret < 0) {
@@ -333,16 +384,17 @@ static int q54sj108a2_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
-	if (ret)
-		return ret;
-
 	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
 	if (!psu)
 		return 0;
 
+	psu->chip = chip_id;
 	psu->client = client;
 
+	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
+	if (ret)
+		return ret;
+
 	debugfs = pmbus_get_debugfs_dir(client);
 
 	q54sj108a2_dir = debugfs_create_dir(client->name, debugfs);
@@ -359,9 +411,6 @@ static int q54sj108a2_probe(struct i2c_client *client)
 	debugfs_create_file("write_protect", 0444, q54sj108a2_dir,
 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_WRITEPROTECT],
 			    &q54sj108a2_fops);
-	debugfs_create_file("store_default", 0200, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_STOREDEFAULT],
-			    &q54sj108a2_fops);
 	debugfs_create_file("vo_ov_response", 0644, q54sj108a2_dir,
 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_VOOV_RESPONSE],
 			    &q54sj108a2_fops);
@@ -383,27 +432,34 @@ static int q54sj108a2_probe(struct i2c_client *client)
 	debugfs_create_file("mfr_location", 0444, q54sj108a2_dir,
 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_MFR_LOCATION],
 			    &q54sj108a2_fops);
-	debugfs_create_file("blackbox_erase", 0200, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE],
-			    &q54sj108a2_fops);
-	debugfs_create_file("blackbox_read_offset", 0444, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET],
-			    &q54sj108a2_fops);
-	debugfs_create_file("blackbox_set_offset", 0200, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET],
-			    &q54sj108a2_fops);
-	debugfs_create_file("blackbox_read", 0444, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ],
-			    &q54sj108a2_fops);
-	debugfs_create_file("flash_key", 0444, q54sj108a2_dir,
-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_FLASH_KEY],
-			    &q54sj108a2_fops);
+	if (psu->chip == q54sj108a2) {
+		debugfs_create_file("store_default", 0200, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_STOREDEFAULT],
+				    &q54sj108a2_fops);
+		debugfs_create_file("blackbox_erase", 0200, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE],
+				    &q54sj108a2_fops);
+		debugfs_create_file("blackbox_read_offset", 0444, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET],
+				    &q54sj108a2_fops);
+		debugfs_create_file("blackbox_read", 0444, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ],
+				    &q54sj108a2_fops);
+		debugfs_create_file("blackbox_set_offset", 0200, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET],
+				    &q54sj108a2_fops);
+		debugfs_create_file("flash_key", 0444, q54sj108a2_dir,
+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_FLASH_KEY],
+				    &q54sj108a2_fops);
+	}
 
 	return 0;
 }
 
 static const struct of_device_id q54sj108a2_of_match[] = {
-	{ .compatible = "delta,q54sj108a2", .data = (void *)q54sj108a2 },
+	{ .compatible = "delta,q50sn12072" },
+	{ .compatible = "delta,q54sj108a2" },
+	{ .compatible = "delta,q54sn120a1" },
 	{ },
 };
 
@@ -421,6 +477,6 @@ static struct i2c_driver q54sj108a2_driver = {
 module_i2c_driver(q54sj108a2_driver);
 
 MODULE_AUTHOR("Xiao.Ma <xiao.mx.ma@deltaww.com>");
-MODULE_DESCRIPTION("PMBus driver for Delta Q54SJ108A2 series modules");
+MODULE_DESCRIPTION("PMBus driver for Delta Q54SJ108A2 and compatibles");
 MODULE_LICENSE("GPL");
 MODULE_IMPORT_NS("PMBUS");

-- 
2.43.0

Re: [PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning 0 on `devm_kzalloc` failure causes silent driver probe failure.
- [Medium] The patch introduces several significant bug fixes and refactorings that are not mentioned in the commit message.
--

commit 64436d69954e3cda83f05bdee9e09fc2269a2808
Author: Jack Cheng <cheng.jackhy@inventec.com>
hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

This commit adds support for the Delta Q50SN12072 and Q54SN120A1 DC-DC
power modules to the q54sj108a2 driver.

> Add support for them to q54sj108a2 driver.

This isn't a bug, but the commit message only mentions adding support for
the new modules. However, the patch also includes several other changes:
adding PMBus locking in the debugfs read/write paths, fixing the
WRITE_PROTECT restore logic, modifying the configuration for the existing
q54sj108a2 module, and refactoring the device identification logic.

Could the commit message be updated to describe these additional changes,
or should they be split into separate patches?

> diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
> --- a/drivers/hwmon/pmbus/q54sj108a2.c
> +++ b/drivers/hwmon/pmbus/q54sj108a2.c

[ ... ]

> @@ -333,16 +384,17 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> -	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
> -	if (ret)
> -		return ret;
> -
>  	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
>  	if (!psu)
>  		return 0;

Because the psu allocation was moved before pmbus_do_probe(), returning 0
on allocation failure now skips registering the hwmon device entirely.
Since the probe function returns success (0), does this leave the driver
in a broken state? Should this return -ENOMEM instead?

>  
> +	psu->chip = chip_id;
>  	psu->client = client;
>  
> +	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
> +	if (ret)
> +		return ret;
> +
>  	debugfs = pmbus_get_debugfs_dir(client);
>  
>  	q54sj108a2_dir = debugfs_create_dir(client->name, debugfs);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429-add-support-for-q50sn12072-and-q54sn120a1-v7-0-f1f83d2039f7@inventec.com?part=2

Re: [PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[Brian Chiang]

On Wed, Apr 29, 2026 at 11:29:37AM +0000, Brian Chiang wrote:
>From: Jack Cheng <cheng.jackhy@inventec.com>
>
>The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
>module from Delta Power Modules.
>
>The Q50SN12072, quarter brick, single output 12V. This product provides up
>to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
>efficiency up to 98.3%@54Vin.
>
>The Q54SN120A1, quarter brick, single output 12V. This product provides up
>to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
>efficiency up to 98.1%@54Vin.
>
>Add support for them to q54sj108a2 driver.

Greetings, I received the feedback from Sashiko for this patch:

```
This isn't a bug, but the commit message only mentions adding support for
the new modules. However, the patch also includes several other changes:
adding PMBus locking in the debugfs read/write paths, fixing the
WRITE_PROTECT restore logic, modifying the configuration for the existing
q54sj108a2 module, and refactoring the device identification logic.
Could the commit message be updated to describe these additional changes,
or should they be split into separate patches?
```

I'm wondering if it is more appropriate to split only `fixing the 
WRITE_PROTECT restore logic` into separate patch? Since disabling 
WRITE_PROTECT was introduced in previous commit. And maybe keeping
other changes Sashiko mentioned in this patch and record them in 
the commit message?

Please let me know if you have any suggestion, thanks.

>
>Signed-off-by: Jack Cheng <cheng.jackhy@inventec.com>
>Co-developed-by: Brian Chiang <chiang.brian@inventec.com>
>Signed-off-by: Brian Chiang <chiang.brian@inventec.com>
>---
> drivers/hwmon/pmbus/q54sj108a2.c | 238 ++++++++++++++++++++++++---------------
> 1 file changed, 147 insertions(+), 91 deletions(-)
>
>diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
>index d5d60a9af8c5..50539555a74e 100644
>--- a/drivers/hwmon/pmbus/q54sj108a2.c
>+++ b/drivers/hwmon/pmbus/q54sj108a2.c
>@@ -22,7 +22,9 @@
> #define PMBUS_FLASH_KEY_WRITE		0xEC
>
> enum chips {
>-	q54sj108a2
>+	q50sn12072,
>+	q54sj108a2,
>+	q54sn120a1
> };
>
> enum {
>@@ -55,10 +57,24 @@ struct q54sj108a2_data {
> #define to_psu(x, y) container_of((x), struct q54sj108a2_data, debugfs_entries[(y)])
>
> static struct pmbus_driver_info q54sj108a2_info[] = {
>-	[q54sj108a2] = {
>+	[q50sn12072] = {
> 		.pages = 1,
>+		/* Source : Delta Q50SN12072 */
>+		.format[PSC_VOLTAGE_OUT] = linear,
>+		.format[PSC_TEMPERATURE] = linear,
>+		.format[PSC_VOLTAGE_IN] = linear,
>+		.format[PSC_CURRENT_OUT] = linear,
>
>+		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
>+		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
>+		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>+		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
>+		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_POUT,
>+	},
>+	[q54sj108a2] = {
>+		.pages = 1,
> 		/* Source : Delta Q54SJ108A2 */
>+		.format[PSC_VOLTAGE_OUT] = linear,
> 		.format[PSC_TEMPERATURE] = linear,
> 		.format[PSC_VOLTAGE_IN] = linear,
> 		.format[PSC_CURRENT_OUT] = linear,
>@@ -69,6 +85,20 @@ static struct pmbus_driver_info q54sj108a2_info[] = {
> 		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> 		PMBUS_HAVE_STATUS_INPUT,
> 	},
>+	[q54sn120a1] = {
>+		.pages = 1,
>+		/* Source : Delta Q54SN120A1 */
>+		.format[PSC_VOLTAGE_OUT] = linear,
>+		.format[PSC_TEMPERATURE] = linear,
>+		.format[PSC_VOLTAGE_IN] = linear,
>+		.format[PSC_CURRENT_OUT] = linear,
>+
>+		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
>+		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
>+		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>+		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
>+		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_POUT,
>+	},
> };
>
> static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
>@@ -83,73 +113,77 @@ static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
> 	char *out = data;
> 	char *res;
>
>+	rc = pmbus_lock_interruptible(psu->client);
>+	if (rc)
>+		return rc;
>+
> 	switch (idx) {
> 	case Q54SJ108A2_DEBUGFS_OPERATION:
> 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_OPERATION);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_WRITEPROTECT:
> 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_WRITE_PROTECT);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
> 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_VOUT_OV_FAULT_RESPONSE);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
> 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_IOUT_OC_FAULT_RESPONSE);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_PMBUS_VERSION:
> 		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_REVISION);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_MFR_ID:
> 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_ID, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
> 		break;
> 	case Q54SJ108A2_DEBUGFS_MFR_MODEL:
> 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_MODEL, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
> 		break;
> 	case Q54SJ108A2_DEBUGFS_MFR_REVISION:
> 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_REVISION, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
> 		break;
> 	case Q54SJ108A2_DEBUGFS_MFR_LOCATION:
> 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_MFR_LOCATION, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
> 		break;
> 	case Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET:
> 		rc = i2c_smbus_read_byte_data(psu->client, READ_HISTORY_EVENT_NUMBER);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		rc = snprintf(data, 3, "%02x", rc);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_BLACKBOX_READ:
> 		rc = i2c_smbus_read_block_data(psu->client, READ_HISTORY_EVENTS, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		res = bin2hex(data_char, data, rc);
> 		rc = res - data_char;
>@@ -158,16 +192,22 @@ static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
> 	case Q54SJ108A2_DEBUGFS_FLASH_KEY:
> 		rc = i2c_smbus_read_block_data(psu->client, PMBUS_FLASH_KEY_WRITE, data);
> 		if (rc < 0)
>-			return rc;
>+			goto unlock;
>
> 		res = bin2hex(data_char, data, rc);
> 		rc = res - data_char;
> 		out = data_char;
> 		break;
> 	default:
>-		return -EINVAL;
>+		rc = -EINVAL;
>+		goto unlock;
> 	}
>
>+unlock:
>+	pmbus_unlock(psu->client);
>+	if (rc < 0)
>+		return rc;
>+
> 	out[rc] = '\n';
> 	rc += 2;
>
>@@ -183,27 +223,51 @@ static ssize_t q54sj108a2_debugfs_write(struct file *file, const char __user *bu
> 	int *idxp = file->private_data;
> 	int idx = *idxp;
> 	struct q54sj108a2_data *psu = to_psu(idxp, idx);
>+	int original_wp;
>+	int rc_restore;
>
>-	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
>-	if (rc)
>-		return rc;
>-
>+	/*
>+	 * Parse user input before acquiring the PMBus lock. Since calling
>+	 * kstrtou8_from_user() while holding pmbus_lock_interruptible()
>+	 * may introduce a denial of service risk.
>+	 */
> 	switch (idx) {
> 	case Q54SJ108A2_DEBUGFS_OPERATION:
>+	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
>+	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
>+	case Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET:
> 		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
> 		if (rc < 0)
> 			return rc;
>+		break;
>+	case Q54SJ108A2_DEBUGFS_CLEARFAULT:
>+	case Q54SJ108A2_DEBUGFS_STOREDEFAULT:
>+	case Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE:
>+		break;
>+	default:
>+		return -EINVAL;
>+	}
>
>-		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_OPERATION, dst_data);
>-		if (rc < 0)
>-			return rc;
>+	rc = pmbus_lock_interruptible(psu->client);
>+	if (rc)
>+		return rc;
>+
>+	original_wp = i2c_smbus_read_byte_data(psu->client, PMBUS_WRITE_PROTECT);
>+	if (original_wp < 0) {
>+		rc = original_wp;
>+		goto unlock;
>+	}
>+
>+	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
>+	if (rc < 0)
>+		goto unlock;
>
>+	switch (idx) {
>+	case Q54SJ108A2_DEBUGFS_OPERATION:
>+		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_OPERATION, dst_data);
> 		break;
> 	case Q54SJ108A2_DEBUGFS_CLEARFAULT:
> 		rc = i2c_smbus_write_byte(psu->client, PMBUS_CLEAR_FAULTS);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
> 	case Q54SJ108A2_DEBUGFS_STOREDEFAULT:
> 		flash_key[0] = 0x7E;
>@@ -212,52 +276,35 @@ static ssize_t q54sj108a2_debugfs_write(struct file *file, const char __user *bu
> 		flash_key[3] = 0x42;
> 		rc = i2c_smbus_write_block_data(psu->client, PMBUS_FLASH_KEY_WRITE, 4, flash_key);
> 		if (rc < 0)
>-			return rc;
>-
>+			break;
> 		rc = i2c_smbus_write_byte(psu->client, STORE_DEFAULT_ALL);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
> 	case Q54SJ108A2_DEBUGFS_VOOV_RESPONSE:
>-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_VOUT_OV_FAULT_RESPONSE, dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
> 	case Q54SJ108A2_DEBUGFS_IOOC_RESPONSE:
>-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		rc = i2c_smbus_write_byte_data(psu->client, PMBUS_IOUT_OC_FAULT_RESPONSE, dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
> 	case Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE:
> 		rc = i2c_smbus_write_byte(psu->client, ERASE_BLACKBOX_DATA);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
> 	case Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET:
>-		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		rc = i2c_smbus_write_byte_data(psu->client, SET_HISTORY_EVENT_OFFSET, dst_data);
>-		if (rc < 0)
>-			return rc;
>-
> 		break;
>-	default:
>-		return -EINVAL;
> 	}
>+	/*
>+	 * Always restore WRITE_PROTECT and preserve the original error in
>+	 * rc; only surface the restore failure if the operation itself was
>+	 * successful.
>+	 */
>+	rc_restore = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, original_wp);
>+	if (rc_restore < 0 && rc >= 0)
>+		rc = rc_restore;
>+
>+unlock:
>+	pmbus_unlock(psu->client);
>+	if (rc < 0)
>+		return rc;
>
> 	return count;
> }
>@@ -270,7 +317,9 @@ static const struct file_operations q54sj108a2_fops = {
> };
>
> static const struct i2c_device_id q54sj108a2_id[] = {
>+	{ "q50sn12072", q50sn12072 },
> 	{ "q54sj108a2", q54sj108a2 },
>+	{ "q54sn120a1", q54sn120a1 },
> 	{ },
> };
>
>@@ -280,6 +329,7 @@ static int q54sj108a2_probe(struct i2c_client *client)
> {
> 	struct device *dev = &client->dev;
> 	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
>+	const struct i2c_device_id *mid;
> 	enum chips chip_id;
> 	int ret, i;
> 	struct dentry *debugfs;
>@@ -292,14 +342,9 @@ static int q54sj108a2_probe(struct i2c_client *client)
> 				     I2C_FUNC_SMBUS_BLOCK_DATA))
> 		return -ENODEV;
>
>-	if (client->dev.of_node)
>-		chip_id = (enum chips)(unsigned long)of_device_get_match_data(dev);
>-	else
>-		chip_id = i2c_match_id(q54sj108a2_id, client)->driver_data;
>-
> 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
> 	if (ret < 0) {
>-		dev_err(&client->dev, "Failed to read Manufacturer ID\n");
>+		dev_err(dev, "Failed to read Manufacturer ID\n");
> 		return ret;
> 	}
> 	if (ret != 6 || strncmp(buf, "DELTA", 5)) {
>@@ -308,19 +353,25 @@ static int q54sj108a2_probe(struct i2c_client *client)
> 		return -ENODEV;
> 	}
>
>-	/*
>-	 * The chips support reading PMBUS_MFR_MODEL.
>-	 */
> 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> 	if (ret < 0) {
> 		dev_err(dev, "Failed to read Manufacturer Model\n");
> 		return ret;
> 	}
>-	if (ret != 14 || strncmp(buf, "Q54SJ108A2", 10)) {
>-		buf[ret] = '\0';
>+	buf[ret] = '\0';
>+	for (mid = q54sj108a2_id; mid->name[0]; mid++) {
>+		if (!strncasecmp(mid->name, buf, strlen(mid->name)))
>+			break;
>+	}
>+	if (!mid->name[0]) {
> 		dev_err(dev, "Unsupported Manufacturer Model '%s'\n", buf);
> 		return -ENODEV;
> 	}
>+	chip_id = mid->driver_data;
>+
>+	if (strcmp(client->name, mid->name) != 0)
>+		dev_notice(dev, "Device mismatch: Configured %s, detected %s\n",
>+			   client->name, mid->name);
>
> 	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_REVISION, buf);
> 	if (ret < 0) {
>@@ -333,16 +384,17 @@ static int q54sj108a2_probe(struct i2c_client *client)
> 		return -ENODEV;
> 	}
>
>-	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
>-	if (ret)
>-		return ret;
>-
> 	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> 	if (!psu)
> 		return 0;
>
>+	psu->chip = chip_id;
> 	psu->client = client;
>
>+	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
>+	if (ret)
>+		return ret;
>+
> 	debugfs = pmbus_get_debugfs_dir(client);
>
> 	q54sj108a2_dir = debugfs_create_dir(client->name, debugfs);
>@@ -359,9 +411,6 @@ static int q54sj108a2_probe(struct i2c_client *client)
> 	debugfs_create_file("write_protect", 0444, q54sj108a2_dir,
> 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_WRITEPROTECT],
> 			    &q54sj108a2_fops);
>-	debugfs_create_file("store_default", 0200, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_STOREDEFAULT],
>-			    &q54sj108a2_fops);
> 	debugfs_create_file("vo_ov_response", 0644, q54sj108a2_dir,
> 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_VOOV_RESPONSE],
> 			    &q54sj108a2_fops);
>@@ -383,27 +432,34 @@ static int q54sj108a2_probe(struct i2c_client *client)
> 	debugfs_create_file("mfr_location", 0444, q54sj108a2_dir,
> 			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_MFR_LOCATION],
> 			    &q54sj108a2_fops);
>-	debugfs_create_file("blackbox_erase", 0200, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE],
>-			    &q54sj108a2_fops);
>-	debugfs_create_file("blackbox_read_offset", 0444, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET],
>-			    &q54sj108a2_fops);
>-	debugfs_create_file("blackbox_set_offset", 0200, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET],
>-			    &q54sj108a2_fops);
>-	debugfs_create_file("blackbox_read", 0444, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ],
>-			    &q54sj108a2_fops);
>-	debugfs_create_file("flash_key", 0444, q54sj108a2_dir,
>-			    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_FLASH_KEY],
>-			    &q54sj108a2_fops);
>+	if (psu->chip == q54sj108a2) {
>+		debugfs_create_file("store_default", 0200, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_STOREDEFAULT],
>+				    &q54sj108a2_fops);
>+		debugfs_create_file("blackbox_erase", 0200, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_ERASE],
>+				    &q54sj108a2_fops);
>+		debugfs_create_file("blackbox_read_offset", 0444, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ_OFFSET],
>+				    &q54sj108a2_fops);
>+		debugfs_create_file("blackbox_read", 0444, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_READ],
>+				    &q54sj108a2_fops);
>+		debugfs_create_file("blackbox_set_offset", 0200, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_BLACKBOX_SET_OFFSET],
>+				    &q54sj108a2_fops);
>+		debugfs_create_file("flash_key", 0444, q54sj108a2_dir,
>+				    &psu->debugfs_entries[Q54SJ108A2_DEBUGFS_FLASH_KEY],
>+				    &q54sj108a2_fops);
>+	}
>
> 	return 0;
> }
>
> static const struct of_device_id q54sj108a2_of_match[] = {
>-	{ .compatible = "delta,q54sj108a2", .data = (void *)q54sj108a2 },
>+	{ .compatible = "delta,q50sn12072" },
>+	{ .compatible = "delta,q54sj108a2" },
>+	{ .compatible = "delta,q54sn120a1" },
> 	{ },
> };
>
>@@ -421,6 +477,6 @@ static struct i2c_driver q54sj108a2_driver = {
> module_i2c_driver(q54sj108a2_driver);
>
> MODULE_AUTHOR("Xiao.Ma <xiao.mx.ma@deltaww.com>");
>-MODULE_DESCRIPTION("PMBus driver for Delta Q54SJ108A2 series modules");
>+MODULE_DESCRIPTION("PMBus driver for Delta Q54SJ108A2 and compatibles");
> MODULE_LICENSE("GPL");
> MODULE_IMPORT_NS("PMBUS");
>
>-- 
>2.43.0
>

Best regards,
Brian

Re: [PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[Guenter Roeck]

On 4/29/26 23:58, Brian Chiang wrote:
> On Wed, Apr 29, 2026 at 11:29:37AM +0000, Brian Chiang wrote:
>> From: Jack Cheng <cheng.jackhy@inventec.com>
>>
>> The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
>> module from Delta Power Modules.
>>
>> The Q50SN12072, quarter brick, single output 12V. This product provides up
>> to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
>> efficiency up to 98.3%@54Vin.
>>
>> The Q54SN120A1, quarter brick, single output 12V. This product provides up
>> to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
>> efficiency up to 98.1%@54Vin.
>>
>> Add support for them to q54sj108a2 driver.
> 
> Greetings, I received the feedback from Sashiko for this patch:
> 
> ```
> This isn't a bug, but the commit message only mentions adding support for
> the new modules. However, the patch also includes several other changes:
> adding PMBus locking in the debugfs read/write paths, fixing the
> WRITE_PROTECT restore logic, modifying the configuration for the existing
> q54sj108a2 module, and refactoring the device identification logic.
> Could the commit message be updated to describe these additional changes,
> or should they be split into separate patches?
> ```
> 
> I'm wondering if it is more appropriate to split only `fixing the WRITE_PROTECT restore logic` into separate patch? Since disabling WRITE_PROTECT was introduced in previous commit. And maybe keeping
> other changes Sashiko mentioned in this patch and record them in the commit message?
> 
> Please let me know if you have any suggestion, thanks.
> 

Sorry, I seem to be missing something. I dont understand the logic above.
What does fixing a bug have to do with how or when it was introduced ?

The missing lock in debugfs functions is a pre-existing bug, isn't it ?
All pre-existing bugs should be fixed first in separate patches so they
can be backported.

Please use guard(pmbus_lock)(client) instead of pmbus_lock_interruptible().

Also, your patch does not apply anymore. Support for q54sn120a1 was already
added in a separate patch. Please rebase your patches to v7.1-rc1.

Thanks,
Guenter

Re: [PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[Brian Chiang]

On Thu, Apr 30, 2026 at 07:01:22AM -0700, Guenter Roeck wrote:
>On 4/29/26 23:58, Brian Chiang wrote:
>>On Wed, Apr 29, 2026 at 11:29:37AM +0000, Brian Chiang wrote:
>>>From: Jack Cheng <cheng.jackhy@inventec.com>
>>>
>>>The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
>>>module from Delta Power Modules.
>>>
>>>The Q50SN12072, quarter brick, single output 12V. This product provides up
>>>to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
>>>efficiency up to 98.3%@54Vin.
>>>
>>>The Q54SN120A1, quarter brick, single output 12V. This product provides up
>>>to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
>>>efficiency up to 98.1%@54Vin.
>>>
>>>Add support for them to q54sj108a2 driver.
>>
>>Greetings, I received the feedback from Sashiko for this patch:
>>
>>```
>>This isn't a bug, but the commit message only mentions adding support for
>>the new modules. However, the patch also includes several other changes:
>>adding PMBus locking in the debugfs read/write paths, fixing the
>>WRITE_PROTECT restore logic, modifying the configuration for the existing
>>q54sj108a2 module, and refactoring the device identification logic.
>>Could the commit message be updated to describe these additional changes,
>>or should they be split into separate patches?
>>```
>>
>>I'm wondering if it is more appropriate to split only `fixing the WRITE_PROTECT restore logic` into separate patch? Since disabling WRITE_PROTECT was introduced in previous commit. And maybe keeping
>>other changes Sashiko mentioned in this patch and record them in the commit message?
>>
>>Please let me know if you have any suggestion, thanks.
>>
>
>Sorry, I seem to be missing something. I dont understand the logic above.
>What does fixing a bug have to do with how or when it was introduced ?
>
>The missing lock in debugfs functions is a pre-existing bug, isn't it ?
>All pre-existing bugs should be fixed first in separate patches so they
>can be backported.
>

Got it, I'll split them into standalone fix patches at the start of the
series.

>Please use guard(pmbus_lock)(client) instead of pmbus_lock_interruptible().
>

Sure, I'll do this in v8.

>Also, your patch does not apply anymore. Support for q54sn120a1 was already
>added in a separate patch. Please rebase your patches to v7.1-rc1.
>

Thank you for the notice, I'll rebase the patchset to lateset tag.


Thanks again for the review and suggestion and apologies for the delayed
response.

>Thanks,
>Guenter

Best Regards,
Brian