[PATCH v6 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137

[PATCH v6 0/2] dt-bindings: hwmon: pmbus: add bindings for isl68137

[Grant Peltier]

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.

v6:
- Amend patch commit messages to explain why the vout-voltage-divider
  property was copied from the max20730 instead of using the iio
  voltage-divider property

v5:
- Fix clang compilation errors related to C23 syntax

v4:
- Revert devicetree property name to "vout-voltage-divider" and refactor
  property description and driver implementation to match existing
  vout-voltage-divider implementation in max20730 as no suitable generic
  voltage divider schema exists.
- Minor fixes based on Guenter's review of v2.
- Initialize voltage dividers for all channels to defaults that simplify
  logic in PMBus word read/write functions.

v3:
- Report and return errors reading the vout-voltage-divider property from
  the devicetree when the property is defined
- Change u64 division/rounding operations to use explicit math64 macros

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         | 147 ++++++++++++
 drivers/hwmon/pmbus/isl68137.c                | 209 +++++++++++++++++-
 2 files changed, 351 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml

-- 
2.39.5

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

[Grant Peltier]

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.

This change copies the implementation of the vout-voltage-divider
devicetree property defined in the maxim,max20730 bindings schema since
it is the best fit for the use case of scaling hwmon PMBus telemetry. The
generic voltage-divider property used by many iio drivers was determined
to be a poor fit because that schema is tied directly to iio and the
isl68137 driver is not an iio driver.

Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
 drivers/hwmon/pmbus/isl68137.c | 209 ++++++++++++++++++++++++++++++++-
 1 file changed, 204 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
index 7e53fb1d5ea3..cd0cc82c18e7 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,13 +176,32 @@ 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;
+	u64 temp;
 
 	switch (reg) {
 	case PMBUS_VIRT_READ_VMON:
 		ret = pmbus_read_word_data(client, page, phase,
 					   RAA_DMPVR2_READ_VMON);
 		break;
+	case PMBUS_READ_POUT:
+	case PMBUS_READ_VOUT:
+		/*
+		 * 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 * Rtotal / Rout
+		 */
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret > 0) {
+			temp = DIV_U64_ROUND_CLOSEST((u64)ret *
+				data->channel[page].vout_voltage_divider[1],
+				data->channel[page].vout_voltage_divider[0]);
+			ret = clamp_val(temp, 0, 0xffff);
+		}
+		break;
 	default:
 		ret = -ENODATA;
 		break;
@@ -178,6 +210,40 @@ 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;
+	u64 temp;
+
+	switch (reg) {
+	case PMBUS_VOUT_MAX:
+	case PMBUS_VOUT_MARGIN_HIGH:
+	case PMBUS_VOUT_MARGIN_LOW:
+	case PMBUS_VOUT_OV_FAULT_LIMIT:
+	case PMBUS_VOUT_UV_FAULT_LIMIT:
+	case PMBUS_VOUT_COMMAND:
+		/*
+		 * 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 * Rout / Rtotal
+		 */
+		temp = DIV_U64_ROUND_CLOSEST((u64)word *
+				data->channel[page].vout_voltage_divider[0],
+				data->channel[page].vout_voltage_divider[1]);
+		ret = clamp_val(temp, 0, 0xffff);
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+	return ret;
+}
+
 static struct pmbus_driver_info raa_dmpvr_info = {
 	.pages = 3,
 	.format[PSC_VOLTAGE_IN] = direct,
@@ -220,14 +286,91 @@ 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, rout, rtotal;
+	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 >= data->info.pages) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", channel, child);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32_array(child, "vout-voltage-divider",
+				data->channel[channel].vout_voltage_divider,
+				ARRAY_SIZE(data->channel[channel].vout_voltage_divider));
+	if (err && err != -EINVAL) {
+		dev_err(dev,
+			"malformed vout-voltage-divider value for channel %d\n",
+			channel);
+		return err;
+	}
+
+	rout = data->channel[channel].vout_voltage_divider[0];
+	rtotal = data->channel[channel].vout_voltage_divider[1];
+	if (rout == 0) {
+		dev_err(dev,
+			"Voltage divider output resistance must be greater than 0\n");
+		return -EINVAL;
+	}
+	if (rtotal < rout) {
+		dev_err(dev,
+			"Voltage divider total resistance is less than output resistance\n");
+		return -EINVAL;
+	}
+
+	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));
+
+	/*
+	 * Initialize all voltage dividers to Rout=1 and Rtotal=1 to simplify
+	 * logic in PMBus word read/write functions
+	 */
+	for (i = 0; i < MAX_CHANNELS; i++)
+		memset(data->channel[i].vout_voltage_divider,
+			1,
+			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 +385,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 +394,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 +409,16 @@ 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;
 	}
 
+	err = isl68137_probe_from_dt(dev, data);
+	if (err)
+		return err;
+
 	return pmbus_do_probe(client, info);
 }
 
@@ -318,11 +469,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

[PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Grant Peltier]

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.

These bindings copy the vout-voltage-divider property defined in the
maxim,max20730 bindings schema since it is the best fit for the use case
of scaling hwmon PMBus telemetry. The generic voltage-divider property
used by many iio drivers was determined to be a poor fit because that
schema is tied directly to iio for the purpose of scaling io-channel
voltages and the isl68137 driver is not an iio driver.

Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
 .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
 1 file changed, 147 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..ed659c2baadf
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
@@ -0,0 +1,147 @@
+# 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
+
+      vout-voltage-divider:
+        description: |
+          Resistances of a voltage divider placed between Vout and the voltage
+          sense (Vsense) pin for the given channel (rail). It has two numbers
+          representing the resistances of the voltage divider provided as
+          <Rout Rtotal> which yields an adjusted Vout as
+          Vout_adj = Vout * Rtotal / Rout given the original Vout as reported
+          by the Vsense pin. Given a circuit configuration similar to the one
+          below, Rtotal = R1 + Rout.
+
+            Vout ----.
+                     |
+                  .-----.
+                  | R1  |
+                  '-----'
+                     |
+                     +---- Vsense
+                     |
+                  .-----.
+                  | Rout|
+                  '-----'
+                     |
+                    GND
+
+        $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>;
+          vout-voltage-divider = <1000 2000>;  // Reported Vout/Pout would be scaled by 2
+        };
+      };
+    };
-- 
2.39.5

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Rob Herring (Arm)]

On Wed, 30 Oct 2024 17:41:39 -0500, Grant Peltier wrote:
> 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.
> 
> These bindings copy the vout-voltage-divider property defined in the
> maxim,max20730 bindings schema since it is the best fit for the use case
> of scaling hwmon PMBus telemetry. The generic voltage-divider property
> used by many iio drivers was determined to be a poor fit because that
> schema is tied directly to iio for the purpose of scaling io-channel
> voltages and the isl68137 driver is not an iio driver.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> ---
>  .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
>  1 file changed, 147 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
> 


Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Missing tags:

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Rob Herring (Arm)]

On Wed, 30 Oct 2024 17:41:39 -0500, Grant Peltier wrote:
> 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.
> 
> These bindings copy the vout-voltage-divider property defined in the
> maxim,max20730 bindings schema since it is the best fit for the use case
> of scaling hwmon PMBus telemetry. The generic voltage-divider property
> used by many iio drivers was determined to be a poor fit because that
> schema is tied directly to iio for the purpose of scaling io-channel
> voltages and the isl68137 driver is not an iio driver.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> ---
>  .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
>  1 file changed, 147 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Guenter Roeck]

On Wed, Oct 30, 2024 at 05:41:39PM -0500, Grant Peltier wrote:
> 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.
> 
> These bindings copy the vout-voltage-divider property defined in the
> maxim,max20730 bindings schema since it is the best fit for the use case
> of scaling hwmon PMBus telemetry. The generic voltage-divider property
> used by many iio drivers was determined to be a poor fit because that
> schema is tied directly to iio for the purpose of scaling io-channel
> voltages and the isl68137 driver is not an iio driver.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Applied.

Thanks,
Guenter

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

[Guenter Roeck]

On Wed, Oct 30, 2024 at 05:40:58PM -0500, 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.
> 
> This change copies the implementation of the vout-voltage-divider
> devicetree property defined in the maxim,max20730 bindings schema since
> it is the best fit for the use case of scaling hwmon PMBus telemetry. The
> generic voltage-divider property used by many iio drivers was determined
> to be a poor fit because that schema is tied directly to iio and the
> isl68137 driver is not an iio driver.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>

Applied, after fixing several alignment problems reported by
checkpatch --strict.

Guenter

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Rob Herring]

On Wed, Oct 30, 2024 at 5:41 PM Grant Peltier <grantpeltier93@gmail.com> wrote:
>
> 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.
>
> These bindings copy the vout-voltage-divider property defined in the
> maxim,max20730 bindings schema since it is the best fit for the use case
> of scaling hwmon PMBus telemetry. The generic voltage-divider property
> used by many iio drivers was determined to be a poor fit because that
> schema is tied directly to iio for the purpose of scaling io-channel
> voltages and the isl68137 driver is not an iio driver.
>
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> ---
>  .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
>  1 file changed, 147 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..ed659c2baadf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
> @@ -0,0 +1,147 @@
> +# 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:

Somehow "isl68137" is missing from your list. "make
dt_compatible_check" reports it as not documented.

> +      - 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

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Guenter Roeck]

On 11/4/24 07:08, Rob Herring wrote:
> On Wed, Oct 30, 2024 at 5:41 PM Grant Peltier <grantpeltier93@gmail.com> wrote:
>>
>> 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.
>>
>> These bindings copy the vout-voltage-divider property defined in the
>> maxim,max20730 bindings schema since it is the best fit for the use case
>> of scaling hwmon PMBus telemetry. The generic voltage-divider property
>> used by many iio drivers was determined to be a poor fit because that
>> schema is tied directly to iio for the purpose of scaling io-channel
>> voltages and the isl68137 driver is not an iio driver.
>>
>> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
>> ---
>>   .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
>>   1 file changed, 147 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..ed659c2baadf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
>> @@ -0,0 +1,147 @@
>> +# 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:
> 
> Somehow "isl68137" is missing from your list. "make
> dt_compatible_check" reports it as not documented.
> 

Turns out it is also documented as "isil,isl68137" in trivial-devices.yaml
(together with isil,isl69260). Both are referenced in .dts files. How should
that be handled ?

Thanks,
Guenter

>> +      - 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

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Rob Herring]

On Mon, Nov 4, 2024 at 7:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/4/24 07:08, Rob Herring wrote:
> > On Wed, Oct 30, 2024 at 5:41 PM Grant Peltier <grantpeltier93@gmail.com> wrote:
> >>
> >> 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.
> >>
> >> These bindings copy the vout-voltage-divider property defined in the
> >> maxim,max20730 bindings schema since it is the best fit for the use case
> >> of scaling hwmon PMBus telemetry. The generic voltage-divider property
> >> used by many iio drivers was determined to be a poor fit because that
> >> schema is tied directly to iio for the purpose of scaling io-channel
> >> voltages and the isl68137 driver is not an iio driver.
> >>
> >> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> >> ---
> >>   .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
> >>   1 file changed, 147 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..ed659c2baadf
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
> >> @@ -0,0 +1,147 @@
> >> +# 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:
> >
> > Somehow "isl68137" is missing from your list. "make
> > dt_compatible_check" reports it as not documented.
> >
>
> Turns out it is also documented as "isil,isl68137" in trivial-devices.yaml
> (together with isil,isl69260). Both are referenced in .dts files. How should
> that be handled ?

Move those compatibles here. And this file should be renamed to
isil,isl68137.yaml or some other actual compatible value.

Rob

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Guenter Roeck]

On 11/5/24 04:19, Rob Herring wrote:
> On Mon, Nov 4, 2024 at 7:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 11/4/24 07:08, Rob Herring wrote:
>>> On Wed, Oct 30, 2024 at 5:41 PM Grant Peltier <grantpeltier93@gmail.com> wrote:
>>>>
>>>> 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.
>>>>
>>>> These bindings copy the vout-voltage-divider property defined in the
>>>> maxim,max20730 bindings schema since it is the best fit for the use case
>>>> of scaling hwmon PMBus telemetry. The generic voltage-divider property
>>>> used by many iio drivers was determined to be a poor fit because that
>>>> schema is tied directly to iio for the purpose of scaling io-channel
>>>> voltages and the isl68137 driver is not an iio driver.
>>>>
>>>> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
>>>> ---
>>>>    .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
>>>>    1 file changed, 147 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..ed659c2baadf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
>>>> @@ -0,0 +1,147 @@
>>>> +# 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:
>>>
>>> Somehow "isl68137" is missing from your list. "make
>>> dt_compatible_check" reports it as not documented.
>>>
>>
>> Turns out it is also documented as "isil,isl68137" in trivial-devices.yaml
>> (together with isil,isl69260). Both are referenced in .dts files. How should
>> that be handled ?
> 
> Move those compatibles here. And this file should be renamed to
> isil,isl68137.yaml or some other actual compatible value.
> 

I guess that is a bit more complicated since Renesas acquired Intersil.
Is there a common guidance explaining how new compatibles should be defined
in such situations ?

Anyway, I had the patches queued in linux-next. I dropped them until
this is sorted out.

Guenter

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

On Tue, Nov 05, 2024 at 07:45:38AM -0800, Guenter Roeck wrote:
> On 11/5/24 04:19, Rob Herring wrote:
> > On Mon, Nov 4, 2024 at 7:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > 
> > > On 11/4/24 07:08, Rob Herring wrote:
> > > > On Wed, Oct 30, 2024 at 5:41 PM Grant Peltier <grantpeltier93@gmail.com> wrote:
> > > > > 
> > > > > 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.
> > > > > 
> > > > > These bindings copy the vout-voltage-divider property defined in the
> > > > > maxim,max20730 bindings schema since it is the best fit for the use case
> > > > > of scaling hwmon PMBus telemetry. The generic voltage-divider property
> > > > > used by many iio drivers was determined to be a poor fit because that
> > > > > schema is tied directly to iio for the purpose of scaling io-channel
> > > > > voltages and the isl68137 driver is not an iio driver.
> > > > > 
> > > > > Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> > > > > ---
> > > > >    .../hwmon/pmbus/renesas,isl68137.yaml         | 147 ++++++++++++++++++
> > > > >    1 file changed, 147 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..ed659c2baadf
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/renesas,isl68137.yaml
> > > > > @@ -0,0 +1,147 @@
> > > > > +# 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:
> > > > 
> > > > Somehow "isl68137" is missing from your list. "make
> > > > dt_compatible_check" reports it as not documented.
> > > > 
> > > 
> > > Turns out it is also documented as "isil,isl68137" in trivial-devices.yaml
> > > (together with isil,isl69260). Both are referenced in .dts files. How should
> > > that be handled ?
> > 
> > Move those compatibles here. And this file should be renamed to
> > isil,isl68137.yaml or some other actual compatible value.
> > 
> 
> I guess that is a bit more complicated since Renesas acquired Intersil.
> Is there a common guidance explaining how new compatibles should be defined
> in such situations ?
> 
> Anyway, I had the patches queued in linux-next. I dropped them until
> this is sorted out.

The old ones with existing compatibles should keep their names, the new ones
should probably match whatever is in their datasheet so that people can
have an easier time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Geert Uytterhoeven]

Hi Conor,

On Tue, Nov 5, 2024 at 7:35 PM Conor Dooley <conor@kernel.org> wrote:
> On Tue, Nov 05, 2024 at 07:45:38AM -0800, Guenter Roeck wrote:
> > On 11/5/24 04:19, Rob Herring wrote:
> > > On Mon, Nov 4, 2024 at 7:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > Turns out it is also documented as "isil,isl68137" in trivial-devices.yaml
> > > > (together with isil,isl69260). Both are referenced in .dts files. How should
> > > > that be handled ?
> > >
> > > Move those compatibles here. And this file should be renamed to
> > > isil,isl68137.yaml or some other actual compatible value.
> > >
> >
> > I guess that is a bit more complicated since Renesas acquired Intersil.
> > Is there a common guidance explaining how new compatibles should be defined
> > in such situations ?
> >
> > Anyway, I had the patches queued in linux-next. I dropped them until
> > this is sorted out.
>
> The old ones with existing compatibles should keep their names, the new ones
> should probably match whatever is in their datasheet so that people can
> have an easier time.

Renesas seems to have a document about that:
https://www.renesas.com/en/document/gde/intersil-part-code-nomenclature-guide

But of course that doesn't help with the vendor prefix...

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