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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
