Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1621758 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-04-12 05:10 +0200
    Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and  Bus Voltage range Jonathan Cameron <jic23@kernel.org> - 2017-04-14 17:20 +0200
      Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range Stefan Bruens <stefan.bruens@rwth-aachen.de> - 2017-04-18 00:10 +0200

#1621758 — [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

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

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/iio/adc/ina2xx-adc.c | 165 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 164 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index d1678f886297..856409ecceb3 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -47,8 +47,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             32
+#define INA219_DEFAULT_PGA              125 /* 1000/8 */
 #define INA226_CONFIG_DEFAULT           0x4327
 #define INA226_DEFAULT_AVG              4
 #define INA226_DEFAULT_IT		1110
@@ -61,6 +63,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)
@@ -125,6 +135,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;
 };
 
@@ -351,6 +363,44 @@ static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
 	return 0;
 }
 
+static int ina219_set_vbus_range(struct ina2xx_chip_info *chip,
+				 unsigned int range,
+				 unsigned int *config)
+{
+	if (range != 16 && range != 32)
+		return -EINVAL;
+
+	chip->range_vbus = range;
+
+	*config &= ~INA219_BRNG_MASK;
+	*config |= INA219_SHIFT_BRNG(range == 32 ? 1 : 0) & INA219_BRNG_MASK;
+
+	return 0;
+}
+
+static const int ina219_vshunt_gain_tab[] = { 125, 250, 500, 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_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
@@ -485,6 +535,92 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 	return len;
 }
 
+static ssize_t ina219_bus_voltage_range_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", chip->range_vbus);
+}
+
+static ssize_t ina219_bus_voltage_range_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t len)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	unsigned long val;
+	unsigned int config, tmp;
+	int ret;
+
+	ret = kstrtoul((const char *) buf, 10, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&chip->state_lock);
+
+	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
+	if (ret)
+		goto err;
+
+	tmp = config;
+
+	ret = ina219_set_vbus_range(chip, val, &config);
+
+	if (!ret && (tmp != config))
+		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+
+	if (!ret)
+		ret = len;
+err:
+	mutex_unlock(&chip->state_lock);
+
+	return ret;
+}
+
+static ssize_t ina219_shunt_voltage_gain_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	int vals[2] = { chip->pga_gain_vshunt, 1000 };
+
+	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
+}
+
+static ssize_t ina219_shunt_voltage_gain_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t len)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	unsigned int config, tmp;
+	int val, val_fract, ret;
+
+	ret = iio_str_to_fixpoint(buf, 100, &val, &val_fract);
+	if (ret)
+		return ret;
+
+	mutex_lock(&chip->state_lock);
+
+	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
+	if (ret)
+		goto err;
+
+	tmp = config;
+
+	ret = ina219_set_vshunt_pga_gain(chip, val * 1000 + val_fract, &config);
+
+	if (!ret && (tmp != config))
+		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+
+	if (!ret)
+		ret = len;
+err:
+	mutex_unlock(&chip->state_lock);
+
+	return ret;
+}
+
 #define INA2XX_CHAN(_type, _index, _address) { \
 	.type = (_type), \
 	.address = (_address), \
@@ -698,6 +834,15 @@ 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");
 
+/* INA219/220 Bus voltage range */
+static IIO_CONST_ATTR_NAMED(ina219_bus_voltage_range_available,
+			    in_bus_voltage_range_available,
+			    "16 32");
+
+/* INA219/220 Shunt voltage PGA gain */
+static IIO_CONST_ATTR_NAMED(ina219_shunt_voltage_gain_available,
+			    in_shunt_voltage_gain_available,
+			    "0.125 0.25 0.5 1");
 
 static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
 		       ina2xx_allow_async_readout_show,
@@ -707,9 +852,25 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
 		       ina2xx_shunt_resistor_show,
 		       ina2xx_shunt_resistor_store, 0);
 
+static IIO_DEVICE_ATTR_NAMED(ina219_bus_voltage_range,
+			     in_bus_voltage_range,
+			     S_IRUGO | S_IWUSR,
+			     ina219_bus_voltage_range_show,
+			     ina219_bus_voltage_range_store, 0);
+
+static IIO_DEVICE_ATTR_NAMED(ina219_shunt_voltage_gain,
+			     in_shunt_voltage_gain,
+			     S_IRUGO | S_IWUSR,
+			     ina219_shunt_voltage_gain_show,
+			     ina219_shunt_voltage_gain_store, 0);
+
 static struct attribute *ina219_attributes[] = {
 	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
 	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
+	&iio_dev_attr_ina219_bus_voltage_range.dev_attr.attr,
+	&iio_const_attr_ina219_bus_voltage_range_available.dev_attr.attr,
+	&iio_dev_attr_ina219_shunt_voltage_gain.dev_attr.attr,
+	&iio_const_attr_ina219_shunt_voltage_gain_available.dev_attr.attr,
 	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
 	NULL,
 };
@@ -809,6 +970,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(chip, INA219_DEFAULT_BRNG, &val);
+		ina219_set_vshunt_pga_gain(chip, INA219_DEFAULT_PGA, &val);
 	}
 
 	ret = ina2xx_init(chip, val);
-- 
2.12.0

[toc] | [next] | [standalone]

#1623711 — Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

On 12/04/17 04:01, Stefan Brüns wrote:
> Reducing shunt and bus voltage range improves the accuracy, so allow
> altering the default settings.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Hi Stefan,

There is new userspace ABI in here, so starting point is to document that.

That would allow the discussion of whether it is the right ABI to begin.

In particular can one of these at least be rolled into the standard
scale attributes that are already supported by the driver?
It looks to me like they both probably can - perhaps in conjunction with
use of the _available callback to notify userspace the range available from
_raw thus allowing easy computation of the range you are providing.

Keeping new ABI to a minimum makes life a lot easier for userspace tooling!

I particularly love the way it's described in the datasheet as a gain
for the shunt voltage but a range for the bus voltage - despite being the
same PGA (at least in the symbolic representation).

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ina2xx-adc.c | 165 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 164 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index d1678f886297..856409ecceb3 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -47,8 +47,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             32
> +#define INA219_DEFAULT_PGA              125 /* 1000/8 */
>  #define INA226_CONFIG_DEFAULT           0x4327
>  #define INA226_DEFAULT_AVG              4
>  #define INA226_DEFAULT_IT		1110
> @@ -61,6 +63,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)
> @@ -125,6 +135,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;
>  };
>  
> @@ -351,6 +363,44 @@ static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>  	return 0;
>  }
>  
> +static int ina219_set_vbus_range(struct ina2xx_chip_info *chip,
> +				 unsigned int range,
> +				 unsigned int *config)
> +{
> +	if (range != 16 && range != 32)
> +		return -EINVAL;
> +
> +	chip->range_vbus = range;
> +
> +	*config &= ~INA219_BRNG_MASK;
> +	*config |= INA219_SHIFT_BRNG(range == 32 ? 1 : 0) & INA219_BRNG_MASK;
> +
> +	return 0;
> +}
> +
> +static const int ina219_vshunt_gain_tab[] = { 125, 250, 500, 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_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long mask)
> @@ -485,6 +535,92 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t ina219_bus_voltage_range_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", chip->range_vbus);
> +}
> +
> +static ssize_t ina219_bus_voltage_range_store(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t len)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	unsigned long val;
> +	unsigned int config, tmp;
> +	int ret;
> +
> +	ret = kstrtoul((const char *) buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&chip->state_lock);
> +
> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> +	if (ret)
> +		goto err;
> +
> +	tmp = config;
> +
> +	ret = ina219_set_vbus_range(chip, val, &config);
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> +	if (!ret)
> +		ret = len;
> +err:
> +	mutex_unlock(&chip->state_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t ina219_shunt_voltage_gain_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	int vals[2] = { chip->pga_gain_vshunt, 1000 };
> +
> +	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
> +}
> +
> +static ssize_t ina219_shunt_voltage_gain_store(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t len)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	unsigned int config, tmp;
> +	int val, val_fract, ret;
> +
> +	ret = iio_str_to_fixpoint(buf, 100, &val, &val_fract);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&chip->state_lock);
> +
> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> +	if (ret)
> +		goto err;
> +
> +	tmp = config;
> +
> +	ret = ina219_set_vshunt_pga_gain(chip, val * 1000 + val_fract, &config);
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> +	if (!ret)
> +		ret = len;
> +err:
> +	mutex_unlock(&chip->state_lock);
> +
> +	return ret;
> +}
> +
>  #define INA2XX_CHAN(_type, _index, _address) { \
>  	.type = (_type), \
>  	.address = (_address), \
> @@ -698,6 +834,15 @@ 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");
>  
> +/* INA219/220 Bus voltage range */
> +static IIO_CONST_ATTR_NAMED(ina219_bus_voltage_range_available,
> +			    in_bus_voltage_range_available,
> +			    "16 32");
> +
> +/* INA219/220 Shunt voltage PGA gain */
> +static IIO_CONST_ATTR_NAMED(ina219_shunt_voltage_gain_available,
> +			    in_shunt_voltage_gain_available,
> +			    "0.125 0.25 0.5 1");
>  
>  static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>  		       ina2xx_allow_async_readout_show,
> @@ -707,9 +852,25 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
>  		       ina2xx_shunt_resistor_show,
>  		       ina2xx_shunt_resistor_store, 0);
>  
> +static IIO_DEVICE_ATTR_NAMED(ina219_bus_voltage_range,
> +			     in_bus_voltage_range,
> +			     S_IRUGO | S_IWUSR,
> +			     ina219_bus_voltage_range_show,
> +			     ina219_bus_voltage_range_store, 0);
> +
> +static IIO_DEVICE_ATTR_NAMED(ina219_shunt_voltage_gain,
> +			     in_shunt_voltage_gain,
> +			     S_IRUGO | S_IWUSR,
> +			     ina219_shunt_voltage_gain_show,
> +			     ina219_shunt_voltage_gain_store, 0);
> +
>  static struct attribute *ina219_attributes[] = {
>  	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
> +	&iio_dev_attr_ina219_bus_voltage_range.dev_attr.attr,
> +	&iio_const_attr_ina219_bus_voltage_range_available.dev_attr.attr,
> +	&iio_dev_attr_ina219_shunt_voltage_gain.dev_attr.attr,
> +	&iio_const_attr_ina219_shunt_voltage_gain_available.dev_attr.attr,

Whole load of new ABI here which must be documented under
Documentation/ABI/testing/sysfs-bus-iio-*


>  	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>  	NULL,
>  };
> @@ -809,6 +970,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(chip, INA219_DEFAULT_BRNG, &val);
> +		ina219_set_vshunt_pga_gain(chip, INA219_DEFAULT_PGA, &val);
>  	}
>  
>  	ret = ina2xx_init(chip, val);
> 

[toc] | [prev] | [next] | [standalone]

#1624885

On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Brüns wrote:
> > Reducing shunt and bus voltage range improves the accuracy, so allow
> > altering the default settings.
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 
> Hi Stefan,
> 
> There is new userspace ABI in here, so starting point is to document that.
> 
> That would allow the discussion of whether it is the right ABI to begin.
> 
> In particular can one of these at least be rolled into the standard
> scale attributes that are already supported by the driver?
> It looks to me like they both probably can - perhaps in conjunction with
> use of the _available callback to notify userspace the range available from
> _raw thus allowing easy computation of the range you are providing.
> 
> Keeping new ABI to a minimum makes life a lot easier for userspace tooling!
> 
> I particularly love the way it's described in the datasheet as a gain
> for the shunt voltage but a range for the bus voltage - despite being the
> same PGA (at least in the symbolic representation).

Unfortunately, correct use of raw and scale is somewhat underdocumented. I 
would expect the raw values to reflect the value read from the device, 
unaltered.

For the INA226, all value registers are 16 bit, while for the INA219 the 
voltage register is 13bit (msb aligned, lowest 3 bits from the register are 
masked), the other 3 registers are 16 bit as well.

The INA219 incorporates the bus range and shunt voltage gain in the register 
value, i.e. the shunt voltage value 0x0100 always corresponds to 256 * 10uV, 
irrespective of the PGA setting.

I think its a bad idea to incorporate the gain settings into the scale 
attribute:
1. Raw values would no longer be raw values
2. Scale for the INA219 would be settable, but readonly for the INA226
3. If the device has a gain setting, it should be exposed as such, and names 
should correspond to the datasheet
4. Any user of the gain settings had to be made aware of the possibility to 
change it, no matter how it is exposed. Making it part of the scale, and thus 
changing the meaning of the raw values, would be breaking the existing ABI.

Kind regards,

Stefan

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web