[PATCH 1/3] dt-bindings: hwmon: Add compatible for ti,ina260

[PATCH 1/3] dt-bindings: hwmon: Add compatible for ti,ina260

[Franz Forstmayr]

Add initial support for power/current monitor INA260

Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
---
 Documentation/devicetree/bindings/hwmon/ina2xx.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
index 02af0d94e921..92983a224109 100644
--- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
+++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
@@ -8,6 +8,7 @@ Required properties:
 	- "ti,ina226" for ina226
 	- "ti,ina230" for ina230
 	- "ti,ina231" for ina231
+	- "ti,ina260" for ina260
 - reg: I2C address
 
 Optional properties:
-- 
2.17.1

[PATCH 2/3] docs: hwmon: Add support for ina2xx

[Franz Forstmayr]

Add documentation for INA260, power/current monitor with I2C interface.

Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
---
 Documentation/hwmon/ina2xx.rst | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
index 94b9a260c518..74267dd433dd 100644
--- a/Documentation/hwmon/ina2xx.rst
+++ b/Documentation/hwmon/ina2xx.rst
@@ -53,6 +53,16 @@ Supported chips:
 
 	       http://www.ti.com/
 
+  * Texas Instruments INA260
+
+    Prefix: 'ina260'
+
+    Addresses: I2C 0x40 - 0x4f
+
+    Datasheet: Publicly available at the Texas Instruments website
+
+         http://www.ti.com/
+
 Author: Lothar Felten <lothar.felten@gmail.com>
 
 Description
@@ -72,14 +82,17 @@ INA230 and INA231 are high or low side current shunt and power monitors
 with an I2C interface. The chips monitor both a shunt voltage drop and
 bus supply voltage.
 
+INA260 is a high or low side current and power monitor with an integrated
+shunt and I2C interface.
+
 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/ina2xx.txt for bindings
 if the device tree is used.
 
-Additionally ina226 supports update_interval attribute as described in
-Documentation/hwmon/sysfs-interface.rst. Internally the interval is the sum of
-bus and shunt voltage conversion times multiplied by the averaging rate. We
+Additionally ina226 and ina260 supports update_interval attribute as described
+in Documentation/hwmon/sysfs-interface.rst. Internally the interval is the sum
+of bus and shunt voltage conversion times multiplied by the averaging rate. We
 don't touch the conversion times and only modify the number of averages. The
 lower limit of the update_interval is 2 ms, the upper limit is 2253 ms.
 The actual programmed interval may vary from the desired value.
-- 
2.17.1

Re: [PATCH 2/3] docs: hwmon: Add support for ina2xx

[Guenter Roeck]

On 2/24/20 3:26 PM, Franz Forstmayr wrote:
> Add documentation for INA260, power/current monitor with I2C interface.
> 

Subject should match description here (this patch does not add support
for ina2xx).

> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
> ---
>   Documentation/hwmon/ina2xx.rst | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> index 94b9a260c518..74267dd433dd 100644
> --- a/Documentation/hwmon/ina2xx.rst
> +++ b/Documentation/hwmon/ina2xx.rst
> @@ -53,6 +53,16 @@ Supported chips:
>   
>   	       http://www.ti.com/
>   
> +  * Texas Instruments INA260
> +
> +    Prefix: 'ina260'
> +
> +    Addresses: I2C 0x40 - 0x4f
> +
> +    Datasheet: Publicly available at the Texas Instruments website
> +
> +         http://www.ti.com/
> +
>   Author: Lothar Felten <lothar.felten@gmail.com>
>   
>   Description
> @@ -72,14 +82,17 @@ INA230 and INA231 are high or low side current shunt and power monitors
>   with an I2C interface. The chips monitor both a shunt voltage drop and
>   bus supply voltage.
>   
> +INA260 is a high or low side current and power monitor with an integrated
> +shunt and I2C interface.
> +
>   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/ina2xx.txt for bindings
>   if the device tree is used.
>   
> -Additionally ina226 supports update_interval attribute as described in
> -Documentation/hwmon/sysfs-interface.rst. Internally the interval is the sum of
> -bus and shunt voltage conversion times multiplied by the averaging rate. We
> +Additionally ina226 and ina260 supports update_interval attribute as described

s/supports/support/

> +in Documentation/hwmon/sysfs-interface.rst. Internally the interval is the sum
> +of bus and shunt voltage conversion times multiplied by the averaging rate. We
>   don't touch the conversion times and only modify the number of averages. The
>   lower limit of the update_interval is 2 ms, the upper limit is 2253 ms.
>   The actual programmed interval may vary from the desired value.
> 

[PATCH 3/3] hwmon: (ina2xx) Add support for ina260

[Franz Forstmayr]

Add initial support for INA260 power monitor with integrated shunt.
Registers are different from other INA2xx devices, that's why a small
translation table is used.

Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
---
 drivers/hwmon/Kconfig  |   5 +-
 drivers/hwmon/ina2xx.c | 144 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 125 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 05a30832c6ba..2916a60dd9b1 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1683,11 +1683,12 @@ config SENSORS_INA2XX
 	select REGMAP_I2C
 	help
 	  If you say yes here you get support for INA219, INA220, INA226,
-	  INA230, and INA231 power monitor chips.
+	  INA230, INA231 and INA260 power monitor chips.
 
 	  The INA2xx driver is configured for the default configuration of
 	  the part as described in the datasheet.
-	  Default value for Rshunt is 10 mOhms.
+	  Default value for Rshunt is 10 mOhms, except for INA260 which has
+	  an 2 mOhm integrated shunt.
 	  This driver can also be built as a module. If so, the module
 	  will be called ina2xx.
 
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index e9e78c0b7212..bc5bb936bac5 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -18,6 +18,11 @@
  * Bi-directional Current/Power Monitor with I2C Interface
  * Datasheet: http://www.ti.com/product/ina230
  *
+ * INA260:
+ * Bi-directional Current/Power Monitor with I2C Interface and integrated
+ * shunt.
+ * Datasheet: http://www.ti.com/product/ina260
+ *
  * Copyright (C) 2012 Lothar Felten <lothar.felten@gmail.com>
  * Thanks to Jan Volkering
  */
@@ -50,8 +55,14 @@
 /* INA226 register definitions */
 #define INA226_MASK_ENABLE		0x06
 #define INA226_ALERT_LIMIT		0x07
+#define INA226_MANUFACTURER_ID		0xFE
 #define INA226_DIE_ID			0xFF
 
+/* INA260 register definitions */
+#define INA260_CURRENT			0x01
+#define INA260_BUS_VOLTAGE		0x02
+#define INA260_POWER			0x03
+
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
@@ -61,12 +72,14 @@
 /* settings - depend on use case */
 #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
 #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
+#define INA260_CONFIG_DEFAULT		0x6127  /* averages=16 */
 
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
 #define INA2XX_CONVERSION_RATE		15
 #define INA2XX_MAX_DELAY		69 /* worst case delay in ms */
 
 #define INA2XX_RSHUNT_DEFAULT		10000
+#define INA260_RSHUNT_DEFAULT           2000
 
 /* bit mask for reading the averaging setting in the configuration register */
 #define INA226_AVG_RD_MASK		0x0E00
@@ -74,8 +87,8 @@
 #define INA226_READ_AVG(reg)		(((reg) & INA226_AVG_RD_MASK) >> 9)
 #define INA226_SHIFT_AVG(val)		((val) << 9)
 
-/* common attrs, ina226 attrs and NULL */
-#define INA2XX_MAX_ATTRIBUTE_GROUPS	3
+/* common attrs, shunt/bus voltage attrs, ina226 attrs and NULL */
+#define INA2XX_MAX_ATTRIBUTE_GROUPS	4
 
 /*
  * Both bus voltage and shunt voltage conversion times for ina226 are set
@@ -88,7 +101,15 @@ static struct regmap_config ina2xx_regmap_config = {
 	.val_bits = 16,
 };
 
-enum ina2xx_ids { ina219, ina226 };
+enum ina2xx_ids { ina219, ina226, ina260 };
+
+/* Translate the ina2xx addresses to ina260 addresses */
+const int ina260_translation[] = { 0,
+	0,
+	INA260_BUS_VOLTAGE,
+	INA260_POWER,
+	INA260_CURRENT };
+
 
 struct ina2xx_config {
 	u16 config_default;
@@ -108,6 +129,7 @@ struct ina2xx_data {
 	long power_lsb_uW;
 	struct mutex config_lock;
 	struct regmap *regmap;
+	enum ina2xx_ids chip;
 
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 };
@@ -131,6 +153,15 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 25,
 	},
+	[ina260] = {
+		.config_default = INA260_CONFIG_DEFAULT,
+		.calibration_value = 0,
+		.registers = INA226_REGISTERS,
+		.shunt_div = 0,
+		.bus_voltage_shift = 0,
+		.bus_voltage_lsb = 1250,
+		.power_lsb_factor = 10,
+	},
 };
 
 /*
@@ -190,7 +221,11 @@ static int ina2xx_init(struct ina2xx_data *data)
 	if (ret < 0)
 		return ret;
 
-	return ina2xx_calibrate(data);
+	/* ina260 has no calibration register */
+	if (data->chip != ina260)
+		return ina2xx_calibrate(data);
+	else
+		return 0;
 }
 
 static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
@@ -215,8 +250,9 @@ static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
 		 * register and reinitialize if needed.
 		 * We do that extra read of the calibration register if there
 		 * is some hint of a chip reset.
+		 * INA260 has an integrated shunt, thus no calibration register
 		 */
-		if (*regval == 0) {
+		if (*regval == 0 && data->chip != ina260) {
 			unsigned int cal;
 
 			ret = regmap_read(data->regmap, INA2XX_CALIBRATION,
@@ -287,20 +323,60 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
 	return val;
 }
 
+static int ina260_get_value(struct ina2xx_data *data, u8 reg,
+	unsigned int regval)
+{
+	int val;
+
+	switch (reg) {
+	case INA260_BUS_VOLTAGE:
+		val = (regval >> data->config->bus_voltage_shift)
+			* data->config->bus_voltage_lsb;
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		break;
+	case INA260_POWER:
+		val = regval * data->power_lsb_uW;
+		break;
+	case INA260_CURRENT:
+		/* signed register, result in mA */
+		val = (s16)regval * data->current_lsb_uA;
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		break;
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+		val = 0;
+		break;
+	}
+	return val;
+}
+
 static ssize_t ina2xx_value_show(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	unsigned int regval;
+	int err;
+
+	if (data->chip == ina260) {
+		err = ina2xx_read_reg(dev,
+			ina260_translation[attr->index], &regval);
+
+		if (err < 0)
+			return err;
 
-	int err = ina2xx_read_reg(dev, attr->index, &regval);
+		return snprintf(buf, PAGE_SIZE, "%d\n", ina260_get_value(data,
+			ina260_translation[attr->index], regval));
+	} else {
+		err = ina2xx_read_reg(dev, attr->index, &regval);
 
-	if (err < 0)
-		return err;
+		if (err < 0)
+			return err;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina2xx_get_value(data, attr->index, regval));
+		return snprintf(buf, PAGE_SIZE, "%d\n", ina2xx_get_value(data,
+			attr->index, regval));
+	}
 }
 
 /*
@@ -409,11 +485,19 @@ static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
 static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
 
 /* pointers to created device attributes */
-static struct attribute *ina2xx_attrs[] = {
-	&sensor_dev_attr_in0_input.dev_attr.attr,
+static struct attribute *ina2xx_common_attrs[] = {
 	&sensor_dev_attr_in1_input.dev_attr.attr,
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_power1_input.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ina2xx_common_group = {
+	.attrs = ina2xx_common_attrs,
+};
+
+static struct attribute *ina2xx_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
 	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
 	NULL,
 };
@@ -451,19 +535,28 @@ static int ina2xx_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	/* set the device type */
+	data->chip = chip;
 	data->config = &ina2xx_config[chip];
 	mutex_init(&data->config_lock);
 
-	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
-		struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
+	if (chip != ina260) {
+		if (of_property_read_u32(dev->of_node,
+			"shunt-resistor", &val) < 0) {
 
-		if (pdata)
-			val = pdata->shunt_uohms;
-		else
-			val = INA2XX_RSHUNT_DEFAULT;
-	}
+			struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
 
-	ina2xx_set_shunt(data, val);
+			if (pdata)
+				val = pdata->shunt_uohms;
+			else
+				val = INA2XX_RSHUNT_DEFAULT;
+		}
+		ina2xx_set_shunt(data, val);
+	} else {
+		data->rshunt = INA260_RSHUNT_DEFAULT;
+		/* ina260 has same LSB value for current and voltage */
+		data->current_lsb_uA = data->config->bus_voltage_lsb;
+		data->power_lsb_uW = data->config->power_lsb_factor;
+	}
 
 	ina2xx_regmap_config.max_register = data->config->registers;
 
@@ -479,9 +572,11 @@ static int ina2xx_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	data->groups[group++] = &ina2xx_group;
-	if (chip == ina226)
+	data->groups[group++] = &ina2xx_common_group;
+	if (chip == ina226 || chip == ina260)
 		data->groups[group++] = &ina226_group;
+	if (chip != ina260)
+		data->groups[group++] = &ina2xx_group;
 
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   data, data->groups);
@@ -500,6 +595,7 @@ static const struct i2c_device_id ina2xx_id[] = {
 	{ "ina226", ina226 },
 	{ "ina230", ina226 },
 	{ "ina231", ina226 },
+	{ "ina260", ina260 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ina2xx_id);
@@ -525,6 +621,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
 		.compatible = "ti,ina231",
 		.data = (void *)ina226
 	},
+	{
+		.compatible = "ti,ina260",
+		.data = (void *)ina260
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ina2xx_of_match);
-- 
2.17.1

Re: [PATCH 3/3] hwmon: (ina2xx) Add support for ina260

[Guenter Roeck]

On 2/24/20 3:26 PM, Franz Forstmayr wrote:
> Add initial support for INA260 power monitor with integrated shunt.
> Registers are different from other INA2xx devices, that's why a small
> translation table is used.
> 
> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>

I think the chip is sufficiently different to other chips that a separate
driver would make much more sense than adding support to the existing driver.
There is no calibration, registers are different, the retry logic is
not needed. A new driver could use the with_info API and would be much
simpler while at the same time not messing up the existing driver.

Guenter

Re: [PATCH 3/3] hwmon: (ina2xx) Add support for ina260

[Michal Simek]

On 26. 02. 20 3:16, Guenter Roeck wrote:
> On 2/24/20 3:26 PM, Franz Forstmayr wrote:
>> Add initial support for INA260 power monitor with integrated shunt.
>> Registers are different from other INA2xx devices, that's why a small
>> translation table is used.
>>
>> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
> 
> I think the chip is sufficiently different to other chips that a separate
> driver would make much more sense than adding support to the existing
> driver.
> There is no calibration, registers are different, the retry logic is
> not needed. A new driver could use the with_info API and would be much
> simpler while at the same time not messing up the existing driver.

Isn't it also better to switch to IIO framework?
As we discussed in past there are two ina226 drivers. One in hwmon and
second based on IIO framework (more advance one?) and would be good to
deprecate hwmon one.
That's why separate driver is necessary.

Thanks,
Michal

Re: [PATCH 3/3] hwmon: (ina2xx) Add support for ina260

[Guenter Roeck]

On 5/18/20 10:21 PM, Michal Simek wrote:
> On 26. 02. 20 3:16, Guenter Roeck wrote:
>> On 2/24/20 3:26 PM, Franz Forstmayr wrote:
>>> Add initial support for INA260 power monitor with integrated shunt.
>>> Registers are different from other INA2xx devices, that's why a small
>>> translation table is used.
>>>
>>> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
>>
>> I think the chip is sufficiently different to other chips that a separate
>> driver would make much more sense than adding support to the existing
>> driver.
>> There is no calibration, registers are different, the retry logic is
>> not needed. A new driver could use the with_info API and would be much
>> simpler while at the same time not messing up the existing driver.
> 
> Isn't it also better to switch to IIO framework?
> As we discussed in past there are two ina226 drivers. One in hwmon and
> second based on IIO framework (more advance one?) and would be good to
> deprecate hwmon one.

"More advanced" is relative. The ina2xx driver in iio doesn't support
alert limits (which is queued in the hwmon driver for 5.8), and the
iio->hwmon bridge doesn't support it either. On top of that, there are
existing users of the hwmon driver, which would have to be converted
first. As for ina260, it would be up to the implementer to determine
if alert limit support is needed or not, and which API would be
appropriate for the intended use case.

Guenter

Re: [PATCH 3/3] hwmon: (ina2xx) Add support for ina260

[Michal Simek]

On 19. 05. 20 16:14, Guenter Roeck wrote:
> On 5/18/20 10:21 PM, Michal Simek wrote:
>> On 26. 02. 20 3:16, Guenter Roeck wrote:
>>> On 2/24/20 3:26 PM, Franz Forstmayr wrote:
>>>> Add initial support for INA260 power monitor with integrated shunt.
>>>> Registers are different from other INA2xx devices, that's why a small
>>>> translation table is used.
>>>>
>>>> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
>>>
>>> I think the chip is sufficiently different to other chips that a separate
>>> driver would make much more sense than adding support to the existing
>>> driver.
>>> There is no calibration, registers are different, the retry logic is
>>> not needed. A new driver could use the with_info API and would be much
>>> simpler while at the same time not messing up the existing driver.
>>
>> Isn't it also better to switch to IIO framework?
>> As we discussed in past there are two ina226 drivers. One in hwmon and
>> second based on IIO framework (more advance one?) and would be good to
>> deprecate hwmon one.
> 
> "More advanced" is relative. The ina2xx driver in iio doesn't support
> alert limits (which is queued in the hwmon driver for 5.8), and the
> iio->hwmon bridge doesn't support it either. On top of that, there are
> existing users of the hwmon driver, which would have to be converted
> first. As for ina260, it would be up to the implementer to determine
> if alert limit support is needed or not, and which API would be
> appropriate for the intended use case.

Good to know. If ina260 is done as separate driver I am fine with it.

Thanks,
Michal

Re: [PATCH 1/3] dt-bindings: hwmon: Add compatible for ti,ina260

[Rob Herring]

On Tue, 25 Feb 2020 00:26:45 +0100, Franz Forstmayr wrote:
> Add initial support for power/current monitor INA260
> 
> Signed-off-by: Franz Forstmayr <forstmayr.franz@gmail.com>
> ---
>  Documentation/devicetree/bindings/hwmon/ina2xx.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>