Re: [PATCH 3/3] hwmon: (pmbus/max34440): Add support for MAX34452

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing unsupported register masking causes CML errors and ALERT pin assertions during probe for max34452.
- [Medium] Brittle hardware revision check assumes max34452 will never reach MAX34451's NA6 revision threshold, risking future hardware misconfiguration.
--

--- Patch [3]: [PATCH 3/3] hwmon: (pmbus/max34440): Add support for MAX34452 ---
commit 35474250b6c0fdacb08c3d70de3d117a0f87d0c2
Author: Carlos Jones Jr <carlosjr.jones@analog.com>

hwmon: (pmbus/max34440): Add support for MAX34452

This commit adds support for the Maxim MAX34452 PMBus 16-Channel V/I Monitor
and 12-Channel Sequencer/Marginer. The device is similar to the MAX34451 and
shares the same configuration functions.

> diff --git a/drivers/hwmon/pmbus/max34440.c b/drivers/hwmon/pmbus/max34440.c
> index 024109df26db5..2e57af09f4780 100644
> --- a/drivers/hwmon/pmbus/max34440.c
> +++ b/drivers/hwmon/pmbus/max34440.c

[ ... ]

> @@ -116,6 +117,10 @@ static int max34440_read_word_data(struct i2c_client *client, int page,
>  			return -ENXIO;
>  		ret = -ENODATA;
>  		break;

[Severity: High]
Does the preceding switch case block (for PMBUS_VIN_OV_FAULT_LIMIT,
PMBUS_MFR_MAX_TEMP_1, etc.) also need to exclude max34452, similar to how it
currently excludes max34451?

Because max34452 enables PMBUS_HAVE_TEMP and PMBUS_HAVE_IOUT, the PMBus core
will dynamically query limit registers during probe. Since max34452 isn't
excluded in that preceding check, it returns -ENODATA instead of -ENXIO. This
causes the core to perform physical I2C reads on unsupported registers, which
can trigger CML errors and assert the hardware ALERT pin on the max34452.

> +	case PMBUS_VOUT_OV_WARN_LIMIT:
> +		if (data->id == max34452)
> +			return -ENXIO;
> +		return -ENODATA;
>  	case PMBUS_VIRT_READ_VOUT_MIN:

[ ... ]

> @@ -340,6 +351,7 @@ static int max34451_set_supported_funcs(struct i2c_client *client,
>  	if (rv < 0)
>  		return rv;
>  
> +	/* MAX34452's latest MFR_REV is only at 0x0004, will skip this part */
>  	if (rv >= MAX34451ETNA6_MFR_REV) {
>  		max34451_na6 = true;
>  		data->info.format[PSC_VOLTAGE_IN] = direct;

[Severity: Medium]
Will this cause issues if a future max34452 silicon revision increments its
MFR_REVISION to 0x0012 (MAX34451ETNA6_MFR_REV) or higher?

Since the check relies solely on the revision number without explicitly
verifying if the device ID is max34451, a future max34452 revision could
mistakenly trigger this MAX34451-specific silicon workaround. This would
unintentionally enable VIN/IIN channels and alter IOUT scale factors.

Could we check the device ID explicitly here to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-max34451_fixes-v1-0-a941b27eaecb@analog.com?part=3