From: Nathan Rossi <[email protected]>
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
From: Nathan Rossi <[email protected]>
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 <[email protected]>
---
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
From: Nathan Rossi <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+
+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 <[email protected]>
+ */
+
+#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, ®val);
+ 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, ®val);
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("ina238 driver");
+MODULE_LICENSE("GPL");
---
2.33.0
On 10/24/21 7:58 PM, Nathan Rossi wrote:
> From: Nathan Rossi <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> +
> +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 <[email protected]>
> + */
> +
> +#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, ®val);
> + 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, ®val);
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("ina238 driver");
> +MODULE_LICENSE("GPL");
> ---
> 2.33.0
>
On Mon, 25 Oct 2021 at 15:06, Guenter Roeck <[email protected]> wrote:
>
> On 10/24/21 7:58 PM, Nathan Rossi wrote:
> > From: Nathan Rossi <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > +
> > +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 <[email protected]>
> > + */
> > +
> > +#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, ®val);
> > + 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, ®val);
> > + 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 <[email protected]>");
> > +MODULE_DESCRIPTION("ina238 driver");
> > +MODULE_LICENSE("GPL");
> > ---
> > 2.33.0
> >
>
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
On Tue, 26 Oct 2021 at 01:45, Guenter Roeck <[email protected]> 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
From: Nathan Rossi <[email protected]>
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
On 10/27/21 12:42 AM, Nathan Rossi wrote:
> From: Nathan Rossi <[email protected]>
>
> 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
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 <[email protected]>
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/[email protected]
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
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/[email protected]