[PATCH 1/3] iio: adc: ina2xx: Mask flag bits in bus voltage register

[PATCH 1/3] iio: adc: ina2xx: Mask flag bits in bus voltage register

[Stefan Brüns]

Lower bits of the INA219/220 bus voltage register are conversion
status flags, properly mask the value.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

 drivers/iio/adc/ina2xx-adc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index f387b972e4f4..361fb4e459d5 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -44,7 +44,6 @@
 
 #define INA226_MASK_ENABLE		0x06
 #define INA226_CVRF			BIT(3)
-#define INA219_CNVR			BIT(1)
 
 #define INA2XX_MAX_REGISTERS            8
 
@@ -79,6 +78,11 @@
 #define INA226_ITS_MASK		GENMASK(5, 3)
 #define INA226_SHIFT_ITS(val)	((val) << 3)
 
+/* INA219 Bus voltage register, low bits are flags */
+#define INA219_OVF		BIT(0)
+#define INA219_CNVR		BIT(1)
+#define INA219_BUS_VOLTAGE_MASK	GENMASK(16, 3)
+
 /* Cosmetic macro giving the sampling period for a full P=UxI cycle */
 #define SAMPLING_PERIOD(c)	((c->int_time_vbus + c->int_time_vshunt) \
 				 * c->avg)
@@ -170,6 +174,10 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 		else
 			*val  = regval;
 
+		if ((chip->config->chip_id == ina219) &&
+		    (chan->address == INA2XX_SHUNT_VOLTAGE))
+			*val &= INA219_BUS_VOLTAGE_MASK;
+
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -639,6 +647,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		if (ret < 0)
 			return ret;
 
+		if ((chip->config->chip_id == ina219) &&
+		    (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_BUS_VOLTAGE))
+			val &= INA219_BUS_VOLTAGE_MASK;
+
 		data[i++] = val;
 
 		if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
-- 
2.14.1

Re: [PATCH 1/3] iio: adc: ina2xx: Mask flag bits in bus voltage register

[Jonathan Cameron]

On Sun, 1 Oct 2017 21:48:16 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Lower bits of the INA219/220 bus voltage register are conversion
> status flags, properly mask the value.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Hi, good to find this issue, but the 'right' fix is perhaps a little
more complex than present here.

Jonathan

> ---
> 
>  drivers/iio/adc/ina2xx-adc.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index f387b972e4f4..361fb4e459d5 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -44,7 +44,6 @@
>  
>  #define INA226_MASK_ENABLE		0x06
>  #define INA226_CVRF			BIT(3)
> -#define INA219_CNVR			BIT(1)
>  
>  #define INA2XX_MAX_REGISTERS            8
>  
> @@ -79,6 +78,11 @@
>  #define INA226_ITS_MASK		GENMASK(5, 3)
>  #define INA226_SHIFT_ITS(val)	((val) << 3)
>  
> +/* INA219 Bus voltage register, low bits are flags */
> +#define INA219_OVF		BIT(0)
> +#define INA219_CNVR		BIT(1)
> +#define INA219_BUS_VOLTAGE_MASK	GENMASK(16, 3)
> +
>  /* Cosmetic macro giving the sampling period for a full P=UxI cycle */
>  #define SAMPLING_PERIOD(c)	((c->int_time_vbus + c->int_time_vshunt) \
>  				 * c->avg)
> @@ -170,6 +174,10 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  		else
>  			*val  = regval;
>  
> +		if ((chip->config->chip_id == ina219) &&
> +		    (chan->address == INA2XX_SHUNT_VOLTAGE))
> +			*val &= INA219_BUS_VOLTAGE_MASK;
> +

This first case is fine - up to the fact that it should really be
shifting it appropriately to not give a false impression of precision.
Also correctly unwinding the case below may require some changes here
as well as the scale will change.

>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -639,6 +647,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  		if (ret < 0)
>  			return ret;
>  
> +		if ((chip->config->chip_id == ina219) &&
> +		    (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_BUS_VOLTAGE))
> +			val &= INA219_BUS_VOLTAGE_MASK;
> +

For this I'm not sure this is the correct fix.   The driver should not be lying
about the available data when describing the channel. Right now the
channel description is:

#define INA219_CHAN_VOLTAGE(_index, _address) { \
	.type = IIO_VOLTAGE, \
	.address = (_address), \
	.indexed = 1, \
	.channel = (_index), \
	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
			      BIT(IIO_CHAN_INFO_SCALE) | \
			      BIT(IIO_CHAN_INFO_INT_TIME), \
	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
	.scan_index = (_index), \
	.scan_type = { \
		.sign = 'u', \
		.realbits = 16, \
		.storagebits = 16, \
		.endianness = IIO_LE, \
	} \
}
Reading the datasheet this should be
.realbits = 13,
.shift = 3,
I think

Userspace is then responsible for masking and shifting the
data to not give a false impression of it's precision.

Clearly this with probably have some knock on effects that
will also need fixing.


>  		data[i++] = val;
>  
>  		if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)

[PATCH 2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Stefan Brüns]

According to the ABI documentation, the shunt resistor value should be
specificied in Ohm. As this is also used/documented for the MAX9611,
use the same for the INA2xx driver.

This poses an ABI break for anyone actually altering the shunt value
through the sysfs interface, it does not alter the default value nor
a value set from the devicetree.

Minor change: Fix comment, 1mA is 10^-3A.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

 drivers/iio/adc/ina2xx-adc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 361fb4e459d5..1930f853e8c5 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -127,7 +127,7 @@ struct ina2xx_chip_info {
 	struct task_struct *task;
 	const struct ina2xx_config *config;
 	struct mutex state_lock;
-	unsigned int shunt_resistor;
+	unsigned int shunt_resistor_uohm;
 	int avg;
 	int int_time_vbus; /* Bus voltage integration time uS */
 	int int_time_vshunt; /* Shunt voltage integration time uS */
@@ -444,7 +444,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
 /*
  * Set current LSB to 1mA, shunt is in uOhms
  * (equation 13 in datasheet). We hardcode a Current_LSB
- * of 1.0 x10-6. The only remaining parameter is RShunt.
+ * of 1.0 x10-3. The only remaining parameter is RShunt.
  * There is no need to expose the CALIBRATION register
  * to the user for now. But we need to reset this register
  * if the user updates RShunt after driver init, e.g upon
@@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
 static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
 {
 	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
-				   chip->shunt_resistor);
+				   chip->shunt_resistor_uohm);
 
 	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
 }
@@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
 	if (val <= 0 || val > chip->config->calibration_factor)
 		return -EINVAL;
 
-	chip->shunt_resistor = val;
+	chip->shunt_resistor_uohm = val;
 
 	return 0;
 }
@@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev,
 					  char *buf)
 {
 	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	int vals[2] = { chip->shunt_resistor_uohm, 1000000 };
 
-	return sprintf(buf, "%d\n", chip->shunt_resistor);
+	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
 }
 
 static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
@@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 					   const char *buf, size_t len)
 {
 	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
-	unsigned long val;
-	int ret;
+	int val, val_fract, ret;
 
-	ret = kstrtoul((const char *) buf, 10, &val);
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
 	if (ret)
 		return ret;
 
-	ret = set_shunt_resistor(chip, val);
+	ret = set_shunt_resistor(chip, val * 1000000 + val_fract);
 	if (ret)
 		return ret;
 
-- 
2.14.1

Re: [PATCH 2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Jonathan Cameron]

On Sun, 1 Oct 2017 21:48:17 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> According to the ABI documentation, the shunt resistor value should be
> specificied in Ohm. As this is also used/documented for the MAX9611,
> use the same for the INA2xx driver.
> 
> This poses an ABI break for anyone actually altering the shunt value
> through the sysfs interface, it does not alter the default value nor
> a value set from the devicetree.

Yeah, I messed up on this an missed that we had a number of different
drivers with different scaling on this..  Sorry about that.

So I think we need to apply this to get consistency - changing shunt
resistance from userspace does seem fairly unusual though so fingers
crossed that no one is doing it.

I'm going to do this the slow way though so hopefully we get shouts
before breaking too many people.  Hence I'm routing this through
the next merge window.   After it's been in mainline all the way
to release, if no-one complains we can request it is added to
stable.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> 
> Minor change: Fix comment, 1mA is 10^-3A.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
> 
>  drivers/iio/adc/ina2xx-adc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 361fb4e459d5..1930f853e8c5 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -127,7 +127,7 @@ struct ina2xx_chip_info {
>  	struct task_struct *task;
>  	const struct ina2xx_config *config;
>  	struct mutex state_lock;
> -	unsigned int shunt_resistor;
> +	unsigned int shunt_resistor_uohm;
>  	int avg;
>  	int int_time_vbus; /* Bus voltage integration time uS */
>  	int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -444,7 +444,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>  /*
>   * Set current LSB to 1mA, shunt is in uOhms
>   * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> + * of 1.0 x10-3. The only remaining parameter is RShunt.
>   * There is no need to expose the CALIBRATION register
>   * to the user for now. But we need to reset this register
>   * if the user updates RShunt after driver init, e.g upon
> @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>  static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>  {
>  	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -				   chip->shunt_resistor);
> +				   chip->shunt_resistor_uohm);
>  
>  	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>  }
> @@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>  	if (val <= 0 || val > chip->config->calibration_factor)
>  		return -EINVAL;
>  
> -	chip->shunt_resistor = val;
> +	chip->shunt_resistor_uohm = val;
>  
>  	return 0;
>  }
> @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev,
>  					  char *buf)
>  {
>  	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	int vals[2] = { chip->shunt_resistor_uohm, 1000000 };
>  
> -	return sprintf(buf, "%d\n", chip->shunt_resistor);
> +	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
>  }
>  
>  static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  					   const char *buf, size_t len)
>  {
>  	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> -	unsigned long val;
> -	int ret;
> +	int val, val_fract, ret;
>  
> -	ret = kstrtoul((const char *) buf, 10, &val);
> +	ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
>  	if (ret)
>  		return ret;
>  
> -	ret = set_shunt_resistor(chip, val);
> +	ret = set_shunt_resistor(chip, val * 1000000 + val_fract);
>  	if (ret)
>  		return ret;
>  

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Maciej Purski]

On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> According to the ABI documentation, the shunt resistor value should be
> specificied in Ohm. As this is also used/documented for the MAX9611,
> use the same for the INA2xx driver.
> 
> This poses an ABI break for anyone actually altering the shunt value
> through the sysfs interface, it does not alter the default value nor
> a value set from the devicetree.
> 
> Minor change: Fix comment, 1mA is 10^-3A.
> 

I have just a minor issue. There could be an inconsistency with units as in my 
patch I make current_lsb adjustable and I need it to be in uA (it used to be 
hardcoded as 1 mA so to achieve better precision we need smaller units). So in 
order to keep calibration register properly scaled, I convert uOhms to mOhms on
each set_calibration(). So if both my changes and your changes were applied, on 
each shunt_resistore_store we would be performing multiplication by 10^6 and 
then in set_calibration() division by 10^3 which seems odd to me.

I guess we could keep it as shunt_resistor_ohms instead of shunt_resistor_uohm. 
We could avoid performing division on each shunt_resistor_show() and perform 
multiplication by 10^3 only once in set_calibration() on each 
shunt_resistore_store(). We could then change the default value and perform 
division only on probing, when reading the shunt_resistance from device tree.

There are many other options. It's not a major issue so maybe we could leave it
as it is or you could suggest some changes in my patch.

> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
> 
>   drivers/iio/adc/ina2xx-adc.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 361fb4e459d5..1930f853e8c5 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -127,7 +127,7 @@ struct ina2xx_chip_info {
>   	struct task_struct *task;
>   	const struct ina2xx_config *config;
>   	struct mutex state_lock;
> -	unsigned int shunt_resistor;
> +	unsigned int shunt_resistor_uohm;
>   	int avg;
>   	int int_time_vbus; /* Bus voltage integration time uS */
>   	int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -444,7 +444,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>   /*
>    * Set current LSB to 1mA, shunt is in uOhms
>    * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> + * of 1.0 x10-3. The only remaining parameter is RShunt.
>    * There is no need to expose the CALIBRATION register
>    * to the user for now. But we need to reset this register
>    * if the user updates RShunt after driver init, e.g upon
> @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>   static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>   {
>   	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -				   chip->shunt_resistor);
> +				   chip->shunt_resistor_uohm);
>   
>   	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>   }
> @@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>   	if (val <= 0 || val > chip->config->calibration_factor)
>   		return -EINVAL;
>   
> -	chip->shunt_resistor = val;
> +	chip->shunt_resistor_uohm = val;
>   
>   	return 0;
>   }
> @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev,
>   					  char *buf)
>   {
>   	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	int vals[2] = { chip->shunt_resistor_uohm, 1000000 };
>   
> -	return sprintf(buf, "%d\n", chip->shunt_resistor);
> +	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
>   }
>   
>   static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>   					   const char *buf, size_t len)
>   {
>   	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> -	unsigned long val;
> -	int ret;
> +	int val, val_fract, ret;
>   
> -	ret = kstrtoul((const char *) buf, 10, &val);
> +	ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
>   	if (ret)
>   		return ret;
>   
> -	ret = set_shunt_resistor(chip, val);
> +	ret = set_shunt_resistor(chip, val * 1000000 + val_fract);
>   	if (ret)
>   		return ret;
>   
> 

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Stefan Bruens]

On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> > According to the ABI documentation, the shunt resistor value should be
> > specificied in Ohm. As this is also used/documented for the MAX9611,
> > use the same for the INA2xx driver.
> > 
> > This poses an ABI break for anyone actually altering the shunt value
> > through the sysfs interface, it does not alter the default value nor
> > a value set from the devicetree.
> > 
> > Minor change: Fix comment, 1mA is 10^-3A.
> 
> I have just a minor issue. There could be an inconsistency with units as in
> my patch I make current_lsb adjustable and I need it to be in uA (it used
> to be hardcoded as 1 mA so to achieve better precision we need smaller
> units). So in order to keep calibration register properly scaled, I convert
> uOhms to mOhms on each set_calibration(). So if both my changes and your
> changes were applied, on each shunt_resistore_store we would be performing
> multiplication by 10^6 and then in set_calibration() division by 10^3 which
> seems odd to me.
> 
> I guess we could keep it as shunt_resistor_ohms instead of
> shunt_resistor_uohm. We could avoid performing division on each
> shunt_resistor_show() and perform multiplication by 10^3 only once in
> set_calibration() on each
> shunt_resistore_store(). We could then change the default value and perform
> division only on probing, when reading the shunt_resistance from device
> tree.
> 
> There are many other options. It's not a major issue so maybe we could leave
> it as it is or you could suggest some changes in my patch.
 
Sorry it took me so long to answer ...

The current fixed current_lsb of 1mA is indeed a bad choice for everything but 
a shunt resistor value of 10mOhm, as it truncates the current value. So what 
is a *good* choice?

One important point is the current register is merely more than a convenience 
register. At least for the INA219/220, it provides nothing not achievable in 
software, and for the INA226 family it only has added value if the current is 
varying faster than the readout frequency and the averaging is used.

The precision of the current register is limited by the precision of the shunt 
voltage register, and may be reduced by the applied scaling/calibration 
factor.

The precision of the shunt voltage register is fixed at 10uV (INA219) resp. 
2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the 
noise and offset, but the lsb value is still fixed.

If one wants to carry over the shunt voltage register precision into the 
current register, its important no (or hardly any) truncation happens. The 
terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):

INA219: current = shunt_voltage * cal_register / 4096
INA226: current = shunt_voltage * cal_register / 2048

So any cal value smaller than 4096 (2048) will introduce truncation errors, 
larger values may introduce overflows, if the full input range is used. Now, 
would it not be wise to always use 4096 (2048) for the calibration value?

The raw values from the IIO subsystem are meaningless without their 
accompanying scale factor. Instead of changing the calibration value, why not 
just change the reported scale factor?

More opinions are very welcome.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Maciej Purski]

On 10/14/2017 08:27 PM, Stefan Bruens wrote:
> On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
>> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
>>> According to the ABI documentation, the shunt resistor value should be
>>> specificied in Ohm. As this is also used/documented for the MAX9611,
>>> use the same for the INA2xx driver.
>>>
>>> This poses an ABI break for anyone actually altering the shunt value
>>> through the sysfs interface, it does not alter the default value nor
>>> a value set from the devicetree.
>>>
>>> Minor change: Fix comment, 1mA is 10^-3A.
>>
>> I have just a minor issue. There could be an inconsistency with units as in
>> my patch I make current_lsb adjustable and I need it to be in uA (it used
>> to be hardcoded as 1 mA so to achieve better precision we need smaller
>> units). So in order to keep calibration register properly scaled, I convert
>> uOhms to mOhms on each set_calibration(). So if both my changes and your
>> changes were applied, on each shunt_resistore_store we would be performing
>> multiplication by 10^6 and then in set_calibration() division by 10^3 which
>> seems odd to me.
>>
>> I guess we could keep it as shunt_resistor_ohms instead of
>> shunt_resistor_uohm. We could avoid performing division on each
>> shunt_resistor_show() and perform multiplication by 10^3 only once in
>> set_calibration() on each
>> shunt_resistore_store(). We could then change the default value and perform
>> division only on probing, when reading the shunt_resistance from device
>> tree.
>>
>> There are many other options. It's not a major issue so maybe we could leave
>> it as it is or you could suggest some changes in my patch.
>   
> Sorry it took me so long to answer ...
> 
> The current fixed current_lsb of 1mA is indeed a bad choice for everything but
> a shunt resistor value of 10mOhm, as it truncates the current value. So what
> is a *good* choice?
> 
> One important point is the current register is merely more than a convenience
> register. At least for the INA219/220, it provides nothing not achievable in
> software, and for the INA226 family it only has added value if the current is
> varying faster than the readout frequency and the averaging is used.
> 
> The precision of the current register is limited by the precision of the shunt
> voltage register, and may be reduced by the applied scaling/calibration
> factor.
> 
> The precision of the shunt voltage register is fixed at 10uV (INA219) resp.
> 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the
> noise and offset, but the lsb value is still fixed.
> 
> If one wants to carry over the shunt voltage register precision into the
> current register, its important no (or hardly any) truncation happens. The
> terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):
> 
> INA219: current = shunt_voltage * cal_register / 4096
> INA226: current = shunt_voltage * cal_register / 2048
> 
> So any cal value smaller than 4096 (2048) will introduce truncation errors,
> larger values may introduce overflows, if the full input range is used. Now,
> would it not be wise to always use 4096 (2048) for the calibration value?
> 
> The raw values from the IIO subsystem are meaningless without their
> accompanying scale factor. Instead of changing the calibration value, why not
> just change the reported scale factor?
> 
> More opinions are very welcome.
> 
> Kind regards,
> 
> Stefan
> 

Thanks for the reply.

I agree that cal_register set to 4096 (2048) allows us to eliminate truncaction 
error. However according to your suggestion, if we made cal_reg a fixed value, 
then current_lsb and r_shunt should be also a fixed value, as they are related 
according to formula 8.5 (1)

cal_register = 0.00512 / (current_lsb * r_shunt)

Therefore, changing the scale value wouldn't affect the calib_reg value, so it 
wouldn't give the user any information on the actual current_lsb of the device.
The real value is calculated like this by the user:

processed_value = raw_value * scale

I think that even after changing the scale value processed_value is expected to 
be approximately the same.

Maybe I'm wrong or I didn't precisely understand what you have suggested. I hope
that someone will also comment on that.

Best regards,

Maciej

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 5248 bytes --]

On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote:
> On 10/14/2017 08:27 PM, Stefan Bruens wrote:
> > On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
> >> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> >>> According to the ABI documentation, the shunt resistor value should be
> >>> specificied in Ohm. As this is also used/documented for the MAX9611,
> >>> use the same for the INA2xx driver.
> >>> 
> >>> This poses an ABI break for anyone actually altering the shunt value
> >>> through the sysfs interface, it does not alter the default value nor
> >>> a value set from the devicetree.
> >>> 
> >>> Minor change: Fix comment, 1mA is 10^-3A.
> >> 
> >> I have just a minor issue. There could be an inconsistency with units as
> >> in
> >> my patch I make current_lsb adjustable and I need it to be in uA (it used
> >> to be hardcoded as 1 mA so to achieve better precision we need smaller
> >> units). So in order to keep calibration register properly scaled, I
> >> convert
> >> uOhms to mOhms on each set_calibration(). So if both my changes and your
> >> changes were applied, on each shunt_resistore_store we would be
> >> performing
> >> multiplication by 10^6 and then in set_calibration() division by 10^3
> >> which
> >> seems odd to me.
> >> 
> >> I guess we could keep it as shunt_resistor_ohms instead of
> >> shunt_resistor_uohm. We could avoid performing division on each
> >> shunt_resistor_show() and perform multiplication by 10^3 only once in
> >> set_calibration() on each
> >> shunt_resistore_store(). We could then change the default value and
> >> perform
> >> division only on probing, when reading the shunt_resistance from device
> >> tree.
> >> 
> >> There are many other options. It's not a major issue so maybe we could
> >> leave it as it is or you could suggest some changes in my patch.
> > 
> > Sorry it took me so long to answer ...
> > 
> > The current fixed current_lsb of 1mA is indeed a bad choice for everything
> > but a shunt resistor value of 10mOhm, as it truncates the current value.
> > So what is a *good* choice?
> > 
> > One important point is the current register is merely more than a
> > convenience register. At least for the INA219/220, it provides nothing
> > not achievable in software, and for the INA226 family it only has added
> > value if the current is varying faster than the readout frequency and the
> > averaging is used.
> > 
> > The precision of the current register is limited by the precision of the
> > shunt voltage register, and may be reduced by the applied
> > scaling/calibration factor.
> > 
> > The precision of the shunt voltage register is fixed at 10uV (INA219)
> > resp.
> > 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the
> > noise and offset, but the lsb value is still fixed.
> > 
> > If one wants to carry over the shunt voltage register precision into the
> > current register, its important no (or hardly any) truncation happens. The
> > terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):
> > 
> > INA219: current = shunt_voltage * cal_register / 4096
> > INA226: current = shunt_voltage * cal_register / 2048
> > 
> > So any cal value smaller than 4096 (2048) will introduce truncation
> > errors,
> > larger values may introduce overflows, if the full input range is used.
> > Now, would it not be wise to always use 4096 (2048) for the calibration
> > value?
> > 
> > The raw values from the IIO subsystem are meaningless without their
> > accompanying scale factor. Instead of changing the calibration value, why
> > not just change the reported scale factor?
> > 
> > More opinions are very welcome.
> > 
> > Kind regards,
> > 
> > Stefan
> 
> Thanks for the reply.
> 
> I agree that cal_register set to 4096 (2048) allows us to eliminate
> truncaction error. However according to your suggestion, if we made cal_reg
> a fixed value, then current_lsb and r_shunt should be also a fixed value,
> as they are related according to formula 8.5 (1)
> 
> cal_register = 0.00512 / (current_lsb * r_shunt)

A fixed cal_register only means the current_lsb is implied by the selected 
shunt resistor value.

If you insert 2048 into the equation above, you get:

current_lsb = 2.5 * 1e-6 * r_shunt,

and using Ohms law to replace r_shunt, thats exactly the resolution of the 
shunt_voltage register as specified in the datasheet. The higher the shunt 
resistor value, the smaller the current_lsb.
 
> Therefore, changing the scale value wouldn't affect the calib_reg value, so
> it wouldn't give the user any information on the actual current_lsb of the
> device. The real value is calculated like this by the user:
> 
> processed_value = raw_value * scale
> 
> I think that even after changing the scale value processed_value is expected
> to be approximately the same.

A fixed cal_register means you change the current_lsb by changing the shunt 
resistor. This exposes the full ADC resolution.
 
The current_lsb *is* the scale value.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Maciej Purski]

On 11/06/2017 11:21 AM, Stefan Brüns wrote:
> On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote:
>> On 10/14/2017 08:27 PM, Stefan Bruens wrote:
>>> On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
>>>> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
>>>>> According to the ABI documentation, the shunt resistor value should be
>>>>> specificied in Ohm. As this is also used/documented for the MAX9611,
>>>>> use the same for the INA2xx driver.
>>>>>
>>>>> This poses an ABI break for anyone actually altering the shunt value
>>>>> through the sysfs interface, it does not alter the default value nor
>>>>> a value set from the devicetree.
>>>>>
>>>>> Minor change: Fix comment, 1mA is 10^-3A.
>>>>
>>>> I have just a minor issue. There could be an inconsistency with units as
>>>> in
>>>> my patch I make current_lsb adjustable and I need it to be in uA (it used
>>>> to be hardcoded as 1 mA so to achieve better precision we need smaller
>>>> units). So in order to keep calibration register properly scaled, I
>>>> convert
>>>> uOhms to mOhms on each set_calibration(). So if both my changes and your
>>>> changes were applied, on each shunt_resistore_store we would be
>>>> performing
>>>> multiplication by 10^6 and then in set_calibration() division by 10^3
>>>> which
>>>> seems odd to me.
>>>>
>>>> I guess we could keep it as shunt_resistor_ohms instead of
>>>> shunt_resistor_uohm. We could avoid performing division on each
>>>> shunt_resistor_show() and perform multiplication by 10^3 only once in
>>>> set_calibration() on each
>>>> shunt_resistore_store(). We could then change the default value and
>>>> perform
>>>> division only on probing, when reading the shunt_resistance from device
>>>> tree.
>>>>
>>>> There are many other options. It's not a major issue so maybe we could
>>>> leave it as it is or you could suggest some changes in my patch.
>>>
>>> Sorry it took me so long to answer ...
>>>
>>> The current fixed current_lsb of 1mA is indeed a bad choice for everything
>>> but a shunt resistor value of 10mOhm, as it truncates the current value.
>>> So what is a *good* choice?
>>>
>>> One important point is the current register is merely more than a
>>> convenience register. At least for the INA219/220, it provides nothing
>>> not achievable in software, and for the INA226 family it only has added
>>> value if the current is varying faster than the readout frequency and the
>>> averaging is used.
>>>
>>> The precision of the current register is limited by the precision of the
>>> shunt voltage register, and may be reduced by the applied
>>> scaling/calibration factor.
>>>
>>> The precision of the shunt voltage register is fixed at 10uV (INA219)
>>> resp.
>>> 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the
>>> noise and offset, but the lsb value is still fixed.
>>>
>>> If one wants to carry over the shunt voltage register precision into the
>>> current register, its important no (or hardly any) truncation happens. The
>>> terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):
>>>
>>> INA219: current = shunt_voltage * cal_register / 4096
>>> INA226: current = shunt_voltage * cal_register / 2048
>>>
>>> So any cal value smaller than 4096 (2048) will introduce truncation
>>> errors,
>>> larger values may introduce overflows, if the full input range is used.
>>> Now, would it not be wise to always use 4096 (2048) for the calibration
>>> value?
>>>
>>> The raw values from the IIO subsystem are meaningless without their
>>> accompanying scale factor. Instead of changing the calibration value, why
>>> not just change the reported scale factor?
>>>
>>> More opinions are very welcome.
>>>
>>> Kind regards,
>>>
>>> Stefan
>>
>> Thanks for the reply.
>>
>> I agree that cal_register set to 4096 (2048) allows us to eliminate
>> truncaction error. However according to your suggestion, if we made cal_reg
>> a fixed value, then current_lsb and r_shunt should be also a fixed value,
>> as they are related according to formula 8.5 (1)
>>
>> cal_register = 0.00512 / (current_lsb * r_shunt)
> 
> A fixed cal_register only means the current_lsb is implied by the selected
> shunt resistor value.
> 
> If you insert 2048 into the equation above, you get:
> 
> current_lsb = 2.5 * 1e-6 * r_shunt,
> 
> and using Ohms law to replace r_shunt, thats exactly the resolution of the
> shunt_voltage register as specified in the datasheet. The higher the shunt
> resistor value, the smaller the current_lsb.
>   
>> Therefore, changing the scale value wouldn't affect the calib_reg value, so
>> it wouldn't give the user any information on the actual current_lsb of the
>> device. The real value is calculated like this by the user:
>>
>> processed_value = raw_value * scale
>>
>> I think that even after changing the scale value processed_value is expected
>> to be approximately the same.
> 
> A fixed cal_register means you change the current_lsb by changing the shunt
> resistor. This exposes the full ADC resolution.
>   
> The current_lsb *is* the scale value.
> 
> Kind regards,
> 
> Stefan
> 

Thanks for your explanation. I can do this the way you suggest, so the only 
change with the original driver would be to make current_lsb (which is a scale 
value) follow changes of shunt_resistance value from userspace.

But before I'd like to ask Jonathan for opinion on that.

Kind regards,

Maciej

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Jonathan Cameron]

On Wed, 08 Nov 2017 14:22:08 +0100
Maciej Purski <m.purski@samsung.com> wrote:

> On 11/06/2017 11:21 AM, Stefan Brüns wrote:
> > On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote:  
> >> On 10/14/2017 08:27 PM, Stefan Bruens wrote:  
> >>> On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:  
> >>>> On 10/01/2017 09:48 PM, Stefan Brüns wrote:  
> >>>>> According to the ABI documentation, the shunt resistor value should be
> >>>>> specificied in Ohm. As this is also used/documented for the MAX9611,
> >>>>> use the same for the INA2xx driver.
> >>>>>
> >>>>> This poses an ABI break for anyone actually altering the shunt value
> >>>>> through the sysfs interface, it does not alter the default value nor
> >>>>> a value set from the devicetree.
> >>>>>
> >>>>> Minor change: Fix comment, 1mA is 10^-3A.  
> >>>>
> >>>> I have just a minor issue. There could be an inconsistency with units as
> >>>> in
> >>>> my patch I make current_lsb adjustable and I need it to be in uA (it used
> >>>> to be hardcoded as 1 mA so to achieve better precision we need smaller
> >>>> units). So in order to keep calibration register properly scaled, I
> >>>> convert
> >>>> uOhms to mOhms on each set_calibration(). So if both my changes and your
> >>>> changes were applied, on each shunt_resistore_store we would be
> >>>> performing
> >>>> multiplication by 10^6 and then in set_calibration() division by 10^3
> >>>> which
> >>>> seems odd to me.
> >>>>
> >>>> I guess we could keep it as shunt_resistor_ohms instead of
> >>>> shunt_resistor_uohm. We could avoid performing division on each
> >>>> shunt_resistor_show() and perform multiplication by 10^3 only once in
> >>>> set_calibration() on each
> >>>> shunt_resistore_store(). We could then change the default value and
> >>>> perform
> >>>> division only on probing, when reading the shunt_resistance from device
> >>>> tree.
> >>>>
> >>>> There are many other options. It's not a major issue so maybe we could
> >>>> leave it as it is or you could suggest some changes in my patch.  
> >>>
> >>> Sorry it took me so long to answer ...
> >>>
> >>> The current fixed current_lsb of 1mA is indeed a bad choice for everything
> >>> but a shunt resistor value of 10mOhm, as it truncates the current value.
> >>> So what is a *good* choice?
> >>>
> >>> One important point is the current register is merely more than a
> >>> convenience register. At least for the INA219/220, it provides nothing
> >>> not achievable in software, and for the INA226 family it only has added
> >>> value if the current is varying faster than the readout frequency and the
> >>> averaging is used.
> >>>
> >>> The precision of the current register is limited by the precision of the
> >>> shunt voltage register, and may be reduced by the applied
> >>> scaling/calibration factor.
> >>>
> >>> The precision of the shunt voltage register is fixed at 10uV (INA219)
> >>> resp.
> >>> 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the
> >>> noise and offset, but the lsb value is still fixed.
> >>>
> >>> If one wants to carry over the shunt voltage register precision into the
> >>> current register, its important no (or hardly any) truncation happens. The
> >>> terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):
> >>>
> >>> INA219: current = shunt_voltage * cal_register / 4096
> >>> INA226: current = shunt_voltage * cal_register / 2048
> >>>
> >>> So any cal value smaller than 4096 (2048) will introduce truncation
> >>> errors,
> >>> larger values may introduce overflows, if the full input range is used.
> >>> Now, would it not be wise to always use 4096 (2048) for the calibration
> >>> value?
> >>>
> >>> The raw values from the IIO subsystem are meaningless without their
> >>> accompanying scale factor. Instead of changing the calibration value, why
> >>> not just change the reported scale factor?
> >>>
> >>> More opinions are very welcome.
> >>>
> >>> Kind regards,
> >>>
> >>> Stefan  
> >>
> >> Thanks for the reply.
> >>
> >> I agree that cal_register set to 4096 (2048) allows us to eliminate
> >> truncaction error. However according to your suggestion, if we made cal_reg
> >> a fixed value, then current_lsb and r_shunt should be also a fixed value,
> >> as they are related according to formula 8.5 (1)
> >>
> >> cal_register = 0.00512 / (current_lsb * r_shunt)  
> > 
> > A fixed cal_register only means the current_lsb is implied by the selected
> > shunt resistor value.
> > 
> > If you insert 2048 into the equation above, you get:
> > 
> > current_lsb = 2.5 * 1e-6 * r_shunt,
> > 
> > and using Ohms law to replace r_shunt, thats exactly the resolution of the
> > shunt_voltage register as specified in the datasheet. The higher the shunt
> > resistor value, the smaller the current_lsb.
> >     
> >> Therefore, changing the scale value wouldn't affect the calib_reg value, so
> >> it wouldn't give the user any information on the actual current_lsb of the
> >> device. The real value is calculated like this by the user:
> >>
> >> processed_value = raw_value * scale
> >>
> >> I think that even after changing the scale value processed_value is expected
> >> to be approximately the same.  
> > 
> > A fixed cal_register means you change the current_lsb by changing the shunt
> > resistor. This exposes the full ADC resolution.
> >   
> > The current_lsb *is* the scale value.
> > 
> > Kind regards,
> > 
> > Stefan
> >   
> 
> Thanks for your explanation. I can do this the way you suggest, so the only 
> change with the original driver would be to make current_lsb (which is a scale 
> value) follow changes of shunt_resistance value from userspace.
> 
> But before I'd like to ask Jonathan for opinion on that.
This is what I was thinking as well.  We basically ensure the scale
is right for the shunt_resistance with the ADC operating at it's
best possible accuracy and let userspace sort out the mess
(as we provide it with the data to do so).

> 
> Kind regards,
> 
> Maciej
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Brüns]

Reducing shunt and bus voltage range improves the accuracy, so allow
altering the default settings.

Both settings are exposed as gain values. While for the shunt voltage
this is straightforward, the bus range settings of 32V (default) and 16V
are mapped to gain values of 1 resp. 2, to provide a uniform API to
userspace.

As the gain settings are incorporated into the raw values by the sensor
itself, adjusting of the scale attributes is not necessary.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

 drivers/iio/adc/ina2xx-adc.c | 111 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 108 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 1930f853e8c5..7e9ce43ad15d 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -48,8 +48,10 @@
 #define INA2XX_MAX_REGISTERS            8
 
 /* settings - depend on use case */
-#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
+#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=1/8, BRNG=32V */
 #define INA219_DEFAULT_IT		532
+#define INA219_DEFAULT_BRNG             1   /* 32V */
+#define INA219_DEFAULT_PGA              125 /* 1000/8 */
 #define INA226_CONFIG_DEFAULT           0x4327
 #define INA226_DEFAULT_AVG              4
 #define INA226_DEFAULT_IT		1110
@@ -62,6 +64,14 @@
  */
 #define INA2XX_MODE_MASK	GENMASK(3, 0)
 
+/* Gain for VShunt: 1/8 (default), 1/4, 1/2, 1 */
+#define INA219_PGA_MASK		GENMASK(12, 11)
+#define INA219_SHIFT_PGA(val)	((val) << 11)
+
+/* VBus range: 32V (default), 16V */
+#define INA219_BRNG_MASK	BIT(13)
+#define INA219_SHIFT_BRNG(val)	((val) << 13)
+
 /* Averaging for VBus/VShunt/Power */
 #define INA226_AVG_MASK		GENMASK(11, 9)
 #define INA226_SHIFT_AVG(val)	((val) << 9)
@@ -131,6 +141,8 @@ struct ina2xx_chip_info {
 	int avg;
 	int int_time_vbus; /* Bus voltage integration time uS */
 	int int_time_vshunt; /* Shunt voltage integration time uS */
+	int range_vbus; /* Bus voltage maximum in V */
+	int pga_gain_vshunt; /* Shunt voltage PGA gain */
 	bool allow_async_readout;
 };
 
@@ -227,6 +239,18 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 			*val = 1;
 			return IIO_VAL_INT;
 		}
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->address) {
+		case INA2XX_SHUNT_VOLTAGE:
+			*val = chip->pga_gain_vshunt;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+
+		case INA2XX_BUS_VOLTAGE:
+			*val = chip->range_vbus == 32 ? 1 : 2;
+			return IIO_VAL_INT;
+		}
 	}
 
 	return -EINVAL;
@@ -361,6 +385,74 @@ static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
 	return 0;
 }
 
+static const int ina219_vbus_range_tab[] = { 1, 2 };
+static int ina219_set_vbus_range_denom(struct ina2xx_chip_info *chip,
+				       unsigned int range,
+				       unsigned int *config)
+{
+	if (range == 1)
+		chip->range_vbus = 32;
+	else if (range == 2)
+		chip->range_vbus = 16;
+	else
+		return -EINVAL;
+
+	*config &= ~INA219_BRNG_MASK;
+	*config |= INA219_SHIFT_BRNG(range == 1 ? 1 : 0) & INA219_BRNG_MASK;
+
+	return 0;
+}
+
+static const int ina219_vshunt_gain_tab[] = { 125, 250, 500, 1000 };
+static const int ina219_vshunt_gain_frac[] = {
+	125, 1000, 250, 1000, 500, 1000, 1000, 1000 };
+
+static int ina219_set_vshunt_pga_gain(struct ina2xx_chip_info *chip,
+				      unsigned int gain,
+				      unsigned int *config)
+{
+	int bits;
+
+	if (gain < 125 || gain > 1000)
+		return -EINVAL;
+
+	bits = find_closest(gain, ina219_vshunt_gain_tab,
+			    ARRAY_SIZE(ina219_vshunt_gain_tab));
+
+	chip->pga_gain_vshunt = ina219_vshunt_gain_tab[bits];
+	bits = 3 - bits;
+
+	*config &= ~INA219_PGA_MASK;
+	*config |= INA219_SHIFT_PGA(bits) & INA219_PGA_MASK;
+
+	return 0;
+}
+
+static int ina2xx_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->address) {
+		case INA2XX_SHUNT_VOLTAGE:
+			*type = IIO_VAL_FRACTIONAL;
+			*length = sizeof(ina219_vshunt_gain_frac) / sizeof(int);
+			*vals = ina219_vshunt_gain_frac;
+			return IIO_AVAIL_LIST;
+
+		case INA2XX_BUS_VOLTAGE:
+			*type = IIO_VAL_INT;
+			*length = sizeof(ina219_vbus_range_tab) / sizeof(int);
+			*vals = ina219_vbus_range_tab;
+			return IIO_AVAIL_LIST;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int ina2xx_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
@@ -403,6 +495,14 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
 		}
 		break;
 
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		if (chan->address == INA2XX_SHUNT_VOLTAGE)
+			ret = ina219_set_vshunt_pga_gain(chip, val * 1000000 +
+							 val2, &tmp);
+		else
+			ret = ina219_set_vbus_range_denom(chip, val, &tmp);
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
@@ -547,7 +647,10 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 	.channel = (_index), \
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
 			      BIT(IIO_CHAN_INFO_SCALE) | \
-			      BIT(IIO_CHAN_INFO_INT_TIME), \
+			      BIT(IIO_CHAN_INFO_INT_TIME) | \
+			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
+	.info_mask_separate_available = \
+			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
 	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
 	.scan_index = (_index), \
 	.scan_type = { \
@@ -758,7 +861,6 @@ static IIO_CONST_ATTR_NAMED(ina226_integration_time_available,
 			    integration_time_available,
 			    "0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
 
-
 static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
 		       ina2xx_allow_async_readout_show,
 		       ina2xx_allow_async_readout_store, 0);
@@ -793,6 +895,7 @@ static const struct iio_info ina219_info = {
 	.driver_module = THIS_MODULE,
 	.attrs = &ina219_attribute_group,
 	.read_raw = ina2xx_read_raw,
+	.read_avail = ina2xx_read_avail,
 	.write_raw = ina2xx_write_raw,
 	.debugfs_reg_access = ina2xx_debug_reg,
 };
@@ -874,6 +977,8 @@ static int ina2xx_probe(struct i2c_client *client,
 		chip->avg = 1;
 		ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
 		ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
+		ina219_set_vbus_range_denom(chip, INA219_DEFAULT_BRNG, &val);
+		ina219_set_vshunt_pga_gain(chip, INA219_DEFAULT_PGA, &val);
 	}
 
 	ret = ina2xx_init(chip, val);
-- 
2.14.1

Re: [PATCH 3/3] iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Jonathan Cameron]

On Sun, 1 Oct 2017 21:48:18 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Reducing shunt and bus voltage range improves the accuracy, so allow
> altering the default settings.
> 
> Both settings are exposed as gain values. While for the shunt voltage
> this is straightforward, the bus range settings of 32V (default) and 16V
> are mapped to gain values of 1 resp. 2, to provide a uniform API to
> userspace.
> 
> As the gain settings are incorporated into the raw values by the sensor
> itself, adjusting of the scale attributes is not necessary.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

cc'd Maciej Purski who is working to improve the current calibration
elements - I'd like you both to review each others patches if possible
as the ABI is getting complex in here!

I think what you have here won't interfere with Maciej's work, but
would like you two to confirm that.  Note that patch set probably
needs a revision as I've suggested a rather different approach.

Thanks,

Jonathan
 
> 
> ---
> 
>  drivers/iio/adc/ina2xx-adc.c | 111 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 108 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 1930f853e8c5..7e9ce43ad15d 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -48,8 +48,10 @@
>  #define INA2XX_MAX_REGISTERS            8
>  
>  /* settings - depend on use case */
> -#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=1/8, BRNG=32V */
>  #define INA219_DEFAULT_IT		532
> +#define INA219_DEFAULT_BRNG             1   /* 32V */
> +#define INA219_DEFAULT_PGA              125 /* 1000/8 */
>  #define INA226_CONFIG_DEFAULT           0x4327
>  #define INA226_DEFAULT_AVG              4
>  #define INA226_DEFAULT_IT		1110
> @@ -62,6 +64,14 @@
>   */
>  #define INA2XX_MODE_MASK	GENMASK(3, 0)
>  
> +/* Gain for VShunt: 1/8 (default), 1/4, 1/2, 1 */
> +#define INA219_PGA_MASK		GENMASK(12, 11)
> +#define INA219_SHIFT_PGA(val)	((val) << 11)
> +
> +/* VBus range: 32V (default), 16V */
> +#define INA219_BRNG_MASK	BIT(13)
> +#define INA219_SHIFT_BRNG(val)	((val) << 13)
> +
>  /* Averaging for VBus/VShunt/Power */
>  #define INA226_AVG_MASK		GENMASK(11, 9)
>  #define INA226_SHIFT_AVG(val)	((val) << 9)
> @@ -131,6 +141,8 @@ struct ina2xx_chip_info {
>  	int avg;
>  	int int_time_vbus; /* Bus voltage integration time uS */
>  	int int_time_vshunt; /* Shunt voltage integration time uS */
> +	int range_vbus; /* Bus voltage maximum in V */
> +	int pga_gain_vshunt; /* Shunt voltage PGA gain */
>  	bool allow_async_readout;
>  };
>  
> @@ -227,6 +239,18 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  			*val = 1;
>  			return IIO_VAL_INT;
>  		}
> +
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		switch (chan->address) {
> +		case INA2XX_SHUNT_VOLTAGE:
> +			*val = chip->pga_gain_vshunt;
> +			*val2 = 1000;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case INA2XX_BUS_VOLTAGE:
> +			*val = chip->range_vbus == 32 ? 1 : 2;
> +			return IIO_VAL_INT;
> +		}
>  	}
>  
>  	return -EINVAL;
> @@ -361,6 +385,74 @@ static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>  	return 0;
>  }
>  
> +static const int ina219_vbus_range_tab[] = { 1, 2 };
> +static int ina219_set_vbus_range_denom(struct ina2xx_chip_info *chip,
> +				       unsigned int range,
> +				       unsigned int *config)
> +{
> +	if (range == 1)
> +		chip->range_vbus = 32;
> +	else if (range == 2)
> +		chip->range_vbus = 16;
> +	else
> +		return -EINVAL;
> +
> +	*config &= ~INA219_BRNG_MASK;
> +	*config |= INA219_SHIFT_BRNG(range == 1 ? 1 : 0) & INA219_BRNG_MASK;
> +
> +	return 0;
> +}
> +
> +static const int ina219_vshunt_gain_tab[] = { 125, 250, 500, 1000 };
> +static const int ina219_vshunt_gain_frac[] = {
> +	125, 1000, 250, 1000, 500, 1000, 1000, 1000 };
> +
> +static int ina219_set_vshunt_pga_gain(struct ina2xx_chip_info *chip,
> +				      unsigned int gain,
> +				      unsigned int *config)
> +{
> +	int bits;
> +
> +	if (gain < 125 || gain > 1000)
> +		return -EINVAL;
> +
> +	bits = find_closest(gain, ina219_vshunt_gain_tab,
> +			    ARRAY_SIZE(ina219_vshunt_gain_tab));
> +
> +	chip->pga_gain_vshunt = ina219_vshunt_gain_tab[bits];
> +	bits = 3 - bits;
> +
> +	*config &= ~INA219_PGA_MASK;
> +	*config |= INA219_SHIFT_PGA(bits) & INA219_PGA_MASK;
> +
> +	return 0;
> +}
> +
> +static int ina2xx_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		switch (chan->address) {
> +		case INA2XX_SHUNT_VOLTAGE:
> +			*type = IIO_VAL_FRACTIONAL;
> +			*length = sizeof(ina219_vshunt_gain_frac) / sizeof(int);
> +			*vals = ina219_vshunt_gain_frac;
> +			return IIO_AVAIL_LIST;
> +
> +		case INA2XX_BUS_VOLTAGE:
> +			*type = IIO_VAL_INT;
> +			*length = sizeof(ina219_vbus_range_tab) / sizeof(int);
> +			*vals = ina219_vbus_range_tab;
> +			return IIO_AVAIL_LIST;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int ina2xx_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long mask)
> @@ -403,6 +495,14 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>  		}
>  		break;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
> +			ret = ina219_set_vshunt_pga_gain(chip, val * 1000000 +
> +							 val2, &tmp);
> +		else
> +			ret = ina219_set_vbus_range_denom(chip, val, &tmp);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -547,7 +647,10 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  	.channel = (_index), \
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>  			      BIT(IIO_CHAN_INFO_SCALE) | \
> -			      BIT(IIO_CHAN_INFO_INT_TIME), \
> +			      BIT(IIO_CHAN_INFO_INT_TIME) | \
> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> +	.info_mask_separate_available = \
> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
>  	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>  	.scan_index = (_index), \
>  	.scan_type = { \
> @@ -758,7 +861,6 @@ static IIO_CONST_ATTR_NAMED(ina226_integration_time_available,
>  			    integration_time_available,
>  			    "0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>  
> -
>  static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>  		       ina2xx_allow_async_readout_show,
>  		       ina2xx_allow_async_readout_store, 0);
> @@ -793,6 +895,7 @@ static const struct iio_info ina219_info = {
>  	.driver_module = THIS_MODULE,
>  	.attrs = &ina219_attribute_group,
>  	.read_raw = ina2xx_read_raw,
> +	.read_avail = ina2xx_read_avail,
>  	.write_raw = ina2xx_write_raw,
>  	.debugfs_reg_access = ina2xx_debug_reg,
>  };
> @@ -874,6 +977,8 @@ static int ina2xx_probe(struct i2c_client *client,
>  		chip->avg = 1;
>  		ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
>  		ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
> +		ina219_set_vbus_range_denom(chip, INA219_DEFAULT_BRNG, &val);
> +		ina219_set_vshunt_pga_gain(chip, INA219_DEFAULT_PGA, &val);
>  	}
>  
>  	ret = ina2xx_init(chip, val);

Re: [PATCH 3/3] iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Maciej Purski]

On 10/08/2017 12:07 PM, Jonathan Cameron wrote:
> On Sun, 1 Oct 2017 21:48:18 +0200
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> 
>> Reducing shunt and bus voltage range improves the accuracy, so allow
>> altering the default settings.
>>
>> Both settings are exposed as gain values. While for the shunt voltage
>> this is straightforward, the bus range settings of 32V (default) and 16V
>> are mapped to gain values of 1 resp. 2, to provide a uniform API to
>> userspace.
>>
>> As the gain settings are incorporated into the raw values by the sensor
>> itself, adjusting of the scale attributes is not necessary.
>>
>> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 
> cc'd Maciej Purski who is working to improve the current calibration
> elements - I'd like you both to review each others patches if possible
> as the ABI is getting complex in here!
> 
> I think what you have here won't interfere with Maciej's work, but
> would like you two to confirm that.  Note that patch set probably
> needs a revision as I've suggested a rather different approach.
> 
> Thanks,
> 
> Jonathan
>   

Hi,
it looks fine to me. The patches shouldn't interfere with each other.

Regards,

Maciej

>>
>> ---
>>
>>   drivers/iio/adc/ina2xx-adc.c | 111 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 108 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index 1930f853e8c5..7e9ce43ad15d 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -48,8 +48,10 @@
>>   #define INA2XX_MAX_REGISTERS            8
>>   
>>   /* settings - depend on use case */
>> -#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
>> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=1/8, BRNG=32V */
>>   #define INA219_DEFAULT_IT		532
>> +#define INA219_DEFAULT_BRNG             1   /* 32V */
>> +#define INA219_DEFAULT_PGA              125 /* 1000/8 */
>>   #define INA226_CONFIG_DEFAULT           0x4327
>>   #define INA226_DEFAULT_AVG              4
>>   #define INA226_DEFAULT_IT		1110
>> @@ -62,6 +64,14 @@
>>    */
>>   #define INA2XX_MODE_MASK	GENMASK(3, 0)
>>   
>> +/* Gain for VShunt: 1/8 (default), 1/4, 1/2, 1 */
>> +#define INA219_PGA_MASK		GENMASK(12, 11)
>> +#define INA219_SHIFT_PGA(val)	((val) << 11)
>> +
>> +/* VBus range: 32V (default), 16V */
>> +#define INA219_BRNG_MASK	BIT(13)
>> +#define INA219_SHIFT_BRNG(val)	((val) << 13)
>> +
>>   /* Averaging for VBus/VShunt/Power */
>>   #define INA226_AVG_MASK		GENMASK(11, 9)
>>   #define INA226_SHIFT_AVG(val)	((val) << 9)
>> @@ -131,6 +141,8 @@ struct ina2xx_chip_info {
>>   	int avg;
>>   	int int_time_vbus; /* Bus voltage integration time uS */
>>   	int int_time_vshunt; /* Shunt voltage integration time uS */
>> +	int range_vbus; /* Bus voltage maximum in V */
>> +	int pga_gain_vshunt; /* Shunt voltage PGA gain */
>>   	bool allow_async_readout;
>>   };
>>   
>> @@ -227,6 +239,18 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>   			*val = 1;
>>   			return IIO_VAL_INT;
>>   		}
>> +
>> +	case IIO_CHAN_INFO_HARDWAREGAIN:
>> +		switch (chan->address) {
>> +		case INA2XX_SHUNT_VOLTAGE:
>> +			*val = chip->pga_gain_vshunt;
>> +			*val2 = 1000;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case INA2XX_BUS_VOLTAGE:
>> +			*val = chip->range_vbus == 32 ? 1 : 2;
>> +			return IIO_VAL_INT;
>> +		}
>>   	}
>>   
>>   	return -EINVAL;
>> @@ -361,6 +385,74 @@ static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>>   	return 0;
>>   }
>>   
>> +static const int ina219_vbus_range_tab[] = { 1, 2 };
>> +static int ina219_set_vbus_range_denom(struct ina2xx_chip_info *chip,
>> +				       unsigned int range,
>> +				       unsigned int *config)
>> +{
>> +	if (range == 1)
>> +		chip->range_vbus = 32;
>> +	else if (range == 2)
>> +		chip->range_vbus = 16;
>> +	else
>> +		return -EINVAL;
>> +
>> +	*config &= ~INA219_BRNG_MASK;
>> +	*config |= INA219_SHIFT_BRNG(range == 1 ? 1 : 0) & INA219_BRNG_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +static const int ina219_vshunt_gain_tab[] = { 125, 250, 500, 1000 };
>> +static const int ina219_vshunt_gain_frac[] = {
>> +	125, 1000, 250, 1000, 500, 1000, 1000, 1000 };
>> +
>> +static int ina219_set_vshunt_pga_gain(struct ina2xx_chip_info *chip,
>> +				      unsigned int gain,
>> +				      unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (gain < 125 || gain > 1000)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(gain, ina219_vshunt_gain_tab,
>> +			    ARRAY_SIZE(ina219_vshunt_gain_tab));
>> +
>> +	chip->pga_gain_vshunt = ina219_vshunt_gain_tab[bits];
>> +	bits = 3 - bits;
>> +
>> +	*config &= ~INA219_PGA_MASK;
>> +	*config |= INA219_SHIFT_PGA(bits) & INA219_PGA_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ina2xx_read_avail(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     const int **vals, int *type, int *length,
>> +			     long mask)
>> +{
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_HARDWAREGAIN:
>> +		switch (chan->address) {
>> +		case INA2XX_SHUNT_VOLTAGE:
>> +			*type = IIO_VAL_FRACTIONAL;
>> +			*length = sizeof(ina219_vshunt_gain_frac) / sizeof(int);
>> +			*vals = ina219_vshunt_gain_frac;
>> +			return IIO_AVAIL_LIST;
>> +
>> +		case INA2XX_BUS_VOLTAGE:
>> +			*type = IIO_VAL_INT;
>> +			*length = sizeof(ina219_vbus_range_tab) / sizeof(int);
>> +			*vals = ina219_vbus_range_tab;
>> +			return IIO_AVAIL_LIST;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>   static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>   			    struct iio_chan_spec const *chan,
>>   			    int val, int val2, long mask)
>> @@ -403,6 +495,14 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>   		}
>>   		break;
>>   
>> +	case IIO_CHAN_INFO_HARDWAREGAIN:
>> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
>> +			ret = ina219_set_vshunt_pga_gain(chip, val * 1000000 +
>> +							 val2, &tmp);
>> +		else
>> +			ret = ina219_set_vbus_range_denom(chip, val, &tmp);
>> +		break;
>> +
>>   	default:
>>   		ret = -EINVAL;
>>   	}
>> @@ -547,7 +647,10 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>   	.channel = (_index), \
>>   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>   			      BIT(IIO_CHAN_INFO_SCALE) | \
>> -			      BIT(IIO_CHAN_INFO_INT_TIME), \
>> +			      BIT(IIO_CHAN_INFO_INT_TIME) | \
>> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
>> +	.info_mask_separate_available = \
>> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
>>   	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>   	.scan_index = (_index), \
>>   	.scan_type = { \
>> @@ -758,7 +861,6 @@ static IIO_CONST_ATTR_NAMED(ina226_integration_time_available,
>>   			    integration_time_available,
>>   			    "0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>>   
>> -
>>   static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>>   		       ina2xx_allow_async_readout_show,
>>   		       ina2xx_allow_async_readout_store, 0);
>> @@ -793,6 +895,7 @@ static const struct iio_info ina219_info = {
>>   	.driver_module = THIS_MODULE,
>>   	.attrs = &ina219_attribute_group,
>>   	.read_raw = ina2xx_read_raw,
>> +	.read_avail = ina2xx_read_avail,
>>   	.write_raw = ina2xx_write_raw,
>>   	.debugfs_reg_access = ina2xx_debug_reg,
>>   };
>> @@ -874,6 +977,8 @@ static int ina2xx_probe(struct i2c_client *client,
>>   		chip->avg = 1;
>>   		ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
>>   		ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
>> +		ina219_set_vbus_range_denom(chip, INA219_DEFAULT_BRNG, &val);
>> +		ina219_set_vshunt_pga_gain(chip, INA219_DEFAULT_PGA, &val);
>>   	}
>>   
>>   	ret = ina2xx_init(chip, val);
> 
> 
> 
>