| Message ID | 422a40e992e047e250a3b1295503e3b81b5515ae.1729646466.git.grantpeltier93@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dt-bindings: hwmon: pmbus: add bindings for isl68137 | expand |
Hi Grant, On Wed, Oct 23, 2024 at 3:58 AM Grant Peltier <grantpeltier93@gmail.com> 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@gmail.com> 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
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
On 10/22/24 18:58, Grant Peltier 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@gmail.com> > --- > drivers/hwmon/pmbus/isl68137.c | 199 ++++++++++++++++++++++++++++++++- > 1 file changed, 194 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c > index 7e53fb1d5ea3..b4f581e1d560 100644 > --- a/drivers/hwmon/pmbus/isl68137.c > +++ b/drivers/hwmon/pmbus/isl68137.c > @@ -13,6 +13,7 @@ > #include <linux/init.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/string.h> > #include <linux/sysfs.h> > > @@ -20,6 +21,7 @@ > > #define ISL68137_VOUT_AVS 0x30 > #define RAA_DMPVR2_READ_VMON 0xc8 > +#define MAX_CHANNELS 4 > > enum chips { > isl68137, > @@ -72,6 +74,17 @@ enum variants { > raa_dmpvr2_hv, > }; > > +struct isl68137_channel { > + u32 vout_voltage_divider[2]; > +}; > + > +struct isl68137_data { > + struct pmbus_driver_info info; > + struct isl68137_channel channel[MAX_CHANNELS]; > +}; > + > +#define to_isl68137_data(x) container_of(x, struct isl68137_data, info) > + > static const struct i2c_device_id raa_dmpvr_id[]; > > static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client, > @@ -163,6 +176,8 @@ static const struct attribute_group *isl68137_attribute_groups[] = { > static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page, > int phase, int reg) > { > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + const struct isl68137_data *data = to_isl68137_data(info); > int ret; > > switch (reg) { > @@ -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]); > + 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; Just add the comment after the last case and drop all the fallthrough; Same above. > + case PMBUS_VOUT_COMMAND: > + if (data->channel[page].vout_voltage_divider[0] > + && data->channel[page].vout_voltage_divider[1]) { It would be better to set defaults instead of having to check this for every executed command (for example by setting R1:=0 and R2:=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])); > + 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) { The actual number of channels (pages) supported by the chip is known here and should be checked, either by passing the number of channels or a pointer to the entire info structure to this function. > + dev_err(dev, "invalid reg %d of %pOFn\n", channel, child); > + return -EINVAL; > + } > + > + of_property_read_u32_array(child, "renesas,vout-voltage-divider", Ultimately this potentially applies to _all_ hardware monitoring chips, so I would very much prefer a generic voltage divider property definition. > + data->channel[channel].vout_voltage_divider, > + ARRAY_SIZE(data->channel[channel].vout_voltage_divider)); The returned data should be be validated here. > + > + return 0; > +} > + > +static int isl68137_probe_from_dt(struct device *dev, > + struct isl68137_data *data) > +{ > + const struct device_node *np = dev->of_node; > + struct device_node *child; > + int err; > + > + for_each_child_of_node(np, child) { > + if (strcmp(child->name, "channel")) > + continue; > + > + err = isl68137_probe_child_from_dt(dev, child, data); > + if (err) > + return err; > + } > + > + return 0; > +} > + > static int isl68137_probe(struct i2c_client *client) > { > + struct device *dev = &client->dev; > struct pmbus_driver_info *info; > + struct isl68137_data *data; > + int i, err; > > - info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL); > - if (!info) > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > return -ENOMEM; > - memcpy(info, &raa_dmpvr_info, sizeof(*info)); > + > + for (i = 0; i < MAX_CHANNELS; i++) > + memset(data->channel[i].vout_voltage_divider, > + 0, > + sizeof(data->channel[i].vout_voltage_divider)); Under what circumstance would this not already be 0 after devm_kzalloc() ? > + > + memcpy(&data->info, &raa_dmpvr_info, sizeof(data->info)); > + info = &data->info; > > switch (i2c_match_id(raa_dmpvr_id, client)->driver_data) { > case raa_dmpvr1_2rail: > @@ -242,6 +373,7 @@ static int isl68137_probe(struct i2c_client *client) > case raa_dmpvr2_1rail: > info->pages = 1; > info->read_word_data = raa_dmpvr2_read_word_data; > + info->write_word_data = raa_dmpvr2_write_word_data; > break; > case raa_dmpvr2_2rail_nontc: > info->func[0] &= ~PMBUS_HAVE_TEMP3; > @@ -250,9 +382,11 @@ static int isl68137_probe(struct i2c_client *client) > case raa_dmpvr2_2rail: > info->pages = 2; > info->read_word_data = raa_dmpvr2_read_word_data; > + info->write_word_data = raa_dmpvr2_write_word_data; > break; > case raa_dmpvr2_3rail: > info->read_word_data = raa_dmpvr2_read_word_data; > + info->write_word_data = raa_dmpvr2_write_word_data; > break; > case raa_dmpvr2_hv: > info->pages = 1; > @@ -263,11 +397,18 @@ static int isl68137_probe(struct i2c_client *client) > info->m[PSC_POWER] = 2; > info->R[PSC_POWER] = -1; > info->read_word_data = raa_dmpvr2_read_word_data; > + info->write_word_data = raa_dmpvr2_write_word_data; > break; > default: > return -ENODEV; > } > > + if (dev->of_node) { This conditional should not be necessary because for_each_child_of_node() ultimately calls __of_get_next_child() which checks if the node pointer is NULL. > + err = isl68137_probe_from_dt(dev, data); > + if (err) > + return err; > + } > + > return pmbus_do_probe(client, info); > } > > @@ -318,11 +459,59 @@ static const struct i2c_device_id raa_dmpvr_id[] = { > > MODULE_DEVICE_TABLE(i2c, raa_dmpvr_id); > > +static const struct of_device_id isl68137_of_match[] = { > + { .compatible = "renesas,isl68137", .data = (void *)raa_dmpvr1_2rail }, > + { .compatible = "renesas,isl68220", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl68221", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl68222", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl68223", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl68224", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl68225", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl68226", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl68227", .data = (void *)raa_dmpvr2_1rail }, > + { .compatible = "renesas,isl68229", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl68233", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl68239", .data = (void *)raa_dmpvr2_3rail }, > + > + { .compatible = "renesas,isl69222", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69223", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl69224", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69225", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69227", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl69228", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl69234", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69236", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69239", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl69242", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69243", .data = (void *)raa_dmpvr2_1rail }, > + { .compatible = "renesas,isl69247", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69248", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69254", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69255", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69256", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69259", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69260", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69268", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,isl69269", .data = (void *)raa_dmpvr2_3rail }, > + { .compatible = "renesas,isl69298", .data = (void *)raa_dmpvr2_2rail }, > + > + { .compatible = "renesas,raa228000", .data = (void *)raa_dmpvr2_hv }, > + { .compatible = "renesas,raa228004", .data = (void *)raa_dmpvr2_hv }, > + { .compatible = "renesas,raa228006", .data = (void *)raa_dmpvr2_hv }, > + { .compatible = "renesas,raa228228", .data = (void *)raa_dmpvr2_2rail_nontc }, > + { .compatible = "renesas,raa229001", .data = (void *)raa_dmpvr2_2rail }, > + { .compatible = "renesas,raa229004", .data = (void *)raa_dmpvr2_2rail }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, isl68137_of_match); > + > /* This is the driver that will be inserted */ > static struct i2c_driver isl68137_driver = { > .driver = { > - .name = "isl68137", > - }, > + .name = "isl68137", > + .of_match_table = isl68137_of_match, > + }, > .probe = isl68137_probe, > .id_table = raa_dmpvr_id, > };
Hi Guenter, Thank you for the review! On Thu, Oct 24, 2024 at 10:48:16AM -0700, Guenter Roeck wrote: > On 10/22/24 18:58, Grant Peltier wrote: > > + [...] > > + 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; > > Just add the comment after the last case and drop all the fallthrough; > Same above. > Will fix in v4 > > + case PMBUS_VOUT_COMMAND: > > + if (data->channel[page].vout_voltage_divider[0] > > + && data->channel[page].vout_voltage_divider[1]) { > > It would be better to set defaults instead of having to check this > for every executed command (for example by setting R1:=0 and R2:=1). > Sounds reasonable. I will adjust the channel initialization process to set defaults instead and will remove the checks in v4. > > [...] > > +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) { > > The actual number of channels (pages) supported by the chip is known here > and should be checked, either by passing the number of channels or a pointer > to the entire info structure to this function. > Will fix in v4. > > + dev_err(dev, "invalid reg %d of %pOFn\n", channel, child); > > + return -EINVAL; > > + } > > + > > + of_property_read_u32_array(child, "renesas,vout-voltage-divider", > > Ultimately this potentially applies to _all_ hardware monitoring chips, > so I would very much prefer a generic voltage divider property definition. > There is a parallel conversation on PATCH v3 2/2 about this. Would you prefer that I match the implementation for maxim20730? > > + data->channel[channel].vout_voltage_divider, > > + ARRAY_SIZE(data->channel[channel].vout_voltage_divider)); > > The returned data should be be validated here. > Fixed in v3. > > + > > + return 0; > > +} > > + > > +static int isl68137_probe_from_dt(struct device *dev, > > + struct isl68137_data *data) > > +{ > > + const struct device_node *np = dev->of_node; > > + struct device_node *child; > > + int err; > > + > > + for_each_child_of_node(np, child) { > > + if (strcmp(child->name, "channel")) > > + continue; > > + > > + err = isl68137_probe_child_from_dt(dev, child, data); > > + if (err) > > + return err; > > + } > > + > > + return 0; > > +} > > + > > static int isl68137_probe(struct i2c_client *client) > > { > > + struct device *dev = &client->dev; > > struct pmbus_driver_info *info; > > + struct isl68137_data *data; > > + int i, err; > > - info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL); > > - if (!info) > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > return -ENOMEM; > > - memcpy(info, &raa_dmpvr_info, sizeof(*info)); > > + > > + for (i = 0; i < MAX_CHANNELS; i++) > > + memset(data->channel[i].vout_voltage_divider, > > + 0, > > + sizeof(data->channel[i].vout_voltage_divider)); > > Under what circumstance would this not already be 0 after devm_kzalloc() ? > Mental lapse on my end. Will change to set harmless defaults discussed above. > > + [...] > > + if (dev->of_node) { > This conditional should not be necessary because for_each_child_of_node() > ultimately calls __of_get_next_child() which checks if the node pointer > is NULL. > Will remove in v4. > > + err = isl68137_probe_from_dt(dev, data); > > + if (err) > > + return err; > > + [...] Thanks again, Grant
On 10/24/24 12:43, Grant Peltier wrote: [ ... ] >>> + of_property_read_u32_array(child, "renesas,vout-voltage-divider", >> >> Ultimately this potentially applies to _all_ hardware monitoring chips, >> so I would very much prefer a generic voltage divider property definition. >> > > There is a parallel conversation on PATCH v3 2/2 about this. Would you > prefer that I match the implementation for maxim20730? > I would prefer, in the order of preference, 1) an applicable generic property definition 2) a definition that is already used elsewhere 3) a new chips specific definition From my perspective, matching the maxim20730 implementation should only be considered if the generic definition does not meet the chip requirements. Thanks, Guenter
diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c index 7e53fb1d5ea3..b4f581e1d560 100644 --- a/drivers/hwmon/pmbus/isl68137.c +++ b/drivers/hwmon/pmbus/isl68137.c @@ -13,6 +13,7 @@ #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/string.h> #include <linux/sysfs.h> @@ -20,6 +21,7 @@ #define ISL68137_VOUT_AVS 0x30 #define RAA_DMPVR2_READ_VMON 0xc8 +#define MAX_CHANNELS 4 enum chips { isl68137, @@ -72,6 +74,17 @@ enum variants { raa_dmpvr2_hv, }; +struct isl68137_channel { + u32 vout_voltage_divider[2]; +}; + +struct isl68137_data { + struct pmbus_driver_info info; + struct isl68137_channel channel[MAX_CHANNELS]; +}; + +#define to_isl68137_data(x) container_of(x, struct isl68137_data, info) + static const struct i2c_device_id raa_dmpvr_id[]; static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client, @@ -163,6 +176,8 @@ static const struct attribute_group *isl68137_attribute_groups[] = { static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page, int phase, int reg) { + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); + const struct isl68137_data *data = to_isl68137_data(info); int ret; switch (reg) { @@ -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]); + 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])); + 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)); + + return 0; +} + +static int isl68137_probe_from_dt(struct device *dev, + struct isl68137_data *data) +{ + const struct device_node *np = dev->of_node; + struct device_node *child; + int err; + + for_each_child_of_node(np, child) { + if (strcmp(child->name, "channel")) + continue; + + err = isl68137_probe_child_from_dt(dev, child, data); + if (err) + return err; + } + + return 0; +} + static int isl68137_probe(struct i2c_client *client) { + struct device *dev = &client->dev; struct pmbus_driver_info *info; + struct isl68137_data *data; + int i, err; - info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL); - if (!info) + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) return -ENOMEM; - memcpy(info, &raa_dmpvr_info, sizeof(*info)); + + for (i = 0; i < MAX_CHANNELS; i++) + memset(data->channel[i].vout_voltage_divider, + 0, + sizeof(data->channel[i].vout_voltage_divider)); + + memcpy(&data->info, &raa_dmpvr_info, sizeof(data->info)); + info = &data->info; switch (i2c_match_id(raa_dmpvr_id, client)->driver_data) { case raa_dmpvr1_2rail: @@ -242,6 +373,7 @@ static int isl68137_probe(struct i2c_client *client) case raa_dmpvr2_1rail: info->pages = 1; info->read_word_data = raa_dmpvr2_read_word_data; + info->write_word_data = raa_dmpvr2_write_word_data; break; case raa_dmpvr2_2rail_nontc: info->func[0] &= ~PMBUS_HAVE_TEMP3; @@ -250,9 +382,11 @@ static int isl68137_probe(struct i2c_client *client) case raa_dmpvr2_2rail: info->pages = 2; info->read_word_data = raa_dmpvr2_read_word_data; + info->write_word_data = raa_dmpvr2_write_word_data; break; case raa_dmpvr2_3rail: info->read_word_data = raa_dmpvr2_read_word_data; + info->write_word_data = raa_dmpvr2_write_word_data; break; case raa_dmpvr2_hv: info->pages = 1; @@ -263,11 +397,18 @@ static int isl68137_probe(struct i2c_client *client) info->m[PSC_POWER] = 2; info->R[PSC_POWER] = -1; info->read_word_data = raa_dmpvr2_read_word_data; + info->write_word_data = raa_dmpvr2_write_word_data; break; default: return -ENODEV; } + if (dev->of_node) { + err = isl68137_probe_from_dt(dev, data); + if (err) + return err; + } + return pmbus_do_probe(client, info); } @@ -318,11 +459,59 @@ static const struct i2c_device_id raa_dmpvr_id[] = { MODULE_DEVICE_TABLE(i2c, raa_dmpvr_id); +static const struct of_device_id isl68137_of_match[] = { + { .compatible = "renesas,isl68137", .data = (void *)raa_dmpvr1_2rail }, + { .compatible = "renesas,isl68220", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl68221", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl68222", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl68223", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl68224", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl68225", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl68226", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl68227", .data = (void *)raa_dmpvr2_1rail }, + { .compatible = "renesas,isl68229", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl68233", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl68239", .data = (void *)raa_dmpvr2_3rail }, + + { .compatible = "renesas,isl69222", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69223", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl69224", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69225", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69227", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl69228", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl69234", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69236", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69239", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl69242", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69243", .data = (void *)raa_dmpvr2_1rail }, + { .compatible = "renesas,isl69247", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69248", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69254", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69255", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69256", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69259", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69260", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69268", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,isl69269", .data = (void *)raa_dmpvr2_3rail }, + { .compatible = "renesas,isl69298", .data = (void *)raa_dmpvr2_2rail }, + + { .compatible = "renesas,raa228000", .data = (void *)raa_dmpvr2_hv }, + { .compatible = "renesas,raa228004", .data = (void *)raa_dmpvr2_hv }, + { .compatible = "renesas,raa228006", .data = (void *)raa_dmpvr2_hv }, + { .compatible = "renesas,raa228228", .data = (void *)raa_dmpvr2_2rail_nontc }, + { .compatible = "renesas,raa229001", .data = (void *)raa_dmpvr2_2rail }, + { .compatible = "renesas,raa229004", .data = (void *)raa_dmpvr2_2rail }, + { }, +}; + +MODULE_DEVICE_TABLE(of, isl68137_of_match); + /* This is the driver that will be inserted */ static struct i2c_driver isl68137_driver = { .driver = { - .name = "isl68137", - }, + .name = "isl68137", + .of_match_table = isl68137_of_match, + }, .probe = isl68137_probe, .id_table = raa_dmpvr_id, };
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@gmail.com> --- drivers/hwmon/pmbus/isl68137.c | 199 ++++++++++++++++++++++++++++++++- 1 file changed, 194 insertions(+), 5 deletions(-)