Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1954539AbdDZGLG (ORCPT ); Wed, 26 Apr 2017 02:11:06 -0400 Received: from saturn.retrosnub.co.uk ([178.18.118.26]:58309 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1950965AbdDZGLA (ORCPT ); Wed, 26 Apr 2017 02:11:00 -0400 Subject: Re: [PATCH 1/2] iio: adc: Fix integration time/averaging for INA219/220 To: Stefan Bruens Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler References: <20170412030140.9328-1-stefan.bruens@rwth-aachen.de> <3db609e9c774475aab2bf7a5c3007247@rwthex-w2-b.rwth-ad.de> <2091508.eyvxVyiBGb@pebbles.site> From: Jonathan Cameron Message-ID: <2c1d03fd-77bc-28dc-9e4b-bc89eb88feb4@kernel.org> Date: Wed, 26 Apr 2017 07:10:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 MIME-Version: 1.0 In-Reply-To: <2091508.eyvxVyiBGb@pebbles.site> Content-Type: text/plain; charset=utf-8 Content-Language: en-GH Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15447 Lines: 478 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 >> >> 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); > >