Re: [PATCH v2 2/2] hwmon: (pmbus/lm25066) add 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] Telemetry coefficient selection mismatch due to unhandled hardware bit swap for current limit configuration.
--

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

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

This patch adds support for the ti,current-limit devicetree property
to configure the current limit via the DEVICE_SETUP register. It dynamically
handles the Bit 4 mapping for High/Low current limits, which is swapped
for the lm25066 compared to other chips in the family.

> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index dd7275a67a0ab..c8e7aa7c3acd3 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-limit", &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;
> +			} else if (strcmp(cl_setting, "low") == 0) {
> +				if (data->id == lm25066)
> +					config_new &= ~LM25066_DEV_SETUP_CL;
> +				else
> +					config_new |= LM25066_DEV_SETUP_CL;

[Severity: High]
This isn't a bug introduced by this patch, but does the telemetry coefficient
selection further down in lm25066_probe() also need to account for this
hardware bit logic swap?

Later in lm25066_probe(), the coefficient selection unconditionally assumes
Bit 4 (LM25066_DEV_SETUP_CL) being set means the High limit
(coeff[PSC_CURRENT_IN_L]) for all chips:

	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 {
		info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].m;
		info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].b;
		...
	}

For chips like the LM5064, if they are configured for the High current limit,
Bit 4 will be 0 based on the DT parsing logic above.

Could this cause the driver to erroneously select the normal coefficients
(Low current limit) instead of the _L coefficients, potentially throwing the
reported current and power telemetry off by a factor of ~2x?

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