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

[sashiko-bot@kernel.org]

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