Re: [PATCH] hwmon: (max6639) Publish the OF module alias

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing i2c_set_clientdata() in probe causes a NULL pointer dereference in PM suspend/resume callbacks.
- [High] Unbalanced regulator disable if I2C access fails during suspend.
--

commit e5868671fda98c3e539843bf646808d132b90df7
Author: hpp.iscas <hppiscas@163.com>

hwmon: (max6639) Publish the OF module alias

This patch publishes the max6639_of_match table as the OF device ID
module table. This ensures the correct OF modalias is emitted by the I2C
core for Device Tree clients, complementing the legacy I2C detection.

> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> index dd5f4b3b128da..bf9236151be19 100644
> --- a/drivers/hwmon/max6639.c
> +++ b/drivers/hwmon/max6639.c
> @@ -790,6 +790,7 @@ static const struct of_device_id max6639_of_match[] = {
>  	{ .compatible = "maxim,max6639", },
>  	{ },
>  };
> +MODULE_DEVICE_TABLE(of, max6639_of_match);
>  
>  static struct i2c_driver max6639_driver = {
>  	.class = I2C_CLASS_HWMON,

[Severity: Critical]
This is a pre-existing issue, but does max6639_probe() forget to set the
driver data for the parent I2C device?

max6639_probe() registers the hwmon device, which binds the driver data to
the new hwmon device:

	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
							 data, &max6639_chip_info,
							 NULL);

However, there is no corresponding call to i2c_set_clientdata() or
dev_set_drvdata() for the parent I2C device.

When the PM core calls max6639_suspend() or max6639_resume(), it passes the
parent I2C device:

static int max6639_suspend(struct device *dev)
{
	struct max6639_data *data = dev_get_drvdata(dev);

	if (data->reg)

Won't dev_get_drvdata(dev) return NULL here, causing an immediate NULL
pointer dereference upon system suspend or resume?

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an
unbalanced regulator disable if the I2C access fails during suspend?

In max6639_suspend():

	if (data->reg)
		regulator_disable(data->reg);

	return regmap_write_bits(data->regmap, MAX6639_REG_GCONFIG,
				 MAX6639_GCONFIG_STANDBY,
				 MAX6639_GCONFIG_STANDBY);

If regmap_write_bits() fails, the error is returned directly but the
regulator remains disabled.

The PM core will abort the suspend transition for this device and it will
conceptually remain active, potentially leading to overheating as the fan
is off. Later, when the driver is unbound, devres will run
max6639_regulator_disable(), which could result in a negative enable count
in the regulator core.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905133643.65365-1-hppiscas@163.com?part=1