Re: [PATCH v3 1/3] hwmon: (pmbus/tps25990): Rework TPS25990 non standatd direct conversion

[Guenter Roeck]

On Tue, Feb 17, 2026 at 10:12:01AM +0200, Stoyan Bogdanov wrote:
> Rework existing implementation for calculation of direct
> format conversion for TPS25990. With this implamentation
> is leveraged code reusability for non standard parameters.
>  - Add enum for parameter
>  - Add m, b, R structure to hold value per device
>  - Add data structure to hold for pmbus_driver_info and
>    local_direct_values
>  - Conversion functions are implemented according to formula from
>    TPS25990 datasheet
>  - Remove previously used defines replace with structure
> 
> Signed-off-by: Stoyan Bogdanov <sbogdanov@xxxxxxxxxxxx>
> ---
>  drivers/hwmon/pmbus/tps25990.c | 115 +++++++++++++++++++++++++--------
>  1 file changed, 88 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index c13edd7e1abf..33f6367f797c 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
> @@ -36,17 +36,58 @@
>  #define  TPS25990_UNLOCKED		BIT(7)
>  
>  #define TPS25990_8B_SHIFT		2
> -#define TPS25990_VIN_OVF_NUM		525100
> -#define TPS25990_VIN_OVF_DIV		10163
> -#define TPS25990_VIN_OVF_OFF		155
> -#define TPS25990_IIN_OCF_NUM		953800
> -#define TPS25990_IIN_OCF_DIV		129278
> -#define TPS25990_IIN_OCF_OFF		157
>  
>  #define PK_MIN_AVG_RST_MASK		(PK_MIN_AVG_RST_PEAK | \
>  					 PK_MIN_AVG_RST_AVG  | \
>  					 PK_MIN_AVG_RST_MIN)
>  
> +enum tps25990_parameters {
> +	TPS25990_VIN_OVF = 0, /* VIN over volatage fault */
> +	TPS25990_IIN_OCF, /* IIN Over currect fault */
> +	TPS25590_DIRECT_VALUES_MAX, /* Max value ensure there enough space */
> +};
> +
> +struct local_direct_value {
> +	int m[TPS25590_DIRECT_VALUES_MAX]; /* mantissa for direct data format */
> +	int b[TPS25590_DIRECT_VALUES_MAX]; /* offset */
> +	int R[TPS25590_DIRECT_VALUES_MAX]; /* exponent */
> +};
> +
> +struct tps25990_data {
> +	struct pmbus_driver_info *info;
> +	struct local_direct_value *info_local;
> +};
> +
> +static int tps25990_raw_to_value(struct i2c_client *client, int param, int raw)
> +{
> +	struct tps25990_data *data = (struct tps25990_data *)of_device_get_match_data(&client->dev);

This returns the global data structure (which is corrupted on
multi-instance systems) instead of per-instance data.

> +	struct local_direct_value *info_local = data->info_local;
> +
> +	/* Formula : X = (Y / 10^R - b) / m */
> +	if (info_local->R[param] >= 0)
> +		raw /= int_pow(10, info_local->R[param]);
> +	else
> +		raw *= int_pow(10, -info_local->R[param]);

This will overflow if the exponent is -6 and raw is larger than ~2147.
This may not happen with current parameters, but it is brittle.

> +
> +	return DIV_ROUND_CLOSEST(raw - info_local->b[param], info_local->m[param]);
> +}
> +
> +static unsigned int tps25990_value_to_raw(struct i2c_client *client, int param, int val)
> +{
> +	struct tps25990_data *data = (struct tps25990_data *)of_device_get_match_data(&client->dev);
> +	struct local_direct_value *info_local = data->info_local;
> +
> +	/* Formula : Y = ( m * X + b) * 10^R */
> +	val = (long)val * info_local->m[param] + info_local->b[param];

This will overflow on 32-bit systems (where long is a 32-bit value).

> +
> +	if (info_local->R[param] >= 0)
> +		val *= int_pow(10, info_local->R[param]);
> +	else
> +		val = DIV_ROUND_CLOSEST(val, int_pow(10, -info_local->R[param]));
> +
> +	return val;
> +}
> +
>  /*
>   * Arbitrary default Rimon value: 1kOhm
>   * This correspond to an overcurrent limit of 55A, close to the specified limit
> @@ -184,9 +225,7 @@ static int tps25990_read_word_data(struct i2c_client *client,
>  		ret = pmbus_read_word_data(client, page, phase, reg);
>  		if (ret < 0)
>  			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> -					TPS25990_VIN_OVF_DIV);
> -		ret += TPS25990_VIN_OVF_OFF;
> +		ret = tps25990_raw_to_value(client, TPS25990_VIN_OVF, ret);
>  		break;
>  
>  	case PMBUS_IIN_OC_FAULT_LIMIT:
> @@ -198,9 +237,7 @@ static int tps25990_read_word_data(struct i2c_client *client,
>  		ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
>  		if (ret < 0)
>  			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
> -					TPS25990_IIN_OCF_DIV);
> -		ret += TPS25990_IIN_OCF_OFF;
> +		ret = tps25990_raw_to_value(client, TPS25990_IIN_OCF, ret);
>  		break;
>  
>  	case PMBUS_VIRT_SAMPLES:
> @@ -246,17 +283,13 @@ static int tps25990_write_word_data(struct i2c_client *client,
>  		break;
>  
>  	case PMBUS_VIN_OV_FAULT_LIMIT:
> -		value -= TPS25990_VIN_OVF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> -					  TPS25990_VIN_OVF_NUM);
> +		value = tps25990_value_to_raw(client, TPS25990_VIN_OVF, value);
>  		value = clamp_val(value, 0, 0xf);
>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		break;
>  
>  	case PMBUS_IIN_OC_FAULT_LIMIT:
> -		value -= TPS25990_IIN_OCF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> -					  TPS25990_IIN_OCF_NUM);
> +		value = tps25990_value_to_raw(client, TPS25990_IIN_OCF, value);
>  		value = clamp_val(value, 0, 0x3f);
>  		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
>  		break;
> @@ -337,7 +370,16 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>  };
>  #endif
>  
> -static const struct pmbus_driver_info tps25990_base_info = {
> +struct local_direct_value tps25590_local_info = {
> +	.m[TPS25990_VIN_OVF] = 10163,
> +	.b[TPS25990_VIN_OVF] = -30081,
> +	.R[TPS25990_VIN_OVF] = -4,
> +	.m[TPS25990_IIN_OCF] = 9538,
> +	.b[TPS25990_IIN_OCF] = 0,
> +	.R[TPS25990_IIN_OCF] = -6,
> +};
> +
> +static struct pmbus_driver_info tps25990_base_info = {
>  	.pages = 1,
>  	.format[PSC_VOLTAGE_IN] = direct,
>  	.m[PSC_VOLTAGE_IN] = 5251,
> @@ -386,14 +428,19 @@ static const struct pmbus_driver_info tps25990_base_info = {
>  #endif
>  };
>  
> +struct tps25990_data data_tps25990 = {
> +	.info = &tps25990_base_info,
> +	.info_local = &tps25590_local_info,
> +};
> +
>  static const struct i2c_device_id tps25990_i2c_id[] = {
> -	{ "tps25990" },
> +	{ .name = "tps25990", .driver_data = (kernel_ulong_t)&data_tps25990 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
>  
>  static const struct of_device_id tps25990_of_match[] = {
> -	{ .compatible = "ti,tps25990" },
> +	{ .compatible = "ti,tps25990", .data = &data_tps25990 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, tps25990_of_match);
> @@ -401,23 +448,37 @@ MODULE_DEVICE_TABLE(of, tps25990_of_match);
>  static int tps25990_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> -	struct pmbus_driver_info *info;
> +	struct tps25990_data *data;
>  	u32 rimon = TPS25990_DEFAULT_RIMON;
> +	struct pmbus_driver_info *info_get;
> +	struct local_direct_value *info_local_get;
>  	int ret;
>  
>  	ret = device_property_read_u32(dev, "ti,rimon-micro-ohms", &rimon);
>  	if (ret < 0 && ret != -EINVAL)
>  		return dev_err_probe(dev, ret, "failed to get rimon\n");
>  
> -	info = devm_kmemdup(dev, &tps25990_base_info, sizeof(*info), GFP_KERNEL);
> -	if (!info)
> +	data = (struct tps25990_data *)of_device_get_match_data(dev);
> +	if (!data)
> +		return -EOPNOTSUPP;

By calling of_device_get_match_data() which returns NULL on non-devicetree
systems, this patch breaks support for such systems.

> +
> +	info_get = data->info;
> +	/* Make copy of pmbus_info and replace it to preserve original values */
> +	data->info = devm_kmemdup(dev, info_get, sizeof(*info_get), GFP_KERNEL);

This overwrites information in the global data structure returned by
of_device_get_match_data(). When multiple instances of the device exist,
each subsequent probe overwrites the global pointer. If any device is
removed, the global pointer now points to freed memory, leading to a
Use-After-Free when other instances access it via of_device_get_match_data
or during their own removal/re-probe.

This also completely breaks multi-instance support.

> +	if (!data->info)
> +		return -ENOMEM;
> +
> +	info_local_get = data->info_local;
> +	/* Make copy of pmbus_info and replace it to preserve original values */
> +	data->info_local = devm_kmemdup(dev, info_local_get, sizeof(*info_local_get), GFP_KERNEL);
> +	if (!data->info_local)
>  		return -ENOMEM;
>  
>  	/* Adapt the current and power scale for each instance */
> -	tps25990_set_m(&info->m[PSC_CURRENT_IN], rimon);
> -	tps25990_set_m(&info->m[PSC_POWER], rimon);
> +	tps25990_set_m(&data->info->m[PSC_CURRENT_IN], rimon);
> +	tps25990_set_m(&data->info->m[PSC_POWER], rimon);
>  
> -	return pmbus_do_probe(client, info);
> +	return pmbus_do_probe(client, data->info);
>  }
>  
>  static struct i2c_driver tps25990_driver = {