On Wed, 24 May 2017 02:09:06 +0200 Stefan Brüns <[email protected]> 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]> One comment inline, but not really anything I want you to change. I would like some input from Marc or perhaps Andrew on this though. > --- > drivers/iio/adc/ina2xx-adc.c | 204 > ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 182 insertions(+), 22 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index bba10a1b2fcb..232c0b80d658 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -49,6 +49,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 > @@ -56,19 +57,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) > > @@ -108,6 +114,7 @@ struct ina2xx_config { > int bus_voltage_shift; > int bus_voltage_lsb; /* uV */ > int power_lsb; /* uW */ > + enum ina2xx_ids chip_id; I wonder if it would be cleaner to have someting more specific than simply an ID? Chances are as more parts get added we'll get a steadily growing list of quirks on different sets of them. Mind you can always make that sort of generalization if / when it is needed ;) > }; > > struct ina2xx_chip_info { > @@ -130,6 +137,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, > @@ -138,6 +146,7 @@ static const struct ina2xx_config ina2xx_config[] = { > .bus_voltage_shift = 0, > .bus_voltage_lsb = 1250, > .power_lsb = 25000, > + .chip_id = ina226, > }, > }; > > @@ -283,6 +292,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) > @@ -308,10 +377,21 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev, > break; > > case IIO_CHAN_INFO_INT_TIME: > - if (chan->address == INA2XX_SHUNT_VOLTAGE) > - ret = ina226_set_int_time_vshunt(chip, val2, &tmp); > - else > - ret = ina226_set_int_time_vbus(chip, val2, &tmp); > + if (chip->config->chip_id == ina226) { > + if (chan->address == INA2XX_SHUNT_VOLTAGE) > + ret = ina226_set_int_time_vshunt(chip, val2, > + &tmp); > + else > + 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: > @@ -412,7 +492,24 @@ static ssize_t ina2xx_shunt_resistor_store(struct device > *dev, > return len; > } > > -#define INA2XX_CHAN(_type, _index, _address) { \ > +#define INA219_CHAN(_type, _index, _address) { \ > + .type = (_type), \ > + .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), \ > + .scan_index = (_index), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_CPU, \ > + } \ > +} > + > +#define INA226_CHAN(_type, _index, _address) { \ > .type = (_type), \ > .address = (_address), \ > .indexed = 1, \ > @@ -434,7 +531,25 @@ 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 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, \ > + } \ > +} > + > +#define INA226_CHAN_VOLTAGE(_index, _address) { \ > .type = IIO_VOLTAGE, \ > .address = (_address), \ > .indexed = 1, \ > @@ -453,11 +568,20 @@ 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), > - INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER), > - INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT), > + > +static const struct iio_chan_spec ina226_channels[] = { > + INA226_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE), > + INA226_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE), > + INA226_CHAN(IIO_POWER, 2, INA2XX_POWER), > + INA226_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), > + INA219_CHAN(IIO_POWER, 2, INA2XX_POWER), > + INA219_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT), > IIO_CHAN_SOFT_TIMESTAMP(4), > }; > > @@ -592,7 +716,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, > @@ -602,20 +733,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 = &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 = &ina2xx_attribute_group, > + .attrs = &ina226_attribute_group, > .read_raw = ina2xx_read_raw, > .write_raw = ina2xx_write_raw, > .debugfs_reg_access = ina2xx_debug_reg, > @@ -686,6 +836,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); > @@ -697,10 +851,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);
