* [PATCH 0/2] Driver for TI INA238 I2C Power Monitor
@ 2021-10-25 2:58 Nathan Rossi
2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Nathan Rossi @ 2021-10-25 2:58 UTC (permalink / raw)
To: linux-hwmon, devicetree, linux-kernel, linux-doc
Cc: Nathan Rossi, Nathan Rossi, Guenter Roeck, Jean Delvare,
Rob Herring, Jonathan Corbet
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 2:58 [PATCH 0/2] Driver for TI INA238 I2C Power Monitor Nathan Rossi @ 2021-10-25 2:58 ` Nathan Rossi 2021-10-25 5:06 ` Guenter Roeck ` (2 more replies) 2021-10-25 2:58 ` [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2 siblings, 3 replies; 16+ messages in thread From: Nathan Rossi @ 2021-10-25 2:58 UTC (permalink / raw) To: linux-hwmon, linux-doc, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Jean Delvare, Guenter Roeck, Jonathan Corbet 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, ®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 <nathan.rossi@digi.com>"); +MODULE_DESCRIPTION("ina238 driver"); +MODULE_LICENSE("GPL"); --- 2.33.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi @ 2021-10-25 5:06 ` Guenter Roeck 2021-10-25 6:27 ` Nathan Rossi 2021-10-27 9:32 ` kernel test robot 2021-11-03 14:52 ` Dan Carpenter 2 siblings, 1 reply; 16+ messages in thread From: Guenter Roeck @ 2021-10-25 5:06 UTC (permalink / raw) To: Nathan Rossi, linux-hwmon, linux-doc, linux-kernel Cc: Nathan Rossi, Jean Delvare, Jonathan Corbet 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, ®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 <nathan.rossi@digi.com>"); > +MODULE_DESCRIPTION("ina238 driver"); > +MODULE_LICENSE("GPL"); > --- > 2.33.0 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 5:06 ` Guenter Roeck @ 2021-10-25 6:27 ` Nathan Rossi 2021-10-25 15:45 ` Guenter Roeck 0 siblings, 1 reply; 16+ messages in thread From: Nathan Rossi @ 2021-10-25 6:27 UTC (permalink / raw) To: Guenter Roeck Cc: linux-hwmon, linux-doc, linux-kernel, Nathan Rossi, Jean Delvare, Jonathan Corbet 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, ®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 <nathan.rossi@digi.com>"); > > +MODULE_DESCRIPTION("ina238 driver"); > > +MODULE_LICENSE("GPL"); > > --- > > 2.33.0 > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 6:27 ` Nathan Rossi @ 2021-10-25 15:45 ` Guenter Roeck 2021-10-26 7:54 ` Nathan Rossi 0 siblings, 1 reply; 16+ messages in thread From: Guenter Roeck @ 2021-10-25 15:45 UTC (permalink / raw) To: Nathan Rossi Cc: linux-hwmon, linux-doc, linux-kernel, Nathan Rossi, Jean Delvare, Jonathan Corbet 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 15:45 ` Guenter Roeck @ 2021-10-26 7:54 ` Nathan Rossi 0 siblings, 0 replies; 16+ messages in thread From: Nathan Rossi @ 2021-10-26 7:54 UTC (permalink / raw) To: Guenter Roeck Cc: linux-hwmon, linux-doc, linux-kernel, Nathan Rossi, Jean Delvare, Jonathan Corbet 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi 2021-10-25 5:06 ` Guenter Roeck @ 2021-10-27 9:32 ` kernel test robot 2021-11-03 14:52 ` Dan Carpenter 2 siblings, 0 replies; 16+ messages in thread From: kernel test robot @ 2021-10-27 9:32 UTC (permalink / raw) To: Nathan Rossi, linux-hwmon, linux-doc, linux-kernel Cc: llvm, kbuild-all, Nathan Rossi, Jean Delvare, Guenter Roeck, Jonathan Corbet [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi 2021-10-25 5:06 ` Guenter Roeck 2021-10-27 9:32 ` kernel test robot @ 2021-11-03 14:52 ` Dan Carpenter 2 siblings, 0 replies; 16+ messages in thread From: Dan Carpenter @ 2021-11-03 14:52 UTC (permalink / raw) To: kbuild, Nathan Rossi, linux-hwmon, linux-doc, linux-kernel Cc: lkp, kbuild-all, Nathan Rossi, Jean Delvare, Guenter Roeck, Jonathan Corbet 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string 2021-10-25 2:58 [PATCH 0/2] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi @ 2021-10-25 2:58 ` Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2 siblings, 0 replies; 16+ messages in thread From: Nathan Rossi @ 2021-10-25 2:58 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Guenter Roeck, Jean Delvare, Rob Herring 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor 2021-10-25 2:58 [PATCH 0/2] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi 2021-10-25 2:58 ` [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi @ 2021-10-27 7:42 ` Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property Nathan Rossi ` (3 more replies) 2 siblings, 4 replies; 16+ messages in thread From: Nathan Rossi @ 2021-10-27 7:42 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel, linux-doc Cc: Nathan Rossi, Nathan Rossi, Guenter Roeck, Jean Delvare, Rob Herring, Jonathan Corbet 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi @ 2021-10-27 7:42 ` Nathan Rossi 2021-10-27 14:12 ` Rob Herring 2021-10-27 7:42 ` [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 Nathan Rossi ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Nathan Rossi @ 2021-10-27 7:42 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Guenter Roeck, Jean Delvare, Rob Herring 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property 2021-10-27 7:42 ` [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property Nathan Rossi @ 2021-10-27 14:12 ` Rob Herring 0 siblings, 0 replies; 16+ messages in thread From: Rob Herring @ 2021-10-27 14:12 UTC (permalink / raw) To: Nathan Rossi Cc: Nathan Rossi, Jean Delvare, Rob Herring, Guenter Roeck, devicetree, linux-hwmon, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property Nathan Rossi @ 2021-10-27 7:42 ` Nathan Rossi 2021-10-27 15:56 ` Guenter Roeck 2021-10-27 7:42 ` [PATCH v2 1/3] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi 2021-10-27 15:57 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Guenter Roeck 3 siblings, 1 reply; 16+ messages in thread From: Nathan Rossi @ 2021-10-27 7:42 UTC (permalink / raw) To: linux-hwmon, linux-doc, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Jean Delvare, Guenter Roeck, Jonathan Corbet 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, ®val); + 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, ®val); + 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, ®val); + 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, ®val); + 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, ®val); + 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, ®val); + 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, ®val); + 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, ®val); + 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 2021-10-27 7:42 ` [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 Nathan Rossi @ 2021-10-27 15:56 ` Guenter Roeck 0 siblings, 0 replies; 16+ messages in thread From: Guenter Roeck @ 2021-10-27 15:56 UTC (permalink / raw) To: Nathan Rossi, linux-hwmon, linux-doc, linux-kernel Cc: Nathan Rossi, Jean Delvare, Jonathan Corbet 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, ®val); > + 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, ®val); > + 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, ®val); > + 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, ®val); > + 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, ®val); > + 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, ®val); > + 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, ®val); > + 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, ®val); > + 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 Nathan Rossi @ 2021-10-27 7:42 ` Nathan Rossi 2021-10-27 15:57 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Guenter Roeck 3 siblings, 0 replies; 16+ messages in thread From: Nathan Rossi @ 2021-10-27 7:42 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel Cc: Nathan Rossi, Nathan Rossi, Guenter Roeck, Jean Delvare, Rob Herring 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi ` (2 preceding siblings ...) 2021-10-27 7:42 ` [PATCH v2 1/3] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi @ 2021-10-27 15:57 ` Guenter Roeck 3 siblings, 0 replies; 16+ messages in thread From: Guenter Roeck @ 2021-10-27 15:57 UTC (permalink / raw) To: Nathan Rossi, linux-hwmon, devicetree, linux-kernel, linux-doc Cc: Nathan Rossi, Jean Delvare, Rob Herring, Jonathan Corbet 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-11-03 14:53 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-25 2:58 [PATCH 0/2] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-25 2:58 ` [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 Nathan Rossi 2021-10-25 5:06 ` Guenter Roeck 2021-10-25 6:27 ` Nathan Rossi 2021-10-25 15:45 ` Guenter Roeck 2021-10-26 7:54 ` Nathan Rossi 2021-10-27 9:32 ` kernel test robot 2021-11-03 14:52 ` Dan Carpenter 2021-10-25 2:58 ` [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Nathan Rossi 2021-10-27 7:42 ` [PATCH v2 2/3] dt-bindings: hwmon: ti,ina2xx: Add ti,shunt-gain property Nathan Rossi 2021-10-27 14:12 ` Rob Herring 2021-10-27 7:42 ` [PATCH v2 3/3] hwmon: Driver for Texas Instruments INA238 Nathan Rossi 2021-10-27 15:56 ` Guenter Roeck 2021-10-27 7:42 ` [PATCH v2 1/3] dt-bindings: hwmon: ti,ina2xx: Document ti,ina238 compatible string Nathan Rossi 2021-10-27 15:57 ` [PATCH v2 0/3] Driver for TI INA238 I2C Power Monitor Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox