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
