INA226/230/231 has integration times per voltage channel and common
averaging setting for both channels, while the INA219/220 only has a
combined integration time/averaging setting per channel.
Only expose the averaging attribute for the INA226, and expose the correct
integration times for the INA219.
Signed-off-by: Stefan Brüns <[email protected]>
---
drivers/iio/adc/ina2xx-adc.c | 179 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 158 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3263231276ca..d1678f886297 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -48,6 +48,7 @@
/* settings - depend on use case */
#define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
+#define INA219_DEFAULT_IT 532
#define INA226_CONFIG_DEFAULT 0x4327
#define INA226_DEFAULT_AVG 4
#define INA226_DEFAULT_IT 1110
@@ -55,19 +56,24 @@
#define INA2XX_RSHUNT_DEFAULT 10000
/*
- * bit mask for reading the averaging setting in the configuration register
+ * bit masks for reading the settings in the configuration register
* FIXME: use regmap_fields.
*/
#define INA2XX_MODE_MASK GENMASK(3, 0)
+/* Averaging for VBus/VShunt/Power */
#define INA226_AVG_MASK GENMASK(11, 9)
#define INA226_SHIFT_AVG(val) ((val) << 9)
/* Integration time for VBus */
+#define INA219_ITB_MASK GENMASK(10, 7)
+#define INA219_SHIFT_ITB(val) ((val) << 7)
#define INA226_ITB_MASK GENMASK(8, 6)
#define INA226_SHIFT_ITB(val) ((val) << 6)
/* Integration time for VShunt */
+#define INA219_ITS_MASK GENMASK(6, 3)
+#define INA219_SHIFT_ITS(val) ((val) << 3)
#define INA226_ITS_MASK GENMASK(5, 3)
#define INA226_SHIFT_ITS(val) ((val) << 3)
@@ -107,6 +113,7 @@ struct ina2xx_config {
int bus_voltage_shift;
int bus_voltage_lsb; /* uV */
int power_lsb; /* uW */
+ enum ina2xx_ids chip_id;
};
struct ina2xx_chip_info {
@@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
.bus_voltage_shift = 3,
.bus_voltage_lsb = 4000,
.power_lsb = 20000,
+ .chip_id = ina219,
},
[ina226] = {
.config_default = INA226_CONFIG_DEFAULT,
@@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
.bus_voltage_shift = 0,
.bus_voltage_lsb = 1250,
.power_lsb = 25000,
+ .chip_id = ina226,
},
};
@@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
return 0;
}
+/* Conversion times in uS. */
+static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532 };
+static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130, 4260,
+ 8510, 17020, 34050, 68100};
+
+static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
+{
+ if (*val_us > 68100 || *val_us < 84)
+ return -EINVAL;
+
+ if (*val_us <= 532) {
+ *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
+ ARRAY_SIZE(ina219_conv_time_tab_subsample));
+ *val_us = ina219_conv_time_tab_subsample[*bits];
+ } else {
+ *bits = find_closest(*val_us, ina219_conv_time_tab_average,
+ ARRAY_SIZE(ina219_conv_time_tab_average));
+ *val_us = ina219_conv_time_tab_average[*bits];
+ *bits |= 0x8;
+ }
+
+ return 0;
+}
+
+static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
+ unsigned int val_us, unsigned int *config)
+{
+ int bits, ret;
+ unsigned int val_us_best = val_us;
+
+ ret = ina219_lookup_int_time(&val_us_best, &bits);
+ if (ret)
+ return ret;
+
+ chip->int_time_vbus = val_us_best;
+
+ *config &= ~INA219_ITB_MASK;
+ *config |= INA219_SHIFT_ITB(bits) & INA219_ITB_MASK;
+
+ return 0;
+}
+
+static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
+ unsigned int val_us, unsigned int *config)
+{
+ int bits, ret;
+ unsigned int val_us_best = val_us;
+
+ ret = ina219_lookup_int_time(&val_us_best, &bits);
+ if (ret)
+ return ret;
+
+ chip->int_time_vshunt = val_us_best;
+
+ *config &= ~INA219_ITS_MASK;
+ *config |= INA219_SHIFT_ITS(bits) & INA219_ITS_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)
@@ -307,10 +376,15 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
break;
case IIO_CHAN_INFO_INT_TIME:
- if (chan->address == INA2XX_SHUNT_VOLTAGE)
+ if ((chip->config->chip_id == ina226) &&
+ (chan->address == INA2XX_SHUNT_VOLTAGE))
ret = ina226_set_int_time_vshunt(chip, val2, &tmp);
- else
+ else if (chip->config->chip_id == ina226)
ret = ina226_set_int_time_vbus(chip, val2, &tmp);
+ else if (chan->address == INA2XX_SHUNT_VOLTAGE)
+ ret = ina219_set_int_time_vshunt(chip, val2, &tmp);
+ else
+ ret = ina219_set_int_time_vbus(chip, val2, &tmp);
break;
default:
@@ -416,10 +490,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
.address = (_address), \
.indexed = 1, \
.channel = (_index), \
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
- | BIT(IIO_CHAN_INFO_SCALE), \
- .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
- BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
.scan_index = (_index), \
.scan_type = { \
.sign = 'u', \
@@ -433,7 +505,26 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
* Sampling Freq is a consequence of the integration times of
* the Voltage channels.
*/
-#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
+#define INA226_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) | \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .scan_index = (_index), \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ } \
+}
+
+#define INA219_CHAN_VOLTAGE(_index, _address) { \
.type = IIO_VOLTAGE, \
.address = (_address), \
.indexed = 1, \
@@ -441,6 +532,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
.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', \
@@ -450,9 +542,18 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
} \
}
-static const struct iio_chan_spec ina2xx_channels[] = {
- INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
- INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
+
+static const struct iio_chan_spec ina226_channels[] = {
+ INA226_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
+ INA226_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
+ INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
+ INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const struct iio_chan_spec ina219_channels[] = {
+ INA219_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
+ INA219_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
IIO_CHAN_SOFT_TIMESTAMP(4),
@@ -589,7 +690,14 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
}
/* Possible integration times for vshunt and vbus */
-static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
+static IIO_CONST_ATTR_NAMED(ina219_integration_time_available,
+ integration_time_available,
+ "0.000084 0.000148 0.000276 0.000532 0.001060 0.002130 0.004260 0.008510 0.017020 0.034050 0.068100");
+
+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,
@@ -599,20 +707,39 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
ina2xx_shunt_resistor_show,
ina2xx_shunt_resistor_store, 0);
-static struct attribute *ina2xx_attributes[] = {
+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_in_shunt_resistor.dev_attr.attr,
+ NULL,
+};
+
+static struct attribute *ina226_attributes[] = {
&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
- &iio_const_attr_integration_time_available.dev_attr.attr,
+ &iio_const_attr_ina226_integration_time_available.dev_attr.attr,
&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
NULL,
};
-static const struct attribute_group ina2xx_attribute_group = {
- .attrs = ina2xx_attributes,
+static const struct attribute_group ina219_attribute_group = {
+ .attrs = ina219_attributes,
};
-static const struct iio_info ina2xx_info = {
+static const struct attribute_group ina226_attribute_group = {
+ .attrs = ina226_attributes,
+};
+
+static const struct iio_info ina219_info = {
.driver_module = THIS_MODULE,
- .attrs = &ina2xx_attribute_group,
+ .attrs = &ina219_attribute_group,
+ .read_raw = ina2xx_read_raw,
+ .write_raw = ina2xx_write_raw,
+ .debugfs_reg_access = ina2xx_debug_reg,
+};
+
+static const struct iio_info ina226_info = {
+ .driver_module = THIS_MODULE,
+ .attrs = &ina226_attribute_group,
.read_raw = ina2xx_read_raw,
.write_raw = ina2xx_write_raw,
.debugfs_reg_access = ina2xx_debug_reg,
@@ -678,6 +805,10 @@ static int ina2xx_probe(struct i2c_client *client,
ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
+ } else {
+ chip->avg = 1;
+ ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
+ ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
}
ret = ina2xx_init(chip, val);
@@ -689,10 +820,16 @@ static int ina2xx_probe(struct i2c_client *client,
indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
indio_dev->dev.parent = &client->dev;
indio_dev->dev.of_node = client->dev.of_node;
- indio_dev->channels = ina2xx_channels;
- indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
+ if (id->driver_data == ina226) {
+ indio_dev->channels = ina226_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
+ indio_dev->info = &ina226_info;
+ } else {
+ indio_dev->channels = ina219_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
+ indio_dev->info = &ina219_info;
+ }
indio_dev->name = id->name;
- indio_dev->info = &ina2xx_info;
indio_dev->setup_ops = &ina2xx_setup_ops;
buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
--
2.12.0
On 12/04/17 04:01, Stefan Brüns wrote:
> INA226/230/231 has integration times per voltage channel and common
> averaging setting for both channels, while the INA219/220 only has a
> combined integration time/averaging setting per channel.
> Only expose the averaging attribute for the INA226, and expose the correct
> integration times for the INA219.
>
> Signed-off-by: Stefan Brüns <[email protected]>
A few bits inline.
The info_mask_shared_by_dir isn't right in the driver currently.
Those elements should be list for every channel in that direction. This
moves where they are rather than fixing that.
Please sort that mess out whilst you are here.
Thanks,
Jonathan
> ---
> drivers/iio/adc/ina2xx-adc.c | 179 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 158 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 3263231276ca..d1678f886297 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -48,6 +48,7 @@
>
> /* settings - depend on use case */
> #define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
> +#define INA219_DEFAULT_IT 532
> #define INA226_CONFIG_DEFAULT 0x4327
> #define INA226_DEFAULT_AVG 4
> #define INA226_DEFAULT_IT 1110
> @@ -55,19 +56,24 @@
> #define INA2XX_RSHUNT_DEFAULT 10000
>
> /*
> - * bit mask for reading the averaging setting in the configuration register
> + * bit masks for reading the settings in the configuration register
> * FIXME: use regmap_fields.
Either fix this or drop the fixme. It's fine to regmap fields if you
like, but it it's not broken if it doesn't use them.
> */
> #define INA2XX_MODE_MASK GENMASK(3, 0)
>
> +/* Averaging for VBus/VShunt/Power */
> #define INA226_AVG_MASK GENMASK(11, 9)
> #define INA226_SHIFT_AVG(val) ((val) << 9)
>
> /* Integration time for VBus */
> +#define INA219_ITB_MASK GENMASK(10, 7)
> +#define INA219_SHIFT_ITB(val) ((val) << 7)
> #define INA226_ITB_MASK GENMASK(8, 6)
> #define INA226_SHIFT_ITB(val) ((val) << 6)
>
> /* Integration time for VShunt */
> +#define INA219_ITS_MASK GENMASK(6, 3)
> +#define INA219_SHIFT_ITS(val) ((val) << 3)
> #define INA226_ITS_MASK GENMASK(5, 3)
> #define INA226_SHIFT_ITS(val) ((val) << 3)
>
> @@ -107,6 +113,7 @@ struct ina2xx_config {
> int bus_voltage_shift;
> int bus_voltage_lsb; /* uV */
> int power_lsb; /* uW */
> + enum ina2xx_ids chip_id;
> };
>
> struct ina2xx_chip_info {
> @@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> .bus_voltage_shift = 3,
> .bus_voltage_lsb = 4000,
> .power_lsb = 20000,
> + .chip_id = ina219,
> },
> [ina226] = {
> .config_default = INA226_CONFIG_DEFAULT,
> @@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> .bus_voltage_shift = 0,
> .bus_voltage_lsb = 1250,
> .power_lsb = 25000,
> + .chip_id = ina226,
> },
> };
>
> @@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> return 0;
> }
>
> +/* Conversion times in uS. */
> +static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532 };
> +static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130, 4260,
> + 8510, 17020, 34050, 68100};
> +
> +static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
> +{
> + if (*val_us > 68100 || *val_us < 84)
> + return -EINVAL;
> +
> + if (*val_us <= 532) {
> + *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
> + ARRAY_SIZE(ina219_conv_time_tab_subsample));
> + *val_us = ina219_conv_time_tab_subsample[*bits];
> + } else {
> + *bits = find_closest(*val_us, ina219_conv_time_tab_average,
> + ARRAY_SIZE(ina219_conv_time_tab_average));
> + *val_us = ina219_conv_time_tab_average[*bits];
> + *bits |= 0x8;
> + }
> +
> + return 0;
> +}
> +
> +static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
> + unsigned int val_us, unsigned int *config)
> +{
> + int bits, ret;
> + unsigned int val_us_best = val_us;
> +
> + ret = ina219_lookup_int_time(&val_us_best, &bits);
> + if (ret)
> + return ret;
> +
> + chip->int_time_vbus = val_us_best;
> +
> + *config &= ~INA219_ITB_MASK;
> + *config |= INA219_SHIFT_ITB(bits) & INA219_ITB_MASK;
> +
> + return 0;
> +}
> +
> +static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> + unsigned int val_us, unsigned int *config)
> +{
> + int bits, ret;
> + unsigned int val_us_best = val_us;
> +
> + ret = ina219_lookup_int_time(&val_us_best, &bits);
> + if (ret)
> + return ret;
> +
> + chip->int_time_vshunt = val_us_best;
> +
> + *config &= ~INA219_ITS_MASK;
> + *config |= INA219_SHIFT_ITS(bits) & INA219_ITS_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)
> @@ -307,10 +376,15 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
> break;
>
> case IIO_CHAN_INFO_INT_TIME:
> - if (chan->address == INA2XX_SHUNT_VOLTAGE)
> + if ((chip->config->chip_id == ina226) &&
> + (chan->address == INA2XX_SHUNT_VOLTAGE))
> ret = ina226_set_int_time_vshunt(chip, val2, &tmp);
> - else
> + else if (chip->config->chip_id == ina226)
> ret = ina226_set_int_time_vbus(chip, val2, &tmp);
> + else if (chan->address == INA2XX_SHUNT_VOLTAGE)
> + ret = ina219_set_int_time_vshunt(chip, val2, &tmp);
> + else
> + ret = ina219_set_int_time_vbus(chip, val2, &tmp);
This new ordering of the if statement is convoluted and difficult to follow.
Just use a nested if and I think it'll be easier to read.
> break;
>
> default:
> @@ -416,10 +490,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> .address = (_address), \
> .indexed = 1, \
> .channel = (_index), \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> - | BIT(IIO_CHAN_INFO_SCALE), \
> - .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
Sometimes it just feels like diff is trying to make it really hard
to see what has changed.
Firstly the new info_mask_separate is just a reformat. Fine, but not
in this patch. Should be in it's own patch.
The dropping of the shared_by_dir however makes me wonder what is going
on. Shared_by_dir elements should be present in every single channel
of that direction. This already isn't true in the driver and should be
fixed to maintain consistency. This makes it worse.
> .scan_index = (_index), \
> .scan_type = { \
> .sign = 'u', \
> @@ -433,7 +505,26 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> * Sampling Freq is a consequence of the integration times of
> * the Voltage channels.
> */
> -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> +#define INA226_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) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + } \
> +}
> +
> +#define INA219_CHAN_VOLTAGE(_index, _address) { \
> .type = IIO_VOLTAGE, \
> .address = (_address), \
> .indexed = 1, \
> @@ -441,6 +532,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> .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), \
This looks sort of like a bug fix. As it is just shared_by_dir it will
apply to all input channels so should have been specified for them all in the
first place. However, if so, the oversampling ratio should be the same.
> .scan_index = (_index), \
> .scan_type = { \
> .sign = 'u', \
> @@ -450,9 +542,18 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> } \
> }
>
> -static const struct iio_chan_spec ina2xx_channels[] = {
> - INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> - INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> +
> +static const struct iio_chan_spec ina226_channels[] = {
> + INA226_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> + INA226_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> + INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
> + INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static const struct iio_chan_spec ina219_channels[] = {
> + INA219_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> + INA219_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
> INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
> IIO_CHAN_SOFT_TIMESTAMP(4),
> @@ -589,7 +690,14 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> }
>
> /* Possible integration times for vshunt and vbus */
> -static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
> +static IIO_CONST_ATTR_NAMED(ina219_integration_time_available,
> + integration_time_available,
> + "0.000084 0.000148 0.000276 0.000532 0.001060 0.002130 0.004260 0.008510 0.017020 0.034050 0.068100");
> +
> +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,
> @@ -599,20 +707,39 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
> ina2xx_shunt_resistor_show,
> ina2xx_shunt_resistor_store, 0);
>
> -static struct attribute *ina2xx_attributes[] = {
> +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_in_shunt_resistor.dev_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute *ina226_attributes[] = {
> &iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> - &iio_const_attr_integration_time_available.dev_attr.attr,
> + &iio_const_attr_ina226_integration_time_available.dev_attr.attr,
> &iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> NULL,
> };
>
> -static const struct attribute_group ina2xx_attribute_group = {
> - .attrs = ina2xx_attributes,
> +static const struct attribute_group ina219_attribute_group = {
> + .attrs = ina219_attributes,
> };
>
> -static const struct iio_info ina2xx_info = {
> +static const struct attribute_group ina226_attribute_group = {
> + .attrs = ina226_attributes,
> +};
> +
> +static const struct iio_info ina219_info = {
> .driver_module = THIS_MODULE,
> - .attrs = &ina2xx_attribute_group,
> + .attrs = &ina219_attribute_group,
> + .read_raw = ina2xx_read_raw,
> + .write_raw = ina2xx_write_raw,
> + .debugfs_reg_access = ina2xx_debug_reg,
> +};
> +
> +static const struct iio_info ina226_info = {
> + .driver_module = THIS_MODULE,
> + .attrs = &ina226_attribute_group,
> .read_raw = ina2xx_read_raw,
> .write_raw = ina2xx_write_raw,
> .debugfs_reg_access = ina2xx_debug_reg,
> @@ -678,6 +805,10 @@ static int ina2xx_probe(struct i2c_client *client,
> ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
> ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
> + } else {
> + chip->avg = 1;
> + ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
> + ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
> }
>
> ret = ina2xx_init(chip, val);
> @@ -689,10 +820,16 @@ static int ina2xx_probe(struct i2c_client *client,
> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> indio_dev->dev.parent = &client->dev;
> indio_dev->dev.of_node = client->dev.of_node;
> - indio_dev->channels = ina2xx_channels;
> - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> + if (id->driver_data == ina226) {
> + indio_dev->channels = ina226_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
> + indio_dev->info = &ina226_info;
> + } else {
> + indio_dev->channels = ina219_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
> + indio_dev->info = &ina219_info;
> + }
> indio_dev->name = id->name;
> - indio_dev->info = &ina2xx_info;
> indio_dev->setup_ops = &ina2xx_setup_ops;
>
> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>
On Freitag, 14. April 2017 17:02:33 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Br?ns wrote:
> > INA226/230/231 has integration times per voltage channel and common
> > averaging setting for both channels, while the INA219/220 only has a
> > combined integration time/averaging setting per channel.
> > Only expose the averaging attribute for the INA226, and expose the correct
> > integration times for the INA219.
> >
> > Signed-off-by: Stefan Br?ns <[email protected]>
>
> A few bits inline.
>
> The info_mask_shared_by_dir isn't right in the driver currently.
> Those elements should be list for every channel in that direction. This
> moves where they are rather than fixing that.
>
> Please sort that mess out whilst you are here.
>
> Thanks,
>
> Jonathan
>
> > ---
> >
> > drivers/iio/adc/ina2xx-adc.c | 179
> > ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 158
> > insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 3263231276ca..d1678f886297 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -48,6 +48,7 @@
> >
> > /* settings - depend on use case */
> > #define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
> >
> > +#define INA219_DEFAULT_IT 532
> >
> > #define INA226_CONFIG_DEFAULT 0x4327
> > #define INA226_DEFAULT_AVG 4
> > #define INA226_DEFAULT_IT 1110
> >
> > @@ -55,19 +56,24 @@
> >
> > #define INA2XX_RSHUNT_DEFAULT 10000
> >
> > /*
> >
> > - * bit mask for reading the averaging setting in the configuration
> > register + * bit masks for reading the settings in the configuration
> > register>
> > * FIXME: use regmap_fields.
>
> Either fix this or drop the fixme. It's fine to regmap fields if you
> like, but it it's not broken if it doesn't use them.
The FIXME was not added nor changed by me. I see no reason to drop it.
> > */
> >
> > #define INA2XX_MODE_MASK GENMASK(3, 0)
> >
> > +/* Averaging for VBus/VShunt/Power */
> >
> > #define INA226_AVG_MASK GENMASK(11, 9)
> > #define INA226_SHIFT_AVG(val) ((val) << 9)
> >
> > /* Integration time for VBus */
> >
> > +#define INA219_ITB_MASK GENMASK(10, 7)
> > +#define INA219_SHIFT_ITB(val) ((val) << 7)
> >
> > #define INA226_ITB_MASK GENMASK(8, 6)
> > #define INA226_SHIFT_ITB(val) ((val) << 6)
> >
> > /* Integration time for VShunt */
> >
> > +#define INA219_ITS_MASK GENMASK(6, 3)
> > +#define INA219_SHIFT_ITS(val) ((val) << 3)
> >
> > #define INA226_ITS_MASK GENMASK(5, 3)
> > #define INA226_SHIFT_ITS(val) ((val) << 3)
> >
> > @@ -107,6 +113,7 @@ struct ina2xx_config {
> >
> > int bus_voltage_shift;
> > int bus_voltage_lsb; /* uV */
> > int power_lsb; /* uW */
> >
> > + enum ina2xx_ids chip_id;
> >
> > };
> >
> > struct ina2xx_chip_info {
> >
> > @@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >
> > .bus_voltage_shift = 3,
> > .bus_voltage_lsb = 4000,
> > .power_lsb = 20000,
> >
> > + .chip_id = ina219,
> >
> > },
> > [ina226] = {
> >
> > .config_default = INA226_CONFIG_DEFAULT,
> >
> > @@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >
> > .bus_voltage_shift = 0,
> > .bus_voltage_lsb = 1250,
> > .power_lsb = 25000,
> >
> > + .chip_id = ina226,
> >
> > },
> >
> > };
> >
> > @@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct
> > ina2xx_chip_info *chip,>
> > return 0;
> >
> > }
> >
> > +/* Conversion times in uS. */
> > +static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532
> > };
> > +static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130,
> > 4260,
> > + 8510, 17020, 34050, 68100};
> > +
> > +static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
> > +{
> > + if (*val_us > 68100 || *val_us < 84)
> > + return -EINVAL;
> > +
> > + if (*val_us <= 532) {
> > + *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
> > + ARRAY_SIZE(ina219_conv_time_tab_subsample));
> > + *val_us = ina219_conv_time_tab_subsample[*bits];
> > + } else {
> > + *bits = find_closest(*val_us, ina219_conv_time_tab_average,
> > + ARRAY_SIZE(ina219_conv_time_tab_average));
> > + *val_us = ina219_conv_time_tab_average[*bits];
> > + *bits |= 0x8;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
> > + unsigned int val_us, unsigned int *config)
> > +{
> > + int bits, ret;
> > + unsigned int val_us_best = val_us;
> > +
> > + ret = ina219_lookup_int_time(&val_us_best, &bits);
> > + if (ret)
> > + return ret;
> > +
> > + chip->int_time_vbus = val_us_best;
> > +
> > + *config &= ~INA219_ITB_MASK;
> > + *config |= INA219_SHIFT_ITB(bits) & INA219_ITB_MASK;
> > +
> > + return 0;
> > +}
> > +
> > +static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> > + unsigned int val_us, unsigned int *config)
> > +{
> > + int bits, ret;
> > + unsigned int val_us_best = val_us;
> > +
> > + ret = ina219_lookup_int_time(&val_us_best, &bits);
> > + if (ret)
> > + return ret;
> > +
> > + chip->int_time_vshunt = val_us_best;
> > +
> > + *config &= ~INA219_ITS_MASK;
> > + *config |= INA219_SHIFT_ITS(bits) & INA219_ITS_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)
> >
> > @@ -307,10 +376,15 @@ static int ina2xx_write_raw(struct iio_dev
> > *indio_dev,>
> > break;
> >
> > case IIO_CHAN_INFO_INT_TIME:
> > - if (chan->address == INA2XX_SHUNT_VOLTAGE)
> > + if ((chip->config->chip_id == ina226) &&
> > + (chan->address == INA2XX_SHUNT_VOLTAGE))
> >
> > ret = ina226_set_int_time_vshunt(chip, val2, &tmp);
> >
> > - else
> > + else if (chip->config->chip_id == ina226)
> >
> > ret = ina226_set_int_time_vbus(chip, val2, &tmp);
> >
> > + else if (chan->address == INA2XX_SHUNT_VOLTAGE)
> > + ret = ina219_set_int_time_vshunt(chip, val2, &tmp);
> > + else
> > + ret = ina219_set_int_time_vbus(chip, val2, &tmp);
>
> This new ordering of the if statement is convoluted and difficult to follow.
> Just use a nested if and I think it'll be easier to read.
I can change this, but the extra indentation level will force a line break to
not exceed the 78 character limit.
> > break;
> >
> > default:
> > @@ -416,10 +490,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> > device *dev,>
> > .address = (_address), \
> > .indexed = 1, \
> > .channel = (_index), \
> >
> > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> > - | BIT(IIO_CHAN_INFO_SCALE), \
> > - .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
>
> Sometimes it just feels like diff is trying to make it really hard
> to see what has changed.
>
> Firstly the new info_mask_separate is just a reformat. Fine, but not
> in this patch. Should be in it's own patch.
>
> The dropping of the shared_by_dir however makes me wonder what is going
> on. Shared_by_dir elements should be present in every single channel
> of that direction. This already isn't true in the driver and should be
> fixed to maintain consistency. This makes it worse.
Unfortunately there is no documentation how the *_shared_by_* fields have to
be used. It seems to work if a bit is only set on some channels of a group.
This patch makes is neither better nor worse. Previously, e.g.
IIO_CHAN_INFO_SAMP_FREQ was set on both the power and current channels, now it
is set on the bus and shunt voltage channels. Both variants create a single
in_sample_frequency attribute.
> > .scan_index = (_index), \
> > .scan_type = { \
> >
> > .sign = 'u', \
> >
> > @@ -433,7 +505,26 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> > device *dev,>
> > * Sampling Freq is a consequence of the integration times of
> > * the Voltage channels.
> > */
> >
> > -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> > +#define INA226_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) | \
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > + .scan_index = (_index), \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 16, \
> > + .storagebits = 16, \
> > + .endianness = IIO_LE, \
> > + } \
> > +}
> > +
> > +#define INA219_CHAN_VOLTAGE(_index, _address) { \
> >
> > .type = IIO_VOLTAGE, \
> > .address = (_address), \
> > .indexed = 1, \
> >
> > @@ -441,6 +532,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> > device *dev,>
> > .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), \
>
> This looks sort of like a bug fix. As it is just shared_by_dir it will
> apply to all input channels so should have been specified for them all in
> the first place. However, if so, the oversampling ratio should be the
> same.
No. TI226 has one oversampling ("averaging" in the DS) setting applying to all
channels. TI219 has no explicit averaging.
Apparently, on the TI226, all 4 channels (current, power, bus voltage, shunt)
should have the IIO_CHAN_INFO_SAMP_FREQ and IIO_CHAN_INFO_OVERSAMPLING bits
set in the info_mask_shared_by_dir, while the TI219 should have set only the
IIO_CHAN_INFO_SAMP_FREQ bit.
> > .scan_index = (_index), \
> > .scan_type = { \
> >
> > .sign = 'u', \
> >
> > @@ -450,9 +542,18 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> > device *dev,>
> > } \
> >
> > }
> >
> > -static const struct iio_chan_spec ina2xx_channels[] = {
> > - INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> > - INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> > +
> > +static const struct iio_chan_spec ina226_channels[] = {
> > + INA226_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> > + INA226_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> > + INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
> > + INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const struct iio_chan_spec ina219_channels[] = {
> > + INA219_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> > + INA219_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> >
> > INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
> > INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
> > IIO_CHAN_SOFT_TIMESTAMP(4),
> >
> > @@ -589,7 +690,14 @@ static int ina2xx_debug_reg(struct iio_dev
> > *indio_dev,
> >
> > }
> >
> >
> > /* Possible integration times for vshunt and vbus */
> >
> > -static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588
> > 0.001100 0.002116 0.004156 0.008244"); +static
> > IIO_CONST_ATTR_NAMED(ina219_integration_time_available,
> > + integration_time_available,
> > + "0.000084 0.000148 0.000276 0.000532 0.001060 0.002130
0.004260
> > 0.008510 0.017020 0.034050 0.068100"); +
> > +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,
> >
> > @@ -599,20 +707,39 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO |
> > S_IWUSR,>
> > ina2xx_shunt_resistor_show,
> > ina2xx_shunt_resistor_store, 0);
> >
> > -static struct attribute *ina2xx_attributes[] = {
> > +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_in_shunt_resistor.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute *ina226_attributes[] = {
> >
> > &iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >
> > - &iio_const_attr_integration_time_available.dev_attr.attr,
> > + &iio_const_attr_ina226_integration_time_available.dev_attr.attr,
> >
> > &iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> > NULL,
> >
> > };
> >
> > -static const struct attribute_group ina2xx_attribute_group = {
> > - .attrs = ina2xx_attributes,
> > +static const struct attribute_group ina219_attribute_group = {
> > + .attrs = ina219_attributes,
> >
> > };
> >
> > -static const struct iio_info ina2xx_info = {
> > +static const struct attribute_group ina226_attribute_group = {
> > + .attrs = ina226_attributes,
> > +};
> > +
> > +static const struct iio_info ina219_info = {
> >
> > .driver_module = THIS_MODULE,
> >
> > - .attrs = &ina2xx_attribute_group,
> > + .attrs = &ina219_attribute_group,
> > + .read_raw = ina2xx_read_raw,
> > + .write_raw = ina2xx_write_raw,
> > + .debugfs_reg_access = ina2xx_debug_reg,
> > +};
> > +
> > +static const struct iio_info ina226_info = {
> > + .driver_module = THIS_MODULE,
> > + .attrs = &ina226_attribute_group,
> >
> > .read_raw = ina2xx_read_raw,
> > .write_raw = ina2xx_write_raw,
> > .debugfs_reg_access = ina2xx_debug_reg,
> >
> > @@ -678,6 +805,10 @@ static int ina2xx_probe(struct i2c_client *client,
> >
> > ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> > ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
> > ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
> >
> > + } else {
> > + chip->avg = 1;
> > + ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
> > + ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
> >
> > }
> >
> > ret = ina2xx_init(chip, val);
> >
> > @@ -689,10 +820,16 @@ static int ina2xx_probe(struct i2c_client *client,
> >
> > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> > indio_dev->dev.parent = &client->dev;
> > indio_dev->dev.of_node = client->dev.of_node;
> >
> > - indio_dev->channels = ina2xx_channels;
> > - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> > + if (id->driver_data == ina226) {
> > + indio_dev->channels = ina226_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
> > + indio_dev->info = &ina226_info;
> > + } else {
> > + indio_dev->channels = ina219_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
> > + indio_dev->info = &ina219_info;
> > + }
> >
> > indio_dev->name = id->name;
> >
> > - indio_dev->info = &ina2xx_info;
> >
> > indio_dev->setup_ops = &ina2xx_setup_ops;
> >
> > buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
--
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424
On 17/04/17 13:41, Stefan Bruens wrote:
> On Freitag, 14. April 2017 17:02:33 CEST Jonathan Cameron wrote:
>> On 12/04/17 04:01, Stefan Brüns wrote:
>>> INA226/230/231 has integration times per voltage channel and common
>>> averaging setting for both channels, while the INA219/220 only has a
>>> combined integration time/averaging setting per channel.
>>> Only expose the averaging attribute for the INA226, and expose the correct
>>> integration times for the INA219.
>>>
>>> Signed-off-by: Stefan Brüns <[email protected]>
>>
>> A few bits inline.
>>
>> The info_mask_shared_by_dir isn't right in the driver currently.
>> Those elements should be list for every channel in that direction. This
>> moves where they are rather than fixing that.
>>
>> Please sort that mess out whilst you are here.
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>
>>> drivers/iio/adc/ina2xx-adc.c | 179
>>> ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 158
>>> insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>>> index 3263231276ca..d1678f886297 100644
>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>> @@ -48,6 +48,7 @@
>>>
>>> /* settings - depend on use case */
>>> #define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
>>>
>>> +#define INA219_DEFAULT_IT 532
>>>
>>> #define INA226_CONFIG_DEFAULT 0x4327
>>> #define INA226_DEFAULT_AVG 4
>>> #define INA226_DEFAULT_IT 1110
>>>
>>> @@ -55,19 +56,24 @@
>>>
>>> #define INA2XX_RSHUNT_DEFAULT 10000
>>>
>>> /*
>>>
>>> - * bit mask for reading the averaging setting in the configuration
>>> register + * bit masks for reading the settings in the configuration
>>> register>
>>> * FIXME: use regmap_fields.
>>
>> Either fix this or drop the fixme. It's fine to regmap fields if you
>> like, but it it's not broken if it doesn't use them.
>
> The FIXME was not added nor changed by me. I see no reason to drop it.
oops., My mistake.
>
>>> */
>>>
>>> #define INA2XX_MODE_MASK GENMASK(3, 0)
>>>
>>> +/* Averaging for VBus/VShunt/Power */
>>>
>>> #define INA226_AVG_MASK GENMASK(11, 9)
>>> #define INA226_SHIFT_AVG(val) ((val) << 9)
>>>
>>> /* Integration time for VBus */
>>>
>>> +#define INA219_ITB_MASK GENMASK(10, 7)
>>> +#define INA219_SHIFT_ITB(val) ((val) << 7)
>>>
>>> #define INA226_ITB_MASK GENMASK(8, 6)
>>> #define INA226_SHIFT_ITB(val) ((val) << 6)
>>>
>>> /* Integration time for VShunt */
>>>
>>> +#define INA219_ITS_MASK GENMASK(6, 3)
>>> +#define INA219_SHIFT_ITS(val) ((val) << 3)
>>>
>>> #define INA226_ITS_MASK GENMASK(5, 3)
>>> #define INA226_SHIFT_ITS(val) ((val) << 3)
>>>
>>> @@ -107,6 +113,7 @@ struct ina2xx_config {
>>>
>>> int bus_voltage_shift;
>>> int bus_voltage_lsb; /* uV */
>>> int power_lsb; /* uW */
>>>
>>> + enum ina2xx_ids chip_id;
>>>
>>> };
>>>
>>> struct ina2xx_chip_info {
>>>
>>> @@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>
>>> .bus_voltage_shift = 3,
>>> .bus_voltage_lsb = 4000,
>>> .power_lsb = 20000,
>>>
>>> + .chip_id = ina219,
>>>
>>> },
>>> [ina226] = {
>>>
>>> .config_default = INA226_CONFIG_DEFAULT,
>>>
>>> @@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>
>>> .bus_voltage_shift = 0,
>>> .bus_voltage_lsb = 1250,
>>> .power_lsb = 25000,
>>>
>>> + .chip_id = ina226,
>>>
>>> },
>>>
>>> };
>>>
>>> @@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct
>>> ina2xx_chip_info *chip,>
>>> return 0;
>>>
>>> }
>>>
>>> +/* Conversion times in uS. */
>>> +static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532
>>> };
>>> +static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130,
>>> 4260,
>>> + 8510, 17020, 34050, 68100};
>>> +
>>> +static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
>>> +{
>>> + if (*val_us > 68100 || *val_us < 84)
>>> + return -EINVAL;
>>> +
>>> + if (*val_us <= 532) {
>>> + *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
>>> + ARRAY_SIZE(ina219_conv_time_tab_subsample));
>>> + *val_us = ina219_conv_time_tab_subsample[*bits];
>>> + } else {
>>> + *bits = find_closest(*val_us, ina219_conv_time_tab_average,
>>> + ARRAY_SIZE(ina219_conv_time_tab_average));
>>> + *val_us = ina219_conv_time_tab_average[*bits];
>>> + *bits |= 0x8;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
>>> + unsigned int val_us, unsigned int *config)
>>> +{
>>> + int bits, ret;
>>> + unsigned int val_us_best = val_us;
>>> +
>>> + ret = ina219_lookup_int_time(&val_us_best, &bits);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + chip->int_time_vbus = val_us_best;
>>> +
>>> + *config &= ~INA219_ITB_MASK;
>>> + *config |= INA219_SHIFT_ITB(bits) & INA219_ITB_MASK;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ina219_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>>> + unsigned int val_us, unsigned int *config)
>>> +{
>>> + int bits, ret;
>>> + unsigned int val_us_best = val_us;
>>> +
>>> + ret = ina219_lookup_int_time(&val_us_best, &bits);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + chip->int_time_vshunt = val_us_best;
>>> +
>>> + *config &= ~INA219_ITS_MASK;
>>> + *config |= INA219_SHIFT_ITS(bits) & INA219_ITS_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)
>>>
>>> @@ -307,10 +376,15 @@ static int ina2xx_write_raw(struct iio_dev
>>> *indio_dev,>
>>> break;
>>>
>>> case IIO_CHAN_INFO_INT_TIME:
>>> - if (chan->address == INA2XX_SHUNT_VOLTAGE)
>>> + if ((chip->config->chip_id == ina226) &&
>>> + (chan->address == INA2XX_SHUNT_VOLTAGE))
>>>
>>> ret = ina226_set_int_time_vshunt(chip, val2, &tmp);
>>>
>>> - else
>>> + else if (chip->config->chip_id == ina226)
>>>
>>> ret = ina226_set_int_time_vbus(chip, val2, &tmp);
>>>
>>> + else if (chan->address == INA2XX_SHUNT_VOLTAGE)
>>> + ret = ina219_set_int_time_vshunt(chip, val2, &tmp);
>>> + else
>>> + ret = ina219_set_int_time_vbus(chip, val2, &tmp);
>>
>> This new ordering of the if statement is convoluted and difficult to follow.
>> Just use a nested if and I think it'll be easier to read.
>
> I can change this, but the extra indentation level will force a line break to
> not exceed the 78 character limit.
I think readability will still be improved so go for it.
>
>>> break;
>>>
>>> default:
>>> @@ -416,10 +490,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct
>>> device *dev,>
>>> .address = (_address), \
>>> .indexed = 1, \
>>> .channel = (_index), \
>>>
>>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
>>> - | BIT(IIO_CHAN_INFO_SCALE), \
>>> - .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>>> - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> + BIT(IIO_CHAN_INFO_SCALE), \
>>
>> Sometimes it just feels like diff is trying to make it really hard
>> to see what has changed.
>>
>> Firstly the new info_mask_separate is just a reformat. Fine, but not
>> in this patch. Should be in it's own patch.
>>
>> The dropping of the shared_by_dir however makes me wonder what is going
>> on. Shared_by_dir elements should be present in every single channel
>> of that direction. This already isn't true in the driver and should be
>> fixed to maintain consistency. This makes it worse.
>
> Unfortunately there is no documentation how the *_shared_by_* fields have to
> be used. It seems to work if a bit is only set on some channels of a group.
It will work just fine but the intent is to have them set for all relevant
channels. Agreed, our documentation could do with improving on this.
>
> This patch makes is neither better nor worse. Previously, e.g.
> IIO_CHAN_INFO_SAMP_FREQ was set on both the power and current channels, now it
> is set on the bus and shunt voltage channels. Both variants create a single
> in_sample_frequency attribute.
If we can clean this up whilst you were here it would be great. If not fair
enough - we'll get to it one day!
>
>>> .scan_index = (_index), \
>>> .scan_type = { \
>>>
>>> .sign = 'u', \
>>>
>>> @@ -433,7 +505,26 @@ static ssize_t ina2xx_shunt_resistor_store(struct
>>> device *dev,>
>>> * Sampling Freq is a consequence of the integration times of
>>> * the Voltage channels.
>>> */
>>>
>>> -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> +#define INA226_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) | \
>>> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> + .scan_index = (_index), \
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 16, \
>>> + .storagebits = 16, \
>>> + .endianness = IIO_LE, \
>>> + } \
>>> +}
>>> +
>>> +#define INA219_CHAN_VOLTAGE(_index, _address) { \
>>>
>>> .type = IIO_VOLTAGE, \
>>> .address = (_address), \
>>> .indexed = 1, \
>>>
>>> @@ -441,6 +532,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct
>>> device *dev,>
>>> .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), \
>>
>> This looks sort of like a bug fix. As it is just shared_by_dir it will
>> apply to all input channels so should have been specified for them all in
>> the first place. However, if so, the oversampling ratio should be the
>> same.
>
> No. TI226 has one oversampling ("averaging" in the DS) setting applying to all
> channels. TI219 has no explicit averaging.
>
> Apparently, on the TI226, all 4 channels (current, power, bus voltage, shunt)
> should have the IIO_CHAN_INFO_SAMP_FREQ and IIO_CHAN_INFO_OVERSAMPLING bits
> set in the info_mask_shared_by_dir, while the TI219 should have set only the
> IIO_CHAN_INFO_SAMP_FREQ bit.
Sounds right to me.
>
>>> .scan_index = (_index), \
>>> .scan_type = { \
>>>
>>> .sign = 'u', \
>>>
>>> @@ -450,9 +542,18 @@ static ssize_t ina2xx_shunt_resistor_store(struct
>>> device *dev,>
>>> } \
>>>
>>> }
>>>
>>> -static const struct iio_chan_spec ina2xx_channels[] = {
>>> - INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> - INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>> +
>>> +static const struct iio_chan_spec ina226_channels[] = {
>>> + INA226_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> + INA226_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>> + INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
>>> + INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
>>> + IIO_CHAN_SOFT_TIMESTAMP(4),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ina219_channels[] = {
>>> + INA219_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> + INA219_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>>
>>> INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
>>> INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
>>> IIO_CHAN_SOFT_TIMESTAMP(4),
>>>
>>> @@ -589,7 +690,14 @@ static int ina2xx_debug_reg(struct iio_dev
>>> *indio_dev,
>>>
>>> }
>>>
>>>
>>> /* Possible integration times for vshunt and vbus */
>>>
>>> -static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588
>>> 0.001100 0.002116 0.004156 0.008244"); +static
>>> IIO_CONST_ATTR_NAMED(ina219_integration_time_available,
>>> + integration_time_available,
>>> + "0.000084 0.000148 0.000276 0.000532 0.001060 0.002130
> 0.004260
>>> 0.008510 0.017020 0.034050 0.068100"); +
>>> +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,
>>>
>>> @@ -599,20 +707,39 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO |
>>> S_IWUSR,>
>>> ina2xx_shunt_resistor_show,
>>> ina2xx_shunt_resistor_store, 0);
>>>
>>> -static struct attribute *ina2xx_attributes[] = {
>>> +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_in_shunt_resistor.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static struct attribute *ina226_attributes[] = {
>>>
>>> &iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>>
>>> - &iio_const_attr_integration_time_available.dev_attr.attr,
>>> + &iio_const_attr_ina226_integration_time_available.dev_attr.attr,
>>>
>>> &iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>> NULL,
>>>
>>> };
>>>
>>> -static const struct attribute_group ina2xx_attribute_group = {
>>> - .attrs = ina2xx_attributes,
>>> +static const struct attribute_group ina219_attribute_group = {
>>> + .attrs = ina219_attributes,
>>>
>>> };
>>>
>>> -static const struct iio_info ina2xx_info = {
>>> +static const struct attribute_group ina226_attribute_group = {
>>> + .attrs = ina226_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ina219_info = {
>>>
>>> .driver_module = THIS_MODULE,
>>>
>>> - .attrs = &ina2xx_attribute_group,
>>> + .attrs = &ina219_attribute_group,
>>> + .read_raw = ina2xx_read_raw,
>>> + .write_raw = ina2xx_write_raw,
>>> + .debugfs_reg_access = ina2xx_debug_reg,
>>> +};
>>> +
>>> +static const struct iio_info ina226_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .attrs = &ina226_attribute_group,
>>>
>>> .read_raw = ina2xx_read_raw,
>>> .write_raw = ina2xx_write_raw,
>>> .debugfs_reg_access = ina2xx_debug_reg,
>>>
>>> @@ -678,6 +805,10 @@ static int ina2xx_probe(struct i2c_client *client,
>>>
>>> ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>>> ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
>>> ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
>>>
>>> + } else {
>>> + chip->avg = 1;
>>> + ina219_set_int_time_vbus(chip, INA219_DEFAULT_IT, &val);
>>> + ina219_set_int_time_vshunt(chip, INA219_DEFAULT_IT, &val);
>>>
>>> }
>>>
>>> ret = ina2xx_init(chip, val);
>>>
>>> @@ -689,10 +820,16 @@ static int ina2xx_probe(struct i2c_client *client,
>>>
>>> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>> indio_dev->dev.parent = &client->dev;
>>> indio_dev->dev.of_node = client->dev.of_node;
>>>
>>> - indio_dev->channels = ina2xx_channels;
>>> - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>>> + if (id->driver_data == ina226) {
>>> + indio_dev->channels = ina226_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
>>> + indio_dev->info = &ina226_info;
>>> + } else {
>>> + indio_dev->channels = ina219_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
>>> + indio_dev->info = &ina219_info;
>>> + }
>>>
>>> indio_dev->name = id->name;
>>>
>>> - indio_dev->info = &ina2xx_info;
>>>
>>> indio_dev->setup_ops = &ina2xx_setup_ops;
>>>
>>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>
>