Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753562AbdDQMln convert rfc822-to-8bit (ORCPT ); Mon, 17 Apr 2017 08:41:43 -0400 Received: from mx-out-1.rwth-aachen.de ([134.130.5.186]:63654 "EHLO mx-out-1.rwth-aachen.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752238AbdDQMlk (ORCPT ); Mon, 17 Apr 2017 08:41:40 -0400 X-IronPort-AV: E=Sophos;i="5.37,215,1488841200"; d="scan'208";a="586456678" From: Stefan Bruens To: Jonathan Cameron CC: , , "Hartmut Knaack" , Lars-Peter Clausen , "Peter Meerwald-Stadler" Subject: Re: [PATCH 1/2] iio: adc: Fix integration time/averaging for INA219/220 Date: Mon, 17 Apr 2017 14:41:36 +0200 Message-ID: <2091508.eyvxVyiBGb@pebbles.site> In-Reply-To: References: <20170412030140.9328-1-stefan.bruens@rwth-aachen.de> <3db609e9c774475aab2bf7a5c3007247@rwthex-w2-b.rwth-ad.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="iso-8859-1" X-Originating-IP: [78.49.227.151] X-ClientProxiedBy: rwthex-w1-a.rwth-ad.de (2002:8682:1a9c::8682:1a9c) To rwthex-w2-b.rwth-ad.de (2002:8682:1a9f::8682:1a9f) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15169 Lines: 474 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. > > */ > > > > #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