[PATCH 0/2] Driver for TI INA238 I2C Power Monitor

[PATCH 0/2] Driver for TI INA238 I2C Power Monitor

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>


Nathan Rossi (2):
  dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string
  hwmon: Driver for Texas Instruments INA238

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml  |   1 +
 Documentation/hwmon/ina238.rst                |  57 +++
 drivers/hwmon/Kconfig                         |  12 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/ina238.c                        | 453 ++++++++++++++++++
 5 files changed, 524 insertions(+)
 create mode 100644 Documentation/hwmon/ina238.rst
 create mode 100644 drivers/hwmon/ina238.c
---
2.33.0

[PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Document the compatible string for the Texas Instruments INA238, this
device is a variant of the existing INA2xx devices and has the same
device tree bindings (shunt resistor).

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 6f0443322a..180573f26c 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -26,6 +26,7 @@ properties:
       - ti,ina226
       - ti,ina230
       - ti,ina231
+      - ti,ina238
 
   reg:
     maxItems: 1
---
2.33.0

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

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

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@digi.com>
---
 Documentation/hwmon/ina238.rst |  57 ++++++
 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@digi.com>
+
+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@digi.com>
+ */
+
+#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>
+
+#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);
+
+	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;
+}
+
+static ssize_t ina238_alert_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);
+	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,
+	&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);
+
+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;
+	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);
+	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);
+	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@digi.com>");
+MODULE_DESCRIPTION("ina238 driver");
+MODULE_LICENSE("GPL");
---
2.33.0

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

[Guenter Roeck]

On 10/24/21 7:58 PM, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> 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@digi.com>
> ---
>   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@digi.com>
> +
> +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@digi.com>
> + */
> +
> +#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@digi.com>");
> +MODULE_DESCRIPTION("ina238 driver");
> +MODULE_LICENSE("GPL");
> ---
> 2.33.0
> 

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

[Nathan Rossi]

On Mon, 25 Oct 2021 at 15:06, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/24/21 7:58 PM, Nathan Rossi wrote:
> > From: Nathan Rossi <nathan.rossi@digi.com>
> >
> > 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@digi.com>
> > ---
> >   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@digi.com>
> > +
> > +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@digi.com>
> > + */
> > +
> > +#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.

I do not have a specific use case for being able to change the shunt
resistor at run time. The only reason this behaviour is here is to
mirror the api that is provided by the ina2xx driver. Since as you
mention its unusual should I remove the write and leave the read?
Being able to determine the resistor value is useful if manually using
the shunt voltage. Though the shunt information could be obtained from
the device tree node?

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

Only to mirror the convention from the ina2xx driver. I will change
this to 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 ?

No reason, I will add them in the updated version.

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

This driver is based on the ina2xx driver, so this was copied. Will
move this to the struct preinit.

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

Yes, I did consider adding configuration for the conversion time and
sampling average as device tree properties. The existing ina2xx driver
handles configuring sampling average via the "update_interval" sysfs
attribute. Our use case does not require changing these at runtime so
did not implement the update_interval and was unsure if changes to
device tree bindings would make sense. Should these be device tree
properties? If yes, should the other ina drivers be updated to support
the properties?

>
> > +     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().

I will update with a v2 to address your comments.

Thanks,
Nathan

>
> > +     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@digi.com>");
> > +MODULE_DESCRIPTION("ina238 driver");
> > +MODULE_LICENSE("GPL");
> > ---
> > 2.33.0
> >
>

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

[Guenter Roeck]

On 10/24/21 11:27 PM, Nathan Rossi wrote:
[ ... ]
>> 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.
> 
> I do not have a specific use case for being able to change the shunt
> resistor at run time. The only reason this behaviour is here is to
> mirror the api that is provided by the ina2xx driver. Since as you
> mention its unusual should I remove the write and leave the read?
> Being able to determine the resistor value is useful if manually using
> the shunt voltage. Though the shunt information could be obtained from
> the device tree node?
> 

Please drop the attribute. There is already probe noise displaying it,
and it is contained in the devicetree data. If there is a use case,
the attribute can always be added later.

[ ... ]

>> Those preinitializations make me wonder if there should be devicetree
>> properties for at least some of the data.
> 
> Yes, I did consider adding configuration for the conversion time and
> sampling average as device tree properties. The existing ina2xx driver
> handles configuring sampling average via the "update_interval" sysfs
> attribute. Our use case does not require changing these at runtime so
> did not implement the update_interval and was unsure if changes to
> device tree bindings would make sense. Should these be device tree
> properties? If yes, should the other ina drivers be updated to support
> the properties?
> 

I wasn't specifically thinking about conversion time or sampling average,
but I do think it would be desirable to be able to configure all those
values via devicetree. The datasheet says "... allows for robust operation
in a variety of noisy environments", and that is definitely a system property.
ADCRANGE should also be configurable via devicetree. The same applies to MODE,
but that would add some complexity so I am not sure if you'd want to get
into that (it would require per-channel entries in devicetree to be able
to enable/disable each channel). FWIW, you _could_ also do that with
standard sysfs attributes if you want to ({in,curr,temp}X_enable, or
hwmon_{temp,in,curr}_enable in the with_info API). That can also be added
later if needed, so there is no need to do it now if it is not required
for your use case.

As for other ina drivers - that is a different question. I would not touch
those unless you have a use case (and a means to test the code). I'd also
consider it more important to convert those drivers to use the _with_info
API before implementing any other changes. There is also the added
complexity that we already have two drivers for those other chips (see
drivers/iio/adc/ina2xx-adc.c), and I'd rather have a better API
between IIO and HWMON and drop drivers/hwmon/ina2xx.c entirely than
making changes to it.

Can you possibly send me a register dump for the INA238 ? That would
enable me to write a module test for the driver.

Thanks,
Guenter

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

[Nathan Rossi]

On Tue, 26 Oct 2021 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/24/21 11:27 PM, Nathan Rossi wrote:
> [ ... ]
> >> 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.
> >
> > I do not have a specific use case for being able to change the shunt
> > resistor at run time. The only reason this behaviour is here is to
> > mirror the api that is provided by the ina2xx driver. Since as you
> > mention its unusual should I remove the write and leave the read?
> > Being able to determine the resistor value is useful if manually using
> > the shunt voltage. Though the shunt information could be obtained from
> > the device tree node?
> >
>
> Please drop the attribute. There is already probe noise displaying it,
> and it is contained in the devicetree data. If there is a use case,
> the attribute can always be added later.
>
> [ ... ]
>
> >> Those preinitializations make me wonder if there should be devicetree
> >> properties for at least some of the data.
> >
> > Yes, I did consider adding configuration for the conversion time and
> > sampling average as device tree properties. The existing ina2xx driver
> > handles configuring sampling average via the "update_interval" sysfs
> > attribute. Our use case does not require changing these at runtime so
> > did not implement the update_interval and was unsure if changes to
> > device tree bindings would make sense. Should these be device tree
> > properties? If yes, should the other ina drivers be updated to support
> > the properties?
> >
>
> I wasn't specifically thinking about conversion time or sampling average,
> but I do think it would be desirable to be able to configure all those
> values via devicetree. The datasheet says "... allows for robust operation
> in a variety of noisy environments", and that is definitely a system property.
> ADCRANGE should also be configurable via devicetree. The same applies to MODE,

I will add a property "ti,shunt-gain". Since the ina209, ina219,
ina220 use the PGA term which is the actual function being provided,
where ADCRANGE=0 is a /4 PGA for ina238 and the other devices have /1,
/2, /4, /8.

> but that would add some complexity so I am not sure if you'd want to get
> into that (it would require per-channel entries in devicetree to be able
> to enable/disable each channel). FWIW, you _could_ also do that with
> standard sysfs attributes if you want to ({in,curr,temp}X_enable, or
> hwmon_{temp,in,curr}_enable in the with_info API). That can also be added
> later if needed, so there is no need to do it now if it is not required
> for your use case.
>
> As for other ina drivers - that is a different question. I would not touch
> those unless you have a use case (and a means to test the code). I'd also
> consider it more important to convert those drivers to use the _with_info
> API before implementing any other changes. There is also the added
> complexity that we already have two drivers for those other chips (see
> drivers/iio/adc/ina2xx-adc.c), and I'd rather have a better API
> between IIO and HWMON and drop drivers/hwmon/ina2xx.c entirely than
> making changes to it.
>
> Can you possibly send me a register dump for the INA238 ? That would
> enable me to write a module test for the driver.

Dump of registers below. Other notes, upper two bits are ignored so
the address space wraps at 0x40, etc. The undocumented/unused
registers between 0x11 and 0x3e are 0xffff.

power on, before probe:
0x00: 0x0000
0x01: 0xfb68
0x02: 0x1000
0x03: 0xffff
0x04: 0x087c
0x05: 0x0e77
0x06: 0x0f10
0x07: 0x087c
0x08: 0x01eac3
0x09: 0xffff
0x10: 0x7ff0
0x11: 0xffff
0x3e: 0x5449
0x3f: 0x2381

after probe, actual state of device ~10mV shunt voltage, 20 mOhm
shunt, ~12V bus, ~6W power, ~30C temperature
0x00: 0x0000
0x01: 0xfb6a
0x02: 0x4000
0x03: 0xffff
0x04: 0x087c
0x05: 0x0e77
0x06: 0x0f10
0x07: 0x087c
0x08: 0x01eac3
0x09: 0xffff
0x10: 0x7ff0
0x11: 0xffff
0x3e: 0x5449
0x3f: 0x2381

Thanks,
Nathan

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

[kernel test robot]

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

Hi Nathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on v5.15-rc7 next-20211026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-r006-20211027 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/aafb1ad1e44aa2604f4d8cd8db258d36fcefadfc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
        git checkout aafb1ad1e44aa2604f4d8cd8db258d36fcefadfc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/ina238.c:297:2: warning: variable 'regval' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
           default:
           ^~~~~~~
   drivers/hwmon/ina238.c:304:48: note: uninitialized use occurs here
           ret = regmap_write(data->regmap, attr->index, regval);
                                                         ^~~~~~
   drivers/hwmon/ina238.c:250:12: note: initialize the variable 'regval' to silence this warning
           int regval;
                     ^
                      = 0
   1 warning generated.


vim +/regval +297 drivers/hwmon/ina238.c

   242	
   243	static ssize_t ina238_alert_store(struct device *dev,
   244					  struct device_attribute *da,
   245					  const char *buf, size_t count)
   246	{
   247		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
   248		struct ina238_data *data = dev_get_drvdata(dev);
   249		long long val;
   250		int regval;
   251		int ret;
   252	
   253		ret = kstrtoll(buf, 10, &val);
   254		if (ret < 0)
   255			return ret;
   256	
   257		/* convert decimal to register value */
   258		switch (attr->index) {
   259		case INA238_SHUNT_OVER_VOLTAGE:
   260		case INA238_SHUNT_UNDER_VOLTAGE:
   261			/* signed */
   262			regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
   263			if (regval > S16_MAX || regval < S16_MIN) {
   264				ret = -EINVAL;
   265				goto abort;
   266			}
   267			break;
   268		case INA238_BUS_OVER_VOLTAGE:
   269		case INA238_BUS_UNDER_VOLTAGE:
   270			regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
   271			if (regval > U16_MAX || regval < 0) {
   272				ret = -EINVAL;
   273				goto abort;
   274			}
   275			break;
   276		case INA238_POWER_LIMIT:
   277			/*
   278			 * Compared against the 24-bit power register, lower 8-bits are
   279			 * truncated. Same conversion to/from uW as POWER register.
   280			 */
   281			regval = div_u64(val * 5 * data->rshunt,
   282					 1000 * INA238_FIXED_SHUNT) >> 8;
   283			if (regval > U16_MAX || regval < 0) {
   284				ret = -EINVAL;
   285				goto abort;
   286			}
   287			break;
   288		case INA238_TEMP_LIMIT:
   289			/* Bits 15-4 of register */
   290			regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
   291			if (regval > S16_MAX || regval < S16_MIN) {
   292				ret = -EINVAL;
   293				goto abort;
   294			}
   295			regval = regval & 0xfff0;
   296			break;
 > 297		default:
   298			WARN_ON_ONCE(1);
   299			break;
   300		}
   301	
   302		mutex_lock(&data->config_lock);
   303	
   304		ret = regmap_write(data->regmap, attr->index, regval);
   305		if (ret < 0)
   306			goto abort;
   307	
   308		ret = count;
   309	abort:
   310		mutex_unlock(&data->config_lock);
   311		return ret;
   312	}
   313	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31277 bytes --]

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

[kernel test robot]

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

Hi Nathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on v5.15-rc7 next-20211026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-r006-20211027 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/aafb1ad1e44aa2604f4d8cd8db258d36fcefadfc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
        git checkout aafb1ad1e44aa2604f4d8cd8db258d36fcefadfc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/ina238.c:297:2: warning: variable 'regval' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
           default:
           ^~~~~~~
   drivers/hwmon/ina238.c:304:48: note: uninitialized use occurs here
           ret = regmap_write(data->regmap, attr->index, regval);
                                                         ^~~~~~
   drivers/hwmon/ina238.c:250:12: note: initialize the variable 'regval' to silence this warning
           int regval;
                     ^
                      = 0
   1 warning generated.


vim +/regval +297 drivers/hwmon/ina238.c

   242	
   243	static ssize_t ina238_alert_store(struct device *dev,
   244					  struct device_attribute *da,
   245					  const char *buf, size_t count)
   246	{
   247		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
   248		struct ina238_data *data = dev_get_drvdata(dev);
   249		long long val;
   250		int regval;
   251		int ret;
   252	
   253		ret = kstrtoll(buf, 10, &val);
   254		if (ret < 0)
   255			return ret;
   256	
   257		/* convert decimal to register value */
   258		switch (attr->index) {
   259		case INA238_SHUNT_OVER_VOLTAGE:
   260		case INA238_SHUNT_UNDER_VOLTAGE:
   261			/* signed */
   262			regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
   263			if (regval > S16_MAX || regval < S16_MIN) {
   264				ret = -EINVAL;
   265				goto abort;
   266			}
   267			break;
   268		case INA238_BUS_OVER_VOLTAGE:
   269		case INA238_BUS_UNDER_VOLTAGE:
   270			regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
   271			if (regval > U16_MAX || regval < 0) {
   272				ret = -EINVAL;
   273				goto abort;
   274			}
   275			break;
   276		case INA238_POWER_LIMIT:
   277			/*
   278			 * Compared against the 24-bit power register, lower 8-bits are
   279			 * truncated. Same conversion to/from uW as POWER register.
   280			 */
   281			regval = div_u64(val * 5 * data->rshunt,
   282					 1000 * INA238_FIXED_SHUNT) >> 8;
   283			if (regval > U16_MAX || regval < 0) {
   284				ret = -EINVAL;
   285				goto abort;
   286			}
   287			break;
   288		case INA238_TEMP_LIMIT:
   289			/* Bits 15-4 of register */
   290			regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
   291			if (regval > S16_MAX || regval < S16_MIN) {
   292				ret = -EINVAL;
   293				goto abort;
   294			}
   295			regval = regval & 0xfff0;
   296			break;
 > 297		default:
   298			WARN_ON_ONCE(1);
   299			break;
   300		}
   301	
   302		mutex_lock(&data->config_lock);
   303	
   304		ret = regmap_write(data->regmap, attr->index, regval);
   305		if (ret < 0)
   306			goto abort;
   307	
   308		ret = count;
   309	abort:
   310		mutex_unlock(&data->config_lock);
   311		return ret;
   312	}
   313	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31277 bytes --]

[PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Changes in v2:
- Added device tree binding for ti,shunt-gain to specify the target
  ADCRANGE for the ina238
- Reworked ina238 driver to use hwmon_chip_info API, and addressed
  various review comments

Nathan Rossi (3):
  dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string
  dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property
  hwmon: Driver for Texas Instruments INA238

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml  |   7 +
 Documentation/hwmon/ina238.rst                |  56 ++
 Documentation/hwmon/index.rst                 |   1 +
 drivers/hwmon/Kconfig                         |  12 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/ina238.c                        | 672 ++++++++++++++++++
 6 files changed, 749 insertions(+)
 create mode 100644 Documentation/hwmon/ina238.rst
 create mode 100644 drivers/hwmon/ina238.c
---
2.33.0

[PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

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@digi.com>
---
Changes in v2:
- Add ina238 documentation to hwmon/index
- Remove unused header includes and sort
- Set regmap_config max_register in struct initialization
- Remove shunt-resistor attribute and associated functions
- Rename crit/lcrit attributes to max/min
- Rework to use the hwmon_chip_info API
- Add max_alarm and min_alarm channels and associated alert/alarm config
- Add device tree ti,shunt-gain property use and ADCRANGE setup
---
 Documentation/hwmon/ina238.rst |  56 ++++
 Documentation/hwmon/index.rst  |   1 +
 drivers/hwmon/Kconfig          |  12 +
 drivers/hwmon/Makefile         |   1 +
 drivers/hwmon/ina238.c         | 672 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 742 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..d9f4799844
--- /dev/null
+++ b/Documentation/hwmon/ina238.rst
@@ -0,0 +1,56 @@
+.. 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@digi.com>
+
+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_min			Minimum shunt voltage threshold (mV)
+in0_min_alarm		Minimum shunt voltage alarm
+in0_max			Maximum shunt voltage threshold (mV)
+in0_max_alarm		Maximum shunt voltage alarm
+
+in1_input		Bus voltage (mV)
+in1_min			Minimum bus voltage threshold (mV)
+in1_min_alarm		Minimum shunt voltage alarm
+in1_max			Maximum bus voltage threshold (mV)
+in1_max_alarm		Maximum shunt voltage alarm
+
+power1_input		Power measurement (uW)
+power1_max		Maximum power threshold (uW)
+power1_max_alarm	Maximum power alarm
+
+curr1_input		Current measurement (mA)
+
+temp1_input		Die temperature measurement (mC)
+temp1_max		Maximum die temperature threshold (mC)
+temp1_max_alarm		Maximum die temperature alarm
+======================= =======================================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 7046bf1870..6f30c8c9c7 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -76,6 +76,7 @@ Hardware Monitoring Kernel Drivers
    ibmpowernv
    ina209
    ina2xx
+   ina238
    ina3221
    intel-m10-bmc-hwmon
    ir35221
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7fde4c6e1e..21aff4cef7 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 alarm 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..f9d031bdfe
--- /dev/null
+++ b/drivers/hwmon/ina238.c
@@ -0,0 +1,672 @@
+// 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@digi.com>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#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_CONFIG_ADCRANGE		BIT(4)
+
+#define INA238_DIAG_ALERT_TMPOL		BIT(7)
+#define INA238_DIAG_ALERT_SHNTOL	BIT(6)
+#define INA238_DIAG_ALERT_SHNTUL	BIT(5)
+#define INA238_DIAG_ALERT_BUSOL		BIT(4)
+#define INA238_DIAG_ALERT_BUSUL		BIT(3)
+#define INA238_DIAG_ALERT_POL		BIT(2)
+
+#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
+/* Configure alerts to be based on averaged value (SLOWALERT) */
+#define INA238_DIAG_ALERT_DEFAULT	0x2000
+/*
+ * 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, gain = 4).
+ * 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 / 4 * gain
+ *  Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain
+ */
+#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 = {
+	.max_register = INA238_REGISTERS,
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
+struct ina238_data {
+	struct i2c_client *client;
+	struct mutex config_lock;
+	struct regmap *regmap;
+	u32 rshunt;
+	u32 gain;
+};
+
+static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
+{
+	u8 data[3];
+	int err;
+
+	/* 24-bit register read */
+	err = i2c_smbus_read_i2c_block_data(client, reg, 3, data);
+	if (err != 3)
+		return err;
+	*val = (data[0] << 16) | (data[1] << 8) | data[2];
+
+	return 0;
+}
+
+static int ina238_read_in(struct device *dev, u32 attr, int channel,
+			  long *val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int reg, mask;
+	int regval;
+	int err;
+
+	switch (channel) {
+	case 0:
+		switch (attr) {
+		case hwmon_in_input:
+			reg = INA238_SHUNT_VOLTAGE;
+			break;
+		case hwmon_in_max:
+			reg = INA238_SHUNT_OVER_VOLTAGE;
+			break;
+		case hwmon_in_min:
+			reg = INA238_SHUNT_UNDER_VOLTAGE;
+			break;
+		case hwmon_in_max_alarm:
+			reg = INA238_DIAG_ALERT;
+			mask = INA238_DIAG_ALERT_SHNTOL;
+			break;
+		case hwmon_in_min_alarm:
+			reg = INA238_DIAG_ALERT;
+			mask = INA238_DIAG_ALERT_SHNTUL;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case 1:
+		switch (attr) {
+		case hwmon_in_input:
+			reg = INA238_BUS_VOLTAGE;
+			break;
+		case hwmon_in_max:
+			reg = INA238_BUS_OVER_VOLTAGE;
+			break;
+		case hwmon_in_min:
+			reg = INA238_BUS_UNDER_VOLTAGE;
+			break;
+		case hwmon_in_max_alarm:
+			reg = INA238_DIAG_ALERT;
+			mask = INA238_DIAG_ALERT_BUSOL;
+			break;
+		case hwmon_in_min_alarm:
+			reg = INA238_DIAG_ALERT;
+			mask = INA238_DIAG_ALERT_BUSUL;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(data->regmap, reg, &regval);
+	if (err < 0)
+		return err;
+
+	switch (attr) {
+	case hwmon_in_input:
+	case hwmon_in_max:
+	case hwmon_in_min:
+		/* signed register, value in mV */
+		if (channel == 0)
+			/* gain of 1 -> LSB / 4 */
+			*val = div_s64((s16)regval * INA238_SHUNT_VOLTAGE_LSB,
+				       1000 * (4 - data->gain + 1));
+		else
+			*val = div_s64((s16)regval * INA238_BUS_VOLTAGE_LSB,
+				       1000);
+		break;
+	case hwmon_in_max_alarm:
+	case hwmon_in_min_alarm:
+		*val = regval & mask ? 1 : 0;
+		break;
+	}
+
+	return 0;
+}
+
+static int ina238_write_in(struct device *dev, u32 attr, int channel,
+			   long val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int regval;
+
+	if (attr != hwmon_in_max && attr != hwmon_in_min)
+		return -EOPNOTSUPP;
+
+	/* convert decimal to register value */
+	switch (channel) {
+	case 0:
+		/* signed */
+		regval = div_s64((s64)val * 1000LL * (4 - data->gain + 1),
+				 INA238_SHUNT_VOLTAGE_LSB);
+		if (regval > S16_MAX || regval < S16_MIN)
+			return -EINVAL;
+
+		switch (attr) {
+		case hwmon_in_max:
+			return regmap_write(data->regmap,
+					    INA238_SHUNT_OVER_VOLTAGE, regval);
+		case hwmon_in_min:
+			return regmap_write(data->regmap,
+					    INA238_SHUNT_UNDER_VOLTAGE, regval);
+		default:
+			return -EOPNOTSUPP;
+		}
+	case 1:
+		regval = div_u64((u64)val * 1000ULL, INA238_BUS_VOLTAGE_LSB);
+		if (regval > U16_MAX || regval < 0)
+			return -EINVAL;
+
+		switch (attr) {
+		case hwmon_in_max:
+			return regmap_write(data->regmap,
+					    INA238_BUS_OVER_VOLTAGE, regval);
+		case hwmon_in_min:
+			return regmap_write(data->regmap,
+					    INA238_BUS_UNDER_VOLTAGE, regval);
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int ina238_read_current(struct device *dev, u32 attr, int channel,
+			       long *val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int regval;
+	int err;
+
+	if (channel != 0)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_curr_input:
+		err = regmap_read(data->regmap, INA238_CURRENT, &regval);
+		if (err < 0)
+			return err;
+
+		/* Signed register, fixed 1mA current lsb. result in mA */
+		*val = div_s64((s16)regval * INA238_FIXED_SHUNT * data->gain,
+			       data->rshunt * 4);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int ina238_read_power(struct device *dev, u32 attr, int channel,
+			     long *val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	long long power;
+	int regval;
+	int err;
+
+	if (channel != 0)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_power_input:
+		err = ina238_read_reg24(data->client, INA238_POWER, &regval);
+		if (err)
+			return err;
+
+		/* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
+		power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
+				data->gain, 20 * data->rshunt);
+		/* Clamp value to maximum value of long */
+		*val = clamp_val(power, 0, LONG_MAX);
+		break;
+	case hwmon_power_max:
+		err = regmap_read(data->regmap, INA238_POWER_LIMIT, &regval);
+		if (err)
+			return err;
+
+		/*
+		 * Truncated 24-bit compare register, lower 8-bits are
+		 * truncated. Same conversion to/from uW as POWER register.
+		 */
+		power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
+			       data->gain, 20 * data->rshunt);
+		/* Clamp value to maximum value of long */
+		*val = clamp_val(power, 0, LONG_MAX);
+		break;
+	case hwmon_power_max_alarm:
+		err = regmap_read(data->regmap, INA238_DIAG_ALERT, &regval);
+		if (err)
+			return err;
+
+		*val = regval & INA238_DIAG_ALERT_POL ? 1 : 0;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int ina238_write_power(struct device *dev, u32 attr, int channel,
+			      long val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int regval;
+
+	if (channel != 0 || attr != hwmon_power_max)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Compared against the 24-bit power register, lower 8-bits are
+	 * truncated. Same conversion to/from uW as POWER register.
+	 */
+	regval = div_u64((u64)val * 20ULL * data->rshunt,
+			 1000ULL * INA238_FIXED_SHUNT * (u64)data->gain);
+	regval = regval >> 8;
+	if (regval > U16_MAX || regval < 0)
+		return -EINVAL;
+
+	return regmap_write(data->regmap, INA238_POWER_LIMIT, regval);
+}
+
+static int ina238_read_temp(struct device *dev, u32 attr, int channel,
+			    long *val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int regval;
+	int err;
+
+	if (channel != 0)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		err = regmap_read(data->regmap, INA238_DIE_TEMP, &regval);
+		if (err)
+			return err;
+
+		/* Signed, bits 15-4 of register, result in mC */
+		*val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+		break;
+	case hwmon_temp_max:
+		err = regmap_read(data->regmap, INA238_TEMP_LIMIT, &regval);
+		if (err)
+			return err;
+
+		/* Signed, bits 15-4 of register, result in mC */
+		*val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+		break;
+	case hwmon_temp_max_alarm:
+		err = regmap_read(data->regmap, INA238_DIAG_ALERT, &regval);
+		if (err)
+			return err;
+
+		*val = regval & INA238_DIAG_ALERT_TMPOL ? 1 : 0;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int ina238_write_temp(struct device *dev, u32 attr, int channel,
+			     long val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int regval;
+
+	if (channel != 0 || attr != hwmon_temp_max)
+		return -EOPNOTSUPP;
+
+	/* Signed, bits 15-4 of register */
+	regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
+	if (regval > S16_MAX || regval < S16_MIN)
+		return -EINVAL;
+	regval = regval & 0xfff0;
+
+	return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval);
+}
+
+static int ina238_read(struct device *dev, enum hwmon_sensor_types type,
+		       u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_in:
+		return ina238_read_in(dev, attr, channel, val);
+	case hwmon_curr:
+		return ina238_read_current(dev, attr, channel, val);
+	case hwmon_power:
+		return ina238_read_power(dev, attr, channel, val);
+	case hwmon_temp:
+		return ina238_read_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int ina238_write(struct device *dev, enum hwmon_sensor_types type,
+		       u32 attr, int channel, long val)
+{
+	struct ina238_data *data = dev_get_drvdata(dev);
+	int err;
+
+	mutex_lock(&data->config_lock);
+
+	switch (type) {
+	case hwmon_in:
+		err = ina238_write_in(dev, attr, channel, val);
+		break;
+	case hwmon_power:
+		err = ina238_write_power(dev, attr, channel, val);
+		break;
+	case hwmon_temp:
+		err = ina238_write_temp(dev, attr, channel, val);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	mutex_unlock(&data->config_lock);
+	return err;
+}
+
+static umode_t ina238_is_visible(const void *drvdata,
+				 enum hwmon_sensor_types type,
+				 u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		if (channel != 0 && channel != 1)
+			return 0;
+
+		switch (attr) {
+		case hwmon_in_input:
+		case hwmon_in_max_alarm:
+		case hwmon_in_min_alarm:
+			return 0444;
+		case hwmon_in_max:
+		case hwmon_in_min:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_curr:
+		if (channel != 0)
+			return 0;
+
+		switch (attr) {
+		case hwmon_curr_input:
+			return 0444;
+		default:
+			return 0;
+		}
+	case hwmon_power:
+		if (channel != 0)
+			return 0;
+
+		switch (attr) {
+		case hwmon_power_input:
+		case hwmon_power_max_alarm:
+			return 0444;
+		case hwmon_power_max:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_temp:
+		if (channel != 0)
+			return 0;
+
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_max_alarm:
+			return 0444;
+		case hwmon_temp_max:
+			return 0644;
+		default:
+			return 0;
+		}
+	default:
+		return 0;
+	}
+}
+
+#define INA238_HWMON_IN_CONFIG (HWMON_I_INPUT | \
+				HWMON_I_MAX | HWMON_I_MAX_ALARM | \
+				HWMON_I_MIN | HWMON_I_MIN_ALARM)
+
+static const struct hwmon_channel_info *ina238_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   /* 0: shunt voltage */
+			   INA238_HWMON_IN_CONFIG,
+			   /* 1: bus voltage */
+			   INA238_HWMON_IN_CONFIG),
+	HWMON_CHANNEL_INFO(curr,
+			   /* 0: current through shunt */
+			   HWMON_C_INPUT),
+	HWMON_CHANNEL_INFO(power,
+			   /* 0: power */
+			   HWMON_P_INPUT | HWMON_P_MAX | HWMON_P_MAX_ALARM),
+	HWMON_CHANNEL_INFO(temp,
+			   /* 0: die temperature */
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_ALARM),
+	NULL
+};
+
+static const struct hwmon_ops ina238_hwmon_ops = {
+	.is_visible = ina238_is_visible,
+	.read = ina238_read,
+	.write = ina238_write,
+};
+
+static const struct hwmon_chip_info ina238_chip_info = {
+	.ops = &ina238_hwmon_ops,
+	.info = ina238_info,
+};
+
+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;
+	int config;
+	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);
+
+	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 */
+	data->rshunt = INA238_RSHUNT_DEFAULT;
+	if (device_property_read_u32(dev, "shunt-resistor", &data->rshunt) < 0 &&
+	    pdata)
+		data->rshunt = pdata->shunt_uohms;
+	if (data->rshunt == 0) {
+		dev_err(dev, "invalid shunt resister value %u\n", data->rshunt);
+		return -EINVAL;
+	}
+
+	/* load shunt gain value */
+	if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
+		data->gain = 4; /* Default of ADCRANGE = 0 */
+	if (data->gain != 1 && data->gain != 4) {
+		dev_err(dev, "invalid shunt gain value %u\n", data->gain);
+		return -EINVAL;
+	}
+
+	/* Setup CONFIG register */
+	config = INA238_CONFIG_DEFAULT;
+	if (data->gain == 1)
+		config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
+	ret = regmap_write(data->regmap, INA238_CONFIG, config);
+	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);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+
+	/* Setup alert/alarm configuration */
+	ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
+			   INA238_DIAG_ALERT_DEFAULT);
+	if (ret < 0) {
+		dev_err(dev, "error configuring the device: %d\n", ret);
+		return -ENODEV;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							 &ina238_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "power monitor %s (Rshunt = %u uOhm, gain = %u)\n",
+		 client->name, data->rshunt, data->gain);
+
+	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@digi.com>");
+MODULE_DESCRIPTION("ina238 driver");
+MODULE_LICENSE("GPL");
---
2.33.0

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

[Guenter Roeck]

On 10/27/21 12:42 AM, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> 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@digi.com>
> ---
> Changes in v2:
> - Add ina238 documentation to hwmon/index
> - Remove unused header includes and sort
> - Set regmap_config max_register in struct initialization
> - Remove shunt-resistor attribute and associated functions
> - Rename crit/lcrit attributes to max/min
> - Rework to use the hwmon_chip_info API
> - Add max_alarm and min_alarm channels and associated alert/alarm config
> - Add device tree ti,shunt-gain property use and ADCRANGE setup
> ---
>   Documentation/hwmon/ina238.rst |  56 ++++
>   Documentation/hwmon/index.rst  |   1 +
>   drivers/hwmon/Kconfig          |  12 +
>   drivers/hwmon/Makefile         |   1 +
>   drivers/hwmon/ina238.c         | 672 +++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 742 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..d9f4799844
> --- /dev/null
> +++ b/Documentation/hwmon/ina238.rst
> @@ -0,0 +1,56 @@
> +.. 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@digi.com>
> +
> +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_min			Minimum shunt voltage threshold (mV)
> +in0_min_alarm		Minimum shunt voltage alarm
> +in0_max			Maximum shunt voltage threshold (mV)
> +in0_max_alarm		Maximum shunt voltage alarm
> +
> +in1_input		Bus voltage (mV)
> +in1_min			Minimum bus voltage threshold (mV)
> +in1_min_alarm		Minimum shunt voltage alarm
> +in1_max			Maximum bus voltage threshold (mV)
> +in1_max_alarm		Maximum shunt voltage alarm
> +
> +power1_input		Power measurement (uW)
> +power1_max		Maximum power threshold (uW)
> +power1_max_alarm	Maximum power alarm
> +
> +curr1_input		Current measurement (mA)
> +
> +temp1_input		Die temperature measurement (mC)
> +temp1_max		Maximum die temperature threshold (mC)
> +temp1_max_alarm		Maximum die temperature alarm
> +======================= =======================================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 7046bf1870..6f30c8c9c7 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -76,6 +76,7 @@ Hardware Monitoring Kernel Drivers
>      ibmpowernv
>      ina209
>      ina2xx
> +   ina238
>      ina3221
>      intel-m10-bmc-hwmon
>      ir35221
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7fde4c6e1e..21aff4cef7 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 alarm 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..f9d031bdfe
> --- /dev/null
> +++ b/drivers/hwmon/ina238.c
> @@ -0,0 +1,672 @@
> +// 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@digi.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon-sysfs.h>

Unnecessary.

> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#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_CONFIG_ADCRANGE		BIT(4)
> +
> +#define INA238_DIAG_ALERT_TMPOL		BIT(7)
> +#define INA238_DIAG_ALERT_SHNTOL	BIT(6)
> +#define INA238_DIAG_ALERT_SHNTUL	BIT(5)
> +#define INA238_DIAG_ALERT_BUSOL		BIT(4)
> +#define INA238_DIAG_ALERT_BUSUL		BIT(3)
> +#define INA238_DIAG_ALERT_POL		BIT(2)
> +
> +#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
> +/* Configure alerts to be based on averaged value (SLOWALERT) */
> +#define INA238_DIAG_ALERT_DEFAULT	0x2000
> +/*
> + * 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, gain = 4).
> + * 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 / 4 * gain
> + *  Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain
> + */
> +#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 = {
> +	.max_register = INA238_REGISTERS,
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +};
> +
> +struct ina238_data {
> +	struct i2c_client *client;
> +	struct mutex config_lock;
> +	struct regmap *regmap;
> +	u32 rshunt;
> +	u32 gain;
> +};
> +
> +static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
> +{
> +	u8 data[3];
> +	int err;
> +
> +	/* 24-bit register read */
> +	err = i2c_smbus_read_i2c_block_data(client, reg, 3, data);
> +	if (err != 3)
> +		return err;

i2c_smbus_read_i2c_block_data() can return 1 or 2, which should be reported as error.
You need something like

	if (err < 0)
		return err;
	if (err != 3)
		return -EIO;

> +	*val = (data[0] << 16) | (data[1] << 8) | data[2];
> +
> +	return 0;
> +}
> +
> +static int ina238_read_in(struct device *dev, u32 attr, int channel,
> +			  long *val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int reg, mask;
> +	int regval;
> +	int err;
> +
> +	switch (channel) {
> +	case 0:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			reg = INA238_SHUNT_VOLTAGE;
> +			break;
> +		case hwmon_in_max:
> +			reg = INA238_SHUNT_OVER_VOLTAGE;
> +			break;
> +		case hwmon_in_min:
> +			reg = INA238_SHUNT_UNDER_VOLTAGE;
> +			break;
> +		case hwmon_in_max_alarm:
> +			reg = INA238_DIAG_ALERT;
> +			mask = INA238_DIAG_ALERT_SHNTOL;
> +			break;
> +		case hwmon_in_min_alarm:
> +			reg = INA238_DIAG_ALERT;
> +			mask = INA238_DIAG_ALERT_SHNTUL;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case 1:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			reg = INA238_BUS_VOLTAGE;
> +			break;
> +		case hwmon_in_max:
> +			reg = INA238_BUS_OVER_VOLTAGE;
> +			break;
> +		case hwmon_in_min:
> +			reg = INA238_BUS_UNDER_VOLTAGE;
> +			break;
> +		case hwmon_in_max_alarm:
> +			reg = INA238_DIAG_ALERT;
> +			mask = INA238_DIAG_ALERT_BUSOL;
> +			break;
> +		case hwmon_in_min_alarm:
> +			reg = INA238_DIAG_ALERT;
> +			mask = INA238_DIAG_ALERT_BUSUL;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_read(data->regmap, reg, &regval);
> +	if (err < 0)
> +		return err;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +	case hwmon_in_max:
> +	case hwmon_in_min:
> +		/* signed register, value in mV */
> +		if (channel == 0)
> +			/* gain of 1 -> LSB / 4 */
> +			*val = div_s64((s16)regval * INA238_SHUNT_VOLTAGE_LSB,
> +				       1000 * (4 - data->gain + 1));
> +		else
> +			*val = div_s64((s16)regval * INA238_BUS_VOLTAGE_LSB,
> +				       1000);

Pleae check the use of 64-bit divide operations. The above operations
never overflow a long, not even a 32-bit long, and div_s64() is therefore
unencessary. I am quite sure the same applies to current read operations,
though I didn't check.

> +		break;
> +	case hwmon_in_max_alarm:
> +	case hwmon_in_min_alarm:
> +		*val = regval & mask ? 1 : 0;

		*val = !!(regval & mask);

> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ina238_write_in(struct device *dev, u32 attr, int channel,
> +			   long val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int regval;
> +
> +	if (attr != hwmon_in_max && attr != hwmon_in_min)
> +		return -EOPNOTSUPP;
> +
> +	/* convert decimal to register value */
> +	switch (channel) {
> +	case 0:
> +		/* signed */
> +		regval = div_s64((s64)val * 1000LL * (4 - data->gain + 1),
> +				 INA238_SHUNT_VOLTAGE_LSB);

I am quite sure the typecast is unnecessary here. Also, if you'd use clamp_val()
prior to the multiply/divide operation, I am quite sure you would not need
to use 64-bit divide operations.

> +		if (regval > S16_MAX || regval < S16_MIN)
> +			return -EINVAL;

We usually handle limit writes out of range with clamp_val() and
do not expect users to find the valid range for a given chip.

		regval = clamp_val(regval, S16_MIN, S16_MAX);

> +
> +		switch (attr) {
> +		case hwmon_in_max:
> +			return regmap_write(data->regmap,
> +					    INA238_SHUNT_OVER_VOLTAGE, regval);
> +		case hwmon_in_min:
> +			return regmap_write(data->regmap,
> +					    INA238_SHUNT_UNDER_VOLTAGE, regval);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	case 1:
> +		regval = div_u64((u64)val * 1000ULL, INA238_BUS_VOLTAGE_LSB);

This converts -1 to a really large value, which then overflows before being divided.
The result is more or less random. I think you'll need to use clamp_val first here.

> +		if (regval > U16_MAX || regval < 0)
> +			return -EINVAL;

		regval = clamp_val(regval, 0, U16_MAX);

> +
> +		switch (attr) {
> +		case hwmon_in_max:
> +			return regmap_write(data->regmap,
> +					    INA238_BUS_OVER_VOLTAGE, regval);
> +		case hwmon_in_min:
> +			return regmap_write(data->regmap,
> +					    INA238_BUS_UNDER_VOLTAGE, regval);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int ina238_read_current(struct device *dev, u32 attr, int channel,
> +			       long *val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int regval;
> +	int err;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		err = regmap_read(data->regmap, INA238_CURRENT, &regval);
> +		if (err < 0)
> +			return err;
> +
> +		/* Signed register, fixed 1mA current lsb. result in mA */
> +		*val = div_s64((s16)regval * INA238_FIXED_SHUNT * data->gain,
> +			       data->rshunt * 4);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ina238_read_power(struct device *dev, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	long long power;
> +	int regval;
> +	int err;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		err = ina238_read_reg24(data->client, INA238_POWER, &regval);
> +		if (err)
> +			return err;
> +
> +		/* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
> +		power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
> +				data->gain, 20 * data->rshunt);
> +		/* Clamp value to maximum value of long */
> +		*val = clamp_val(power, 0, LONG_MAX);
> +		break;
> +	case hwmon_power_max:
> +		err = regmap_read(data->regmap, INA238_POWER_LIMIT, &regval);
> +		if (err)
> +			return err;
> +
> +		/*
> +		 * Truncated 24-bit compare register, lower 8-bits are
> +		 * truncated. Same conversion to/from uW as POWER register.
> +		 */
> +		power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
> +			       data->gain, 20 * data->rshunt);
> +		/* Clamp value to maximum value of long */
> +		*val = clamp_val(power, 0, LONG_MAX);
> +		break;
> +	case hwmon_power_max_alarm:
> +		err = regmap_read(data->regmap, INA238_DIAG_ALERT, &regval);
> +		if (err)
> +			return err;
> +
> +		*val = regval & INA238_DIAG_ALERT_POL ? 1 : 0;

		*val = !!(regval & INA238_DIAG_ALERT_POL);

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ina238_write_power(struct device *dev, u32 attr, int channel,
> +			      long val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int regval;
> +
> +	if (channel != 0 || attr != hwmon_power_max)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Compared against the 24-bit power register, lower 8-bits are
> +	 * truncated. Same conversion to/from uW as POWER register.
> +	 */
> +	regval = div_u64((u64)val * 20ULL * data->rshunt,
> +			 1000ULL * INA238_FIXED_SHUNT * (u64)data->gain);

Same problem as above - negative values of val will have in unpredictable results.
The last typecat should be unnecessary.

> +	regval = regval >> 8;
> +	if (regval > U16_MAX || regval < 0)
> +		return -EINVAL;

	regval = clamp_val(...);

> +
> +	return regmap_write(data->regmap, INA238_POWER_LIMIT, regval);
> +}
> +
> +static int ina238_read_temp(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int regval;
> +	int err;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		err = regmap_read(data->regmap, INA238_DIE_TEMP, &regval);
> +		if (err)
> +			return err;
> +
> +		/* Signed, bits 15-4 of register, result in mC */
> +		*val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
> +		break;
> +	case hwmon_temp_max:
> +		err = regmap_read(data->regmap, INA238_TEMP_LIMIT, &regval);
> +		if (err)
> +			return err;
> +
> +		/* Signed, bits 15-4 of register, result in mC */
> +		*val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
> +		break;
> +	case hwmon_temp_max_alarm:
> +		err = regmap_read(data->regmap, INA238_DIAG_ALERT, &regval);
> +		if (err)
> +			return err;
> +
> +		*val = regval & INA238_DIAG_ALERT_TMPOL ? 1 : 0;

		*val = !!(regval & INA238_DIAG_ALERT_TMPOL);

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ina238_write_temp(struct device *dev, u32 attr, int channel,
> +			     long val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int regval;
> +
> +	if (channel != 0 || attr != hwmon_temp_max)
> +		return -EOPNOTSUPP;
> +
> +	/* Signed, bits 15-4 of register */
> +	regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
> +	if (regval > S16_MAX || regval < S16_MIN)
> +		return -EINVAL;

	regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;

> +	regval = regval & 0xfff0;
> +
> +	return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval);
> +}
> +
> +static int ina238_read(struct device *dev, enum hwmon_sensor_types type,
> +		       u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ina238_read_in(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return ina238_read_current(dev, attr, channel, val);
> +	case hwmon_power:
> +		return ina238_read_power(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return ina238_read_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int ina238_write(struct device *dev, enum hwmon_sensor_types type,
> +		       u32 attr, int channel, long val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	mutex_lock(&data->config_lock);
> +
> +	switch (type) {
> +	case hwmon_in:
> +		err = ina238_write_in(dev, attr, channel, val);
> +		break;
> +	case hwmon_power:
> +		err = ina238_write_power(dev, attr, channel, val);
> +		break;
> +	case hwmon_temp:
> +		err = ina238_write_temp(dev, attr, channel, val);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->config_lock);
> +	return err;
> +}
> +
> +static umode_t ina238_is_visible(const void *drvdata,
> +				 enum hwmon_sensor_types type,
> +				 u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		if (channel != 0 && channel != 1)
> +			return 0;

FWIW, channel checks are not really needed.

> +
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_max_alarm:
> +		case hwmon_in_min_alarm:
> +			return 0444;
> +		case hwmon_in_max:
> +		case hwmon_in_min:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_curr:
> +		if (channel != 0)
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_power:
> +		if (channel != 0)
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_power_input:
> +		case hwmon_power_max_alarm:
> +			return 0444;
> +		case hwmon_power_max:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_temp:
> +		if (channel != 0)
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_max_alarm:
> +			return 0444;
> +		case hwmon_temp_max:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	default:
> +		return 0;
> +	}
> +}
> +
> +#define INA238_HWMON_IN_CONFIG (HWMON_I_INPUT | \
> +				HWMON_I_MAX | HWMON_I_MAX_ALARM | \
> +				HWMON_I_MIN | HWMON_I_MIN_ALARM)
> +
> +static const struct hwmon_channel_info *ina238_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   /* 0: shunt voltage */
> +			   INA238_HWMON_IN_CONFIG,
> +			   /* 1: bus voltage */
> +			   INA238_HWMON_IN_CONFIG),
> +	HWMON_CHANNEL_INFO(curr,
> +			   /* 0: current through shunt */
> +			   HWMON_C_INPUT),
> +	HWMON_CHANNEL_INFO(power,
> +			   /* 0: power */
> +			   HWMON_P_INPUT | HWMON_P_MAX | HWMON_P_MAX_ALARM),
> +	HWMON_CHANNEL_INFO(temp,
> +			   /* 0: die temperature */
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_ALARM),
> +	NULL
> +};
> +
> +static const struct hwmon_ops ina238_hwmon_ops = {
> +	.is_visible = ina238_is_visible,
> +	.read = ina238_read,
> +	.write = ina238_write,
> +};
> +
> +static const struct hwmon_chip_info ina238_chip_info = {
> +	.ops = &ina238_hwmon_ops,
> +	.info = ina238_info,
> +};
> +
> +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;
> +	int config;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	/* set the device type */

Pointless comment. I don't even understand what it is supposed to mean.

> +	mutex_init(&data->config_lock);
> +
> +	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 */
> +	data->rshunt = INA238_RSHUNT_DEFAULT;
> +	if (device_property_read_u32(dev, "shunt-resistor", &data->rshunt) < 0 &&
> +	    pdata)

Drop the continuation line. Lines longer than 80 columns are fine when
improving readability, and that is definitely the case here.

> +		data->rshunt = pdata->shunt_uohms;
> +	if (data->rshunt == 0) {
> +		dev_err(dev, "invalid shunt resister value %u\n", data->rshunt);
> +		return -EINVAL;
> +	}
> +
> +	/* load shunt gain value */
> +	if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
> +		data->gain = 4; /* Default of ADCRANGE = 0 */
> +	if (data->gain != 1 && data->gain != 4) {
> +		dev_err(dev, "invalid shunt gain value %u\n", data->gain);
> +		return -EINVAL;
> +	}

I think ti,shunt-gain needs a better explanation in the .yaml file, for
people writing devicetree files to understand how the gain value converts
to a register value.

> +
> +	/* Setup CONFIG register */
> +	config = INA238_CONFIG_DEFAULT;
> +	if (data->gain == 1)
> +		config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
> +	ret = regmap_write(data->regmap, INA238_CONFIG, config);
> +	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);
> +	if (ret < 0) {
> +		dev_err(dev, "error configuring the device: %d\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	/* Setup alert/alarm configuration */
> +	ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
> +			   INA238_DIAG_ALERT_DEFAULT);
> +	if (ret < 0) {
> +		dev_err(dev, "error configuring the device: %d\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> +							 &ina238_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "power monitor %s (Rshunt = %u uOhm, gain = %u)\n",
> +		 client->name, data->rshunt, data->gain);
> +
> +	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@digi.com>");
> +MODULE_DESCRIPTION("ina238 driver");
> +MODULE_LICENSE("GPL");
> ---
> 2.33.0
> 

[PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Add a property to the binding to define the selected shunt voltage gain.
This specifies the range and accuracy that applies to the shunt circuit.
This property only applies to devices that have a selectable shunt
voltage range via PGA or ADCRANGE register configuration.

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
Changes in v2:
- Added binding for shunt-gain
---
 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 180573f26c..6a70e2fe9d 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -36,6 +36,12 @@ properties:
       Shunt resistor value in micro-Ohm.
     $ref: /schemas/types.yaml#/definitions/uint32
 
+  ti,shunt-gain:
+    description:
+      Programmable gain divisor for the shunt voltage accuracy and range. This
+      property only applies to devices that have configurable PGA/ADCRANGE.
+    enum: [1, 2, 4, 8]
+
 required:
   - compatible
   - reg
---
2.33.0

Re: [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property

[Rob Herring]

On Wed, 27 Oct 2021 07:42:12 +0000, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> Add a property to the binding to define the selected shunt voltage gain.
> This specifies the range and accuracy that applies to the shunt circuit.
> This property only applies to devices that have a selectable shunt
> voltage range via PGA or ADCRANGE register configuration.
> 
> Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
> ---
> Changes in v2:
> - Added binding for shunt-gain
> ---
>  Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml: properties:ti,shunt-gain: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('enum' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml: properties:ti,shunt-gain: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	1 is not of type 'string'
		hint: A vendor string property with exact values has an implicit type
	2 is not of type 'string'
		hint: A vendor string property with exact values has an implicit type
	4 is not of type 'string'
		hint: A vendor string property with exact values has an implicit type
	8 is not of type 'string'
		hint: A vendor string property with exact values has an implicit type
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml: ignoring, error in schema: properties: ti,shunt-gain
warning: no schema found in file: ./Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
Documentation/devicetree/bindings/hwmon/ti,ina2xx.example.dt.yaml:0:0: /example-0/i2c/power-sensor@44: failed to match any schema with compatible: ['ti,ina220']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1546789

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v2 1/3] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Document the compatible string for the Texas Instruments INA238, this
device is a variant of the existing INA2xx devices and has the same
device tree bindings (shunt resistor).

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 6f0443322a..180573f26c 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -26,6 +26,7 @@ properties:
       - ti,ina226
       - ti,ina230
       - ti,ina231
+      - ti,ina238
 
   reg:
     maxItems: 1
---
2.33.0

Re: [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor

[Guenter Roeck]

On 10/27/21 12:42 AM, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> Changes in v2:
> - Added device tree binding for ti,shunt-gain to specify the target
>    ADCRANGE for the ina238
> - Reworked ina238 driver to use hwmon_chip_info API, and addressed
>    various review comments
> 
> Nathan Rossi (3):
>    dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string
>    dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property
>    hwmon: Driver for Texas Instruments INA238
> 
>   .../devicetree/bindings/hwmon/ti,ina2xx.yaml  |   7 +
>   Documentation/hwmon/ina238.rst                |  56 ++
>   Documentation/hwmon/index.rst                 |   1 +
>   drivers/hwmon/Kconfig                         |  12 +
>   drivers/hwmon/Makefile                        |   1 +
>   drivers/hwmon/ina238.c                        | 672 ++++++++++++++++++
>   6 files changed, 749 insertions(+)
>   create mode 100644 Documentation/hwmon/ina238.rst
>   create mode 100644 drivers/hwmon/ina238.c
> ---
> 2.33.0
> 
General comment: Please never send a patch series as reply to a previous one.

Guenter

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

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211025025805.618566-2-nathan@nathanrossi.com>
References: <20211025025805.618566-2-nathan@nathanrossi.com>
TO: Nathan Rossi <nathan@nathanrossi.com>
TO: linux-hwmon(a)vger.kernel.org
TO: linux-doc(a)vger.kernel.org
TO: linux-kernel(a)vger.kernel.org
CC: Nathan Rossi <nathan@nathanrossi.com>
CC: Jean Delvare <jdelvare@suse.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Jonathan Corbet <corbet@lwn.net>

Hi Nathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on v5.15-rc7 next-20211027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: x86_64-randconfig-m001-20211027 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/ina238.c:304 ina238_alert_store() error: uninitialized symbol 'regval'.

vim +/regval +304 drivers/hwmon/ina238.c

aafb1ad1e44aa2 Nathan Rossi 2021-10-25  242  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  243  static ssize_t ina238_alert_store(struct device *dev,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  244  				  struct device_attribute *da,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  245  				  const char *buf, size_t count)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  246  {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  247  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  248  	struct ina238_data *data = dev_get_drvdata(dev);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  249  	long long val;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  250  	int regval;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  251  	int ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  252  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  253  	ret = kstrtoll(buf, 10, &val);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  254  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  255  		return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  256  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  257  	/* convert decimal to register value */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  258  	switch (attr->index) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  259  	case INA238_SHUNT_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  260  	case INA238_SHUNT_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  261  		/* signed */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  262  		regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  263  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  264  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  265  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  266  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  267  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  268  	case INA238_BUS_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  269  	case INA238_BUS_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  270  		regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  271  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  272  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  273  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  274  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  275  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  276  	case INA238_POWER_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  277  		/*
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  278  		 * Compared against the 24-bit power register, lower 8-bits are
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  279  		 * truncated. Same conversion to/from uW as POWER register.
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  280  		 */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  281  		regval = div_u64(val * 5 * data->rshunt,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  282  				 1000 * INA238_FIXED_SHUNT) >> 8;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  283  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  284  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  285  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  286  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  287  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  288  	case INA238_TEMP_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  289  		/* Bits 15-4 of register */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  290  		regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  291  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  292  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  293  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  294  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  295  		regval = regval & 0xfff0;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  296  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  297  	default:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  298  		WARN_ON_ONCE(1);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  299  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  300  	}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  301  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  302  	mutex_lock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  303  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25 @304  	ret = regmap_write(data->regmap, attr->index, regval);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  305  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  306  		goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  307  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  308  	ret = count;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  309  abort:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  310  	mutex_unlock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  311  	return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  312  }
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  313  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37432 bytes --]

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

[Dan Carpenter]

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

Hi Nathan,

url:    https://github.com/0day-ci/linux/commits/Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20211027 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/ina238.c:304 ina238_alert_store() error: uninitialized symbol 'regval'.

vim +/regval +304 drivers/hwmon/ina238.c

aafb1ad1e44aa2 Nathan Rossi 2021-10-25  243  static ssize_t ina238_alert_store(struct device *dev,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  244  				  struct device_attribute *da,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  245  				  const char *buf, size_t count)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  246  {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  247  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  248  	struct ina238_data *data = dev_get_drvdata(dev);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  249  	long long val;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  250  	int regval;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  251  	int ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  252  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  253  	ret = kstrtoll(buf, 10, &val);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  254  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  255  		return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  256  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  257  	/* convert decimal to register value */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  258  	switch (attr->index) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  259  	case INA238_SHUNT_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  260  	case INA238_SHUNT_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  261  		/* signed */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  262  		regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  263  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  264  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  265  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  266  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  267  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  268  	case INA238_BUS_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  269  	case INA238_BUS_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  270  		regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  271  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  272  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  273  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  274  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  275  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  276  	case INA238_POWER_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  277  		/*
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  278  		 * Compared against the 24-bit power register, lower 8-bits are
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  279  		 * truncated. Same conversion to/from uW as POWER register.
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  280  		 */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  281  		regval = div_u64(val * 5 * data->rshunt,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  282  				 1000 * INA238_FIXED_SHUNT) >> 8;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  283  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  284  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  285  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  286  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  287  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  288  	case INA238_TEMP_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  289  		/* Bits 15-4 of register */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  290  		regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  291  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  292  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  293  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  294  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  295  		regval = regval & 0xfff0;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  296  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  297  	default:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  298  		WARN_ON_ONCE(1);

regval isn't set on this path.

aafb1ad1e44aa2 Nathan Rossi 2021-10-25  299  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  300  	}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  301  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  302  	mutex_lock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  303  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25 @304  	ret = regmap_write(data->regmap, attr->index, regval);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  305  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  306  		goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  307  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  308  	ret = count;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  309  abort:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  310  	mutex_unlock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  311  	return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  312  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Dan Carpenter]

Hi Nathan,

url:    https://github.com/0day-ci/linux/commits/Nathan-Rossi/Driver-for-TI-INA238-I2C-Power-Monitor/20211025-105911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20211027 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/ina238.c:304 ina238_alert_store() error: uninitialized symbol 'regval'.

vim +/regval +304 drivers/hwmon/ina238.c

aafb1ad1e44aa2 Nathan Rossi 2021-10-25  243  static ssize_t ina238_alert_store(struct device *dev,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  244  				  struct device_attribute *da,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  245  				  const char *buf, size_t count)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  246  {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  247  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  248  	struct ina238_data *data = dev_get_drvdata(dev);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  249  	long long val;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  250  	int regval;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  251  	int ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  252  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  253  	ret = kstrtoll(buf, 10, &val);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  254  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  255  		return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  256  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  257  	/* convert decimal to register value */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  258  	switch (attr->index) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  259  	case INA238_SHUNT_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  260  	case INA238_SHUNT_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  261  		/* signed */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  262  		regval = div_s64((val * 1000), INA238_SHUNT_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  263  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  264  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  265  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  266  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  267  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  268  	case INA238_BUS_OVER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  269  	case INA238_BUS_UNDER_VOLTAGE:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  270  		regval = div_u64((val * 1000), INA238_BUS_VOLTAGE_LSB);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  271  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  272  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  273  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  274  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  275  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  276  	case INA238_POWER_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  277  		/*
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  278  		 * Compared against the 24-bit power register, lower 8-bits are
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  279  		 * truncated. Same conversion to/from uW as POWER register.
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  280  		 */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  281  		regval = div_u64(val * 5 * data->rshunt,
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  282  				 1000 * INA238_FIXED_SHUNT) >> 8;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  283  		if (regval > U16_MAX || regval < 0) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  284  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  285  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  286  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  287  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  288  	case INA238_TEMP_LIMIT:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  289  		/* Bits 15-4 of register */
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  290  		regval = (div_s64(val, INA238_DIE_TEMP_LSB) << 4);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  291  		if (regval > S16_MAX || regval < S16_MIN) {
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  292  			ret = -EINVAL;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  293  			goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  294  		}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  295  		regval = regval & 0xfff0;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  296  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  297  	default:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  298  		WARN_ON_ONCE(1);

regval isn't set on this path.

aafb1ad1e44aa2 Nathan Rossi 2021-10-25  299  		break;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  300  	}
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  301  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  302  	mutex_lock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  303  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25 @304  	ret = regmap_write(data->regmap, attr->index, regval);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  305  	if (ret < 0)
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  306  		goto abort;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  307  
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  308  	ret = count;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  309  abort:
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  310  	mutex_unlock(&data->config_lock);
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  311  	return ret;
aafb1ad1e44aa2 Nathan Rossi 2021-10-25  312  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org