[PATCH v2 0/5] hwmon: ina2xx: fixes & extensions

[PATCH v2 0/5] hwmon: ina2xx: fixes & extensions

[Bartosz Golaszewski]

This series fixes an initialization issue we've experienced with the ina2xx
driver and 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.

Bartosz Golaszewski (5):
  hwmon: ina2xx: bail-out from ina2xx_probe() in case of configuration errors
  hwmon: ina2xx: make shunt resistance configurable at run-time
  hwmon: ina2xx: allow to change the averaging rate at run-time
  hwmon: ina2xx: change hex constants to lower-case
  hwmon: ina2xx: documentation update for new sysfs attributes

 Documentation/hwmon/ina2xx |  10 ++-
 drivers/hwmon/ina2xx.c     | 187 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 181 insertions(+), 16 deletions(-)

-- 
2.1.3

[PATCH v2 1/5] hwmon: ina2xx: bail-out from ina2xx_probe() in case of configuration errors

[Bartosz Golaszewski]

The return value of i2c_smbus_write_word_swapped() isn't checked in
ina2xx_probe(). This leads to devices being registered even if they cannot
be physically detected (e.g. device is not powered-up at boot-time).

Even after restoring power to such device, it is left unconfigured as the
configuration has never been actually written to the register.

Error out in case of write errors in probe and notify the user.

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

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index bfd3f3e..0a6b60f 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -223,6 +223,7 @@ static int ina2xx_probe(struct i2c_client *client,
 	struct device *hwmon_dev;
 	long shunt = 10000; /* default shunt value 10mOhms */
 	u32 val;
+	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
@@ -247,12 +248,24 @@ static int ina2xx_probe(struct i2c_client *client,
 	data->config = &ina2xx_config[data->kind];
 
 	/* device configuration */
-	i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-				     data->config->config_default);
-	/* set current LSB to 1mA, shunt is in uOhms */
-	/* (equation 13 in datasheet) */
-	i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
-				     data->config->calibration_factor / shunt);
+	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);
+		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);
+	if (ret < 0) {
+		dev_err(dev,
+			"error writing to the calibration register: %d", ret);
+		return -ENODEV;
+	}
 
 	data->client = client;
 	mutex_init(&data->update_lock);
-- 
2.1.3

Re: [PATCH v2 1/5] hwmon: ina2xx: bail-out from ina2xx_probe() in case of configuration errors

[Guenter Roeck]

On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> The return value of i2c_smbus_write_word_swapped() isn't checked in
> ina2xx_probe(). This leads to devices being registered even if they cannot
> be physically detected (e.g. device is not powered-up at boot-time).
>
> Even after restoring power to such device, it is left unconfigured as the
> configuration has never been actually written to the register.
>
> Error out in case of write errors in probe and notify the user.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Applied to -next.

Thanks,
Guenter

[PATCH v2 2/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>
---
 drivers/hwmon/ina2xx.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 0a6b60f..341da67 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,6 +51,8 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
+/* shunt resistor sysfs attribute index */
+#define INA2XX_RSHUNT			0x8
 
 /* register count */
 #define INA219_REGISTERS		6
@@ -65,6 +67,9 @@
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
 #define INA2XX_CONVERSION_RATE		15
 
+/* default shunt resistance */
+#define INA2XX_RSHUNT_DEFAULT		10000
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -87,6 +92,8 @@ struct ina2xx_data {
 
 	int kind;
 	u16 regs[INA2XX_MAX_REGISTERS];
+
+	long rshunt;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -110,6 +117,11 @@ static const struct ina2xx_config ina2xx_config[] = {
 	},
 };
 
+static u16 ina2xx_calibration_val(const struct ina2xx_data *data)
+{
+	return data->config->calibration_factor / data->rshunt;
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -164,6 +176,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_RSHUNT:
+		val = data->rshunt;
+		break;
 	default:
 		/* programmer goofed */
 		WARN_ON_ONCE(1);
@@ -187,6 +202,35 @@ 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);
+	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)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	data->rshunt = val;
+	status = i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
+					      ina2xx_calibration_val(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);
@@ -203,12 +247,17 @@ 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 | S_IWGRP,
+			  ina2xx_show_value, ina2xx_set_shunt, INA2XX_RSHUNT);
+
 /* 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);
@@ -221,7 +270,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,13 +282,15 @@ 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)
+	if (data->rshunt <= 0)
 		return -ENODEV;
 
 	/* set the device type */
@@ -260,7 +310,7 @@ static int ina2xx_probe(struct i2c_client *client,
 	 * (equation 13 in datasheet).
 	 */
 	ret = i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
-				data->config->calibration_factor / shunt);
+					   ina2xx_calibration_val(data));
 	if (ret < 0) {
 		dev_err(dev,
 			"error writing to the calibration register: %d", ret);
@@ -276,7 +326,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

Re: [PATCH v2 2/5] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> 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>
> ---
>   drivers/hwmon/ina2xx.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 0a6b60f..341da67 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,6 +51,8 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>
> +/* shunt resistor sysfs attribute index */
> +#define INA2XX_RSHUNT			0x8
>
>   /* register count */
>   #define INA219_REGISTERS		6
> @@ -65,6 +67,9 @@
>   /* worst case is 68.10 ms (~14.6Hz, ina219) */
>   #define INA2XX_CONVERSION_RATE		15
>
> +/* default shunt resistance */
> +#define INA2XX_RSHUNT_DEFAULT		10000
> +
>   enum ina2xx_ids { ina219, ina226 };
>
>   struct ina2xx_config {
> @@ -87,6 +92,8 @@ struct ina2xx_data {
>
>   	int kind;
>   	u16 regs[INA2XX_MAX_REGISTERS];
> +
> +	long rshunt;
>   };
>
>   static const struct ina2xx_config ina2xx_config[] = {
> @@ -110,6 +117,11 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	},
>   };
>
> +static u16 ina2xx_calibration_val(const struct ina2xx_data *data)
> +{
> +	return data->config->calibration_factor / data->rshunt;
> +}
> +
>   static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -164,6 +176,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_RSHUNT:
> +		val = data->rshunt;
> +		break;

That doesn't make sense. You neither read the current shunt resistor value
with update_device, nor use the register value to calculate the programmed
shunt resistor value. Just define a separate show function which reports
rshunt as configured.

A better alternative would be to actually _read_ and report rshunt as
programmed into the chip. Otherwise you'll get the "old" value of rshunt
even if the chip was, as your use case suggests, pulled and re-inserted.

>   	default:
>   		/* programmer goofed */
>   		WARN_ON_ONCE(1);
> @@ -187,6 +202,35 @@ 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);
> +	long val;

This should be unsigned long since you now use kstrtoul.

> +	int status;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	status = kstrtoul(buf, 10, &val);
> +	if (status < 0)
> +		return status;

This will accept a value of 0xffffffff which is then converted into -1
since data->rshunt is defined as long.

> +
> +	if (val == 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	data->rshunt = val;
> +	status = i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
> +					      ina2xx_calibration_val(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);
> @@ -203,12 +247,17 @@ 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 | S_IWGRP,

This will have to be S_IRUGO | S_IWUSR. We don't permit group writes.

> +			  ina2xx_show_value, ina2xx_set_shunt, INA2XX_RSHUNT);
> +
>   /* 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);
> @@ -221,7 +270,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,13 +282,15 @@ 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)
> +	if (data->rshunt <= 0)
>   		return -ENODEV;
>
>   	/* set the device type */
> @@ -260,7 +310,7 @@ static int ina2xx_probe(struct i2c_client *client,
>   	 * (equation 13 in datasheet).
>   	 */
>   	ret = i2c_smbus_write_word_swapped(client, INA2XX_CALIBRATION,
> -				data->config->calibration_factor / shunt);
> +					   ina2xx_calibration_val(data));
>   	if (ret < 0) {
>   		dev_err(dev,
>   			"error writing to the calibration register: %d", ret);
> @@ -276,7 +326,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;
>   }
>

Re: [PATCH v2 2/5] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> 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>
> ---
>   drivers/hwmon/ina2xx.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 0a6b60f..341da67 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,6 +51,8 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>
> +/* shunt resistor sysfs attribute index */
> +#define INA2XX_RSHUNT			0x8
>
After reading the datasheet again ... those defines were meant
to reflect registers, not to create 'dummy' index values for software.
Please rearrange your code to not require such constructs; it is just
confusing for others (including me).

Guenter

[PATCH v2 3/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.

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

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 341da67..7fdb586 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -54,6 +54,9 @@
 /* shunt resistor sysfs attribute index */
 #define INA2XX_RSHUNT			0x8
 
+/* INA226 averaging sysfs index */
+#define INA226_AVG			0x9
+
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
@@ -70,6 +73,12 @@
 /* default shunt resistance */
 #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)
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -117,11 +126,57 @@ 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 u16 ina2xx_calibration_val(const struct ina2xx_data *data)
 {
 	return data->config->calibration_factor / data->rshunt;
 }
 
+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)) {
+		WARN_ON_ONCE(1);
+		return -1;
+	}
+
+	return ina226_avg_tab[bits];
+}
+
+static inline int ina226_update_avg(struct ina2xx_data *data, int avg)
+{
+	int status;
+	u16 conf;
+
+	mutex_lock(&data->update_lock);
+	conf = (data->regs[INA2XX_CONFIG] & INA226_AVG_WR_MASK) | (avg << 9);
+	status = i2c_smbus_write_word_swapped(data->client,
+					      INA2XX_CONFIG, conf);
+	mutex_unlock(&data->update_lock);
+
+	return status;
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -179,6 +234,10 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
 	case INA2XX_RSHUNT:
 		val = data->rshunt;
 		break;
+	case INA226_AVG:
+		val = ina226_avg_val(INA226_READ_AVG(
+				     data->regs[INA2XX_CONFIG]));
+		break;
 	default:
 		/* programmer goofed */
 		WARN_ON_ONCE(1);
@@ -202,10 +261,22 @@ static ssize_t ina2xx_show_value(struct device *dev,
 			ina2xx_get_value(data, attr->index));
 }
 
-static ssize_t ina2xx_set_shunt(struct device *dev,
+static ssize_t ina226_show_avg(struct device *dev,
+			       struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+
+	if (data->kind != ina226)
+		return -ENXIO;
+
+	return ina2xx_show_value(dev, da, buf);
+}
+
+static ssize_t ina2xx_set_value(struct device *dev,
 				struct device_attribute *da,
 				const char *buf, size_t count)
 {
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct ina2xx_data *data = ina2xx_update_device(dev);
 	long val;
 	int status;
@@ -217,16 +288,38 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	if (status < 0)
 		return status;
 
-	if (val == 0)
-		return -EINVAL;
+	switch (attr->index) {
+	case INA2XX_RSHUNT:
+		if (val == 0)
+			return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->rshunt = val;
-	status = i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
-					      ina2xx_calibration_val(data));
-	mutex_unlock(&data->update_lock);
-	if (status < 0)
-		return status;
+		mutex_lock(&data->update_lock);
+		data->rshunt = val;
+		status = i2c_smbus_write_word_swapped(data->client,
+			INA2XX_CALIBRATION, ina2xx_calibration_val(data));
+		mutex_unlock(&data->update_lock);
+		if (status < 0)
+			return status;
+
+		break;
+	case INA226_AVG:
+		if (data->kind != ina226)
+			return -ENXIO;
+
+		status = ina226_avg_bits(val);
+		if (status < 0)
+			return status;
+
+		status = ina226_update_avg(data, status);
+		if (status < 0)
+			return status;
+		break;
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+		val = 0;
+		break;
+	}
 
 	return count;
 }
@@ -249,7 +342,11 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
 
 /* shunt resistance */
 static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR | S_IWGRP,
-			  ina2xx_show_value, ina2xx_set_shunt, INA2XX_RSHUNT);
+			  ina2xx_show_value, ina2xx_set_value, INA2XX_RSHUNT);
+
+/* averaging rate */
+static SENSOR_DEVICE_ATTR(avg_rate, S_IRUGO | S_IWUSR | S_IWGRP,
+			  ina226_show_avg, ina2xx_set_value, INA226_AVG);
 
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
@@ -258,6 +355,7 @@ static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_power1_input.dev_attr.attr,
 	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
+	&sensor_dev_attr_avg_rate.dev_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina2xx);
-- 
2.1.3

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

[Guenter Roeck]

On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> The averaging rate of ina226 is hardcoded to 16 in the driver.
>
> Make it modifiable at run-time via a new sysfs attribute.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/hwmon/ina2xx.c | 120 ++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 341da67..7fdb586 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -54,6 +54,9 @@
>   /* shunt resistor sysfs attribute index */
>   #define INA2XX_RSHUNT			0x8
>
> +/* INA226 averaging sysfs index */
> +#define INA226_AVG			0x9
> +
>   /* register count */
>   #define INA219_REGISTERS		6
>   #define INA226_REGISTERS		8
> @@ -70,6 +73,12 @@
>   /* default shunt resistance */
>   #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)
> +
>   enum ina2xx_ids { ina219, ina226 };
>
>   struct ina2xx_config {
> @@ -117,11 +126,57 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	},
>   };
>
> +/* Available averaging rates for ina226. The indices correspond with

We use standard multi-line comment style in hwmon.

> + * 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 u16 ina2xx_calibration_val(const struct ina2xx_data *data)
>   {
>   	return data->config->calibration_factor / data->rshunt;
>   }
>
> +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;

Reads beyond the end of the table.

> +	}
> +
> +	return -EINVAL;

You are expecting the user to know the valid averages.

> +}
> +
> +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)) {
> +		WARN_ON_ONCE(1);

Really ? Traceback if you read an unexpected value from the chip ?

> +		return -1;

Please use a defined error code.

> +	}
> +
> +	return ina226_avg_tab[bits];
> +}
> +
> +static inline int ina226_update_avg(struct ina2xx_data *data, int avg)
> +{
> +	int status;
> +	u16 conf;
> +
> +	mutex_lock(&data->update_lock);
> +	conf = (data->regs[INA2XX_CONFIG] & INA226_AVG_WR_MASK) | (avg << 9);
> +	status = i2c_smbus_write_word_swapped(data->client,
> +					      INA2XX_CONFIG, conf);
> +	mutex_unlock(&data->update_lock);
> +
> +	return status;
> +}
> +
>   static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -179,6 +234,10 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
>   	case INA2XX_RSHUNT:
>   		val = data->rshunt;
>   		break;
> +	case INA226_AVG:
> +		val = ina226_avg_val(INA226_READ_AVG(
> +				     data->regs[INA2XX_CONFIG]));
> +		break;
>   	default:
>   		/* programmer goofed */
>   		WARN_ON_ONCE(1);
> @@ -202,10 +261,22 @@ static ssize_t ina2xx_show_value(struct device *dev,
>   			ina2xx_get_value(data, attr->index));
>   }
>
> -static ssize_t ina2xx_set_shunt(struct device *dev,

Use separate functions. There is no common code.

> +static ssize_t ina226_show_avg(struct device *dev,
> +			       struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +
> +	if (data->kind != ina226)
> +		return -ENXIO;
> +
No.

> +	return ina2xx_show_value(dev, da, buf);
> +}
> +
> +static ssize_t ina2xx_set_value(struct device *dev,
>   				struct device_attribute *da,
>   				const char *buf, size_t count)
>   {
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>   	struct ina2xx_data *data = ina2xx_update_device(dev);
>   	long val;
>   	int status;
> @@ -217,16 +288,38 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>   	if (status < 0)
>   		return status;
>
> -	if (val == 0)
> -		return -EINVAL;
> +	switch (attr->index) {
> +	case INA2XX_RSHUNT:
> +		if (val == 0)
> +			return -EINVAL;
>
> -	mutex_lock(&data->update_lock);
> -	data->rshunt = val;
> -	status = i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
> -					      ina2xx_calibration_val(data));
> -	mutex_unlock(&data->update_lock);
> -	if (status < 0)
> -		return status;
> +		mutex_lock(&data->update_lock);
> +		data->rshunt = val;
> +		status = i2c_smbus_write_word_swapped(data->client,
> +			INA2XX_CALIBRATION, ina2xx_calibration_val(data));
> +		mutex_unlock(&data->update_lock);
> +		if (status < 0)
> +			return status;
> +
> +		break;
> +	case INA226_AVG:
> +		if (data->kind != ina226)
> +			return -ENXIO;
> +
For other chips the attribute should not exist or be handled correctly.
ENXIO is inappropriate.

> +		status = ina226_avg_bits(val);
> +		if (status < 0)
> +			return status;
> +
> +		status = ina226_update_avg(data, status);
> +		if (status < 0)
> +			return status;
> +		break;
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +		val = 0;
> +		break;
> +	}
>
>   	return count;
>   }
> @@ -249,7 +342,11 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
>
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR | S_IWGRP,
> -			  ina2xx_show_value, ina2xx_set_shunt, INA2XX_RSHUNT);
> +			  ina2xx_show_value, ina2xx_set_value, INA2XX_RSHUNT);
> +
> +/* averaging rate */
> +static SENSOR_DEVICE_ATTR(avg_rate, S_IRUGO | S_IWUSR | S_IWGRP,
> +			  ina226_show_avg, ina2xx_set_value, INA226_AVG);
>
>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
> @@ -258,6 +355,7 @@ static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_curr1_input.dev_attr.attr,
>   	&sensor_dev_attr_power1_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
> +	&sensor_dev_attr_avg_rate.dev_attr.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(ina2xx);
>

[PATCH v2 4/5] hwmon: ina2xx: change hex constants to lower-case

[Bartosz Golaszewski]

Make ina2xx uniform with the majority of the kernel code.

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

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 7fdb586..5095a7a 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -49,7 +49,7 @@
 /* INA226 register definitions */
 #define INA226_MASK_ENABLE		0x06
 #define INA226_ALERT_LIMIT		0x07
-#define INA226_DIE_ID			0xFF
+#define INA226_DIE_ID			0xff
 
 /* shunt resistor sysfs attribute index */
 #define INA2XX_RSHUNT			0x8
@@ -64,7 +64,7 @@
 #define INA2XX_MAX_REGISTERS		8
 
 /* settings - depend on use case */
-#define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
+#define INA219_CONFIG_DEFAULT		0x399f	/* PGA=8 */
 #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
 
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
@@ -74,8 +74,8 @@
 #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_AVG_RD_MASK		0x0e00
+#define INA226_AVG_WR_MASK		0xf1ff
 
 #define INA226_READ_AVG(reg) ((reg & INA226_AVG_RD_MASK) >> 9)
 
-- 
2.1.3

Re: [PATCH v2 4/5] hwmon: ina2xx: change hex constants to lower-case

[Guenter Roeck]

On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> Make ina2xx uniform with the majority of the kernel code.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Not a checkpatch error, warning,or even note. Please don't fix non-problems.

Thanks,
Guenter

[PATCH v2 5/5] hwmon: ina2xx: documentation update for new sysfs attributes

[Bartosz Golaszewski]

Include the rshunt and avg sysfs attributes for ina2xx in the documentation.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 Documentation/hwmon/ina2xx | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index 4223c2d..c100c24 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -44,6 +44,10 @@ 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
-if the device tree is used.
+The shunt value in micro-ohms can be set via platform data or device tree in
+compile-time or via the shunt_resistor attribute in sysfs in 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.
-- 
2.1.3