linux-kernel - Re: [PATCH 1/3] hwmon: (pmbus) Add Delta Q54SN120A1 Q54SW120A7 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa95034e-2de9-46e6-bc4b-2f117df7b240@roeck-us.net>
Date: Thu, 5 Feb 2026 07:01:40 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Colin Huang <u8813345@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, Colin.Huang2@....com, Carl.Lee@....com,
	Peter.Shen@....com
Subject: Re: [PATCH 1/3] hwmon: (pmbus) Add Delta Q54SN120A1 Q54SW120A7 driver

On Thu, Feb 05, 2026 at 09:34:35PM +0800, Colin Huang wrote:
> Add the pmbus driver for DELTA Q54SN120A1, Q54SW120A7,
> 1/4 Brick DC/DC Regulated Power Module with PMBus support
> 

This isn't adding the driver, it is adding support for the chips
to the q54sj108a2 driver.

> Signed-off-by: Colin Huang <u8813345@...il.com>
> ---
>  drivers/hwmon/pmbus/q54sj108a2.c | 47 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
> index 4d7086d83aa3..dca084c98fba 100644
> --- a/drivers/hwmon/pmbus/q54sj108a2.c
> +++ b/drivers/hwmon/pmbus/q54sj108a2.c
> @@ -21,7 +21,9 @@
>  #define PMBUS_FLASH_KEY_WRITE		0xEC
>  
>  enum chips {
> -	q54sj108a2
> +	q54sj108a2,
> +	q54sn120a1,
> +	q54sw120a7
>  };
>  
>  enum {
> @@ -62,6 +64,34 @@ static struct pmbus_driver_info q54sj108a2_info[] = {
>  		.format[PSC_VOLTAGE_IN] = linear,
>  		.format[PSC_CURRENT_OUT] = linear,
>  
> +		.func[0] = PMBUS_HAVE_VIN |
> +		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,
> +	},
> +	[q54sn120a1] = {
> +		.pages = 1,
> +
> +		/* Source : Delta Q54SN120A1 */
> +		.format[PSC_TEMPERATURE] = linear,
> +		.format[PSC_VOLTAGE_IN] = linear,
> +		.format[PSC_CURRENT_OUT] = linear,
> +
> +		.func[0] = PMBUS_HAVE_VIN |
> +		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,
> +	},
> +	[q54sw120a7] = {
> +		.pages = 1,
> +
> +		/* Source : Delta Q54SW120A7 */
> +		.format[PSC_TEMPERATURE] = linear,
> +		.format[PSC_VOLTAGE_IN] = linear,
> +		.format[PSC_CURRENT_OUT] = linear,
> +

Unless I am missing something, those are all the same. That means there is
no need for separate entries.

>  		.func[0] = PMBUS_HAVE_VIN |
>  		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
>  		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> @@ -269,6 +299,8 @@ static const struct file_operations q54sj108a2_fops = {
>  
>  static const struct i2c_device_id q54sj108a2_id[] = {
>  	{ "q54sj108a2", q54sj108a2 },
> +	{ "q54sn120a1", q54sn120a1 },
> +	{ "q54sw120a7", q54sw120a7 },

Delta sells a variety of power bricks, but a Google search for q54sn120a1 or
q54sw120a7 comes up empty. Worse, searching for the entire series (q54sn or
q54sw) comes up empty as well.

Please provide information confirming that the referenced chips do exist.

>  	{ },
>  };
>  
> @@ -278,6 +310,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;
> @@ -314,8 +347,12 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  		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;
>  	}
> @@ -325,7 +362,7 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  		dev_err(dev, "Failed to read Manufacturer Revision\n");
>  		return ret;
>  	}
> -	if (ret != 4 || buf[0] != 'S') {
> +	if (buf[0] != 'S') {

This relaxes the revision number checks significantly. Please provide evidence
explaining why this is needed.

>  		buf[ret] = '\0';
>  		dev_err(dev, "Unsupported Manufacturer Revision '%s'\n", buf);
>  		return -ENODEV;
> @@ -402,6 +439,8 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  
>  static const struct of_device_id q54sj108a2_of_match[] = {
>  	{ .compatible = "delta,q54sj108a2", .data = (void *)q54sj108a2 },
> +	{ .compatible = "delta,q54sn120a1", .data = (void *)q54sn120a1 },
> +	{ .compatible = "delta,q54sw120a7", .data = (void *)q54sw120a7 },
>  	{ },
>  };
>  
> 
> -- 
> 2.34.1
>