Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238

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

 



On 10/24/21 7:58 PM, Nathan Rossi wrote:
From: Nathan Rossi <nathan.rossi@xxxxxxxx>

The INA238 is a I2C power monitor similar to other INA2xx devices,
providing shunt voltage, bus voltage, current, power and temperature
measurements.

Signed-off-by: Nathan Rossi <nathan.rossi@xxxxxxxx>
---
  Documentation/hwmon/ina238.rst |  57 ++++++

Needs to be added to index.rst.

  drivers/hwmon/Kconfig          |  12 ++
  drivers/hwmon/Makefile         |   1 +
  drivers/hwmon/ina238.c         | 453 +++++++++++++++++++++++++++++++++++++++++
  4 files changed, 523 insertions(+)
  create mode 100644 Documentation/hwmon/ina238.rst
  create mode 100644 drivers/hwmon/ina238.c

diff --git a/Documentation/hwmon/ina238.rst b/Documentation/hwmon/ina238.rst
new file mode 100644
index 0000000000..612fab185d
--- /dev/null
+++ b/Documentation/hwmon/ina238.rst
@@ -0,0 +1,57 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Kernel driver ina238
+====================
+
+Supported chips:
+
+  * Texas Instruments INA238
+
+    Prefix: 'ina238'
+
+    Addresses: I2C 0x40 - 0x4f
+
+    Datasheet:
+	https://www.ti.com/lit/gpn/ina238
+
+Author: Nathan Rossi <nathan.rossi@xxxxxxxx>
+
+Description
+-----------
+
+The INA238 is a current shunt, power and temperature monitor with an I2C
+interface. It includes a number of programmable functions including alerts,
+conversion rate, sample averaging and selectable shunt voltage accuracy.
+
+The shunt value in micro-ohms can be set via platform data or device tree at
+compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
+refer to the Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml for bindings
+if the device tree is used.
+
+Sysfs entries
+-------------
+
+======================= =======================================================
+in0_input		Shunt voltage (mV)
+in0_lcrit		Critical low shunt voltage
+in0_crit		Critical high shunt voltage
+
+in1_input		Bus voltage (mV)
+in1_lcrit		Critical low bus voltage (mV)
+in1_crit		Critical high bus voltage (mV)
+
+power1_input		Power measurement (uW)
+power1_crit		Critical power limit (uW)
+
+curr1_input		Current measurement (mA)
+
+temp1_input		Die temperature measurement (mC)
+temp1_crit		Critical die temperature limit (mC)
+
+shunt_resistor		Shunt resistance (uOhm)
+======================= =======================================================
+
+.. note::
+
+   - Configure `shunt_resistor` before configure `power1_crit`, because power
+     value is calculated based on `shunt_resistor` set.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7fde4c6e1e..cae8e62734 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1872,6 +1872,18 @@ config SENSORS_INA2XX
  	  This driver can also be built as a module. If so, the module
  	  will be called ina2xx.
+config SENSORS_INA238
+	tristate "Texas Instruments INA238"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for the INA238 power monitor
+	  chip. This driver supports voltage, current, power and temperature
+	  measurements as well as alert configuration.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called ina238.
+
  config SENSORS_INA3221
  	tristate "Texas Instruments INA3221 Triple Power Monitor"
  	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index baee6a8d4d..1ddb26f57a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o
  obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
  obj-$(CONFIG_SENSORS_INA209)	+= ina209.o
  obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
+obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
  obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
  obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
  obj-$(CONFIG_SENSORS_IT87)	+= it87.o
diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
new file mode 100644
index 0000000000..001b490b79
--- /dev/null
+++ b/drivers/hwmon/ina238.c
@@ -0,0 +1,453 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Texas Instruments INA238 power monitor chip
+ * Datasheet: https://www.ti.com/product/ina238
+ *
+ * Copyright (C) 2021 Nathan Rossi <nathan.rossi@xxxxxxxx>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/util_macros.h>
+#include <linux/regmap.h>
+

Alphabetic include file order please. Also, please make sure that there are no
unecessary include files. I don't immediately see where jiffies.h and delay.h
are needed.

+#include <linux/platform_data/ina2xx.h>
+
+/* INA238 register definitions */
+#define INA238_CONFIG			0x0
+#define INA238_ADC_CONFIG		0x1
+#define INA238_SHUNT_CALIBRATION	0x2
+#define INA238_SHUNT_VOLTAGE		0x4
+#define INA238_BUS_VOLTAGE		0x5
+#define INA238_DIE_TEMP			0x6
+#define INA238_CURRENT			0x7
+#define INA238_POWER			0x8
+#define INA238_DIAG_ALERT		0xb
+#define INA238_SHUNT_OVER_VOLTAGE	0xc
+#define INA238_SHUNT_UNDER_VOLTAGE	0xd
+#define INA238_BUS_OVER_VOLTAGE		0xe
+#define INA238_BUS_UNDER_VOLTAGE	0xf
+#define INA238_TEMP_LIMIT		0x10
+#define INA238_POWER_LIMIT		0x11
+#define INA238_DEVICE_ID		0x3f
+
+#define INA238_REGISTERS		0x11
+
+#define INA238_RSHUNT_DEFAULT		10000 /* uOhm */
+
+/* Default configuration of device on reset. */
+#define INA238_CONFIG_DEFAULT		0
+/* 16 sample averaging, 1052us conversion time, continuous mode */
+#define INA238_ADC_CONFIG_DEFAULT	0xfb6a
+/*
+ * This driver uses a fixed calibration value in order to scale current/power
+ * based on a fixed shunt resistor value. This allows for conversion within the
+ * device to avoid integer limits whilst current/power accuracy is scaled
+ * relative to the shunt resistor value within the driver. This is similar to
+ * how the ina2xx driver handles current/power scaling.
+ *
+ * The end result of this is that increasing shunt values (from a fixed 20 mOhm
+ * shunt) increase the effective current/power accuracy whilst limiting the
+ * range and decreasing shunt values decrease the effective accuracy but
+ * increase the range.
+ *
+ * The value of the Current register is calculated given the following:
+ *   Current (A) = (shunt voltage register * 5) * calibration / 81920
+ *
+ * The maximum shunt voltage is 163.835 mV (0x7fff, ADC_RANGE = 0). With the
+ * maximum current value of 0x7fff and a fixed shunt value results in a
+ * calibration value of 16384 (0x4000).
+ *
+ *   0x7fff = (0x7fff * 5) * calibration / 81920
+ *   calibration = 0x4000
+ *
+ * Equivalent calibration is applied for the Power register (maximum value for
+ * bus voltage is 102396.875 mV, 0x7fff), where the maximum power that can
+ * occur is ~16776192 uW (register value 0x147a8):
+ *
+ * This scaling means the resulting values for Current and Power registers need
+ * to be scaled by the difference between the fixed shunt resistor and the
+ * actual shunt resistor:
+ *
+ *  shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb)
+ *
+ *  Current (mA) = register value * 20000 / rshunt
+ *  Power (W) = 0.2 * register value * 20000 / rshunt
+ */
+#define INA238_CALIBRATION_VALUE	16384
+#define INA238_FIXED_SHUNT		20000
+
+#define INA238_SHUNT_VOLTAGE_LSB	5 /* 5 uV/lsb */
+#define INA238_BUS_VOLTAGE_LSB		3125 /* 3.125 mV/lsb */
+#define INA238_DIE_TEMP_LSB		125 /* 125 mC/lsb */
+
+static struct regmap_config ina238_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
+struct ina238_data {
+	struct i2c_client *client;
+	struct mutex config_lock;
+	struct regmap *regmap;
+	long rshunt;
+};
+
+static ssize_t ina238_value_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct ina238_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	long long val = 0;
+	u8 regdata[3];
+	int err;
+
+	if (attr->index == INA238_POWER) {
+		/* Handle reading the POWER register as 24-bit */
+		err = i2c_smbus_read_i2c_block_data(data->client, attr->index, 3,
+						    regdata);
+		if (err != 3)
+			return err;
+		regval = (regdata[0] << 16) | (regdata[1] << 8) | regdata[2];
+	} else {
+		err = regmap_read(data->regmap, attr->index, &regval);
+		if (err < 0)
+			return err;
+	}
+
+	switch (attr->index) {
+	case INA238_SHUNT_VOLTAGE:
+		/* Signed register, result in mV */
+		val = div_s64((s16)regval * INA238_SHUNT_VOLTAGE_LSB,
+					1000);
+		break;
+	case INA238_BUS_VOLTAGE:
+		/* Result in mV */
+		val = div_s64((s16)regval * INA238_BUS_VOLTAGE_LSB, 1000);
+		break;
+	case INA238_CURRENT:
+		/* Signed register, fixed 1mA current lsb. result in mA */
+		val = div_s64((s16)regval * INA238_FIXED_SHUNT, data->rshunt);
+		break;
+	case INA238_POWER:
+		/* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
+		val = div_u64(regval * 1000LL * INA238_FIXED_SHUNT, 5 * data->rshunt);
+		break;
+	case INA238_DIE_TEMP:
+		/* Bits 15-4 of register, result in mC */
+		val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+		break;
+	case INA238_SHUNT_CALIBRATION:
+		val = regval;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%lld\n", val);
+}
+
+static int ina238_set_shunt(struct device *dev, struct ina238_data *data,
+			    long val)
+{
+	if (val == 0)
+		return -EINVAL;
+
+	mutex_lock(&data->config_lock);
+	data->rshunt = val;
+	mutex_unlock(&data->config_lock);

rshunt is used outside the lock for calculations.
The lock here does therefore not add any value.

+
+	return 0;
+}
+
+static ssize_t ina238_shunt_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%li\n", data->rshunt);
+}
+
+static ssize_t ina238_shunt_store(struct device *dev,
+				  struct device_attribute *da,
+				  const char *buf, size_t count)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int status;
+
+	status = kstrtoul(buf, 10, &val);
+	if (status < 0)
+		return status;
+
+	status = ina238_set_shunt(dev, data, val);
+	if (status < 0)
+		return status;
+	return count;
+}
+

Is there reason to believe that the shunt register value needs to be visible
and writeable with sysfs attributes ? This is quite unusual nowadays.
If so, please provide a use case.

+static ssize_t ina238_alert_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
"Alert" is normally used for alarms and provides boolean values (0/1).
It is used for limits here, making the code quite confusing (I was
trying to understand how the code relates to alarms). Please use a more
appropriate function name.

+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct ina238_data *data = dev_get_drvdata(dev);
+	long long val = 0;
+	int regval;
+	int ret;
+
+	ret = regmap_read(data->regmap, attr->index, &regval);
+	if (ret)
+		return ret;
+
+	switch (attr->index) {
+	case INA238_SHUNT_OVER_VOLTAGE:
+	case INA238_SHUNT_UNDER_VOLTAGE:
+		val = div_s64((s16)regval * INA238_SHUNT_VOLTAGE_LSB, 1000);
+		break;
+	case INA238_BUS_OVER_VOLTAGE:
+	case INA238_BUS_UNDER_VOLTAGE:
+		val = div_u64(regval * INA238_BUS_VOLTAGE_LSB, 1000);
+		break;
+	case INA238_POWER_LIMIT:
+		/*
+		 * Truncated 24-bit compare register, lower 8-bits are
+		 * truncated. Same conversion to/from uW as POWER register.
+		 */
+		val = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT,
+			      5 * data->rshunt);
+		break;
+	case INA238_TEMP_LIMIT:
+		/* Signed, bits 15-4 of register */
+		val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%lld\n", val);
+}
+
+static ssize_t ina238_alert_store(struct device *dev,
+				  struct device_attribute *da,
+				  const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct ina238_data *data = dev_get_drvdata(dev);
+	long long val;
+	int regval;
+	int ret;
+
+	ret = kstrtoll(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	/* convert decimal to register value */
+	switch (attr->index) {
+	case INA238_SHUNT_OVER_VOLTAGE:
+	case INA238_SHUNT_UNDER_VOLTAGE:
+		/* signed */
+		regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
+		if (regval > S16_MAX || regval < S16_MIN) {
+			ret = -EINVAL;
+			goto abort;
+		}
+		break;
+	case INA238_BUS_OVER_VOLTAGE:
+	case INA238_BUS_UNDER_VOLTAGE:
+		regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
+		if (regval > U16_MAX || regval < 0) {
+			ret = -EINVAL;
+			goto abort;
+		}
+		break;
+	case INA238_POWER_LIMIT:
+		/*
+		 * Compared against the 24-bit power register, lower 8-bits are
+		 * truncated. Same conversion to/from uW as POWER register.
+		 */
+		regval = div_u64(val * 5 * data->rshunt,
+				 1000 * INA238_FIXED_SHUNT) >> 8; > +		if (regval > U16_MAX || regval < 0) {
+			ret = -EINVAL;
+			goto abort;
+		}
+		break;
+	case INA238_TEMP_LIMIT:
+		/* Bits 15-4 of register */
+		regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
+		if (regval > S16_MAX || regval < S16_MIN) {
+			ret = -EINVAL;
+			goto abort;
+		}
+		regval = regval & 0xfff0;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
+
+	mutex_lock(&data->config_lock);
+
+	ret = regmap_write(data->regmap, attr->index, regval);
+	if (ret < 0)
+		goto abort;
+
+	ret = count;
+abort:
+	mutex_unlock(&data->config_lock);
+	return ret;
+}
+
+/* shunt voltage */
+static SENSOR_DEVICE_ATTR_RO(in0_input, ina238_value, INA238_SHUNT_VOLTAGE);
+/* shunt voltage over/under alert */
+static SENSOR_DEVICE_ATTR_RW(in0_crit, ina238_alert, INA238_SHUNT_OVER_VOLTAGE);
+static SENSOR_DEVICE_ATTR_RW(in0_lcrit, ina238_alert,
+			     INA238_SHUNT_UNDER_VOLTAGE);
+
+/* bus voltage */
+static SENSOR_DEVICE_ATTR_RO(in1_input, ina238_value, INA238_BUS_VOLTAGE);
+/* bus voltage over/under alert */
+static SENSOR_DEVICE_ATTR_RW(in1_crit, ina238_alert, INA238_BUS_OVER_VOLTAGE);
+static SENSOR_DEVICE_ATTR_RW(in1_lcrit, ina238_alert, INA238_BUS_UNDER_VOLTAGE);
+
+/* calculated current */
+static SENSOR_DEVICE_ATTR_RO(curr1_input, ina238_value, INA238_CURRENT);
+
+/* calculated power */
+static SENSOR_DEVICE_ATTR_RO(power1_input, ina238_value, INA238_POWER);
+static SENSOR_DEVICE_ATTR_RW(power1_crit, ina238_alert, INA238_POWER_LIMIT);
+
+/* die temperature */
+static SENSOR_DEVICE_ATTR_RO(temp1_input, ina238_value, INA238_DIE_TEMP);
+static SENSOR_DEVICE_ATTR_RW(temp1_crit, ina238_alert, INA238_TEMP_LIMIT);
+
+/* shunt resistance */
+static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina238_shunt,
+			     INA238_SHUNT_CALIBRATION);
+
+static struct attribute *ina238_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in0_crit.dev_attr.attr,
+	&sensor_dev_attr_in0_lcrit.dev_attr.attr,

Any special reason for using crit / lcrit instead of max/min ?

+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_crit.dev_attr.attr,
+	&sensor_dev_attr_in1_lcrit.dev_attr.attr,
+	&sensor_dev_attr_curr1_input.dev_attr.attr,
+	&sensor_dev_attr_power1_input.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ina238);
+

Any reason for not supporting alarm attributes ?

+static int ina238_probe(struct i2c_client *client)
+{
+	struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct ina238_data *data;
+	u32 val;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	/* set the device type */
+	mutex_init(&data->config_lock);
+
+	ina238_regmap_config.max_register = INA238_REGISTERS;

Why is this done here instead of preinitializing it ?
ina238_regmap_config should really be const if possible.

+	data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	/* load shunt value */
+	val = INA238_RSHUNT_DEFAULT;
+	if (device_property_read_u32(dev, "shunt-resistor", &val) < 0 && pdata)
+		val = pdata->shunt_uohms;
+	ret = ina238_set_shunt(dev, data, val);
+	if (ret) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return ret;
+	}
+
+	/* Setup CONFIG register */
+	ret = regmap_write(data->regmap, INA238_CONFIG, INA238_CONFIG_DEFAULT);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+
+	/* Setup ADC_CONFIG register */
+	ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
+			   INA238_ADC_CONFIG_DEFAULT);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+
+	/* Setup SHUNT_CALIBRATION register with fixed value */
+	ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
+			   INA238_CALIBRATION_VALUE);

Those preinitializations make me wonder if there should be devicetree
properties for at least some of the data.

+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+							   data, ina238_groups);

Please rework the driver to use devm_hwmon_device_register_with_info().

+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
+		 client->name, data->rshunt);
+
+	return 0;
+}
+
+static const struct i2c_device_id ina238_id[] = {
+	{ "ina238", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ina238_id);
+
+static const struct of_device_id __maybe_unused ina238_of_match[] = {
+	{ .compatible = "ti,ina238" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ina238_of_match);
+
+static struct i2c_driver ina238_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "ina238",
+		.of_match_table = of_match_ptr(ina238_of_match),
+	},
+	.probe_new	= ina238_probe,
+	.id_table	= ina238_id,
+};
+
+module_i2c_driver(ina238_driver);
+
+MODULE_AUTHOR("Nathan Rossi <nathan.rossi@xxxxxxxx>");
+MODULE_DESCRIPTION("ina238 driver");
+MODULE_LICENSE("GPL");
---
2.33.0





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux