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

On 7/29/26 18:14, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Returning -ENODATA does not mask unsupported PMBus limit registers, incorrectly exposing unscaled limit attributes if the hardware responds.
> --
> 
> --- Patch [2]: [PATCH v5 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support ---
> commit 852e7cf36da1bb5ec1c29eff09bb747755762b5b
> Author: Alexis Czezar Torreno <[email protected]>
> 
> hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
> 
> This commit adds support for external voltage divider scaling in the max20830
> driver. It reads resistor values from device tree properties and applies a
> scaling formula to VOUT readings to report the actual output voltage.
> 
>> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
>> --- a/drivers/hwmon/pmbus/max20830.c
>> +++ b/drivers/hwmon/pmbus/max20830.c
> [ ... ]
>> @@ -14,6 +15,56 @@
> [ ... ]
>> +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:
> [ ... ]
>> +		return ret;
>> +	default:
>> +		return -ENODATA;
> 
> [Severity: Medium]
> Does returning -ENODATA here actually mask the unsupported limit registers as
> intended?
> 
> In the PMBus core framework, returning -ENODATA from the driver's read
> callback is treated as a signal to fall back to a standard I2C read. It
> does not block the attribute.
> 
> If the hardware responds to the limit register reads over standard I2C, the
> PMBus core will still create the sysfs attributes. Because scaling is only
> applied to PMBUS_READ_VOUT in this patch, these exposed limit attributes
> (e.g., in1_max) would report raw, unscaled feedback voltages, while in1_input
> reports scaled actual voltages, creating an inconsistent interface.
> 
> Should this return -ENXIO instead, which explicitly tells the PMBus core that
> the register is unsupported and prevents the sysfs attributes from being
> created?
> That would require explicitly listing either all supported or all unsupported
attributes. Let's not do that unless it is really necessary.

Guenter