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

[Stoyan Bogdanov]

On Sun, Mar 8, 2026 at 7:27 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> 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.

Noted, will fix that in next revision.

> > +     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.
>

I understand that the whole calculation is fragile
Another approach will be to break it in to phases, but it will lost precision

// make sure R is positive
u32 r_pos = info_local->R[param] >= 0 ? info_local->R[param] :
-info_local->R[param];

// if (r_pos < 9) will overflow 32bit
u32 r_pow = int_pow(10, r_pos);

if (info_local->R[param] >= 0) {
    val = DIV_ROUND_CLOSEST_ULL(raw , r_pow); // result should fit in 32bit
    val -= info_local->b[param]; // fit 32bit
    val = DIV_ROUND_CLOSEST(val , info_local->m[param]); // fit 32bit
} else { // info_local->R[param] < 0
    // val = raw * r_pos; // most likely will overflow 32 bit with big numbers
    // raw * r_pow / m
    // assume r_pow is bigger number
    val = DIV_ROUND_CLOSEST(r_pow , info_local->m[param]); // fit 32bit
    val = raw * val;
    b_div = DIV_ROUND_CLOSEST(info_local->b[param],
info_local->m[param]); // fit 32bit
    val -= b_div;
}

> > +
> > +     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).
>

Similar approach could be used as in tps25990_raw_to_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.
>

I understand , will fix that in next revision

> > +
> > +     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.
>

I understand , will fix that in next revision


> > +     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 = {