Re: [PATCH 4/6] 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] Applying a voltage scaling multiplier directly to the PMBus word data for VOUT will result in severe silent truncation by the PMBus core framework if the scaled mantissa exceeds 16 bits.
- [High] Voltage divider scaling is applied only to PMBUS_READ_VOUT, leaving other VOUT-related PMBus commands and fault limits unscaled and ignoring write operations.
--

commit 690eab506d6910945bf8fa7dc51390239ffa2898
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.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index cb3a39d747ede..a3abd24437e8e 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> @@ -15,6 +16,35 @@
>  
>  #define MAX20830_IC_DEVICE_ID_LENGTH	9
>  
> +struct max20830_data {
> +	struct pmbus_driver_info info;
> +	u32 vout_rfb1;
> +	u32 vout_rfb2;
> +};
> +
> +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:
[Severity: High]
Does this code apply voltage divider scaling only to PMBUS_READ_VOUT, leaving
other VOUT-related PMBus commands and fault limits unscaled?

If userspace reads limit registers (e.g., in1_max), the switch statement
misses them and returns raw values.

> +		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);
[Severity: High]
Can this scaling cause silent truncation?

When userspace reads the output voltage via sysfs, this code multiplies the
raw 16-bit register value by the resistor ratio. The PMBus core reads this
scaled word, but in pmbus_reg2data_linear(), it casts the value to a 16-bit
unsigned integer:

    mantissa = (u16)sensor->data;

This will silently truncate any value that exceeds 65535, corrupting the
voltage telemetry.

> +		return ret;
> +	default:
> +		return -ENODATA;
> +	}
> +}
> +
>  static struct pmbus_driver_info max20830_info = {
>  	.pages = 1,
>  	.format[PSC_VOLTAGE_IN] = linear,
> @@ -25,14 +55,26 @@ 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]
Should this also provide a write_word_data callback?

Without a write_word_data override, any user writes to voltage limits are
passed unscaled to the device. The writes go directly to the PMBus core
without inverse scaling, causing hardware protection limits (over/under
voltage) to be misconfigured.

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-dev-max20830c-v1-0-a02786bde470@analog.com?part=4