| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Apr 2017 16:02:33 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Stefan Brüns <stefan.bruens@...h-aachen.de>,
linux-iio@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>
Subject: Re: [PATCH 1/2] iio: adc: Fix integration time/averaging for
INA219/220
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@...h-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.
> */
> #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);
>
Powered by blists - more mailing lists