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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The PMBus coefficient selection logic uses swapped Bit 4 semantics for non-LM25066 chips, causing incorrect telemetry scaling.
- [High] Missing `R` exponent update when alternative current limit (`_L`) coefficients are selected.
--

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

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

Add support for the 'ti,current-range' devicetree property to configure
the current limit via the DEVICE_SETUP (0xD9) register, overriding the
physical CL pin setting.

> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index dd7275a67a0ab..53d274033d66f 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c

[ ... ]

> @@ -484,6 +485,42 @@ static int lm25066_probe(struct i2c_client *client)
>  
>  	data->id = (enum chips)(unsigned long)i2c_get_match_data(client);
>  
> +	if (data->id != lm25056) {
> +		int config_new = config;
> +		const char *cl_setting;
> +		int ret;
> +
> +		if (!of_property_read_string(client->dev.of_node,
> +					     "ti,current-range", &cl_setting)) {
> +			config_new |= LM25066_DEV_SETUP_CL_CFG;
> +			if (strcmp(cl_setting, "high") == 0) {
> +				if (data->id == lm25066)
> +					config_new |= LM25066_DEV_SETUP_CL;
> +				else
> +					config_new &= ~LM25066_DEV_SETUP_CL;

[Severity: High]
This is a pre-existing issue, but does this bit swapping logic expose a
mismatch in how the PMBus coefficients are selected later in lm25066_probe()?

While this patch correctly clears LM25066_DEV_SETUP_CL for the 'high' range
on non-LM25066 chips, the pre-existing coefficient logic unconditionally
checks Bit 4 without accounting for the chip ID:

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

For example, on an LM5066, requesting 'high' sets Bit 4 to 0. This causes
the logic to take the else branch and use the standard CURRENT_IN coefficients
(intended for the 'low' limit). Will this cause the hardware monitoring driver
to report current and power values that are off by roughly a factor of 2?

> +			} else if (strcmp(cl_setting, "low") == 0) {
> +				if (data->id == lm25066)
> +					config_new &= ~LM25066_DEV_SETUP_CL;
> +				else
> +					config_new |= LM25066_DEV_SETUP_CL;
> +			} else {
> +				dev_err(&client->dev,
> +					"invalid current-range setting: %s\n",
> +					cl_setting);
> +				return -EINVAL;
> +			}
> +		}
> +
> +		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 isn't a bug introduced by this patch, but while reviewing the coefficient
selection logic in lm25066_probe(), is there a missing update for the R
exponent?

When the alternative current limit (_L) coefficients are selected, the m and b
values are updated but R is not:

    info->R[PSC_POWER] = coeff[PSC_POWER].R;
    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;
        info->m[PSC_POWER] = coeff[PSC_POWER_L].m;
        info->b[PSC_POWER] = coeff[PSC_POWER_L].b;
    } else {
        ...

For the lm25056 chip, the POWER_L coefficient array entry has R = -4, while
the standard POWER coefficient has R = -3. Because info->R[PSC_POWER] is
only initialized from the base coefficient, could power readings be
miscalculated by a factor of 10 when the alternative limit is active?

>  	info = &data->info;
>  
>  	info->pages = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-lm25066-cl-config-v3-0-decb4f5b0b77@gmail.com?part=2