[RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

=== Background ===
The ina2xx chip has a CALIBRATION register that's used to configure
calibration value to get the scale of current and power measurement
results from their registers. It basically follows two equations:
1) CAL = calibration factor / (current_lsb_A * rshunt_Ohm)
   [ calibration factor: 0.04096 for ina219; 0.00512 for ina226 ]
2) current_lsb_A = max_curr_A / 2^15

According to these two equations, calibration value, current scale,
max_curr_A, and shunt resistor value are related factors.

The driver has been using two methods for these two equations:
A) Used a fixed current_lsb at 1 mA scale
B) Using a fixed calibration value at 4096|2048 for ina219|226

=== Problem ===
Both methods simplify software routine by fixing one factor, which
sacrifices the precision of the hardware measurement results.

Using ina226 for example, with method A, the current scale was 1mA
and the power scale was 25mA.

With method B, calibration value is fixed at 2048 so the precision
is decided by shunt resistor value. It sounds reasonable since the
hardware engineers can use a larger shunt resistor when they need
higher resolution. However, they often concern power burning across
the resistor as well, so the resistor usually won't be so large: a
typical value 1000 micro-ohms, which results in a current scale at
2.5 mA and a power sacle at 62.5 mW.

When measuring a 1.8v voltage running a small current (e.g. 33 mA),
the power value (that's supposed to be 59.4 mW) becomes inaccurate
due to the larger scale (25mA for method A; 62.5 mA for method B).

Also, worth mentioning: method B was added by commit 5d389b125186
("hwmon: (ina2xx) Make calibration register value fixed") stating
that 4096|2048 is the best value for ina219|in226. However, their
datasheets do not seem to mention this "best" keyword. Actually,
they are merely minimum calibration values. See below calculation:

Equation 1, mentioned at Background section:
   CAL = calibration factor / (current_lsb_A * rshunt_Ohm)
=> min CAL = calibration factor / (max current_lsb_A * rshunt_Ohm)
   [calibration factor is 0.00512 for ina226]

Equation 2, mentioned at Background section:
   current_lsb_A = max_curr_A / 2^15
=> max current_lsb_A = (upper bound of max_curr_A) / 2^15

Equation Aux:
   (upper bound of max_curr_A) = max Vshunt / rshunt_Ohm
   [max Vshunt is 81.92 mA mentioned in ina226 datasheet]

Combining three equations:
   min CAL = 0.00512 / (0.08192 / rshunt_Ohm / 2^15 * rshunt_Ohm)
	   = 0.00512 / (0.08192 / 2^15)
	   = 2048

=== Solution ===
Being implied in the previous two paragraphs, both methods result
in inaccurate measurements due to the large lsb values because of
the fixed factors for the calculations.

To address this problem, this patch introduces a sysfs attribute
"max_curr" (maximum expected current_lsb_mA) to allow user to set
the current_lsb_uA via this max_curr according to Equation 2, for
precision based on the use case, so they'll be able to indirectly
control the calibration_value through Equation 1.

(In order not to break existing platforms, this change continues
 using 4096|2048 as a default calibration value but renames them
 from "best" to "min")

Then the change introduces a pair of macros for Equation 1 and 2
and adds the calibration_factor back for Equation 1.

=== Testing ===
Fixed factors:
  shunt_resistor = 1000 uO
  in1_input = 1799 mV

Before:
  max_curr = 81920 mA
  calibration_value = 2048
  when curr1_input = 33 mA,
    power1_input = 125000 uW (expected 59367 uW)
  when curr1_input = 35 mA
    power1_input = 125000 uW (expected 62965 uW)

After:
  set "max_curr" to 5145 mA (smallest within the valid range)
  calibration_value = 32611
  when curr1_input = 32 mA
    power1_input = 58875 uW (expected 57568 uW)
  when curr1_input = 35 mA
    power1_input = 62800 uW (expected 62965 uW)

=== Summary ===
- "shunt_resistor" attribute sets rshunt and "max_curr" boundaries
- "max_curr" sysfs attribute decides current_lsb_uA and power_lsb_uW
- ina2xx_init() still uses 4096|2048 as default calibration setting
* All of them will calibrate as the factor of Equation 1 is changed

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Note:
This change is rebased upon the following Documentation change:
  Documentation: hwmon: Add descriptions for ina2xx sysfs entries
which was sent to the mail-list and is now under review already.

 Documentation/hwmon/ina2xx |   2 +
 drivers/hwmon/ina2xx.c     | 233 ++++++++++++++++++++++++++++++-------
 2 files changed, 192 insertions(+), 43 deletions(-)

diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index 0f36c021192d..ffe53f0f15e1 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -71,6 +71,8 @@ in1_input		Bus voltage(mV) channel
 curr1_input		Current(mA) measurement channel
 power1_input		Power(uW) measurement channel
 shunt_resistor		Shunt resistance(uOhm) channel
+max_curr		Maximum expected current used to improve precision
+			of readings of current and power measurements
 
 Sysfs entries for ina226, ina230 and ina231 only
 -------------
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 07ee19573b3f..2ec909a8ed34 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -86,6 +86,43 @@
  */
 #define INA226_TOTAL_CONV_TIME_DEFAULT	2200
 
+/* Largest calibration value for a 15-bit valid range */
+#define INA2XX_CALIBRATION_MAX		0x7FFF
+
+/*
+ * According to the Equation 1 in the datasheet,
+ *   CAL = (calibration_factor * 10^-6) / (current_lsb_A * rshunt_Ohm)
+ *       = (calibration_factor * 10^6) / (current_lsb_uA * rshunt_uOhm)
+ *
+ * Note: result of calculation is truncated according to datasheet
+ */
+#define INA2XX_EQUATION_1_CAL(factor, x, rshunt_uOhm) \
+	DIV_ROUND_DOWN_ULL((u64)factor * 1000000, x * rshunt_uOhm)
+/*
+ * By exchanging CAL and current_lsb_uA:
+ *   current_lsb_uA = (calibration_factor * 10^6) / (CAL * rshunt_uOhm)
+ *
+ * Since CAL was truncated (see above), using ROUND_UP here
+ */
+#define INA2XX_EQUATION_1_LSB(factor, x, rshunt_uOhm) \
+	DIV_ROUND_UP_ULL((u64)factor * 1000000, x * rshunt_uOhm)
+
+/*
+ * According to the Equation 2 in the datasheet,
+ *   current_lsb_A = max_curr_A / 2^15
+ *   => current_lsb_uA = max_curr_uA / 2^15
+ *                     = (max_curr_mA * 10^3) / 2^15
+ */
+#define INA2XX_EQUATION_2_LSB(max_curr_mA) \
+	DIV_ROUND_CLOSEST_ULL(max_curr_mA * 1000, 1 << 15)
+
+/*
+ * By exchanging current_lsb_uA and max_curr_mA:
+ *   max_curr_mA =  current_lsb_uA * 2^15 / 10^3
+ */
+#define INA2XX_EQUATION_2_MAX(current_lsb_uA) \
+	DIV_ROUND_CLOSEST_ULL(current_lsb_uA * (1 << 15), 1000)
+
 static struct regmap_config ina2xx_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -95,12 +132,14 @@ enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
 	u16 config_default;
-	int calibration_value;
+	int calibration_factor;
+	int calibration_value_min;
 	int registers;
 	int shunt_div;
 	int bus_voltage_shift;
 	int bus_voltage_lsb;	/* uV */
 	int power_lsb_factor;
+	int shunt_voltage_max;
 };
 
 struct ina2xx_data {
@@ -109,6 +148,8 @@ struct ina2xx_data {
 	long rshunt;
 	long current_lsb_uA;
 	long power_lsb_uW;
+	long max_curr_upper;
+	long max_curr_lower;
 	struct mutex config_lock;
 	struct regmap *regmap;
 
@@ -118,21 +159,25 @@ struct ina2xx_data {
 static const struct ina2xx_config ina2xx_config[] = {
 	[ina219] = {
 		.config_default = INA219_CONFIG_DEFAULT,
-		.calibration_value = 4096,
+		.calibration_factor = 40960,
+		.calibration_value_min = 4096,
 		.registers = INA219_REGISTERS,
 		.shunt_div = 100,
 		.bus_voltage_shift = 3,
 		.bus_voltage_lsb = 4000,
 		.power_lsb_factor = 20,
+		.shunt_voltage_max = 32000,	/* uV */
 	},
 	[ina226] = {
 		.config_default = INA226_CONFIG_DEFAULT,
-		.calibration_value = 2048,
+		.calibration_factor = 5120,
+		.calibration_value_min = 2048,
 		.registers = INA226_REGISTERS,
 		.shunt_div = 400,
 		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 25,
+		.shunt_voltage_max = 81920,	/* uV */
 	},
 };
 
@@ -172,28 +217,92 @@ static u16 ina226_interval_to_reg(int interval)
 }
 
 /*
- * Calibration register is set to the best value, which eliminates
- * truncation errors on calculating current register in hardware.
- * According to datasheet (eq. 3) the best values are 2048 for
- * ina226 and 4096 for ina219. They are hardcoded as calibration_value.
+ * Update shunt resistor value and boundaries of maximum current
+ *
+ * The range of maximum current should be within two boundaries:
+ * -- upper <= maximum shunt voltage / shunt resistor value
+ * -- lower <= minimum current_lsb_uA and Equation 2
+ *             |_ minimum current_lsb_uA <= max calibration and Equation 1
  */
-static int ina2xx_calibrate(struct ina2xx_data *data)
+static int ina2xx_set_shunt(struct ina2xx_data *data, long rshunt)
 {
-	return regmap_write(data->regmap, INA2XX_CALIBRATION,
-			    data->config->calibration_value);
+	const struct ina2xx_config *config = data->config;
+	int shunt_voltage_max = config->shunt_voltage_max;
+	long current_lsb_uA_min;
+
+	if (rshunt <= 0)
+		return -EINVAL;
+
+	data->rshunt = rshunt;
+
+	/* Calculate upper limit of max current based on rshunt */
+	data->max_curr_upper = DIV_ROUND_CLOSEST_ULL(shunt_voltage_max * 1000,
+						     rshunt);
+
+	/* Calculate lower limit of max current from INA2XX_CALIBRATION_MAX */
+	current_lsb_uA_min = INA2XX_EQUATION_1_LSB(config->calibration_factor,
+					       INA2XX_CALIBRATION_MAX, rshunt);
+	data->max_curr_lower = INA2XX_EQUATION_2_MAX(current_lsb_uA_min);
+
+	return 0;
 }
 
 /*
- * Initialize the configuration and calibration registers.
+ * Calculate calibration value and configure it
+ *
+ * This function will truncate the calibration value with two boundaries
+ * so callers should not rely on it to do value validation but should do
+ * value check before calling this function.
  */
+static int ina2xx_calibrate(struct ina2xx_data *data)
+{
+	unsigned long cal;
+
+	cal = INA2XX_EQUATION_1_CAL(data->config->calibration_factor,
+				    data->current_lsb_uA, data->rshunt);
+	/*
+	 * Do not bounce an out-of-range value here as this function might
+	 * be called by ina2xx_store_shunt which reflects hardware design
+	 * over a software configuration. Besides, the out-of-range value
+	 * might be resulted from the rounding errors during calculations.
+	 *
+	 * So truncate calibration_value with the lower and upper bounds
+	 */
+	if (cal < data->config->calibration_value_min)
+		cal = data->config->calibration_value_min;
+	if (cal > INA2XX_CALIBRATION_MAX)
+		cal = INA2XX_CALIBRATION_MAX;
+
+	return regmap_write(data->regmap, INA2XX_CALIBRATION, cal);
+}
+
 static int ina2xx_init(struct ina2xx_data *data)
 {
-	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
-			       data->config->config_default);
-	if (ret < 0)
-		return ret;
+	int calibration_factor = data->config->calibration_factor;
+	int cal_min = data->config->calibration_value_min;
+	int ret;
 
-	return ina2xx_calibrate(data);
+	mutex_lock(&data->config_lock);
+
+	/* Initialize current_lsb_uA and power_lsb_uW with cal_min */
+	if (data->current_lsb_uA == 0) {
+		data->current_lsb_uA = INA2XX_EQUATION_1_LSB(calibration_factor,
+							cal_min, data->rshunt);
+		data->power_lsb_uW = data->config->power_lsb_factor *
+				     data->current_lsb_uA;
+	}
+
+	/* Pass data->rshunt for validation and max_curr boundary calculation */
+	ret = ina2xx_set_shunt(data, data->rshunt);
+	if (ret)
+		goto out;
+
+	ret = ina2xx_calibrate(data);
+
+out:
+	mutex_unlock(&data->config_lock);
+
+	return ret;
 }
 
 static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
@@ -306,29 +415,6 @@ static ssize_t ina2xx_show_value(struct device *dev,
 			ina2xx_get_value(data, attr->index, regval));
 }
 
-/*
- * In order to keep calibration register value fixed, the product
- * of current_lsb and shunt_resistor should also be fixed and equal
- * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order
- * to keep the scale.
- */
-static int ina2xx_set_shunt(struct ina2xx_data *data, long val)
-{
-	unsigned int dividend = DIV_ROUND_CLOSEST(1000000000,
-						  data->config->shunt_div);
-	if (val <= 0 || val > dividend)
-		return -EINVAL;
-
-	mutex_lock(&data->config_lock);
-	data->rshunt = val;
-	data->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val);
-	data->power_lsb_uW = data->config->power_lsb_factor *
-			     data->current_lsb_uA;
-	mutex_unlock(&data->config_lock);
-
-	return 0;
-}
-
 static ssize_t ina2xx_show_shunt(struct device *dev,
 			      struct device_attribute *da,
 			      char *buf)
@@ -350,10 +436,67 @@ static ssize_t ina2xx_store_shunt(struct device *dev,
 	if (status < 0)
 		return status;
 
+	mutex_lock(&data->config_lock);
+
 	status = ina2xx_set_shunt(data, val);
 	if (status < 0)
-		return status;
-	return count;
+		goto out;
+
+	status = ina2xx_calibrate(data);
+
+out:
+	mutex_unlock(&data->config_lock);
+
+	return status ? status : count;
+}
+
+static ssize_t ina2xx_show_max_curr(struct device *dev,
+				    struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%llu\n",
+			INA2XX_EQUATION_2_MAX(data->current_lsb_uA));
+}
+
+static ssize_t ina2xx_store_max_curr(struct device *dev,
+				     struct device_attribute *da,
+				     const char *buf, size_t count)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	const struct ina2xx_config *config = data->config;
+	unsigned long max_curr_mA;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &max_curr_mA);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Validate the max expected current value with two boundaries coming
+	 * from the shunt resistor value and valid range of calibration_value
+	 */
+	if (max_curr_mA < data->max_curr_lower ||
+	    max_curr_mA > data->max_curr_upper) {
+		dev_warn(dev, "max_curr out of range [%ld, %ld]\n",
+			 data->max_curr_lower, data->max_curr_upper);
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->config_lock);
+
+	/* max_curr_mA decides the current_lsb_uA directly */
+	data->current_lsb_uA = INA2XX_EQUATION_2_LSB(max_curr_mA);
+	data->power_lsb_uW = config->power_lsb_factor * data->current_lsb_uA;
+
+	ret = ina2xx_calibrate(data);
+
+	mutex_unlock(&data->config_lock);
+
+	dev_dbg(dev, "setting current_lsb_uA %ld, power_lsb_uW = %ld\n",
+		data->current_lsb_uA, data->power_lsb_uW);
+
+	return ret ? ret : count;
 }
 
 static ssize_t ina226_set_interval(struct device *dev,
@@ -415,6 +558,10 @@ static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
 			  ina2xx_show_shunt, ina2xx_store_shunt,
 			  INA2XX_CALIBRATION);
 
+/* max expected current */
+static SENSOR_DEVICE_ATTR(max_curr, 0644,
+			  ina2xx_show_max_curr, ina2xx_store_max_curr, 0);
+
 /* update interval (ina226 only) */
 static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
 			  ina226_show_interval, ina226_set_interval, 0);
@@ -426,6 +573,7 @@ static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_power1_input.dev_attr.attr,
 	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
+	&sensor_dev_attr_max_curr.dev_attr.attr,
 	NULL,
 };
 
@@ -473,8 +621,7 @@ static int ina2xx_probe(struct i2c_client *client,
 		else
 			val = INA2XX_RSHUNT_DEFAULT;
 	}
-
-	ina2xx_set_shunt(data, val);
+	data->rshunt = val;
 
 	ina2xx_regmap_config.max_register = data->config->registers;
 
-- 
2.17.1

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Brüns, Stefan]

On Mittwoch, 21. November 2018 02:26:29 CET Nicolin Chen wrote:
> === Background ===
[...]
> 
> === Problem ===
> Both methods simplify software routine by fixing one factor, which
> sacrifices the precision of the hardware measurement results.
> 
> Using ina226 for example, with method A, the current scale was 1mA
> and the power scale was 25mA.
> 
> With method B, calibration value is fixed at 2048 so the precision
> is decided by shunt resistor value. It sounds reasonable since the
> hardware engineers can use a larger shunt resistor when they need
> higher resolution. However, they often concern power burning across
> the resistor as well, so the resistor usually won't be so large: a
> typical value 1000 micro-ohms, which results in a current scale at
> 2.5 mA and a power sacle at 62.5 mW.

Power loss surely is a concern, but figures should be kept reasonable.

1. You mention 1.8V bus voltage, and currents in the 30mA range. Using the 
1mOhm current shunt:

U_S = R_S * I_S 1e-3 Ohm * 30e-3 A = 30e-6 V  (30uV)
P_S = U_S * I_S = 30e-6V * 30e-3 A = 900e-9W = 0.9 uW

INA219 Power Supply (Datasheet)
Min operating Voltage: 3V
Quiescent Current: 0.7mA
-> Min power: 2.1mW

So the INA219 alone uses 2.1mW, 1000 times more than the shunt.

Another concern may be voltage drop over the shunt, but for this case you have 
a nominal voltage of 1.8V, so 30uV are 0.001%.
 
> When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> the power value (that's supposed to be 59.4 mW) becomes inaccurate
> due to the larger scale (25mA for method A; 62.5 mA for method B).

Another look into the datasheet reveals, even at full gain (PGA=1), the LSB is
40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads out as 3*LSB, 
this anything between 25mA and 35mA. This is the best case figure.

On top of quantisation error, you have the ADC offset voltage (V_OS), which is 
given as (for PGA=1, best case): (+-) 10uV typical, (+-) 100uV max.


So, if you want to have meaningful readouts adjust your shunt to a reasonable 
value. Even 100 times the current value will have no measurable effect on you 
system (power loss: 90uW, voltage drop: 3mV).

Kind regards,

Stefan

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

(Removing "m.purski@samsung.com" since it's not reachable any more)

Hi Stefan,

Thank you for the comments.

On Wed, Nov 21, 2018 at 04:13:01PM +0000, Brüns, Stefan wrote:
> > === Problem ===
> > Both methods simplify software routine by fixing one factor, which
> > sacrifices the precision of the hardware measurement results.
> > 
> > Using ina226 for example, with method A, the current scale was 1mA
> > and the power scale was 25mA.
> > 
> > With method B, calibration value is fixed at 2048 so the precision
> > is decided by shunt resistor value. It sounds reasonable since the
> > hardware engineers can use a larger shunt resistor when they need
> > higher resolution. However, they often concern power burning across
> > the resistor as well, so the resistor usually won't be so large: a
> > typical value 1000 micro-ohms, which results in a current scale at
> > 2.5 mA and a power sacle at 62.5 mW.
> 
> Power loss surely is a concern, but figures should be kept reasonable.
> 
> 1. You mention 1.8V bus voltage, and currents in the 30mA range. Using the 
> 1mOhm current shunt:
> 
> U_S = R_S * I_S 1e-3 Ohm * 30e-3 A = 30e-6 V  (30uV)
> P_S = U_S * I_S = 30e-6V * 30e-3 A = 900e-9W = 0.9 uW
> 
> INA219 Power Supply (Datasheet)
> Min operating Voltage: 3V
> Quiescent Current: 0.7mA
> -> Min power: 2.1mW
> 
> So the INA219 alone uses 2.1mW, 1000 times more than the shunt.

Chip can enter power-down or one-shot mode. Though this upstream
driver doesn't have these two mode supports yet, I am working on
it so they'll be added.

> Another concern may be voltage drop over the shunt, but for this case you have 
> a nominal voltage of 1.8V, so 30uV are 0.001%.
>  
> > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > due to the larger scale (25mA for method A; 62.5 mA for method B).

Just found out that I have typos here: 25mW and 62.5mW.

> Another look into the datasheet reveals, even at full gain (PGA=1), the LSB is
> 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads out as 3*LSB, 
> this anything between 25mA and 35mA. This is the best case figure.

Current read doesn't get affected a lot actually, since hwmon ABI
also reports current value in unit mA. However, the power read is
the matter here. With a 62.5mW power_lsb, power results are kinda
useless on my system.

> On top of quantisation error, you have the ADC offset voltage (V_OS), which is 
> given as (for PGA=1, best case): (+-) 10uV typical, (+-) 100uV max.
>
> So, if you want to have meaningful readouts adjust your shunt to a reasonable 
> value. Even 100 times the current value will have no measurable effect on you 
> system (power loss: 90uW, voltage drop: 3mV).

I understand your last point. But I believe that'd be a separate
idea to improve precision but not an alternative one from driver
aspect. Yes, I agree using a larger shunt resistor is always one
way to get precision, but it doesn't give an excuse for software
driver to be less optimized.

The truth is that calibration value is set to the minimum value,
giving the best range of current measurement meanwhile the worst
precision. You can argue that our hardware engineers might have
been too conservative by putting a not-big-enough resistor. But
they may also argue that software driver doesn't optimize while
the register is totally configurable. I believe both sides have
their own reasons.

But apparently configuring in software is fairly less expensive.
And it doesn't sound convincing to me by telling them "hey guys,
go to change your hardware design because we will not set that
register": We have more than thousands of products in the market
(all battery powered so being conservative), so not possible to
recall all of them back just to place larger shunt resistors.

>From my point of view, there's nothing wrong for hardware folks
being conservative to put a smaller resistor since the software
has the capability to improve precision with a single I2C write.

A separate topic, I actually thought about setting the default
calibration value using a number somewhere in the middle of the
best range and the best precision. But it looks that I am right
to keep the default to minimum value so that no existing users
will be affected.

So my patch merely gives an opportunity for those conservative
designers to fine-tune the software for a better precision. It
is necessary since there are such designers/users and products.
And it isn't supposed to affect existing users.

Thank you
Nicolin

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Brüns, Stefan]

On Mittwoch, 21. November 2018 20:40:52 CET Nicolin Chen wrote:
> (Removing "m.purski@samsung.com" since it's not reachable any more)
> 
> Hi Stefan,
> 
> Thank you for the comments.
> 
> On Wed, Nov 21, 2018 at 04:13:01PM +0000, Brüns, Stefan wrote:
> > > === Problem ===
> > > Both methods simplify software routine by fixing one factor, which
> > > sacrifices the precision of the hardware measurement results.
> > > 
> > > Using ina226 for example, with method A, the current scale was 1mA
> > > and the power scale was 25mA.
> > > 
> > > With method B, calibration value is fixed at 2048 so the precision
> > > is decided by shunt resistor value. It sounds reasonable since the
> > > hardware engineers can use a larger shunt resistor when they need
> > > higher resolution. However, they often concern power burning across
> > > the resistor as well, so the resistor usually won't be so large: a
> > > typical value 1000 micro-ohms, which results in a current scale at
> > > 2.5 mA and a power sacle at 62.5 mW.
> > 
> > Power loss surely is a concern, but figures should be kept reasonable.
> > 
> > 1. You mention 1.8V bus voltage, and currents in the 30mA range. Using the
> > 1mOhm current shunt:
> > 
> > U_S = R_S * I_S 1e-3 Ohm * 30e-3 A = 30e-6 V  (30uV)
> > P_S = U_S * I_S = 30e-6V * 30e-3 A = 900e-9W = 0.9 uW
> > 
> > INA219 Power Supply (Datasheet)
> > Min operating Voltage: 3V
> > Quiescent Current: 0.7mA
> > -> Min power: 2.1mW
> > 
> > So the INA219 alone uses 2.1mW, 1000 times more than the shunt.
> 
> Chip can enter power-down or one-shot mode. Though this upstream
> driver doesn't have these two mode supports yet, I am working on
> it so they'll be added.

The power-down current is 6uA, so even if you never leave power-down mode, you 
are down to 18uW. But on top of that, you need power for the conversion, and 
you need power for communication.

> > Another concern may be voltage drop over the shunt, but for this case you
> > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > 
> > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> 
> Just found out that I have typos here: 25mW and 62.5mW.
> 
> > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > figure.
> Current read doesn't get affected a lot actually, since hwmon ABI
> also reports current value in unit mA. However, the power read is
> the matter here. With a 62.5mW power_lsb, power results are kinda
> useless on my system.

The reported current does not matter here, actually. Internally, the ADC value 
will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
18mW. And thats *only* the quantization noise. It wont get better than that.

Also note, you are apparently using the ina2xx hwmon driver - I strongly 
advise against it, you should either use the ina2xx driver from the IIO 
subsystem directly, or use the IIO driver via iio-hwmon.

1. INA219 is not properly supported by the hwmon driver, see the changes in 
the IIO driver.
2. The IIO driver has many more features:
  - setting the PGA (INA219)
  - setting the bus voltage range (INA219)
  - selecting the conversion time (INA219/226)

There is also always the possibility to read the bus and shunt voltage 
registers and calculate the power manually.

Regards,

Stefan

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Guenter Roeck]

On Wed, Nov 21, 2018 at 10:16:09PM +0000, Brüns, Stefan wrote:
> On Mittwoch, 21. November 2018 20:40:52 CET Nicolin Chen wrote:
> > (Removing "m.purski@samsung.com" since it's not reachable any more)
> > 
> > Hi Stefan,
> > 
> > Thank you for the comments.
> > 
> > On Wed, Nov 21, 2018 at 04:13:01PM +0000, Brüns, Stefan wrote:
> > > > === Problem ===
> > > > Both methods simplify software routine by fixing one factor, which
> > > > sacrifices the precision of the hardware measurement results.
> > > > 
> > > > Using ina226 for example, with method A, the current scale was 1mA
> > > > and the power scale was 25mA.
> > > > 
> > > > With method B, calibration value is fixed at 2048 so the precision
> > > > is decided by shunt resistor value. It sounds reasonable since the
> > > > hardware engineers can use a larger shunt resistor when they need
> > > > higher resolution. However, they often concern power burning across
> > > > the resistor as well, so the resistor usually won't be so large: a
> > > > typical value 1000 micro-ohms, which results in a current scale at
> > > > 2.5 mA and a power sacle at 62.5 mW.
> > > 
> > > Power loss surely is a concern, but figures should be kept reasonable.
> > > 
> > > 1. You mention 1.8V bus voltage, and currents in the 30mA range. Using the
> > > 1mOhm current shunt:
> > > 
> > > U_S = R_S * I_S 1e-3 Ohm * 30e-3 A = 30e-6 V  (30uV)
> > > P_S = U_S * I_S = 30e-6V * 30e-3 A = 900e-9W = 0.9 uW
> > > 
> > > INA219 Power Supply (Datasheet)
> > > Min operating Voltage: 3V
> > > Quiescent Current: 0.7mA
> > > -> Min power: 2.1mW
> > > 
> > > So the INA219 alone uses 2.1mW, 1000 times more than the shunt.
> > 
> > Chip can enter power-down or one-shot mode. Though this upstream
> > driver doesn't have these two mode supports yet, I am working on
> > it so they'll be added.
> 
> The power-down current is 6uA, so even if you never leave power-down mode, you 
> are down to 18uW. But on top of that, you need power for the conversion, and 
> you need power for communication.
> 
> > > Another concern may be voltage drop over the shunt, but for this case you
> > > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > > 
> > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> > 
> > Just found out that I have typos here: 25mW and 62.5mW.
> > 
> > > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > > figure.
> > Current read doesn't get affected a lot actually, since hwmon ABI
> > also reports current value in unit mA. However, the power read is
> > the matter here. With a 62.5mW power_lsb, power results are kinda
> > useless on my system.
> 
> The reported current does not matter here, actually. Internally, the ADC value 
> will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
> 18mW. And thats *only* the quantization noise. It wont get better than that.
> 
> Also note, you are apparently using the ina2xx hwmon driver - I strongly 
> advise against it, you should either use the ina2xx driver from the IIO 
> subsystem directly, or use the IIO driver via iio-hwmon.
> 
> 1. INA219 is not properly supported by the hwmon driver, see the changes in 
> the IIO driver.
> 2. The IIO driver has many more features:
>   - setting the PGA (INA219)
>   - setting the bus voltage range (INA219)
>   - selecting the conversion time (INA219/226)
> 

I agree. Not because of the functionality per se ("not properly supported, see
changes in IIO driver" is a bit vague given the number of changes there),
but you don't really use both ina3221 and ina219 as hwmon device. Afer all,
one-shot mode effectively turns off limit tracking.

Ultimately it would be nice if iio would support limits and alarms, and iio-hwmon
could read and report those. This would enable us to drop the iio-hwmon driver.

Guenter

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

Hi Stefan,

Sorry for a super late reply. I took a long vacation.

On Wed, Nov 21, 2018 at 10:16:09PM +0000, Brüns, Stefan wrote:
> > > Another concern may be voltage drop over the shunt, but for this case you
> > > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > > 
> > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> > 
> > Just found out that I have typos here: 25mW and 62.5mW.
> > 
> > > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > > figure.
> > Current read doesn't get affected a lot actually, since hwmon ABI
> > also reports current value in unit mA. However, the power read is
> > the matter here. With a 62.5mW power_lsb, power results are kinda
> > useless on my system.
> 
> The reported current does not matter here, actually. Internally, the ADC 
> value 
> will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
> 18mW. And thats *only* the quantization noise. It wont get better than that.

The fact is that I do get better power results after setting the
calibration value to 0x7ff. That's the necessity of this change.

> Also note, you are apparently using the ina2xx hwmon driver - I strongly 
> advise against it, you should either use the ina2xx driver from the IIO 
> subsystem directly, or use the IIO driver via iio-hwmon.

The IIO version is also using the minimum calibration value. It
will not solve my problem here.

> There is also always the possibility to read the bus and shunt voltage 
> registers and calculate the power manually.

Won't that be a waste since the hardware could have provided a
better accuracy? It would need more I2C bus reads and cpu cycles
for calculation.

I don't get why you're against a setting for calibration value.
This is how the hardware got designed to cover different cases.
Since we do have such a case that needs some accuracy, it'd be
fair to add it into the driver. Plus, the feature won't change
the minimum calibration value at all -- everyone would be happy.

Thanks
Nicolin

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

Hi Stefan,

Sorry for a super late reply. I took a long vacation.

On Wed, Nov 21, 2018 at 10:16:09PM +0000, Brüns, Stefan wrote:
> > > Another concern may be voltage drop over the shunt, but for this case you
> > > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > > 
> > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> > 
> > Just found out that I have typos here: 25mW and 62.5mW.
> > 
> > > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > > figure.
> > Current read doesn't get affected a lot actually, since hwmon ABI
> > also reports current value in unit mA. However, the power read is
> > the matter here. With a 62.5mW power_lsb, power results are kinda
> > useless on my system.
> 
> The reported current does not matter here, actually. Internally, the ADC value 
> will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
> 18mW. And thats *only* the quantization noise. It wont get better than that.

The fact is that I do get better power results after setting the
calibration value to 0x7ff. That's the necessity of this change.

> Also note, you are apparently using the ina2xx hwmon driver - I strongly 
> advise against it, you should either use the ina2xx driver from the IIO 
> subsystem directly, or use the IIO driver via iio-hwmon.

The IIO version is also using the minimum calibration value. It
will not solve my problem here.

> There is also always the possibility to read the bus and shunt voltage 
> registers and calculate the power manually.

Won't that be a waste since the hardware could have provided a
better accuracy? It would need more I2C bus reads and cpu cycles
for calculation.

I don't get why you're against a setting for calibration value.
This is how the hardware got designed to cover different cases.
Since we do have such a case that needs some accuracy, it'd be
fair to add it into the driver. Plus, the feature won't change
the minimum calibration value at all -- everyone would be happy.

Thanks
Nicolin

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

On Fri, Jan 04, 2019 at 05:26:42PM -0800, Nicolin Chen wrote:
> Hi Stefan,
> 
> Sorry for a super late reply. I took a long vacation.
> 
> On Wed, Nov 21, 2018 at 10:16:09PM +0000, Brüns, Stefan wrote:
> > > > Another concern may be voltage drop over the shunt, but for this case you
> > > > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > > > 
> > > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> > > 
> > > Just found out that I have typos here: 25mW and 62.5mW.
> > > 
> > > > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > > > figure.
> > > Current read doesn't get affected a lot actually, since hwmon ABI
> > > also reports current value in unit mA. However, the power read is
> > > the matter here. With a 62.5mW power_lsb, power results are kinda
> > > useless on my system.
> > 
> > The reported current does not matter here, actually. Internally, the ADC value 
> > will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
> > 18mW. And thats *only* the quantization noise. It wont get better than that.
> 
> The fact is that I do get better power results after setting the
> calibration value to 0x7ff. That's the necessity of this change.
> 
> > Also note, you are apparently using the ina2xx hwmon driver - I strongly 
> > advise against it, you should either use the ina2xx driver from the IIO 
> > subsystem directly, or use the IIO driver via iio-hwmon.
> 
> The IIO version is also using the minimum calibration value. It
> will not solve my problem here.
> 
> > There is also always the possibility to read the bus and shunt voltage 
> > registers and calculate the power manually.
> 
> Won't that be a waste since the hardware could have provided a
> better accuracy? It would need more I2C bus reads and cpu cycles
> for calculation.
> 
> I don't get why you're against a setting for calibration value.
> This is how the hardware got designed to cover different cases.
> Since we do have such a case that needs some accuracy, it'd be
> fair to add it into the driver. Plus, the feature won't change
> the minimum calibration value at all -- everyone would be happy.

Stefan,

Would you please kindly give an ack to this intention so that
we can at least move forward for patch review?

Neither changing hardware resistor values nor simply ignoring
the inaccuracy is acceptable for us. Since configuring that
calibration register value can help our use cases, we really
need this setting to be available in the driver.

Guenter,

Do you have any input regarding this change? I would like to
hear an opinion from you.

Thank you

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Guenter Roeck]

On Thu, Jan 17, 2019 at 02:38:28PM -0800, Nicolin Chen wrote:
> On Fri, Jan 04, 2019 at 05:26:42PM -0800, Nicolin Chen wrote:
> > Hi Stefan,
> > 
> > Sorry for a super late reply. I took a long vacation.
> > 
> > On Wed, Nov 21, 2018 at 10:16:09PM +0000, Brüns, Stefan wrote:
> > > > > Another concern may be voltage drop over the shunt, but for this case you
> > > > > have a nominal voltage of 1.8V, so 30uV are 0.001%.
> > > > > 
> > > > > > When measuring a 1.8v voltage running a small current (e.g. 33 mA),
> > > > > > the power value (that's supposed to be 59.4 mW) becomes inaccurate
> > > > > > due to the larger scale (25mA for method A; 62.5 mA for method B).
> > > > 
> > > > Just found out that I have typos here: 25mW and 62.5mW.
> > > > 
> > > > > Another look into the datasheet reveals, even at full gain (PGA=1), the
> > > > > LSB is 40mV / 2^12 = 40mV / 4096 ~ 10uV. So when the current ADC reads
> > > > > out as 3*LSB, this anything between 25mA and 35mA. This is the best case
> > > > > figure.
> > > > Current read doesn't get affected a lot actually, since hwmon ABI
> > > > also reports current value in unit mA. However, the power read is
> > > > the matter here. With a 62.5mW power_lsb, power results are kinda
> > > > useless on my system.
> > > 
> > > The reported current does not matter here, actually. Internally, the ADC value 
> > > will have an uncertainty of 10mA (at PGA=1). At 1.8V, your uncertainty is 
> > > 18mW. And thats *only* the quantization noise. It wont get better than that.
> > 
> > The fact is that I do get better power results after setting the
> > calibration value to 0x7ff. That's the necessity of this change.
> > 
> > > Also note, you are apparently using the ina2xx hwmon driver - I strongly 
> > > advise against it, you should either use the ina2xx driver from the IIO 
> > > subsystem directly, or use the IIO driver via iio-hwmon.
> > 
> > The IIO version is also using the minimum calibration value. It
> > will not solve my problem here.
> > 
> > > There is also always the possibility to read the bus and shunt voltage 
> > > registers and calculate the power manually.
> > 
> > Won't that be a waste since the hardware could have provided a
> > better accuracy? It would need more I2C bus reads and cpu cycles
> > for calculation.
> > 
> > I don't get why you're against a setting for calibration value.
> > This is how the hardware got designed to cover different cases.
> > Since we do have such a case that needs some accuracy, it'd be
> > fair to add it into the driver. Plus, the feature won't change
> > the minimum calibration value at all -- everyone would be happy.
> 
> Stefan,
> 
> Would you please kindly give an ack to this intention so that
> we can at least move forward for patch review?
> 
> Neither changing hardware resistor values nor simply ignoring
> the inaccuracy is acceptable for us. Since configuring that
> calibration register value can help our use cases, we really
> need this setting to be available in the driver.
> 
> Guenter,
> 
> Do you have any input regarding this change? I would like to
> hear an opinion from you.
> 

I have one claim stating that your change won't make a difference,
and your claim that it would. That leaves me with no choice but to
spend a large amount of time with the datasheet, and possibly with
my evaluation boards, trying to find out who is correct. Unfortunately,
time is scarce to come by those days. I would very much prefer for
you folks to sort out your differences and present me with a single
opinion.

Long term the best solution would really be to replace the hwmon
driver with the iio driver and use the iio-hwmon bridge. The ina2xx
driver doesn't support limits, so there is no real benefit of using
the hwmon driver over the iio driver, at least assuming that the
iio driver supports all the attributes. So maybe you should make your
case for the changes in the iio driver.

Guenter

Re: [RFC][PATCH] hwmon: (ina2xx) Improve current and power reading precision

[Nicolin Chen]

Hi Guenter,

On Thu, Jan 17, 2019 at 03:13:23PM -0800, Guenter Roeck wrote:

> I have one claim stating that your change won't make a difference,
> and your claim that it would. That leaves me with no choice but to
> spend a large amount of time with the datasheet, and possibly with
> my evaluation boards, trying to find out who is correct. Unfortunately,
> time is scarce to come by those days. I would very much prefer for
> you folks to sort out your differences and present me with a single
> opinion.
> 
> Long term the best solution would really be to replace the hwmon
> driver with the iio driver and use the iio-hwmon bridge. The ina2xx
> driver doesn't support limits, so there is no real benefit of using
> the hwmon driver over the iio driver, at least assuming that the
> iio driver supports all the attributes. So maybe you should make your
> case for the changes in the iio driver.

Thanks for the input. Let's drop this hwmon change then.