Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()

[Alexandru Ardelean <ardeleanalex@gmail.com>]

On Wed, Nov 11, 2020 at 4:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> > In order to account for any potential overflows that could occur.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Thinking about it, this can only really happen if the user provides
> excessive values for limit attributes. Those are currently clamped
> later, after the conversion. I think it would be better to modify
> the code to apply a clamp _before_ the conversion as well instead
> of trying to solve the overflow problem with unsigned long long.

Coming back to this, I think that using the shunt resistor value
(which is in micro-ohms), and multiplying with multiples of 1000, the
chances of overflow grow quite a lot.
I could be wrong, but that is how it looks when I try to do some math
with the shunt resistor in place.

Looking at the clamping code, it looks like the initial version is
quite simple & straightforward.
I mean when doing the math and getting values out of range, clamping
afterwards to 0xffffff for power values is quite handy.
And clamping everything else to 0xfff for voltage also looks pretty simple.
We can do some clamping before, but it looks like it's extra math that
is already done in the conversion code anyway.

Hopefully, I'm not missing something here :)

>
> Either case, can you send me a register dump for this chip ?
> I'd like to write a module test script to actually check if there
> are any over/underflows or other problems.
>
> Thanks,
> Guenter
>
> > ---
> >  drivers/hwmon/ltc2945.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 1cea710df668..6d4569a25471 100644
> > --- a/drivers/hwmon/ltc2945.c
> > +++ b/drivers/hwmon/ltc2945.c
> > @@ -155,7 +155,7 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> >  }
> >
> >  static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> > -                           unsigned long val)
> > +                           unsigned long long val)
> >  {
> >       struct ltc2945_state *st = dev_get_drvdata(dev);
> >       struct regmap *regmap = st->regmap;
> > @@ -181,14 +181,14 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >                       return ret;
> >               if (control & CONTROL_MULT_SELECT) {
> >                       /* 25 mV * 25 uV = 0.625 uV resolution. */
> > -                     val = DIV_ROUND_CLOSEST(val, 625);
> > +                     val = DIV_ROUND_CLOSEST_ULL(val, 625);
> >               } else {
> >                       /*
> >                        * 0.5 mV * 25 uV = 0.0125 uV resolution.
> >                        * Divide first to avoid overflow;
> >                        * accept loss of accuracy.
> >                        */
> > -                     val = DIV_ROUND_CLOSEST(val, 25) * 2;
> > +                     val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
> >               }
> >               break;
> >       case LTC2945_VIN_H:
> > @@ -197,7 +197,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >       case LTC2945_MAX_VIN_THRES_H:
> >       case LTC2945_MIN_VIN_THRES_H:
> >               /* 25 mV resolution. */
> > -             val /= 25;
> > +             val = div_u64(val, 25);
> >               break;
> >       case LTC2945_ADIN_H:
> >       case LTC2945_MAX_ADIN_H:
> > @@ -219,7 +219,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >                * dividing the reported current by the sense resistor value
> >                * in mOhm.
> >                */
> > -             val = DIV_ROUND_CLOSEST(val, 25);
> > +             val = DIV_ROUND_CLOSEST_ULL(val, 25);
> >               break;
> >       default:
> >               return -EINVAL;
> > @@ -247,7 +247,7 @@ static ssize_t ltc2945_value_store(struct device *dev,
> >       struct ltc2945_state *st = dev_get_drvdata(dev);
> >       struct regmap *regmap = st->regmap;
> >       u8 reg = attr->index;
> > -     unsigned long val;
> > +     unsigned long long val;
> >       u8 regbuf[3];
> >       int num_regs;
> >       int regval;
> >
>