Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Race condition: sysfs registration before hardware initialization
- [Medium] Incorrect error code for out-of-range values
- [Medium] hwmon update_interval attribute violates ABI units (returns raw values instead of milliseconds)
- [Medium] Architectural layering violation: PMIC directly registered as hwmon I2C driver
- [Low] C11 syntax error due to declaration directly after a label
--

--- Patch [1]: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs ---
commit 4dd6cacf5a9ff70a4083c91afc8539b2aae42832
Author: Stephen Horvath <[email protected]>

hwmon: Add Support for PMIC5000 Power Management ICs

Add support for sensors present on PMIC5000 (JEDEC JESD301) compliant
power management ICs. These chips are commonly found on DDR5 memory
modules.

> diff --git a/drivers/hwmon/pmic5000.c b/drivers/hwmon/pmic5000.c
> index 0000000000000..d3f3126dd9c80
> --- /dev/null
> +++ b/drivers/hwmon/pmic5000.c

[ ... ]

> +static int pmic5000_read_temp(struct regmap *regmap, u32 attr, int channel,
> +			      long *val)
> +{
> +	int err;
> +	u32 regval;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_input: {
> +		err = regmap_read(regmap, PMIC5000_REG_TEMPERATURE, &regval);
> +		if (err)
> +			return err;
> +		regval >>= 5;
> +		/* Below 85°C */
> +		if (regval == 0)
> +			return -EOPNOTSUPP;

[Severity: Medium]
Since returning -EOPNOTSUPP causes userspace tools to treat the attribute 
as permanently unsupported by the chip, should this return -ENODATA instead 
when the temperature is temporarily out of range?

> +		/* 0b001 = 85°C, 0b010 = 95°C, etc. */
> +		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}

[ ... ]

> +static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr,
> +					 int channel, long *val)
> +{

[ ... ]

> +	volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0];
> +	volt_set += (set_regval >> 1) * 5;
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		/* 10%, 12.5%, Reserved, Reserved */
> +		const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE };

[Severity: Low]
Can declaring a variable immediately following a case label cause a build 
failure? The C11 standard expects a statement after a label, rather than 
a declaration. Would wrapping the contents of this case block in braces 
resolve this syntax error?

> +		*val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	case hwmon_in_max:
> +		/* 7.5%, 10%, 12.5%, Reserved */
> +		const int max_permilles[4] = { 75, 100, 125, PERMILLE };
> +		*val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[ ... ]

> +static int pmic5000_read_interval(struct regmap *regmap, u32 attr, long *val)
> +{
> +	unsigned int regval;
> +	int err;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = 1 << (regval & 0x03);
> +	return 0;
> +}

[Severity: Medium]
Is this exposing raw values like 1, 2, 4, or 8 instead of milliseconds? 
The hwmon ABI specifies that update_interval should be reported and set 
in milliseconds. Do these values need to be translated to match the ABI 
requirements?

[ ... ]

> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)
> +{
> +	unsigned int revision, vendor, bank;
> +	struct pmic5000_data *data;
> +	struct device *hwmon_dev;
> +	int err;

[ ... ]

> +	hwmon_dev = devm_hwmon_device_register_with_info(
> +		dev, "pmic5000", data, &pmic5000_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "DDR5 PMIC sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
> +		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1,
> +		 ((revision >> 1) & 0x07) + 1);
> +
> +	/* Enable individual measurements and enable ADC */
> +	err = regmap_update_bits(regmap, PMIC5000_REG_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT);
> +	if (err)
> +		return err;
> +	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
> +				 PMIC5000_ADC_ENABLE, PMIC5000_ADC_ENABLE);

[Severity: Medium]
Does this expose the sysfs files before the ADC is actually enabled? If 
userspace polling tools access the sysfs attributes right after 
devm_hwmon_device_register_with_info() completes, could it trigger reads on 
uninitialized hardware? Should the hardware initialization happen before 
registration?

[ ... ]

> +static struct i2c_driver pmic5000_i2c_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "pmic5000",
> +		.of_match_table = pmic5000_of_ids,
> +		.pm = pm_sleep_ptr(&pmic5000_pm_ops),
> +	},
> +	.probe		= pmic5000_i2c_probe,
> +	.id_table	= pmic5000_i2c_id,
> +};
> +
> +module_i2c_driver(pmic5000_i2c_driver);

[Severity: Medium]
Is this driver placed in the correct subsystem? For PMIC chips where hardware 
monitoring isn't the primary function, the hwmon subsystem guidelines state 
the core driver should reside in drivers/mfd or drivers/regulator, and 
instantiate the hwmon functionality as an auxiliary device. Does binding 
directly to the I2C device here prevent regulator drivers from attaching to 
this PMIC later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1