Re: [PATCH 2/2] hwmon: (pmbus/lm25066) add SMBus current limit configuration support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver incorrectly scales current and power telemetry for LM5064/LM5066/LM5066i chips by a factor of 2 because it fails to account for a swapped hardware bit mapping when selecting PMBus coefficients.
--

commit c83de5ba055b8d019bcdbf2292f57ea088b2924b
Author: Potin Lai <potin.lai.pt@gmail.com>

hwmon: (pmbus/lm25066) add SMBus current limit configuration support

This commit adds support for the 'ti,cl-smbus-high' and 'ti,cl-smbus-low'
devicetree properties to configure the current limit via SMBus. It sets
the Current Limit Configuration and Current Limit Setting bits, accounting
for swapped bit mapping on certain chips.

> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index dd7275a67a0ab..20e114bdc882d 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c
> @@ -464,6 +465,8 @@ MODULE_DEVICE_TABLE(of, lm25066_of_match);
>  static int lm25066_probe(struct i2c_client *client)
>  {
>  	int config;
> +	int config_new;
> +	int ret;
>  	u32 shunt;
>  	struct lm25066_data *data;
>  	struct pmbus_driver_info *info;

[ ... ]

> @@ -484,6 +487,28 @@ static int lm25066_probe(struct i2c_client *client)
>  
>  	data->id = (enum chips)(unsigned long)i2c_get_match_data(client);
>  
> +	config_new = config;
> +	if (of_property_read_bool(client->dev.of_node, "ti,cl-smbus-high")) {
> +		config_new |= LM25066_DEV_SETUP_CL_CFG;
> +		if (data->id == lm25056 || data->id == lm25066)
> +			config_new |= LM25066_DEV_SETUP_CL;
> +		else
> +			config_new &= ~LM25066_DEV_SETUP_CL;
> +	} else if (of_property_read_bool(client->dev.of_node, "ti,cl-smbus-low")) {
> +		config_new |= LM25066_DEV_SETUP_CL_CFG;
> +		if (data->id == lm25056 || data->id == lm25066)
> +			config_new &= ~LM25066_DEV_SETUP_CL;
> +		else
> +			config_new |= LM25066_DEV_SETUP_CL;
> +	}
> +
> +	if (config_new != config) {
> +		ret = i2c_smbus_write_byte_data(client, LM25066_DEVICE_SETUP, config_new);
> +		if (ret < 0)
> +			return ret;
> +		config = config_new;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this patch leave the new explicit
device tree configuration feature broken for some chips?

The new code correctly identifies that the DEVICE_SETUP Current Limit bit
(Bit 4) mapping is swapped between the LM25056/LM25066 chips (1 = High
limit) and LM5064/LM5066/LM5066i chips (0 = High limit).

However, further down in lm25066_probe(), the coefficient selection logic
fails to account for this chip-specific swap:

    if (config & LM25066_DEV_SETUP_CL) {
        info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].m;
        info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].b;
        ...
    } else {
        info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].m;
        info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].b;
        ...
    }

When an LM5066 is configured to the high limit via ti,cl-smbus-high, the
driver clears Bit 4. The coefficient logic then sees Bit 4 is 0 and
incorrectly selects the PSC_CURRENT_IN (Low limit) coefficients.

Could this cause the calculated current and power telemetry for the LM5064,
LM5066, and LM5066i chips to be under-reported by roughly 2x?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-lm25066-cl-config-v1-0-02e567bf3d91@gmail.com?part=2