[PATCH v6 0/5] hwmon: ina2xx: implement chip reinitialization and add new attributes

[PATCH v6 0/5] hwmon: ina2xx: implement chip reinitialization and add new attributes

[Bartosz Golaszewski]

This series implements a mechanism to detect if the chip is in its POR state
and reinitialize it if needed. It also extends the sysfs interface to make the
driver configurable at run-time.

The shunt_resistor attribute allows to change the shunt resistance value
at run-time in cases where ina2xx used to do the measurement isn't integrated
with the shunt.

The avg_rate attribute allows to increase/decrease noise reduction.

v6:
- moved the chip initialization and calibration to a separate function and
  extended the update_device function to check if the chip hasn't been
  reset and, if so, reinitialize it,
- added a new variable to struct ina2xx_data which holds current configuration
  value in order to be able to restore the config after a reset and reinit,
- decided to keep the rshunt value in ina2xx_data in order to be able to
  restore the shunt value when reinitializing the device,
- other minor fixes and improvements,

v5:
https://www.mail-archive.com/linux-kernel%40vger.kernel.org/msg785734.html

Bartosz Golaszewski (5):
  hwmon: ina2xx: reinitialize the chip in case it's been reset
  hwmon: ina2xx: remove a stray new line
  hwmon: ina2xx: don't accept shunt values greater than the calibration factor
  hwmon: ina2xx: make shunt resistance configurable at run-time
  hwmon: ina2xx: allow to change the averaging rate at run-time

 Documentation/hwmon/ina2xx |   8 +-
 drivers/hwmon/ina2xx.c     | 266 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 243 insertions(+), 31 deletions(-)

-- 
2.1.3

[PATCH v6 1/5] hwmon: ina2xx: reinitialize the chip in case it's been reset

[Bartosz Golaszewski]

Chips from the ina family don't like to be uninitialized. In case the power
is cut-off and restored again the calibration register will be reset
to 0 and both the power and current registers will remain at 0.

Check the calibration register in ina2xx_update_device() and reinitialize
the chip if needed.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 90 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index e01feba..4c5c9ab 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -35,6 +35,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/of.h>
+#include <linux/delay.h>
 
 #include <linux/platform_data/ina2xx.h>
 
@@ -64,6 +65,9 @@
 
 /* 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
 
 enum ina2xx_ids { ina219, ina226 };
 
@@ -81,6 +85,8 @@ struct ina2xx_data {
 	struct i2c_client *client;
 	const struct ina2xx_config *config;
 
+	long rshunt;
+
 	struct mutex update_lock;
 	bool valid;
 	unsigned long last_updated;
@@ -110,6 +116,28 @@ static const struct ina2xx_config ina2xx_config[] = {
 	},
 };
 
+/*
+ * Initialize the configuration and calibration registers.
+ */
+static int ina2xx_init(struct ina2xx_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	/* device configuration */
+	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
+					   data->config->config_default);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Set current LSB to 1mA, shunt is in uOhms
+	 * (equation 13 in datasheet).
+	 */
+	return i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
+			data->config->calibration_factor / data->rshunt);
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -121,19 +149,45 @@ static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 	if (time_after(jiffies, data->last_updated +
 		       HZ / INA2XX_CONVERSION_RATE) || !data->valid) {
 
-		int i;
+		int i, rv;
 
 		dev_dbg(&client->dev, "Starting ina2xx update\n");
 
+again:
 		/* Read all registers */
 		for (i = 0; i < data->config->registers; i++) {
-			int rv = i2c_smbus_read_word_swapped(client, i);
+			rv = i2c_smbus_read_word_swapped(client, i);
 			if (rv < 0) {
 				ret = ERR_PTR(rv);
 				goto abort;
 			}
 			data->regs[i] = rv;
 		}
+
+		/*
+		 * If the current value in the calibration register is 0, the
+		 * power and current registers will also remain at 0. In case
+		 * the chip has been reset let's check the calibration
+		 * register and reinitialize if needed.
+		 */
+		if (data->regs[INA2XX_CALIBRATION] == 0) {
+			/* Reinitialize the chip */
+			dev_warn(dev, "chip not calibrated, reinitializing\n");
+
+			rv = ina2xx_init(data);
+			if (rv < 0) {
+				ret = ERR_PTR(rv);
+				goto abort;
+			}
+
+			/*
+			 * Let's make sure the power and current registers
+			 * have been updated before trying again.
+			 */
+			mdelay(INA2XX_MAX_DELAY);
+			goto again;
+		}
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}
@@ -221,7 +275,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
-	long shunt = 10000; /* default shunt value 10mOhms */
 	u32 val;
 	int ret;
 
@@ -234,41 +287,28 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	if (dev_get_platdata(dev)) {
 		pdata = dev_get_platdata(dev);
-		shunt = pdata->shunt_uohms;
+		data->rshunt = pdata->shunt_uohms;
 	} else if (!of_property_read_u32(dev->of_node,
 					 "shunt-resistor", &val)) {
-		shunt = val;
+		data->rshunt = val;
+	} else {
+		data->rshunt = INA2XX_RSHUNT_DEFAULT;
 	}
 
-	if (shunt <= 0)
-		return -ENODEV;
-
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
+	data->client = client;
 
-	/* device configuration */
-	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-					   data->config->config_default);
-	if (ret < 0) {
-		dev_err(dev,
-			"error writing to the config register: %d", ret);
+	if (data->rshunt <= 0)
 		return -ENODEV;
-	}
 
-	/*
-	 * Set current LSB to 1mA, shunt is in uOhms
-	 * (equation 13 in datasheet).
-	 */
-	ret = i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
-				data->config->calibration_factor / shunt);
+	ret = ina2xx_init(data);
 	if (ret < 0) {
-		dev_err(dev,
-			"error writing to the calibration register: %d", ret);
+		dev_err(dev, "error configuring the device: %d\n", ret);
 		return -ENODEV;
 	}
 
-	data->client = client;
 	mutex_init(&data->update_lock);
 
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
@@ -277,7 +317,7 @@ static int ina2xx_probe(struct i2c_client *client,
 		return PTR_ERR(hwmon_dev);
 
 	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
-		 id->name, shunt);
+		 id->name, data->rshunt);
 
 	return 0;
 }
-- 
2.1.3

[PATCH v6 2/5] hwmon: ina2xx: remove a stray new line

[Bartosz Golaszewski]

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4c5c9ab..24fce2a 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -52,7 +52,6 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
-
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
-- 
2.1.3

[PATCH v6 3/5] hwmon: ina2xx: don't accept shunt values greater than the calibration factor

[Bartosz Golaszewski]

Shunt resistance values greater than the chip's calibration factor make no
sense since the actual value written to the register equals:

	<calibration factor> / <shunt>

Bail-out from ina2xx_probe() if the configured value is greater than the
calibration factor.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 24fce2a..12f0e12 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -299,7 +299,8 @@ static int ina2xx_probe(struct i2c_client *client,
 	data->config = &ina2xx_config[data->kind];
 	data->client = client;
 
-	if (data->rshunt <= 0)
+	if (data->rshunt <= 0 ||
+	    data->rshunt > data->config->calibration_factor)
 		return -ENODEV;
 
 	ret = ina2xx_init(data);
-- 
2.1.3

[PATCH v6 4/5] hwmon: ina2xx: make shunt resistance configurable at run-time

[Bartosz Golaszewski]

The shunt resistance can only be set via platform_data or device tree. This
isn't suitable for devices in which the shunt resistance can change/isn't
known at boot-time.

Add a sysfs attribute that allows to read and set the shunt resistance.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 Documentation/hwmon/ina2xx |  5 +++--
 drivers/hwmon/ina2xx.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index 4223c2d..320dd69 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -44,6 +44,7 @@ The INA226 monitors both a shunt voltage drop and bus supply voltage.
 The INA230 is a high or low side current shunt and power monitor with an I2C
 interface. The INA230 monitors both a shunt voltage drop and bus supply voltage.
 
-The shunt value in micro-ohms can be set via platform data or device tree.
-Please refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
+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/i2c/ina2xx.txt for bindings
 if the device tree is used.
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 12f0e12..c7358e8 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -115,6 +115,12 @@ static const struct ina2xx_config ina2xx_config[] = {
 	},
 };
 
+static int ina2xx_calibrate(struct ina2xx_data *data)
+{
+	return i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
+			data->config->calibration_factor / data->rshunt);
+}
+
 /*
  * Initialize the configuration and calibration registers.
  */
@@ -133,8 +139,7 @@ static int ina2xx_init(struct ina2xx_data *data)
 	 * Set current LSB to 1mA, shunt is in uOhms
 	 * (equation 13 in datasheet).
 	 */
-	return i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
-			data->config->calibration_factor / data->rshunt);
+	return ina2xx_calibrate(data);
 }
 
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
@@ -217,6 +222,9 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
 		/* signed register, LSB=1mA (selected), in mA */
 		val = (s16)data->regs[reg];
 		break;
+	case INA2XX_CALIBRATION:
+		val = data->config->calibration_factor / data->regs[reg];
+		break;
 	default:
 		/* programmer goofed */
 		WARN_ON_ONCE(1);
@@ -240,6 +248,36 @@ static ssize_t ina2xx_show_value(struct device *dev,
 			ina2xx_get_value(data, attr->index));
 }
 
+static ssize_t ina2xx_set_shunt(struct device *dev,
+				struct device_attribute *da,
+				const char *buf, size_t count)
+{
+	struct ina2xx_data *data = ina2xx_update_device(dev);
+	unsigned long val;
+	int status;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	status = kstrtoul(buf, 10, &val);
+	if (status < 0)
+		return status;
+
+	if (val == 0 ||
+	    /* Values greater than the calibration factor make no sense. */
+	    val > data->config->calibration_factor)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	data->rshunt = val;
+	status = ina2xx_calibrate(data);
+	mutex_unlock(&data->update_lock);
+	if (status < 0)
+		return status;
+
+	return count;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -256,12 +294,18 @@ static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_value, NULL,
 static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_POWER);
 
+/* shunt resistance */
+static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
+			  ina2xx_show_value, ina2xx_set_shunt,
+			  INA2XX_CALIBRATION);
+
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
 	&sensor_dev_attr_in1_input.dev_attr.attr,
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_power1_input.dev_attr.attr,
+	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina2xx);
-- 
2.1.3

[PATCH v6 5/5] hwmon: ina2xx: allow to change the averaging rate at run-time

[Bartosz Golaszewski]

The averaging rate of ina226 is hardcoded to 16 in the driver. Make it
modifiable at run-time via a new sysfs attribute.

While we're at it - add an additional variable to ina2xx_data, which holds
the current configuration settings - this way we'll be able to restore the
configuration in case of an unexpected chip reset.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 Documentation/hwmon/ina2xx |   3 ++
 drivers/hwmon/ina2xx.c     | 132 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 131 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index 320dd69..a11256d 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -48,3 +48,6 @@ 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/i2c/ina2xx.txt for bindings
 if the device tree is used.
+
+The averaging rate of INA226 and INA230 can be changed via the avg_rate sysfs
+attribute. The available rates are: 1, 4, 16, 64, 128, 256, 512 and 1024.
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index c7358e8..38c1f32 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -68,6 +68,15 @@
 
 #define INA2XX_RSHUNT_DEFAULT		10000
 
+/* bit masks for the averaging setting in the configuration register */
+#define INA226_AVG_RD_MASK		0x0E00
+#define INA226_AVG_WR_MASK		0xF1FF
+
+#define INA226_READ_AVG(reg) ((reg & INA226_AVG_RD_MASK) >> 9)
+
+/* common attrs, ina226 attrs and NULL */
+#define INA2XX_MAX_ATTRIBUTE_GROUPS	3
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -85,12 +94,14 @@ struct ina2xx_data {
 	const struct ina2xx_config *config;
 
 	long rshunt;
+	u16 curr_config;
 
 	struct mutex update_lock;
 	bool valid;
 	unsigned long last_updated;
 
 	int kind;
+	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 	u16 regs[INA2XX_MAX_REGISTERS];
 };
 
@@ -115,6 +126,38 @@ static const struct ina2xx_config ina2xx_config[] = {
 	},
 };
 
+/*
+ * Available averaging rates for ina226. The indices correspond with
+ * the bit values expected by the chip (according to the ina226 datasheet,
+ * table 3 AVG bit settings, found at
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ */
+static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
+
+static int ina226_avg_bits(int avg)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ina226_avg_tab); i++) {
+		if (avg == ina226_avg_tab[i])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int ina226_avg_val(int bits)
+{
+	/*
+	 * Value read from the config register should be correct, but do check
+	 * the boundary just in case.
+	 */
+	if (bits >= ARRAY_SIZE(ina226_avg_tab))
+		return -ENODEV;
+
+	return ina226_avg_tab[bits];
+}
+
 static int ina2xx_calibrate(struct ina2xx_data *data)
 {
 	return i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
@@ -131,7 +174,7 @@ static int ina2xx_init(struct ina2xx_data *data)
 
 	/* device configuration */
 	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-					   data->config->config_default);
+					   data->curr_config);
 	if (ret < 0)
 		return ret;
 
@@ -278,6 +321,66 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	return count;
 }
 
+static ssize_t ina226_show_avg(struct device *dev,
+			       struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = ina2xx_update_device(dev);
+	int avg, i, written = 0;
+	const char *fmt;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	avg = ina226_avg_val(INA226_READ_AVG(data->regs[INA2XX_CONFIG]));
+	if (avg < 0)
+		return avg;
+
+	for (i = 0; i < ARRAY_SIZE(ina226_avg_tab); i++) {
+		if (avg == ina226_avg_tab[i])
+			fmt = "\t[%d]";
+		else
+			fmt = "\t%d";
+
+		written += snprintf(buf + written, PAGE_SIZE - written,
+				    fmt, ina226_avg_tab[i]);
+	}
+	written += snprintf(buf + written, PAGE_SIZE - written, "\n");
+
+	return written;
+}
+
+static ssize_t ina226_set_avg(struct device *dev,
+			      struct device_attribute *da,
+			      const char *buf, size_t count)
+{
+	struct ina2xx_data *data = ina2xx_update_device(dev);
+	long val;
+	int status, avg;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	status = kstrtol(buf, 10, &val);
+	if (status < 0)
+		return status;
+
+	avg = ina226_avg_bits(val);
+	if (avg < 0)
+		return avg;
+
+	mutex_lock(&data->update_lock);
+	data->curr_config = (data->regs[INA2XX_CONFIG] &
+			     INA226_AVG_WR_MASK) | (avg << 9);
+	status = i2c_smbus_write_word_swapped(data->client,
+					      INA2XX_CONFIG,
+					      data->curr_config);
+	mutex_unlock(&data->update_lock);
+	if (status < 0)
+		return status;
+
+	return count;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -299,6 +402,10 @@ static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
 			  ina2xx_show_value, ina2xx_set_shunt,
 			  INA2XX_CALIBRATION);
 
+/* averaging rate */
+static SENSOR_DEVICE_ATTR(avg_rate, S_IRUGO | S_IWUSR,
+			  ina226_show_avg, ina226_set_avg, 0);
+
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
@@ -308,7 +415,19 @@ static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(ina2xx);
+
+static const struct attribute_group ina2xx_group = {
+	.attrs = ina2xx_attrs,
+};
+
+static struct attribute *ina226_attrs[] = {
+	&sensor_dev_attr_avg_rate.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ina226_group = {
+	.attrs = ina226_attrs,
+};
 
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
@@ -319,7 +438,7 @@ static int ina2xx_probe(struct i2c_client *client,
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
 	u32 val;
-	int ret;
+	int ret, group = 0;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
@@ -341,6 +460,7 @@ static int ina2xx_probe(struct i2c_client *client,
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
+	data->curr_config = data->config->config_default;
 	data->client = client;
 
 	if (data->rshunt <= 0 ||
@@ -355,8 +475,12 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	mutex_init(&data->update_lock);
 
+	data->groups[group++] = &ina2xx_group;
+	if (data->kind == ina226)
+		data->groups[group++] = &ina226_group;
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data, ina2xx_groups);
+							   data, data->groups);
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
 
-- 
2.1.3

Re: [PATCH v6 1/5] hwmon: ina2xx: reinitialize the chip in case it's been reset

[Guenter Roeck]

On Fri, Dec 12, 2014 at 03:37:51PM +0100, Bartosz Golaszewski wrote:
> Chips from the ina family don't like to be uninitialized. In case the power
> is cut-off and restored again the calibration register will be reset
> to 0 and both the power and current registers will remain at 0.
> 
> Check the calibration register in ina2xx_update_device() and reinitialize
> the chip if needed.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/hwmon/ina2xx.c | 90 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index e01feba..4c5c9ab 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -35,6 +35,7 @@
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/jiffies.h>
>  #include <linux/of.h>
> +#include <linux/delay.h>
>  
>  #include <linux/platform_data/ina2xx.h>
>  
> @@ -64,6 +65,9 @@
>  
>  /* 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
>  
>  enum ina2xx_ids { ina219, ina226 };
>  
> @@ -81,6 +85,8 @@ struct ina2xx_data {
>  	struct i2c_client *client;
>  	const struct ina2xx_config *config;
>  
> +	long rshunt;
> +
>  	struct mutex update_lock;
>  	bool valid;
>  	unsigned long last_updated;
> @@ -110,6 +116,28 @@ static const struct ina2xx_config ina2xx_config[] = {
>  	},
>  };
>  
> +/*
> + * Initialize the configuration and calibration registers.
> + */
> +static int ina2xx_init(struct ina2xx_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	/* device configuration */
> +	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
> +					   data->config->config_default);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Set current LSB to 1mA, shunt is in uOhms
> +	 * (equation 13 in datasheet).
> +	 */
> +	return i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
> +			data->config->calibration_factor / data->rshunt);
> +}
> +
>  static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>  {
>  	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -121,19 +149,45 @@ static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>  	if (time_after(jiffies, data->last_updated +
>  		       HZ / INA2XX_CONVERSION_RATE) || !data->valid) {
>  
> -		int i;
> +		int i, rv;
>  
>  		dev_dbg(&client->dev, "Starting ina2xx update\n");
>  
> +again:
>  		/* Read all registers */
>  		for (i = 0; i < data->config->registers; i++) {
> -			int rv = i2c_smbus_read_word_swapped(client, i);
> +			rv = i2c_smbus_read_word_swapped(client, i);
>  			if (rv < 0) {
>  				ret = ERR_PTR(rv);
>  				goto abort;
>  			}
>  			data->regs[i] = rv;
>  		}
> +
> +		/*
> +		 * If the current value in the calibration register is 0, the
> +		 * power and current registers will also remain at 0. In case
> +		 * the chip has been reset let's check the calibration
> +		 * register and reinitialize if needed.
> +		 */
> +		if (data->regs[INA2XX_CALIBRATION] == 0) {
> +			/* Reinitialize the chip */
> +			dev_warn(dev, "chip not calibrated, reinitializing\n");
> +
> +			rv = ina2xx_init(data);
> +			if (rv < 0) {
> +				ret = ERR_PTR(rv);
> +				goto abort;
> +			}
> +
> +			/*
> +			 * Let's make sure the power and current registers
> +			 * have been updated before trying again.
> +			 */
> +			mdelay(INA2XX_MAX_DELAY);

That should be msleep() or usleep_range().

> +			goto again;

That is not a good idea. We'll have to have a retry count to handle this case
to avoid an endless loop. Sure, that should be unlikely but would be easy to
trigger by instantiating the driver on some different chip which always
returns 0 when reading the calibration register.

I would suggest to implement a retry loop. Possibly add a helper function
to handle the actual update to avoid deep indentation.

Thanks,
Guenter

> +		}
> +
>  		data->last_updated = jiffies;
>  		data->valid = 1;
>  	}
> @@ -221,7 +275,6 @@ static int ina2xx_probe(struct i2c_client *client,
>  	struct device *dev = &client->dev;
>  	struct ina2xx_data *data;
>  	struct device *hwmon_dev;
> -	long shunt = 10000; /* default shunt value 10mOhms */
>  	u32 val;
>  	int ret;
>  
> @@ -234,41 +287,28 @@ static int ina2xx_probe(struct i2c_client *client,
>  
>  	if (dev_get_platdata(dev)) {
>  		pdata = dev_get_platdata(dev);
> -		shunt = pdata->shunt_uohms;
> +		data->rshunt = pdata->shunt_uohms;
>  	} else if (!of_property_read_u32(dev->of_node,
>  					 "shunt-resistor", &val)) {
> -		shunt = val;
> +		data->rshunt = val;
> +	} else {
> +		data->rshunt = INA2XX_RSHUNT_DEFAULT;
>  	}
>  
> -	if (shunt <= 0)
> -		return -ENODEV;
> -
>  	/* set the device type */
>  	data->kind = id->driver_data;
>  	data->config = &ina2xx_config[data->kind];
> +	data->client = client;
>  
> -	/* device configuration */
> -	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
> -					   data->config->config_default);
> -	if (ret < 0) {
> -		dev_err(dev,
> -			"error writing to the config register: %d", ret);
> +	if (data->rshunt <= 0)
>  		return -ENODEV;
> -	}
>  
> -	/*
> -	 * Set current LSB to 1mA, shunt is in uOhms
> -	 * (equation 13 in datasheet).
> -	 */
> -	ret = i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
> -				data->config->calibration_factor / shunt);
> +	ret = ina2xx_init(data);
>  	if (ret < 0) {
> -		dev_err(dev,
> -			"error writing to the calibration register: %d", ret);
> +		dev_err(dev, "error configuring the device: %d\n", ret);
>  		return -ENODEV;
>  	}
>  
> -	data->client = client;
>  	mutex_init(&data->update_lock);
>  
>  	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> @@ -277,7 +317,7 @@ static int ina2xx_probe(struct i2c_client *client,
>  		return PTR_ERR(hwmon_dev);
>  
>  	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
> -		 id->name, shunt);
> +		 id->name, data->rshunt);
>  
>  	return 0;
>  }
> -- 
> 2.1.3
>