diff mbox series

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

Message ID 20200224232647.29213-3-forstmayr.franz@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [1/3] dt-bindings: hwmon: Add compatible for ti,ina260 | expand

Commit Message

Franz Forstmayr Feb. 24, 2020, 11:26 p.m. UTC
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(-)

Comments

Guenter Roeck Feb. 26, 2020, 2:16 a.m. UTC | #1
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
Michal Simek May 19, 2020, 5:21 a.m. UTC | #2
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
Guenter Roeck May 19, 2020, 2:14 p.m. UTC | #3
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
Michal Simek May 19, 2020, 3:28 p.m. UTC | #4
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
diff mbox series

Patch

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);