Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout

[Grant Peltier <grantpeltier93@gmail.com>]

Hi Geert,

On Wed, Oct 23, 2024 at 09:34:36AM +0200, Geert Uytterhoeven wrote:
> > [...]
> > +       case PMBUS_READ_VOUT:
> > +               ret = pmbus_read_word_data(client, page, phase, reg);
> > +               if (ret > 0 && data->channel[page].vout_voltage_divider[0]
> > +                       && data->channel[page].vout_voltage_divider[1]) {
> > +                       u64 temp = DIV_ROUND_CLOSEST_ULL((u64)ret *
> > +                               (data->channel[page].vout_voltage_divider[0]
> > +                               + data->channel[page].vout_voltage_divider[1]),
> > +                               data->channel[page].vout_voltage_divider[1]);
> 
> You are casting "ret" to u64 to force a 64-bit multiplication, as the
> product may not fit in 32 bits. However, DIV_ROUND_CLOSEST_ULL()
> does a 32-bit division on 32-bit platforms.  So this should use
> DIV_U64_ROUND_CLOSEST() instead.
> The sum of vout_voltage_divider[0] + vout_voltage_divider[1] might
> not fit in 32 bits, so that should be changed to a 64-bit addition.
> Unfortunately there is no rounding version of mul_u64_u32_div() yet,
> so you have to open-code it.
> 
> > +                       ret = clamp_val(temp, 0, 0xffff);
> > +               }
> > +               break;
> >         default:
> >                 ret = -ENODATA;
> > [...]
> > +                       u64 temp = DIV_ROUND_CLOSEST_ULL((u64)word *
> > +                               data->channel[page].vout_voltage_divider[1],
> > +                               (data->channel[page].vout_voltage_divider[0] +
> > +                                data->channel[page].vout_voltage_divider[1]));
> 
> Similar comments, but here the sum is the divisor, so you have to use
> a full 64-by-64 division, using DIV64_U64_ROUND_CLOSEST().
> 
> > +                       ret = clamp_val(temp, 0, 0xffff);
> > +               } else {
> > +                       ret = -ENODATA;
> > +               }
> > +               break;
> > +       default:
> > +               ret = -ENODATA;
> > +               break;
> > +       }
> > +       return ret;
> > +}
> > [...]
> > +
> > +       of_property_read_u32_array(child, "renesas,vout-voltage-divider",
> > +                               data->channel[channel].vout_voltage_divider,
> > +                               ARRAY_SIZE(data->channel[channel].vout_voltage_divider));
> 
> Shouldn't the return value be checked for errors different from -EINVAL?
> 

Thank you for your review! I will make the requested changes and submit a
new version.

Best regards,
Grant