Hi Grant,
On Wed, Oct 23, 2024 at 3:58 AM Grant Peltier <grantpeltier93@xxxxxxxxx> wrote:
> Some applications require Vout to be higher than the detectable voltage
> range of the Vsense pin for a given rail. In such applications, a voltage
> divider may be placed between Vout and the Vsense pin, but this results
> in erroneous telemetry being read back from the part. This change adds
> support for a voltage divider to be defined in the devicetree for a (or
> multiple) specific rail(s) for a supported digital multiphase device and
> for the applicable Vout telemetry to be scaled based on the voltage
> divider configuration.
>
> Signed-off-by: Grant Peltier <grantpeltier93@xxxxxxxxx>
Thanks for your patch!
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -170,6 +185,25 @@ static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
> ret = pmbus_read_word_data(client, page, phase,
> RAA_DMPVR2_READ_VMON);
> break;
> + case PMBUS_READ_POUT:
> + /*
> + * In cases where a voltage divider is attached to the target
> + * rail between Vout and the Vsense pin, both Vout and Pout
> + * should be scaled by the voltage divider scaling factor.
> + * I.e. Vout = Vsense * (R1 + R2) / R2
> + */
> + fallthrough;
> + 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;
> break;
> @@ -178,6 +212,50 @@ static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
> return ret;
> }
>
> +static int raa_dmpvr2_write_word_data(struct i2c_client *client, int page,
> + int reg, u16 word)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + const struct isl68137_data *data = to_isl68137_data(info);
> + int ret;
> +
> + switch (reg) {
> + case PMBUS_VOUT_MAX:
> + /*
> + * In cases where a voltage divider is attached to the target
> + * rail between Vout and the Vsense pin, Vout related PMBus
> + * commands should be scaled based on the expected voltage
> + * at the Vsense pin.
> + * I.e. Vsense = Vout * R2 / (R1 + R2)
> + */
> + fallthrough;
> + case PMBUS_VOUT_MARGIN_HIGH:
> + fallthrough;
> + case PMBUS_VOUT_MARGIN_LOW:
> + fallthrough;
> + case PMBUS_VOUT_OV_FAULT_LIMIT:
> + fallthrough;
> + case PMBUS_VOUT_UV_FAULT_LIMIT:
> + fallthrough;
> + case PMBUS_VOUT_COMMAND:
> + if (data->channel[page].vout_voltage_divider[0]
> + && data->channel[page].vout_voltage_divider[1]) {
> + 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;
> +}
> +
> static struct pmbus_driver_info raa_dmpvr_info = {
> .pages = 3,
> .format[PSC_VOLTAGE_IN] = direct,
> @@ -220,14 +298,67 @@ static struct pmbus_driver_info raa_dmpvr_info = {
> | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
> };
>
> +static int isl68137_probe_child_from_dt(struct device *dev,
> + struct device_node *child,
> + struct isl68137_data *data)
> +{
> + u32 channel;
> + int err;
> +
> + err = of_property_read_u32(child, "reg", &channel);
> + if (err) {
> + dev_err(dev, "missing reg property of %pOFn\n", child);
> + return err;
> + }
> + if (channel >= MAX_CHANNELS) {
> + dev_err(dev, "invalid reg %d of %pOFn\n", channel, child);
> + return -EINVAL;
> + }
> +
> + 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?
> +
> + return 0;
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
