lkml.org 
[lkml]   [2017]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/2] iio: adc: Fix integration time/averaging for INA219/220
    From
    Date
    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 <stefan.bruens@rwth-aachen.de>
    >>
    >> 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);
    >
    >

    \
     
     \ /
      Last update: 2017-04-26 08:11    [W:3.195 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site