[lm-sensors] [PATCH v5 0/3] hwmon: ina2xx: new attributes

[lm-sensors] [PATCH v5 0/3] hwmon: ina2xx: new attributes

[Bartosz Golaszewski]

This series 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.

v5:
- avoid a "division by zero in kernel" if reading from an uninitialized
  register

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

Bartosz Golaszewski (3):
  hwmon: ina2xx: make shunt resistance configurable at run-time
  hwmon: ina2xx: allow to change the averaging rate at run-time
  hwmon: ina2xx: documentation update for new sysfs attributes

 Documentation/hwmon/ina2xx |  10 ++-
 drivers/hwmon/ina2xx.c     | 199 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 194 insertions(+), 15 deletions(-)

-- 
2.1.3


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v5 0/3] hwmon: ina2xx: new attributes

[Bartosz Golaszewski]

This series 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.

v5:
- avoid a "division by zero in kernel" if reading from an uninitialized
  register

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

Bartosz Golaszewski (3):
  hwmon: ina2xx: make shunt resistance configurable at run-time
  hwmon: ina2xx: allow to change the averaging rate at run-time
  hwmon: ina2xx: documentation update for new sysfs attributes

 Documentation/hwmon/ina2xx |  10 ++-
 drivers/hwmon/ina2xx.c     | 199 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 194 insertions(+), 15 deletions(-)

-- 
2.1.3

[lm-sensors] [PATCH v5 1/3] 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index e01feba..6e73add 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,7 +51,6 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
-
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
@@ -65,6 +64,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 +89,8 @@ struct ina2xx_data {
 
 	int kind;
 	u16 regs[INA2XX_MAX_REGISTERS];
+
+	long rshunt;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -110,6 +114,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 +173,13 @@ 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:
+		if (data->regs[reg] = 0)
+			val = 0;
+		else
+			val = data->config->calibration_factor
+						/ data->regs[reg];
+		break;
 	default:
 		/* programmer goofed */
 		WARN_ON_ONCE(1);
@@ -187,6 +203,38 @@ 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 ||
+	    val > LONG_MAX)
+		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 +251,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);
@@ -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,19 +287,22 @@ 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];
 
+	if (data->rshunt <= 0 ||
+	    data->rshunt > data->config->calibration_factor)
+		return -ENODEV;
+
 	/* device configuration */
 	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
 					   data->config->config_default);
@@ -261,7 +317,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);
@@ -277,7 +333,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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v5 1/3] 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index e01feba..6e73add 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -51,7 +51,6 @@
 #define INA226_ALERT_LIMIT		0x07
 #define INA226_DIE_ID			0xFF
 
-
 /* register count */
 #define INA219_REGISTERS		6
 #define INA226_REGISTERS		8
@@ -65,6 +64,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 +89,8 @@ struct ina2xx_data {
 
 	int kind;
 	u16 regs[INA2XX_MAX_REGISTERS];
+
+	long rshunt;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -110,6 +114,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 +173,13 @@ 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:
+		if (data->regs[reg] == 0)
+			val = 0;
+		else
+			val = data->config->calibration_factor
+						/ data->regs[reg];
+		break;
 	default:
 		/* programmer goofed */
 		WARN_ON_ONCE(1);
@@ -187,6 +203,38 @@ 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 ||
+	    val > LONG_MAX)
+		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 +251,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);
@@ -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,19 +287,22 @@ 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];
 
+	if (data->rshunt <= 0 ||
+	    data->rshunt > data->config->calibration_factor)
+		return -ENODEV;
+
 	/* device configuration */
 	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
 					   data->config->config_default);
@@ -261,7 +317,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);
@@ -277,7 +333,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: [lm-sensors] [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On 12/10/2014 02:38 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index e01feba..6e73add 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,7 +51,6 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>
> -

While nice, this is an unrelated change.

>   /* register count */
>   #define INA219_REGISTERS		6
>   #define INA226_REGISTERS		8
> @@ -65,6 +64,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 +89,8 @@ struct ina2xx_data {
>
>   	int kind;
>   	u16 regs[INA2XX_MAX_REGISTERS];
> +
> +	long rshunt;
>   };
>
>   static const struct ina2xx_config ina2xx_config[] = {
> @@ -110,6 +114,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 +173,13 @@ 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:
> +		if (data->regs[reg] = 0)
> +			val = 0;
> +		else
> +			val = data->config->calibration_factor
> +						/ data->regs[reg];
> +		break;

This doesn't really make sense. What you want to show is rshunt, not the above.
I think it would be better to write a separate show function to display it.

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

data->config->calibration_factor is <= LONG_MAX, so the second check is unnecessary.
Actually, given that calibration_factor is chip dependent and not necessarily known
by the user, it would make more sense to only bail out on = 0 and then use clamp_val
to limit the range to (1, data->config->calibration_factor).

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On 12/10/2014 02:38 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index e01feba..6e73add 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,7 +51,6 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>
> -

While nice, this is an unrelated change.

>   /* register count */
>   #define INA219_REGISTERS		6
>   #define INA226_REGISTERS		8
> @@ -65,6 +64,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 +89,8 @@ struct ina2xx_data {
>
>   	int kind;
>   	u16 regs[INA2XX_MAX_REGISTERS];
> +
> +	long rshunt;
>   };
>
>   static const struct ina2xx_config ina2xx_config[] = {
> @@ -110,6 +114,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 +173,13 @@ 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:
> +		if (data->regs[reg] == 0)
> +			val = 0;
> +		else
> +			val = data->config->calibration_factor
> +						/ data->regs[reg];
> +		break;

This doesn't really make sense. What you want to show is rshunt, not the above.
I think it would be better to write a separate show function to display it.

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

data->config->calibration_factor is <= LONG_MAX, so the second check is unnecessary.
Actually, given that calibration_factor is chip dependent and not necessarily known
by the user, it would make more sense to only bail out on == 0 and then use clamp_val
to limit the range to (1, data->config->calibration_factor).

Guenter

Re: [lm-sensors] [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Bartosz Golaszewski]

2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> +       case INA2XX_CALIBRATION:
>> +               if (data->regs[reg] = 0)
>> +                       val = 0;
>> +               else
>> +                       val = data->config->calibration_factor
>> +                                               / data->regs[reg];
>> +               break;
>
>
> This doesn't really make sense. What you want to show is rshunt, not the
> above.
> I think it would be better to write a separate show function to display it.

Hi Guenter,

this is the only way to display values read from the chip. It also
tells the user what the actual programmed value is. In fact it was
your suggestion (https://lkml.org/lkml/2014/11/30/233) and I agree
that it's a better alternative. Are you sure you don't want it done
this way?

Bart

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Bartosz Golaszewski]

2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> +       case INA2XX_CALIBRATION:
>> +               if (data->regs[reg] == 0)
>> +                       val = 0;
>> +               else
>> +                       val = data->config->calibration_factor
>> +                                               / data->regs[reg];
>> +               break;
>
>
> This doesn't really make sense. What you want to show is rshunt, not the
> above.
> I think it would be better to write a separate show function to display it.

Hi Guenter,

this is the only way to display values read from the chip. It also
tells the user what the actual programmed value is. In fact it was
your suggestion (https://lkml.org/lkml/2014/11/30/233) and I agree
that it's a better alternative. Are you sure you don't want it done
this way?

Bart

Re: [lm-sensors] [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On Wed, Dec 10, 2014 at 05:46:43PM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> >> +       case INA2XX_CALIBRATION:
> >> +               if (data->regs[reg] = 0)
> >> +                       val = 0;
> >> +               else
> >> +                       val = data->config->calibration_factor
> >> +                                               / data->regs[reg];
> >> +               break;
> >
> >
> > This doesn't really make sense. What you want to show is rshunt, not the
> > above.
> > I think it would be better to write a separate show function to display it.
> 
> Hi Guenter,
> 
> this is the only way to display values read from the chip. It also
> tells the user what the actual programmed value is. In fact it was
> your suggestion (https://lkml.org/lkml/2014/11/30/233) and I agree
> that it's a better alternative. Are you sure you don't want it done
> this way?
> 
I don't really want it at all ;-). Seems to me all those options are broken
one way or another. The only real solution would be to re-instantiate the
driver if the chip was removed and re-inserted, and to provide parameters
either with platform data or with devicetree data.

On a side note, data->rshunt is not really needed anymore with your current
code. The only reason for storing it in data is to use it in
ina2xx_calibration_val(), but you could as well pass it in as parameter
to that function. Even better would be to have a function such as
ina2xx_calibrate() and let it handle the write to the calibration register.
Also, I would suggest to move the above code into its own show function.
It is probably not a good idea to have a single function for all those
conversions in the first place, and converting from 'calibration' to
rshunt goes a bit beyond the original intent to convert from one representation
to the other.  

That still doesn't really solve the structural problem of having to deal with
an uninitialized chip which doesn't like to be uninitialized. But then on the
other side I guess that is really a problem with your platform, not a driver
problem.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On Wed, Dec 10, 2014 at 05:46:43PM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> >> +       case INA2XX_CALIBRATION:
> >> +               if (data->regs[reg] == 0)
> >> +                       val = 0;
> >> +               else
> >> +                       val = data->config->calibration_factor
> >> +                                               / data->regs[reg];
> >> +               break;
> >
> >
> > This doesn't really make sense. What you want to show is rshunt, not the
> > above.
> > I think it would be better to write a separate show function to display it.
> 
> Hi Guenter,
> 
> this is the only way to display values read from the chip. It also
> tells the user what the actual programmed value is. In fact it was
> your suggestion (https://lkml.org/lkml/2014/11/30/233) and I agree
> that it's a better alternative. Are you sure you don't want it done
> this way?
> 
I don't really want it at all ;-). Seems to me all those options are broken
one way or another. The only real solution would be to re-instantiate the
driver if the chip was removed and re-inserted, and to provide parameters
either with platform data or with devicetree data.

On a side note, data->rshunt is not really needed anymore with your current
code. The only reason for storing it in data is to use it in
ina2xx_calibration_val(), but you could as well pass it in as parameter
to that function. Even better would be to have a function such as
ina2xx_calibrate() and let it handle the write to the calibration register.
Also, I would suggest to move the above code into its own show function.
It is probably not a good idea to have a single function for all those
conversions in the first place, and converting from 'calibration' to
rshunt goes a bit beyond the original intent to convert from one representation
to the other.  

That still doesn't really solve the structural problem of having to deal with
an uninitialized chip which doesn't like to be uninitialized. But then on the
other side I guess that is really a problem with your platform, not a driver
problem.

Guenter

Re: [lm-sensors] [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Bartosz Golaszewski]

2014-12-10 19:31 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> I don't really want it at all ;-). Seems to me all those options are broken
> one way or another. The only real solution would be to re-instantiate the
> driver if the chip was removed and re-inserted, and to provide parameters
> either with platform data or with devicetree data.

I see your point, but as Benoit mentioned in one of the previous
messages, in case of our platform it would be impossible to have an
exhaustive list of possible shunt values.

> On a side note, data->rshunt is not really needed anymore with your current
> code. The only reason for storing it in data is to use it in
> ina2xx_calibration_val(), but you could as well pass it in as parameter
> to that function. Even better would be to have a function such as
> ina2xx_calibrate() and let it handle the write to the calibration register.
> Also, I would suggest to move the above code into its own show function.
> It is probably not a good idea to have a single function for all those
> conversions in the first place, and converting from 'calibration' to
> rshunt goes a bit beyond the original intent to convert from one representation
> to the other.

I agree about having separate functions for this parameter and about
not needing data->rshunt in my current code, but we still aren't clear
about whether to read the value from the register or to store it in
memory.

> That still doesn't really solve the structural problem of having to deal with
> an uninitialized chip which doesn't like to be uninitialized. But then on the
> other side I guess that is really a problem with your platform, not a driver
> problem.

This might be a stupid idea, but what about reading the calibration
register first in ina2xx_update_device() and, in case it's value is 0
(POR value according to the datasheet), reinitializing the chip?

Bart

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Bartosz Golaszewski]

2014-12-10 19:31 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> I don't really want it at all ;-). Seems to me all those options are broken
> one way or another. The only real solution would be to re-instantiate the
> driver if the chip was removed and re-inserted, and to provide parameters
> either with platform data or with devicetree data.

I see your point, but as Benoit mentioned in one of the previous
messages, in case of our platform it would be impossible to have an
exhaustive list of possible shunt values.

> On a side note, data->rshunt is not really needed anymore with your current
> code. The only reason for storing it in data is to use it in
> ina2xx_calibration_val(), but you could as well pass it in as parameter
> to that function. Even better would be to have a function such as
> ina2xx_calibrate() and let it handle the write to the calibration register.
> Also, I would suggest to move the above code into its own show function.
> It is probably not a good idea to have a single function for all those
> conversions in the first place, and converting from 'calibration' to
> rshunt goes a bit beyond the original intent to convert from one representation
> to the other.

I agree about having separate functions for this parameter and about
not needing data->rshunt in my current code, but we still aren't clear
about whether to read the value from the register or to store it in
memory.

> That still doesn't really solve the structural problem of having to deal with
> an uninitialized chip which doesn't like to be uninitialized. But then on the
> other side I guess that is really a problem with your platform, not a driver
> problem.

This might be a stupid idea, but what about reading the calibration
register first in ina2xx_update_device() and, in case it's value is 0
(POR value according to the datasheet), reinitializing the chip?

Bart

Re: [lm-sensors] [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On Thu, Dec 11, 2014 at 11:26:07AM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 19:31 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > I don't really want it at all ;-). Seems to me all those options are broken
> > one way or another. The only real solution would be to re-instantiate the
> > driver if the chip was removed and re-inserted, and to provide parameters
> > either with platform data or with devicetree data.
> 
> I see your point, but as Benoit mentioned in one of the previous
> messages, in case of our platform it would be impossible to have an
> exhaustive list of possible shunt values.
> 
> > On a side note, data->rshunt is not really needed anymore with your current
> > code. The only reason for storing it in data is to use it in
> > ina2xx_calibration_val(), but you could as well pass it in as parameter
> > to that function. Even better would be to have a function such as
> > ina2xx_calibrate() and let it handle the write to the calibration register.
> > Also, I would suggest to move the above code into its own show function.
> > It is probably not a good idea to have a single function for all those
> > conversions in the first place, and converting from 'calibration' to
> > rshunt goes a bit beyond the original intent to convert from one representation
> > to the other.
> 
> I agree about having separate functions for this parameter and about
> not needing data->rshunt in my current code, but we still aren't clear
> about whether to read the value from the register or to store it in
> memory.
> 
Let's take the value from the register.

> > That still doesn't really solve the structural problem of having to deal with
> > an uninitialized chip which doesn't like to be uninitialized. But then on the
> > other side I guess that is really a problem with your platform, not a driver
> > problem.
> 
> This might be a stupid idea, but what about reading the calibration
> register first in ina2xx_update_device() and, in case it's value is 0
> (POR value according to the datasheet), reinitializing the chip?
> 
Not that stupid. Without programming the calibration register, the chip
will not report power and current. Expecting user space to "fix" the
situation if that happens seems to be at least as stupid as doing it in
the kernel (and, for example, the 'sensors' command won't be able to fix
it, nor any other program not running as root).

So it might make sense to move the chip initialization into an _init function,
call it from _probe, and call it from _update if, as you say, the calibration
register returns 0. The init function should initialize both the configuration
register and the calibration register. Make sure you update
data->reg[INA2XX_CALIBRATION] when you do that. In the latter case (update
function detects that chip is not initialized), you might also want to add
a dev_warn() as this is an unexpected situation.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck]

On Thu, Dec 11, 2014 at 11:26:07AM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 19:31 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > I don't really want it at all ;-). Seems to me all those options are broken
> > one way or another. The only real solution would be to re-instantiate the
> > driver if the chip was removed and re-inserted, and to provide parameters
> > either with platform data or with devicetree data.
> 
> I see your point, but as Benoit mentioned in one of the previous
> messages, in case of our platform it would be impossible to have an
> exhaustive list of possible shunt values.
> 
> > On a side note, data->rshunt is not really needed anymore with your current
> > code. The only reason for storing it in data is to use it in
> > ina2xx_calibration_val(), but you could as well pass it in as parameter
> > to that function. Even better would be to have a function such as
> > ina2xx_calibrate() and let it handle the write to the calibration register.
> > Also, I would suggest to move the above code into its own show function.
> > It is probably not a good idea to have a single function for all those
> > conversions in the first place, and converting from 'calibration' to
> > rshunt goes a bit beyond the original intent to convert from one representation
> > to the other.
> 
> I agree about having separate functions for this parameter and about
> not needing data->rshunt in my current code, but we still aren't clear
> about whether to read the value from the register or to store it in
> memory.
> 
Let's take the value from the register.

> > That still doesn't really solve the structural problem of having to deal with
> > an uninitialized chip which doesn't like to be uninitialized. But then on the
> > other side I guess that is really a problem with your platform, not a driver
> > problem.
> 
> This might be a stupid idea, but what about reading the calibration
> register first in ina2xx_update_device() and, in case it's value is 0
> (POR value according to the datasheet), reinitializing the chip?
> 
Not that stupid. Without programming the calibration register, the chip
will not report power and current. Expecting user space to "fix" the
situation if that happens seems to be at least as stupid as doing it in
the kernel (and, for example, the 'sensors' command won't be able to fix
it, nor any other program not running as root).

So it might make sense to move the chip initialization into an _init function,
call it from _probe, and call it from _update if, as you say, the calibration
register returns 0. The init function should initialize both the configuration
register and the calibration register. Make sure you update
data->reg[INA2XX_CALIBRATION] when you do that. In the latter case (update
function detects that chip is not initialized), you might also want to add
a dev_warn() as this is an unexpected situation.

Thanks,
Guenter

[lm-sensors] [PATCH v5 2/3] 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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 6e73add..b86474e 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -67,6 +67,15 @@
 /* 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)
+
+/* common attrs, ina226 attrs and NULL */
+#define INA2XX_MAX_ATTRIBUTE_GROUPS	3
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -88,6 +97,7 @@ struct ina2xx_data {
 	unsigned long last_updated;
 
 	int kind;
+	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 	u16 regs[INA2XX_MAX_REGISTERS];
 
 	long rshunt;
@@ -114,11 +124,43 @@ 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))
+		return -ENODEV;
+
+	return ina226_avg_tab[bits];
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -235,6 +277,63 @@ 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;
+	u16 conf;
+
+	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);
+	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 count;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -256,6 +355,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,
@@ -265,7 +368,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)
@@ -276,7 +391,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;
@@ -327,8 +442,12 @@ static int ina2xx_probe(struct i2c_client *client,
 	data->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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v5 2/3] 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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 6e73add..b86474e 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -67,6 +67,15 @@
 /* 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)
+
+/* common attrs, ina226 attrs and NULL */
+#define INA2XX_MAX_ATTRIBUTE_GROUPS	3
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -88,6 +97,7 @@ struct ina2xx_data {
 	unsigned long last_updated;
 
 	int kind;
+	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 	u16 regs[INA2XX_MAX_REGISTERS];
 
 	long rshunt;
@@ -114,11 +124,43 @@ 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))
+		return -ENODEV;
+
+	return ina226_avg_tab[bits];
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -235,6 +277,63 @@ 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;
+	u16 conf;
+
+	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);
+	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 count;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -256,6 +355,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,
@@ -265,7 +368,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)
@@ -276,7 +391,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;
@@ -327,8 +442,12 @@ static int ina2xx_probe(struct i2c_client *client,
 	data->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

[lm-sensors] [PATCH v5 3/3] 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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v5 3/3] 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

Re: [lm-sensors] [PATCH v5 3/3] hwmon: ina2xx: documentation update for new sysfs attributes

[Guenter Roeck]

On 12/10/2014 02:38 AM, Bartosz Golaszewski wrote:
> 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.
>
Please merge the documentation updates into the actual patches as appropriate.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v5 3/3] hwmon: ina2xx: documentation update for new sysfs attributes

[Guenter Roeck]

On 12/10/2014 02:38 AM, Bartosz Golaszewski wrote:
> 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.
>
Please merge the documentation updates into the actual patches as appropriate.

Guenter