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

On 7/27/26 20:45, [email protected] wrote:
> 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.
> 

I would suggest to add a comment explaining that this series does not support
any limit registers. That should "address" the issues raised by Sashiko, and also
help future readers understand why the limit registers are not scaled.

Thanks,
Guenter

>> +
>>   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.
>