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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---
  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,
  };





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux