Re: [PATCH v2 3/5] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Scaled `PMBUS_READ_VOUT` value can exceed the 16-bit capacity of the PMBus `LINEAR16` format, causing silent truncation and incorrect voltage reporting.
- [High] Missing software scaling for VOUT limit attributes and missing `.write_word_data` callback causes hardware limits to be programmed completely out of bounds.
--

commit 19e6041d100f0e80c9e49784d17900b141e3ea61
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
    
Implement support for external voltage divider scaling using the
adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties.

When the desired output voltage exceeds VREF, a resistor divider
(RFB1 and RFB2) is used to scale down the feedback voltage. The
driver reads these resistor values from device tree and applies
the scaling formula: VOUT_actual = VOUT_measured × (1 + RFB1/RFB2)

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index cb2c23672166d..5aad4448054fc 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> @@ -14,6 +15,45 @@
>  
>  #define MAX20830_IC_DEVICE_ID_LENGTH	9
>  
> +struct max20830_data {
> +	struct pmbus_driver_info info;
> +	u32 vout_rfb1;
> +	u32 vout_rfb2;
> +};
> +
> +/*
> + * Driver currently only supports READ_VOUT for monitoring that needs scaling to
> + * convert from feedback voltage to actual output voltage.
> + *
> + * vout_actual = vout_fb × (1 + RFB1/RFB2)
> + *
> + * VOUT_COMMAND and VOUT_MAX are currently not in use, but work with feedback
> + * voltage. If regulator support is added, VOUT_COMMAND values would need
> + * to be adjusted accordingly.
> + */
> +static int max20830_read_word_data(struct i2c_client *client, int page,
> +				   int phase, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	const struct max20830_data *data = container_of(info, struct max20830_data, info);
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_READ_VOUT:
> +		ret = pmbus_read_word_data(client, page, phase, reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Apply voltage divider scaling if resistors are non-zero */
> +		if (data->vout_rfb1 && data->vout_rfb2)
> +			ret = DIV_ROUND_CLOSEST_ULL((u64)ret * (data->vout_rfb1 +
> +						    data->vout_rfb2), data->vout_rfb2);
> +		return ret;

[Severity: High]
Does this returned value need bounds checking?

If a large resistor divider is used, the scaled PMBus word calculated here
can exceed 0xFFFF. The generic PMBus core function pmbus_reg2data_linear()
will cast this returned value to a u16, silently wrapping the value modulo
65536 and resulting in wildly incorrect telemetry (e.g., reporting 2V instead
of 12V).

Should this be clamped using clamp_val(ret, 0, 0xffff)?

> +	default:
> +		return -ENODATA;
> +	}
> +}

[Severity: High]
Will returning -ENODATA bypass software scaling for VOUT limit attributes?

The driver sets PMBUS_HAVE_VOUT below, which exposes hardware VOUT limits
(like inX_max or inX_lcrit). Because only PMBUS_READ_VOUT is intercepted
here, the core will read those limits unscaled from the hardware.

> +
>  static struct pmbus_driver_info max20830_info = {
>  	.pages = 1,
>  	.format[PSC_VOLTAGE_IN] = linear,
> @@ -24,13 +64,25 @@ static struct pmbus_driver_info max20830_info = {
>  		PMBUS_HAVE_TEMP |
>  		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
>  		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
> +	.read_word_data = max20830_read_word_data,

[Severity: High]
Does this struct also need a .write_word_data callback?

Since PMBUS_HAVE_VOUT is enabled, user writes to limit attributes will fall
through to the PMBus core, which writes the scaled PMBus word directly into
the hardware's limit register. Because the hardware expects limits in the
unscaled feedback voltage range, the written value will be completely out
of bounds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-dev-max20830c-v2-0-37761e89bb5f@analog.com?part=3