Re: [PATCH v4 2/4] hwmon: (pmbus/tps25990): Rework TPS25990 direct conversion handling

[Guenter Roeck]

On 5/22/26 01:23, Stoyan Bogdanov wrote:
> Rework the existing implementation of direct format conversion for
> TPS25990 non-standard parameters to improve code reusability and
> integration with the PMBus direct conversion helpers.
>
> Changes include:
>   - Add an enum describing the supported parameters
>   - Add structure to hold m, b, R per-device coefficients
>   - Add data structures for pmbus_driver_info and local direct values
>   - Use the generic PMBus conversion helpers:
>       pmbus_reg2data_direct_calc()
>       pmbus_data2reg_direct_calc()
>   - Replace previously used defines with structured data
>
> This reduces duplicated conversion logic and makes handling of
> non-standard parameters more maintainable.
>
> Signed-off-by: Stoyan Bogdanov <sbogdanov@xxxxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/tps25990.c | 197 +++++++++++++++++++++------------
>   1 file changed, 127 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 05c6288ecafc..1e252844217b 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
> @@ -36,17 +36,63 @@
>   #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 chips {
> +	tps25990,
> +};
> +
> +enum tps25990_parameters {
> +	TPS25990_VIN_OVF = 0, /* VIN over volatage fault */
> +	TPS25990_IIN_OCF, /* IIN Over currect fault */
> +	TPS25990_DIRECT_VALUES_COUNT,
> +};
> +
> +struct tps25990_local_direct_value {
> +	int m[TPS25990_DIRECT_VALUES_COUNT]; /* mantissa */
> +	int b[TPS25990_DIRECT_VALUES_COUNT]; /* offset */
> +	int R[TPS25990_DIRECT_VALUES_COUNT]; /* exponent */
> +};
> +
> +struct tps25990_data {
> +	struct pmbus_driver_info info;
> +	struct tps25990_local_direct_value info_local;
> +};
> +
> +static s64 tps25990_reg2data_direct(struct i2c_client *client, int param, s32 raw)
> +{
> +	struct pmbus_driver_info *info = i2c_get_clientdata(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
> +	struct tps25990_local_direct_value *info_local = &data->info_local;
> +	s64 b, val;
> +	s32 m, R;
> +
> +	val = (s16)raw;
> +	m = info_local->m[param];
> +	b = info_local->b[param];
> +	R = info_local->R[param];
> +
> +	return pmbus_reg2data_direct_calc(val, b, m, R);
> +}
> +
> +static u16 tps25990_data2reg_direct(struct i2c_client *client, int param, s64 val)
> +{
> +	struct pmbus_driver_info *info = i2c_get_clientdata(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
> +	struct tps25990_local_direct_value *info_local = &data->info_local;
> +	s32 m, R;
> +	s64 b;
> +
> +	m = info_local->m[param];
> +	b = info_local->b[param];
> +	R = info_local->R[param];
> +
> +	return pmbus_data2reg_direct_calc(val, b, m, R);
> +}
> +
>   /*
>    * Arbitrary default Rimon value: 1kOhm
>    * This correspond to an overcurrent limit of 55A, close to the specified limit
> @@ -184,9 +230,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_reg2data_direct(client, TPS25990_VIN_OVF, ret);

The result is again evaluated by the PMBus core as direct register value,
not as calculated fault limit. I don't think this works as expected.
This isn't about converting a register value to a voltage, it is about
converting one direct format into another. Using (or trying to use)
reg2data_direct is at the very least misleading, if not completely wrong.

The m/b/R parameters below are the coefficients from the datasheet to convert
the register values into voltages. The PMBus core will take that value and
convert it again into a voltage, this time using the m/b/R coefficients
for input voltages. This can not be correct. To be technically correct,
the core would have to convert the register value to a voltage using one
set of m/b/R coefficients and then convert it back to direct format using
the other set of m/b/R coefficients. The current code does that by adjusting
the values directly. I don't think changing is really beneficial. If anything,
documenting it would be more helpful.

Thanks,
Guenter