On Wed, 04 Oct 2017 09:11:31 +0200 Maciej Purski <m.purski@xxxxxxxxxxx> wrote: > On 10/01/2017 12:29 PM, Jonathan Cameron wrote: > > On Thu, 28 Sep 2017 14:50:12 +0200 > > Maciej Purski <m.purski@xxxxxxxxxxx> wrote: > > > >> Max expected current is used for calculating calibration register value, > >> Current LSB and Power LSB according to equations found in ina datasheet. > >> Max expected current is now implicitly set to default value, > >> which is 2^15, thanks to which Current LSB is equal to 1 mA and > >> Power LSB is equal to 20000 uW or 25000 uW depending on ina model. > >> > >> Make max expected current configurable, just like it's already done > >> with shunt resistance: from device tree, platform_data or later > >> from sysfs. On each max_expected_current change, calculate new values > >> for Current LSB and Power LSB. According to datasheet Current LSB should > >> be calculated by dividing max expected current by 2^15, as values read > >> from device registers are in this case 16-bit integers. Power LSB > >> is calculated by multiplying Current LSB by a factor, which is defined > >> in ina documentation. > > > > One odd bit of casting inline. Also this is new userspace ABI. > > It needs documenting in > > > > Documentation/ABI/testing/sysfs-bus-iio* as appropriate. > > I'm also unclear on one element about this - is it a value used only > > for calibration or are we talking about the actual 'range' of the device? > > > > This is used for calibration. The device measures directly only voltage. > However, it has also current and power registers. Their values are > calculated by the device using the calibration register which is calculated > using max expected current. So I guess that it's not what you mean > by the actual 'range' of the device. > > > The interpretation of this value isn't clear against the more general > > ABI. > > > > In particular it is it in raw units (adc counts) or mA? Docs say > > that but the naming of the attribute doesn't make this clear. > > > > It's in mA. I can make it clear in the attribute name. > > > Also I'm unconvinced this isn't better represented using the > > range specifications available for any IIO attribute on the raw > > value in combination with adjusting the scale value. > > Note not many drivers yet provide ranges on their raw outputs > > but we do have core support for it. I've been meaning to start > > pushing this out into drivers, but been busy since we introduced > > the core support. The dpot-dac driver does use it for examplel > > > > > I'm not sure if what I'm about to add is similar to what is done > in the mentioned dpot-dac driver. It seems that the callback read_avail > returns information on raw values which can be obtained from the device. > What I need is an adjustable value, which is then used by the device internally > in order to calculate current with the requested precision. Max expected current > is also used for calculating the scale value. > Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and > your solution fits in here. > I think I answered this in the other branch of the thread. _calibscale is what you want here as it's internal to the device. It's not one often used for ADCs but quite a few other types of device provide some front end analog adjustment (whilst it is digital here, it is applied within the device - so we don't need to care). Jonathan > Best regards, > > Maciej > > This moves the burden of calculating the 1lsb value to userspace, > > but importantly it would give us a consistent ABI where this fits > > in with existing elements (largely buy not introducing any new > > ones :). > > > > Thanks, > > > > Jonathan > >> > >> Signed-off-by: Maciej Purski <m.purski@xxxxxxxxxxx> > >> --- > >> drivers/iio/adc/ina2xx-adc.c | 110 ++++++++++++++++++++++++++++++----- > >> include/linux/platform_data/ina2xx.h | 2 + > >> 2 files changed, 98 insertions(+), 14 deletions(-) > >> > >> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > >> index f387b97..883fede 100644 > >> --- a/drivers/iio/adc/ina2xx-adc.c > >> +++ b/drivers/iio/adc/ina2xx-adc.c > >> @@ -56,6 +56,7 @@ > >> #define INA226_DEFAULT_IT 1110 > >> > >> #define INA2XX_RSHUNT_DEFAULT 10000 > >> +#define INA2XX_MAX_EXPECTED_A_DEFAULT (1 << 15) /* current_lsb = 1 mA */ > >> > >> /* > >> * bit masks for reading the settings in the configuration register > >> @@ -114,7 +115,7 @@ struct ina2xx_config { > >> int shunt_div; > >> int bus_voltage_shift; > >> int bus_voltage_lsb; /* uV */ > >> - int power_lsb; /* uW */ > >> + int power_lsb_factor; > >> enum ina2xx_ids chip_id; > >> }; > >> > >> @@ -123,7 +124,10 @@ struct ina2xx_chip_info { > >> struct task_struct *task; > >> const struct ina2xx_config *config; > >> struct mutex state_lock; > >> - unsigned int shunt_resistor; > >> + unsigned int shunt_resistor; /* uOhms */ > >> + unsigned int max_expected_current; /* mA */ > >> + int current_lsb; /* uA */ > >> + int power_lsb; /* uW */ > >> int avg; > >> int int_time_vbus; /* Bus voltage integration time uS */ > >> int int_time_vshunt; /* Shunt voltage integration time uS */ > >> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = { > >> .shunt_div = 100, > >> .bus_voltage_shift = 3, > >> .bus_voltage_lsb = 4000, > >> - .power_lsb = 20000, > >> + .power_lsb_factor = 20, > >> .chip_id = ina219, > >> }, > >> [ina226] = { > >> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = { > >> .shunt_div = 400, > >> .bus_voltage_shift = 0, > >> .bus_voltage_lsb = 1250, > >> - .power_lsb = 25000, > >> + .power_lsb_factor = 25, > >> .chip_id = ina226, > >> }, > >> }; > >> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev, > >> > >> case INA2XX_POWER: > >> /* processed (mW) = raw*lsb (uW) / 1000 */ > >> - *val = chip->config->power_lsb; > >> + *val = chip->power_lsb; > >> *val2 = 1000; > >> return IIO_VAL_FRACTIONAL; > >> > >> case INA2XX_CURRENT: > >> - /* processed (mA) = raw (mA) */ > >> - *val = 1; > >> - return IIO_VAL_INT; > >> + /* processed (mA) = raw*lsb (uA) / 1000 */ > >> + *val = chip->current_lsb; > >> + *val2 = 1000; > >> + return IIO_VAL_FRACTIONAL; > >> } > >> } > >> > >> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev, > >> } > >> > >> /* > >> - * Set current LSB to 1mA, shunt is in uOhms > >> - * (equation 13 in datasheet). We hardcode a Current_LSB > >> - * of 1.0 x10-6. The only remaining parameter is RShunt. > >> + * Calculate calibration value according to equation 1 in ina226 datasheet > >> + * http://www.ti.com/lit/ds/symlink/ina226.pdf. > >> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be > >> + * converted to mOhms in order to keep the scale. > >> * There is no need to expose the CALIBRATION register > >> * to the user for now. But we need to reset this register > >> - * if the user updates RShunt after driver init, e.g upon > >> - * reading an EEPROM/Probe-type value. > >> + * if the user updates RShunt or max expected current after driver > >> + * init, e.g upon reading an EEPROM/Probe-type value. > >> */ > >> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip) > >> { > >> + unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000); > >> u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, > >> - chip->shunt_resistor); > >> + chip->current_lsb * rshunt); > >> > >> return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); > >> } > >> > >> +/* > >> + * Set max_expected_current (mA) and calculate current_lsb (uA), > >> + * according to equation 2 in ina226 datasheet. Power LSB is calculated > >> + * by multiplying Current LSB by a given factor, which may vary depending > >> + * on ina version. > >> + */ > >> +static int set_max_expected_current(struct ina2xx_chip_info *chip, > >> + unsigned int val) > >> +{ > >> + if (val <= 0 || val > chip->config->calibration_factor) > >> + return -EINVAL; > >> + > >> + chip->max_expected_current = val; > >> + chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000, > >> + 1 << 15); > >> + chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor; > >> + > >> + return 0; > >> +} > >> + > >> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val) > >> { > >> + > >> if (val <= 0 || val > chip->config->calibration_factor) > >> return -EINVAL; > >> > >> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev, > >> return len; > >> } > >> > >> +static ssize_t ina2xx_max_expected_current_show(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > >> + > >> + return sprintf(buf, "%d\n", chip->max_expected_current); > >> +} > >> + > >> +static ssize_t ina2xx_max_expected_current_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t len) > >> +{ > >> + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > >> + unsigned long val; > >> + int ret; > >> + > >> + ret = kstrtoul((const char *) buf, 10, &val); > > > > Odd bit of casting given that's what it already is... > > > >> + if (ret) > >> + return ret; > >> + > >> + ret = set_max_expected_current(chip, val); > >> + if (ret) > >> + return ret; > >> + > >> + /* Update the Calibration register */ > >> + ret = ina2xx_set_calibration(chip); > >> + if (ret) > >> + return ret; > >> + > >> + return len; > >> +} > >> + > >> #define INA219_CHAN(_type, _index, _address) { \ > >> .type = (_type), \ > >> .address = (_address), \ > >> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR, > >> ina2xx_shunt_resistor_show, > >> ina2xx_shunt_resistor_store, 0); > >> > >> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR, > >> + ina2xx_max_expected_current_show, > >> + ina2xx_max_expected_current_store, 0); > >> + > >> 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, > >> + &iio_dev_attr_in_max_expected_current.dev_attr.attr, > >> NULL, > >> }; > >> > >> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = { > >> &iio_dev_attr_in_allow_async_readout.dev_attr.attr, > >> &iio_const_attr_ina226_integration_time_available.dev_attr.attr, > >> &iio_dev_attr_in_shunt_resistor.dev_attr.attr, > >> + &iio_dev_attr_in_max_expected_current.dev_attr.attr, > >> NULL, > >> }; > >> > >> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client, > >> if (ret) > >> return ret; > >> > >> + if (of_property_read_u32(client->dev.of_node, > >> + "max-expected-current", &val) < 0) { > >> + struct ina2xx_platform_data *pdata = > >> + dev_get_platdata(&client->dev); > >> + > >> + if (pdata && pdata->max_mA != 0) > >> + val = pdata->max_mA; > >> + else > >> + val = INA2XX_MAX_EXPECTED_A_DEFAULT; > >> + } > >> + > >> + ret = set_max_expected_current(chip, val); > >> + if (ret) > >> + return ret; > >> + > >> /* Patch the current config register with default. */ > >> val = chip->config->config_default; > >> > >> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h > >> index 9abc0ca..f02b1d8 100644 > >> --- a/include/linux/platform_data/ina2xx.h > >> +++ b/include/linux/platform_data/ina2xx.h > >> @@ -13,7 +13,9 @@ > >> /** > >> * struct ina2xx_platform_data - ina2xx info > >> * @shunt_uohms shunt resistance in microohms > >> + * @max_mA max expected current in mA > >> */ > >> struct ina2xx_platform_data { > >> long shunt_uohms; > >> + int max_mA; > >> }; > > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html