* [PATCH v2 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137
@ 2024-10-23 1:56 Grant Peltier
2024-10-23 1:58 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout Grant Peltier
2024-10-23 1:58 ` [PATCH v2 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers Grant Peltier
0 siblings, 2 replies; 8+ messages in thread
From: Grant Peltier @ 2024-10-23 1:56 UTC (permalink / raw)
To: robh, linux, geert+renesas, magnus.damm
Cc: grant.peltier.jg, brandon.howell.jg, linux-hwmon, devicetree,
linux-doc
Renesas digital multiphase voltage regulators are capable of regulating
output voltages that exceed the range that their Vsense pins can detect.
In such applications, users may place a voltage divider between Vout and
the Vsense pin for a given rail. However, the driver currently has no
way of knowing if a voltage divider is being used which results in
erroneous telemetry being reported over hwmon.
This patch set defines a devicetree bindings schema for Renesas digital
multiphase voltage regulators that are supported by the isl68137 driver
to allow users to add voltage divider definitions for any rail powered
by the device. This patch set also includes the required changes to the
isl68137 driver to enable scaling Vout/Pout telemetry for rails with a
defined voltage divider.
v2:
- Fix devicetree bindings schema errors
- Add "renesas," vendor prefix to "vout-voltage-divider" property
- Rebase patch series on v6.12-rc1
Grant Peltier (2):
hwmon: (pmbus/isl68137) add support for voltage divider on Vout
dt-bindings: hwmon: isl68137: add bindings to support voltage dividers
.../hwmon/pmbus/renesas,isl68137.yaml | 131 ++++++++++++
drivers/hwmon/pmbus/isl68137.c | 199 +++++++++++++++++-
2 files changed, 325 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
--
2.39.5
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-23 1:56 [PATCH v2 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137 Grant Peltier
@ 2024-10-23 1:58 ` Grant Peltier
2024-10-23 7:34 ` Geert Uytterhoeven
2024-10-24 17:48 ` Guenter Roeck
2024-10-23 1:58 ` [PATCH v2 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers Grant Peltier
1 sibling, 2 replies; 8+ messages in thread
From: Grant Peltier @ 2024-10-23 1:58 UTC (permalink / raw)
To: robh, linux, geert+renesas, magnus.damm
Cc: grant.peltier.jg, brandon.howell.jg, linux-hwmon, devicetree,
linux-doc
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;
+ 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,
};
--
2.39.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers
2024-10-23 1:56 [PATCH v2 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137 Grant Peltier
2024-10-23 1:58 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout Grant Peltier
@ 2024-10-23 1:58 ` Grant Peltier
1 sibling, 0 replies; 8+ messages in thread
From: Grant Peltier @ 2024-10-23 1:58 UTC (permalink / raw)
To: robh, linux, geert+renesas, magnus.damm
Cc: grant.peltier.jg, brandon.howell.jg, linux-hwmon, devicetree,
linux-doc
Add devicetree bindings to support declaring optional voltage dividers to
the rail outputs of supported digital multiphase regulators. Some
applications require Vout to exceed the voltage range that the Vsense pin
can detect. This binding definition allows users to define the
characteristics of a voltage divider placed between Vout and the Vsense
pin for any rail powered by the device.
Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
.../hwmon/pmbus/renesas,isl68137.yaml | 131 ++++++++++++++++++
1 file changed, 131 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
new file mode 100644
index 000000000000..af10c55d547f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
@@ -0,0 +1,131 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/pmbus/renesas,isl68137.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas Digital Multiphase Voltage Regulators with PMBus
+
+maintainers:
+ - Grant Peltier <grant.peltier.jg@renesas.com>
+
+description: |
+ Renesas digital multiphase voltage regulators with PMBus.
+ https://www.renesas.com/en/products/power-management/multiphase-power/multiphase-dcdc-switching-controllers
+
+properties:
+ compatible:
+ enum:
+ - renesas,isl68220
+ - renesas,isl68221
+ - renesas,isl68222
+ - renesas,isl68223
+ - renesas,isl68224
+ - renesas,isl68225
+ - renesas,isl68226
+ - renesas,isl68227
+ - renesas,isl68229
+ - renesas,isl68233
+ - renesas,isl68239
+ - renesas,isl69222
+ - renesas,isl69223
+ - renesas,isl69224
+ - renesas,isl69225
+ - renesas,isl69227
+ - renesas,isl69228
+ - renesas,isl69234
+ - renesas,isl69236
+ - renesas,isl69239
+ - renesas,isl69242
+ - renesas,isl69243
+ - renesas,isl69247
+ - renesas,isl69248
+ - renesas,isl69254
+ - renesas,isl69255
+ - renesas,isl69256
+ - renesas,isl69259
+ - renesas,isl69260
+ - renesas,isl69268
+ - renesas,isl69269
+ - renesas,isl69298
+ - renesas,raa228000
+ - renesas,raa228004
+ - renesas,raa228006
+ - renesas,raa228228
+ - renesas,raa229001
+ - renesas,raa229004
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+patternProperties:
+ "^channel@([0-3])$":
+ type: object
+ description:
+ Container for properties specific to a particular channel (rail).
+
+ properties:
+ reg:
+ description: The channel (rail) index.
+ items:
+ minimum: 0
+ maximum: 3
+
+ renesas,vout-voltage-divider:
+ description:
+ Resistances of a voltage divider placed between Vout and the voltage
+ sense pin for the given channel (rail). It has two numbers
+ representing the resistances of the voltage divider provided as
+ <R1 R2> which yields an adjusted Vout as
+ Vout_adj = Vout * (R1 + R2) / R2 given the original Vout as reported
+ by the Vsense pin.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 2
+ maxItems: 2
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isl68239@60 {
+ compatible = "renesas,isl68239";
+ reg = <0x60>;
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isl68239@60 {
+ compatible = "renesas,isl68239";
+ reg = <0x60>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ renesas,vout-voltage-divider = <1000 1000>; // Reported Vout/Pout would be scaled by 2
+ };
+ };
+ };
--
2.39.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-23 1:58 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout Grant Peltier
@ 2024-10-23 7:34 ` Geert Uytterhoeven
2024-10-23 16:40 ` Grant Peltier
2024-10-24 17:48 ` Guenter Roeck
1 sibling, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2024-10-23 7:34 UTC (permalink / raw)
To: Grant Peltier
Cc: robh, linux, magnus.damm, grant.peltier.jg, brandon.howell.jg,
linux-hwmon, devicetree, linux-doc
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-23 7:34 ` Geert Uytterhoeven
@ 2024-10-23 16:40 ` Grant Peltier
0 siblings, 0 replies; 8+ messages in thread
From: Grant Peltier @ 2024-10-23 16:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: robh, linux, magnus.damm, grant.peltier.jg, brandon.howell.jg,
linux-hwmon, devicetree, linux-doc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-23 1:58 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout Grant Peltier
2024-10-23 7:34 ` Geert Uytterhoeven
@ 2024-10-24 17:48 ` Guenter Roeck
2024-10-24 19:43 ` Grant Peltier
1 sibling, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2024-10-24 17:48 UTC (permalink / raw)
To: Grant Peltier, robh, geert+renesas, magnus.damm
Cc: grant.peltier.jg, brandon.howell.jg, linux-hwmon, devicetree,
linux-doc
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,
> };
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-24 17:48 ` Guenter Roeck
@ 2024-10-24 19:43 ` Grant Peltier
2024-10-24 20:03 ` Guenter Roeck
0 siblings, 1 reply; 8+ messages in thread
From: Grant Peltier @ 2024-10-24 19:43 UTC (permalink / raw)
To: Guenter Roeck
Cc: robh, geert+renesas, magnus.damm, grant.peltier.jg,
brandon.howell.jg, linux-hwmon, devicetree, linux-doc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout
2024-10-24 19:43 ` Grant Peltier
@ 2024-10-24 20:03 ` Guenter Roeck
0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2024-10-24 20:03 UTC (permalink / raw)
To: Grant Peltier
Cc: robh, geert+renesas, magnus.damm, grant.peltier.jg,
brandon.howell.jg, linux-hwmon, devicetree, linux-doc
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-10-24 20:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-23 1:56 [PATCH v2 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137 Grant Peltier
2024-10-23 1:58 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout Grant Peltier
2024-10-23 7:34 ` Geert Uytterhoeven
2024-10-23 16:40 ` Grant Peltier
2024-10-24 17:48 ` Guenter Roeck
2024-10-24 19:43 ` Grant Peltier
2024-10-24 20:03 ` Guenter Roeck
2024-10-23 1:58 ` [PATCH v2 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers Grant Peltier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).