Re: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete VOUT scaling introduces UAPI inconsistency and dangerous hardware limit writes.
--

--- Patch [2]: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support ---
commit 4d1fa3c4cb8d1f4bfe3ca16a79ad028a0dd9bfee
Author: Alexis Czezar Torreno <[email protected]>

hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

This commit adds support for external voltage divider scaling using the
adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties. It reads
these properties and scales the feedback voltage for PMBUS_READ_VOUT.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index cb2c23672166d..7c1e94c43ac4e 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c
[ ... ]
> @@ -14,6 +15,51 @@
[ ... ]
> +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;
> +	u64 temp;
> +
> +	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) {
> +			temp = (u64)data->vout_rfb1 + (u64)data->vout_rfb2;
> +			temp = DIV_ROUND_CLOSEST_ULL((u64)ret * temp, data->vout_rfb2);
> +			ret = clamp_val(temp, 0, 0xFFFF);
> +		}
> +		return ret;
> +	default:
> +		return -ENODATA;
> +	}
> +}

[Severity: High]
Does this code introduce a UAPI inconsistency by only scaling PMBUS_READ_VOUT?

When in1_input is read, it returns the scaled output voltage.

However, because this switch returns -ENODATA for limits (like
PMBUS_VOUT_OV_WARN_LIMIT or PMBUS_VOUT_OV_FAULT_LIMIT), reading limits like
in1_max will return the unscaled feedback voltage directly from the chip.

> +
>  static struct pmbus_driver_info max20830_info = {
>  	.pages = 1,
>  	.format[PSC_VOLTAGE_IN] = linear,
> @@ -24,13 +70,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]
Should there be a matching write_word_data callback here to reverse the
scaling for limit registers?

Since there is no write_word_data callback, if a user writes a new limit using
the scaled unit, the core will write it directly to the hardware without
reverse-scaling.

The chip treats this as a feedback limit, which could drastically raise the
physical overvoltage trip point and bypass hardware protection.

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