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

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

[Bartosz Golaszewski]

This set of patches fixes an initialization issue we've experienced with the
ina2xx driver and extends the sysfs interface to make the driver configurable
at run-time.

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     | 204 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 198 insertions(+), 16 deletions(-)

-- 
2.1.3

[PATCH 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 can not
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 | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index bfd3f3e..660f8ca 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -110,6 +110,40 @@ static const struct ina2xx_config ina2xx_config[] = {
 	},
 };
 
+static int ina2xx_write_register(const struct i2c_client *client,
+						u8 reg, u16 value)
+{
+	return i2c_smbus_write_word_swapped(client, reg, value);
+}
+
+static int ina2xx_configure(const struct i2c_client *client, u16 value)
+{
+	int status;
+
+	status = ina2xx_write_register(client, INA2XX_CONFIG, value);
+	if (status < 0) {
+		dev_err(&client->dev,
+			"error writing to configuration register: %d\n",
+			status);
+	}
+
+	return status;
+}
+
+static int ina2xx_calibrate(const struct i2c_client *client, u16 value)
+{
+	int status;
+
+	status = ina2xx_write_register(client, INA2XX_CALIBRATION, value);
+	if (status < 0) {
+		dev_err(&client->dev,
+			"error writing to calibration register: %d\n",
+			status);
+	}
+
+	return status;
+}
+
 static struct ina2xx_data *ina2xx_update_device(struct device *dev)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -247,12 +281,15 @@ 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);
+	if (ina2xx_configure(client, data->config->config_default) < 0)
+		return -ENODEV;
+
+	/* Set current LSB to 1mA, shunt is in uOhms
+	 * (equation 13 in datasheet).
+	 */
+	if (ina2xx_calibrate(client,
+			data->config->calibration_factor / shunt) < 0)
+		return -ENODEV;
 
 	data->client = client;
 	mutex_init(&data->update_lock);
-- 
2.1.3

[PATCH 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 | 58 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 660f8ca..eb66081 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 (u16)(data->config->calibration_factor / data->rshunt);
+}
+
 static int ina2xx_write_register(const struct i2c_client *client,
 						u8 reg, u16 value)
 {
@@ -198,6 +210,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);
@@ -221,6 +236,30 @@ 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;
+	s32 status;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if ((kstrtol(buf, 10, &val) == -EINVAL) || (val <= 0))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	data->rshunt = val;
+	status = ina2xx_calibrate(data->client, ina2xx_calibration_val(data));
+	mutex_unlock(&data->update_lock);
+	if (status < 0)
+		return -ENODEV;
+
+	return count;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -237,12 +276,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(rshunt, 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_rshunt.dev_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina2xx);
@@ -255,7 +299,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;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
@@ -267,13 +310,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 */
@@ -287,8 +332,7 @@ static int ina2xx_probe(struct i2c_client *client,
 	/* Set current LSB to 1mA, shunt is in uOhms
 	 * (equation 13 in datasheet).
 	 */
-	if (ina2xx_calibrate(client,
-			data->config->calibration_factor / shunt) < 0)
+	if (ina2xx_calibrate(client, ina2xx_calibration_val(data)) < 0)
 		return -ENODEV;
 
 	data->client = client;
@@ -300,7 +344,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 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index eb66081..0914a72 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,6 +126,13 @@ 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 (u16)(data->config->calibration_factor / data->rshunt);
@@ -156,6 +172,45 @@ static int ina2xx_calibrate(const struct i2c_client *client, u16 value)
 	return status;
 }
 
+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;
+	}
+
+	/* Invalid value */
+	return -1;
+}
+
+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 = ina2xx_configure(data->client, 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);
@@ -213,6 +268,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);
@@ -236,13 +295,25 @@ 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;
-	s32 status;
+	int status;
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -250,12 +321,33 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	if ((kstrtol(buf, 10, &val) == -EINVAL) || (val <= 0))
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->rshunt = val;
-	status = ina2xx_calibrate(data->client, ina2xx_calibration_val(data));
-	mutex_unlock(&data->update_lock);
-	if (status < 0)
-		return -ENODEV;
+	switch (attr->index) {
+	case INA2XX_RSHUNT:
+		mutex_lock(&data->update_lock);
+		data->rshunt = val;
+		status = ina2xx_calibrate(data->client,
+					  ina2xx_calibration_val(data));
+		mutex_unlock(&data->update_lock);
+		if (status < 0)
+			return -ENODEV;
+		break;
+	case INA226_AVG:
+		if (data->kind != ina226)
+			return -ENXIO;
+
+		status = ina226_avg_bits(val);
+		if (status < 0)
+			return -EINVAL;
+
+		if (ina226_update_avg(data, status) < 0)
+			return -ENODEV;
+		break;
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+		val = 0;
+		break;
+	}
 
 	return count;
 }
@@ -278,7 +370,11 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
 
 /* shunt resistance */
 static SENSOR_DEVICE_ATTR(rshunt, 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, S_IRUGO | S_IWUSR | S_IWGRP,
+			  ina226_show_avg, ina2xx_set_value, INA226_AVG);
 
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
@@ -287,6 +383,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_rshunt.dev_attr.attr,
+	&sensor_dev_attr_avg.dev_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina2xx);
-- 
2.1.3

[PATCH 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 0914a72..41a5ad3 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

[PATCH 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..621cf98 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 rshunt 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 sysfs
+attribute. The available rates are: 1, 4, 16, 64, 128, 256, 512 and 1024.
-- 
2.1.3

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

[Guenter Roeck]

On 11/25/2014 07:46 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 can not
> 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 | 49 +++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index bfd3f3e..660f8ca 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -110,6 +110,40 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	},
>   };
>
> +static int ina2xx_write_register(const struct i2c_client *client,
> +						u8 reg, u16 value)
> +{
> +	return i2c_smbus_write_word_swapped(client, reg, value);
> +}
> +
> +static int ina2xx_configure(const struct i2c_client *client, u16 value)
> +{
> +	int status;
> +
> +	status = ina2xx_write_register(client, INA2XX_CONFIG, value);
> +	if (status < 0) {
> +		dev_err(&client->dev,
> +			"error writing to configuration register: %d\n",
> +			status);
> +	}
> +
> +	return status;
> +}
> +
> +static int ina2xx_calibrate(const struct i2c_client *client, u16 value)
> +{
> +	int status;
> +
> +	status = ina2xx_write_register(client, INA2XX_CALIBRATION, value);
> +	if (status < 0) {
> +		dev_err(&client->dev,
> +			"error writing to calibration register: %d\n",
> +			status);
> +	}
> +
> +	return status;
> +}
> +

You are introducing those two functions with the same code just to display
a different error message. That does not provide enough value to have extra
functions and just increases code size.

Just check the return code from ina2xx_write_register directly in the probe
function and bail out there.

Thanks,
Guenter


>   static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -247,12 +281,15 @@ 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);
> +	if (ina2xx_configure(client, data->config->config_default) < 0)
> +		return -ENODEV;
> +
> +	/* Set current LSB to 1mA, shunt is in uOhms
> +	 * (equation 13 in datasheet).
> +	 */
> +	if (ina2xx_calibrate(client,
> +			data->config->calibration_factor / shunt) < 0)
> +		return -ENODEV;
>
>   	data->client = client;
>   	mutex_init(&data->update_lock);
>

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

[Guenter Roeck]

On 11/25/2014 07:47 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.
>
For a given system it should always be known, and it appears unlikely
that there is a system out there where the shunt resistor value can change.

What system exactly are you talking about ?

Thanks,
Guenter

> 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 | 58 ++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 660f8ca..eb66081 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 (u16)(data->config->calibration_factor / data->rshunt);
> +}
> +
>   static int ina2xx_write_register(const struct i2c_client *client,
>   						u8 reg, u16 value)
>   {
> @@ -198,6 +210,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);
> @@ -221,6 +236,30 @@ 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;
> +	s32 status;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if ((kstrtol(buf, 10, &val) == -EINVAL) || (val <= 0))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	data->rshunt = val;
> +	status = ina2xx_calibrate(data->client, ina2xx_calibration_val(data));
> +	mutex_unlock(&data->update_lock);
> +	if (status < 0)
> +		return -ENODEV;
> +
> +	return count;
> +}
> +
>   /* shunt voltage */
>   static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
>   			  INA2XX_SHUNT_VOLTAGE);
> @@ -237,12 +276,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(rshunt, 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_rshunt.dev_attr.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(ina2xx);
> @@ -255,7 +299,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;
>
>   	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
> @@ -267,13 +310,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 */
> @@ -287,8 +332,7 @@ static int ina2xx_probe(struct i2c_client *client,
>   	/* Set current LSB to 1mA, shunt is in uOhms
>   	 * (equation 13 in datasheet).
>   	 */
> -	if (ina2xx_calibrate(client,
> -			data->config->calibration_factor / shunt) < 0)
> +	if (ina2xx_calibrate(client, ina2xx_calibration_val(data)) < 0)
>   		return -ENODEV;
>
>   	data->client = client;
> @@ -300,7 +344,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 3/5] hwmon: ina2xx: allow to change the averaging rate at run-time

[Guenter Roeck]

On 11/25/2014 07:47 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.

I don't see enough value in this to make it configurable.

Guenter

>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/hwmon/ina2xx.c | 115 +++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 106 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index eb66081..0914a72 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,6 +126,13 @@ 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 (u16)(data->config->calibration_factor / data->rshunt);
> @@ -156,6 +172,45 @@ static int ina2xx_calibrate(const struct i2c_client *client, u16 value)
>   	return status;
>   }
>
> +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;
> +	}
> +
> +	/* Invalid value */
> +	return -1;
> +}
> +
> +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 = ina2xx_configure(data->client, 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);
> @@ -213,6 +268,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);
> @@ -236,13 +295,25 @@ 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;
> -	s32 status;
> +	int status;
>
>   	if (IS_ERR(data))
>   		return PTR_ERR(data);
> @@ -250,12 +321,33 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>   	if ((kstrtol(buf, 10, &val) == -EINVAL) || (val <= 0))
>   		return -EINVAL;
>
> -	mutex_lock(&data->update_lock);
> -	data->rshunt = val;
> -	status = ina2xx_calibrate(data->client, ina2xx_calibration_val(data));
> -	mutex_unlock(&data->update_lock);
> -	if (status < 0)
> -		return -ENODEV;
> +	switch (attr->index) {
> +	case INA2XX_RSHUNT:
> +		mutex_lock(&data->update_lock);
> +		data->rshunt = val;
> +		status = ina2xx_calibrate(data->client,
> +					  ina2xx_calibration_val(data));
> +		mutex_unlock(&data->update_lock);
> +		if (status < 0)
> +			return -ENODEV;
> +		break;
> +	case INA226_AVG:
> +		if (data->kind != ina226)
> +			return -ENXIO;
> +
> +		status = ina226_avg_bits(val);
> +		if (status < 0)
> +			return -EINVAL;
> +
> +		if (ina226_update_avg(data, status) < 0)
> +			return -ENODEV;
> +		break;
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +		val = 0;
> +		break;
> +	}
>
>   	return count;
>   }
> @@ -278,7 +370,11 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
>
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR(rshunt, 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, S_IRUGO | S_IWUSR | S_IWGRP,
> +			  ina226_show_avg, ina2xx_set_value, INA226_AVG);
>
>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
> @@ -287,6 +383,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_rshunt.dev_attr.attr,
> +	&sensor_dev_attr_avg.dev_attr.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(ina2xx);
>

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

[Bartosz Gołaszewski]

2014-11-25 16:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> For a given system it should always be known, and it appears unlikely
> that there is a system out there where the shunt resistor value can change.
>
> What system exactly are you talking about ?

We're talking about the ACME cape for BeagleBone Black:
http://baylibre.com/acme/

The probes you can see on the page can be connected in various
configurations and different probes have different shunt resistors.
Their values are set from the user-space after booting, hence the
patch.

Bart

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

[Bartosz Golaszewski]

2014-11-25 16:58 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> You are introducing those two functions with the same code just to display
> a different error message. That does not provide enough value to have extra
> functions and just increases code size.
>
> Just check the return code from ina2xx_write_register directly in the probe
> function and bail out there.

With all six patches from this set applied, each of the two functions
is called twice in the code. Together with the return value checks and
repeated error messages it bloats the code even more. What about a
single function taking the name of the register (in our case
"configuration" or "calibration") as argument in order to print a nice
error message? If this is still too dirty, than we can just print the
hex value of the register.

Bart

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

[Guenter Roeck]

On 11/25/2014 08:25 AM, Bartosz Golaszewski wrote:
> 2014-11-25 16:58 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> You are introducing those two functions with the same code just to display
>> a different error message. That does not provide enough value to have extra
>> functions and just increases code size.
>>
>> Just check the return code from ina2xx_write_register directly in the probe
>> function and bail out there.
>
> With all six patches from this set applied, each of the two functions
> is called twice in the code. Together with the return value checks and
> repeated error messages it bloats the code even more. What about a
> single function taking the name of the register (in our case
> "configuration" or "calibration") as argument in order to print a nice
> error message? If this is still too dirty, than we can just print the
> hex value of the register.
>
> Bart
>

static int ina2xx_write_register(const struct i2c_client *client,
+						u8 reg, u16 value)
+{
+	return i2c_smbus_write_word_swapped(client, reg, value);
+}

Don't tell me that makes any sense. We don't introduce new functions
just to introduce new functions.

The new functions _might_ make a bit more sense if you would
implement the necessary calculations in the functions, but you are
not doing that. Instead, in the next patch, you introduce yet
another function to do the calibration calculation, just to add it
as parameter to the actual calibration function whenever you call it.
To me that is just adding unnecessary code, and I won't accept it.

The following applies to the entire series.

- I don't accept unnecessary ( ).
- I don't accept unnecessary typecasts.
- If you don't accept negative values, use kstrtoul().
- kstrtol can at least in theory return other errors besides -EINVAL.
- Locking should be done in the calling code. It is not needed during
   probe and more appropriate in set functions.
- Any reason for selecting "rshunt" as sysfs attribute name instead
   of, say, shunt-resistor or maybe shunt_resistor ?
- Returning -ENODEV from a set function doesn't make much sense.
- We don't overwrite error codes except in probe functions.

Thanks,
Guenter

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

[Bartosz Golaszewski]

2014-11-25 17:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> The new functions _might_ make a bit more sense if you would
> implement the necessary calculations in the functions, but you are
> not doing that. Instead, in the next patch, you introduce yet
> another function to do the calibration calculation, just to add it
> as parameter to the actual calibration function whenever you call it.

This wasn't my intention, but I'll keep that in mind for the next version.

> - I don't accept unnecessary ( ).
> - I don't accept unnecessary typecasts.
> - If you don't accept negative values, use kstrtoul().
> - kstrtol can at least in theory return other errors besides -EINVAL.

I'll fix those in the next version.

> - Locking should be done in the calling code. It is not needed during
>   probe and more appropriate in set functions.

I don't use locks in probe - they're used directly in
ina2xx_set_value() or indirectly in ina226_update_avg(), but these
functions are never called from probe.

> - Any reason for selecting "rshunt" as sysfs attribute name instead
>   of, say, shunt-resistor or maybe shunt_resistor ?

I'll change it to shunt_resistor for more readability.

> - Returning -ENODEV from a set function doesn't make much sense.

It does make sense in case of ACME (http://baylibre.com/acme/) - a
probe can be disconnected at run-time, which, even without these
patches, would cause ina2xx_update_device() to error out when called
by ina2xx_show_value():

231                         int rv = i2c_smbus_read_word_swapped(client, i);
232                         if (rv < 0) {
233                                 ret = ERR_PTR(rv);
234                                 goto abort;

I just reproduced this behavior in ina2xx_set_value().

> - We don't overwrite error codes except in probe functions.

I'll fix that too.

Bart

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

[Guenter Roeck]

On 11/25/2014 09:50 AM, Bartosz Golaszewski wrote:
> 2014-11-25 17:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> The new functions _might_ make a bit more sense if you would
>> implement the necessary calculations in the functions, but you are
>> not doing that. Instead, in the next patch, you introduce yet
>> another function to do the calibration calculation, just to add it
>> as parameter to the actual calibration function whenever you call it.
>
> This wasn't my intention, but I'll keep that in mind for the next version.
>
>> - I don't accept unnecessary ( ).
>> - I don't accept unnecessary typecasts.
>> - If you don't accept negative values, use kstrtoul().
>> - kstrtol can at least in theory return other errors besides -EINVAL.
>
> I'll fix those in the next version.
>
>> - Locking should be done in the calling code. It is not needed during
>>    probe and more appropriate in set functions.
>
> I don't use locks in probe - they're used directly in
> ina2xx_set_value() or indirectly in ina226_update_avg(), but these
> functions are never called from probe.
>
>> - Any reason for selecting "rshunt" as sysfs attribute name instead
>>    of, say, shunt-resistor or maybe shunt_resistor ?
>
> I'll change it to shunt_resistor for more readability.
>
>> - Returning -ENODEV from a set function doesn't make much sense.
>
> It does make sense in case of ACME (http://baylibre.com/acme/) - a
> probe can be disconnected at run-time, which, even without these
> patches, would cause ina2xx_update_device() to error out when called
> by ina2xx_show_value():
>

It seems to me this is a problem of your architecture. That doesn't
make it a generic problem. Your architecture should detect that a
probe was disconnected and de-instantiate the device automatically
if so, and re-instantiate it if it is re-inserted. Ultimately this
should be done with, for example, devicetree overlays.

Even worse, the remove/reinsertion sequence results in a non-initialized
chip.

This makes me wonder: Is the shunt resistor value set by software,
or by replacing one probe with another ?

Guenter

> 231                         int rv = i2c_smbus_read_word_swapped(client, i);
> 232                         if (rv < 0) {
> 233                                 ret = ERR_PTR(rv);
> 234                                 goto abort;
>
> I just reproduced this behavior in ina2xx_set_value().
>
>> - We don't overwrite error codes except in probe functions.
>
> I'll fix that too.
>
> Bart
>

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

[Bartosz Golaszewski]

2014-11-25 18:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> It seems to me this is a problem of your architecture. That doesn't
> make it a generic problem. Your architecture should detect that a
> probe was disconnected and de-instantiate the device automatically
> if so, and re-instantiate it if it is re-inserted. Ultimately this
> should be done with, for example, devicetree overlays.

You're right and it's planned to be done this way in the future, when
this project exits its prototype phase (around Q2 2015). Nevertheless
I still think that if we're adding a set function, it should behave
like the getters and check the value returned by
i2c_smbus_write_word_swapped().

Bart

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

[Guenter Roeck]

On 11/25/2014 10:22 AM, Bartosz Golaszewski wrote:
> 2014-11-25 18:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>> It seems to me this is a problem of your architecture. That doesn't
>> make it a generic problem. Your architecture should detect that a
>> probe was disconnected and de-instantiate the device automatically
>> if so, and re-instantiate it if it is re-inserted. Ultimately this
>> should be done with, for example, devicetree overlays.
>
> You're right and it's planned to be done this way in the future, when
> this project exits its prototype phase (around Q2 2015). Nevertheless
> I still think that if we're adding a set function, it should behave
> like the getters and check the value returned by
> i2c_smbus_write_word_swapped().
>

I do not question that. I question changing the return value to -ENODEV.

Repeating my earlier question: Is the resistor value changed by software
or by changing the probe ?

Thanks,
Guenter

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

[Guenter Roeck]

On 11/25/2014 10:30 AM, Guenter Roeck wrote:
> On 11/25/2014 10:22 AM, Bartosz Golaszewski wrote:
>> 2014-11-25 18:59 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
>>> It seems to me this is a problem of your architecture. That doesn't
>>> make it a generic problem. Your architecture should detect that a
>>> probe was disconnected and de-instantiate the device automatically
>>> if so, and re-instantiate it if it is re-inserted. Ultimately this
>>> should be done with, for example, devicetree overlays.
>>
>> You're right and it's planned to be done this way in the future, when
>> this project exits its prototype phase (around Q2 2015). Nevertheless
>> I still think that if we're adding a set function, it should behave
>> like the getters and check the value returned by
>> i2c_smbus_write_word_swapped().
>>
>
> I do not question that. I question changing the return value to -ENODEV.
>
> Repeating my earlier question: Is the resistor value changed by software
> or by changing the probe ?
>

Looking into the available documents, I am quite sure that the resistor
is changed by replacing the probe, in other words by pulling the board
with the ina226 and replacing it with another one. Given that, configuring
the shunt resistor value with a sysfs attribute is really the wrong way
to do it; you should use probe specific devicetree overlays instead.

Guenter

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

[Bartosz Golaszewski]

2014-11-26 4:05 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> On 11/25/2014 10:30 AM, Guenter Roeck wrote:
>> Repeating my earlier question: Is the resistor value changed by software
>> or by changing the probe ?
>>
>
> Looking into the available documents, I am quite sure that the resistor
> is changed by replacing the probe, in other words by pulling the board
> with the ina226 and replacing it with another one. Given that, configuring
> the shunt resistor value with a sysfs attribute is really the wrong way
> to do it; you should use probe specific devicetree overlays instead.

Yes, it's changed by replacing the probes.

As for the averaging rate: it's a programmable feature of the chip and
it's useful for our user interface for noise reduction (or the
opposite - to be able to see the actual distortion), can this be
accepted after applying the fixes according to your review?

Bart

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

[Benoit Cousson]

Hi Guenter,

On 26/11/2014 04:05, Guenter Roeck wrote:

[...]

> Looking into the available documents, I am quite sure that the resistor
> is changed by replacing the probe, in other words by pulling the board
> with the ina226 and replacing it with another one. Given that, configuring
> the shunt resistor value with a sysfs attribute is really the wrong way
> to do it; you should use probe specific devicetree overlays instead.

Unfortunately, that's not dynamic enough for all the use cases we need 
to support with the probes.
In fact, most customers will rather put the shunts on their board and 
thus use a shunt-less version of the probe to do the measurement. In 
that case, there is no way we can hard code, even in a DTS, the shunt value.

That's for that kind of usage that we do need to be able to set the 
shunt value at runtime. The probe in that case can be pluged dynamically 
on different board jumpers to do the measurement.

Later, thanks to the web UI, the user will be able to configure the 
shunt value based on the way they were plugged to its boards.

sysfs seems to be the easiest way to do that. I don't think DT overlay 
can handle that, since it is depend of the targeted system and not on 
the measurement system.

Thanks,
Benoit

-- 
Benoît Cousson
BayLibre
Embedded Linux Technology Lab
www.baylibre.com

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

[Guenter Roeck]

On 11/26/2014 01:38 AM, Benoit Cousson wrote:
> Hi Guenter,
>
> On 26/11/2014 04:05, Guenter Roeck wrote:
>
> [...]
>
>> Looking into the available documents, I am quite sure that the resistor
>> is changed by replacing the probe, in other words by pulling the board
>> with the ina226 and replacing it with another one. Given that, configuring
>> the shunt resistor value with a sysfs attribute is really the wrong way
>> to do it; you should use probe specific devicetree overlays instead.
>
> Unfortunately, that's not dynamic enough for all the use cases we need to support with the probes.
> In fact, most customers will rather put the shunts on their board and thus use a shunt-less version of the probe to do the measurement. In that case, there is no way we can hard code, even in a DTS, the shunt value.
>
> That's for that kind of usage that we do need to be able to set the shunt value at runtime. The probe in that case can be pluged dynamically on different board jumpers to do the measurement.
>
> Later, thanks to the web UI, the user will be able to configure the shunt value based on the way they were plugged to its boards.
>
> sysfs seems to be the easiest way to do that. I don't think DT overlay can handle that, since it is depend of the targeted system and not on the measurement system.
>

I just noticed that you did not copy the lm-sensors mailing list.

I am not really happy with this, and want to get some more feedback
from the list before I accept more or less arbitrary attributes.
Jean, any comments ?

Thanks,
Guenter

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

[Guenter Roeck]

On 11/26/2014 01:38 AM, Benoit Cousson wrote:
> Hi Guenter,
>
> On 26/11/2014 04:05, Guenter Roeck wrote:
>
> [...]
>
>> Looking into the available documents, I am quite sure that the resistor
>> is changed by replacing the probe, in other words by pulling the board
>> with the ina226 and replacing it with another one. Given that, configuring
>> the shunt resistor value with a sysfs attribute is really the wrong way
>> to do it; you should use probe specific devicetree overlays instead.
>
> Unfortunately, that's not dynamic enough for all the use cases we need to support with the probes.
> In fact, most customers will rather put the shunts on their board and thus use a shunt-less version of the probe to do the measurement. In that case, there is no way we can hard code, even in a DTS, the shunt value.
>
> That's for that kind of usage that we do need to be able to set the shunt value at runtime. The probe in that case can be pluged dynamically on different board jumpers to do the measurement.
>
> Later, thanks to the web UI, the user will be able to configure the shunt value based on the way they were plugged to its boards.
>
> sysfs seems to be the easiest way to do that. I don't think DT overlay can handle that, since it is depend of the targeted system and not on the measurement system.
>

I just noticed that you did not copy the lm-sensors mailing list.

I am not really happy with this, and want to get some more feedback
from the list before I accept more or less arbitrary attributes.
Jean, any comments ?

Thanks,
Guenter


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

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

[Jean Delvare]

On Wed, 26 Nov 2014 11:04:35 -0800, Guenter Roeck wrote:
> On 11/26/2014 01:38 AM, Benoit Cousson wrote:
> > Unfortunately, that's not dynamic enough for all the use cases we need to support with the probes.
> > In fact, most customers will rather put the shunts on their board and thus use a shunt-less version of the probe to do the measurement. In that case, there is no way we can hard code, even in a DTS, the shunt value.
> >
> > That's for that kind of usage that we do need to be able to set the shunt value at runtime. The probe in that case can be pluged dynamically on different board jumpers to do the measurement.
> >
> > Later, thanks to the web UI, the user will be able to configure the shunt value based on the way they were plugged to its boards.
> >
> > sysfs seems to be the easiest way to do that. I don't think DT overlay can handle that, since it is depend of the targeted system and not on the measurement system.
> >
> 
> I just noticed that you did not copy the lm-sensors mailing list.
> 
> I am not really happy with this, and want to get some more feedback
> from the list before I accept more or less arbitrary attributes.
> Jean, any comments ?

No opinion on the matter, sorry.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Wed, 26 Nov 2014 11:04:35 -0800, Guenter Roeck wrote:
> On 11/26/2014 01:38 AM, Benoit Cousson wrote:
> > Unfortunately, that's not dynamic enough for all the use cases we need to support with the probes.
> > In fact, most customers will rather put the shunts on their board and thus use a shunt-less version of the probe to do the measurement. In that case, there is no way we can hard code, even in a DTS, the shunt value.
> >
> > That's for that kind of usage that we do need to be able to set the shunt value at runtime. The probe in that case can be pluged dynamically on different board jumpers to do the measurement.
> >
> > Later, thanks to the web UI, the user will be able to configure the shunt value based on the way they were plugged to its boards.
> >
> > sysfs seems to be the easiest way to do that. I don't think DT overlay can handle that, since it is depend of the targeted system and not on the measurement system.
> >
> 
> I just noticed that you did not copy the lm-sensors mailing list.
> 
> I am not really happy with this, and want to get some more feedback
> from the list before I accept more or less arbitrary attributes.
> Jean, any comments ?

No opinion on the matter, sorry.

-- 
Jean Delvare
SUSE L3 Support

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