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

[Guenter Roeck]

On Thu, Mar 26, 2026 at 01:48:06PM +0000, Brian Chiang wrote:
> From: Jack Cheng <cheng.jackhy@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> Co-developed-by: Brian Chiang <chiang.brian@xxxxxxxxxxxx>
> Signed-off-by: Brian Chiang <chiang.brian@xxxxxxxxxxxx>
> ---
>  drivers/hwmon/pmbus/q54sj108a2.c | 97 ++++++++++++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
> index d5d60a9af8c5..cc2b32ad935c 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,
> @@ -270,7 +300,9 @@ static const struct file_operations q54sj108a2_fops = {
>  };
>  
>  static const struct i2c_device_id q54sj108a2_id[] = {
> +	{ "q50sn12072", q50sn12072 },
>  	{ "q54sj108a2", q54sj108a2 },
> +	{ "q54sn120a1", q54sn120a1 },
>  	{ },
>  };
>  
> @@ -280,6 +312,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 +325,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 +336,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)) {
> +	for (mid = q54sj108a2_id; mid->name[0]; mid++) {
> +		if (ret == strlen(mid->name) && !strncasecmp(mid->name, buf, ret))
> +			break;
> +	}
> +	if (!mid->name[0]) {
>  		buf[ret] = '\0';
>  		dev_err(dev, "Unsupported Manufacturer Model '%s'\n", buf);
>  		return -ENODEV;
>  	}

>From Sashiko feedback:

In the original driver, the PMBUS_MFR_MODEL response length was explicitly
expected to be 14 bytes (if (ret != 14 || strncmp(buf, "Q54SJ108A2", 10))),
indicating the hardware returns the 10-character name padded with 4 extra
bytes. The patch changes the detection logic to loop through supported
devices and strictly require the returned length to match the device name's
length exactly (`ret == strlen(mid->name)`). Since `ret` will be 14 for the
original hardware, and strlen("q54sj108a2") is 10, the condition 14 == 10
evaluates to false. This causes the loop to exit without matching,
erroneously printing 'Unsupported Manufacturer Model' and returning -ENODEV.
This completely breaks driver probing and hardware monitoring for the
pre-existing Q54SJ108A2 device."

> +	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) {
> @@ -341,6 +375,7 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  	if (!psu)
>  		return 0;
>  
> +	psu->chip = chip_id;
>  	psu->client = client;
>  
>  	debugfs = pmbus_get_debugfs_dir(client);
> @@ -359,9 +394,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 +415,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);

What is the purpose/value of keeping this file outside the if() block ?

> -	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_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);
> +	}
>  
>  	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" },

Why drop .data here ?

Thanks,
Guenter

>  	{ },
>  };
>  
> @@ -421,6 +460,6 @@ static struct i2c_driver q54sj108a2_driver = {
>  module_i2c_driver(q54sj108a2_driver);
>  
>  MODULE_AUTHOR("Xiao.Ma <xiao.mx.ma@xxxxxxxxxxx>");
> -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
>