| Message ID | 7ee955b2-a58f-45e0-985d-1f7508d41ae7@rwthex-w2-a.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 1 Oct 2017 21:48:17 +0200 Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > According to the ABI documentation, the shunt resistor value should be > specificied in Ohm. As this is also used/documented for the MAX9611, > use the same for the INA2xx driver. > > This poses an ABI break for anyone actually altering the shunt value > through the sysfs interface, it does not alter the default value nor > a value set from the devicetree. Yeah, I messed up on this an missed that we had a number of different drivers with different scaling on this.. Sorry about that. So I think we need to apply this to get consistency - changing shunt resistance from userspace does seem fairly unusual though so fingers crossed that no one is doing it. I'm going to do this the slow way though so hopefully we get shouts before breaking too many people. Hence I'm routing this through the next merge window. After it's been in mainline all the way to release, if no-one complains we can request it is added to stable. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > > Minor change: Fix comment, 1mA is 10^-3A. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > > drivers/iio/adc/ina2xx-adc.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index 361fb4e459d5..1930f853e8c5 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -127,7 +127,7 @@ 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_uohm; > int avg; > int int_time_vbus; /* Bus voltage integration time uS */ > int int_time_vshunt; /* Shunt voltage integration time uS */ > @@ -444,7 +444,7 @@ 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. > + * of 1.0 x10-3. The only remaining parameter is RShunt. > * 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 > @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev, > static int ina2xx_set_calibration(struct ina2xx_chip_info *chip) > { > u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, > - chip->shunt_resistor); > + chip->shunt_resistor_uohm); > > return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); > } > @@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val) > if (val <= 0 || val > chip->config->calibration_factor) > return -EINVAL; > > - chip->shunt_resistor = val; > + chip->shunt_resistor_uohm = val; > > return 0; > } > @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev, > char *buf) > { > struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > + int vals[2] = { chip->shunt_resistor_uohm, 1000000 }; > > - return sprintf(buf, "%d\n", chip->shunt_resistor); > + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals); > } > > static ssize_t ina2xx_shunt_resistor_store(struct device *dev, > @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev, > const char *buf, size_t len) > { > struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > - unsigned long val; > - int ret; > + int val, val_fract, ret; > > - ret = kstrtoul((const char *) buf, 10, &val); > + ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract); > if (ret) > return ret; > > - ret = set_shunt_resistor(chip, val); > + ret = set_shunt_resistor(chip, val * 1000000 + val_fract); > if (ret) > return ret; > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/01/2017 09:48 PM, Stefan Brüns wrote: > According to the ABI documentation, the shunt resistor value should be > specificied in Ohm. As this is also used/documented for the MAX9611, > use the same for the INA2xx driver. > > This poses an ABI break for anyone actually altering the shunt value > through the sysfs interface, it does not alter the default value nor > a value set from the devicetree. > > Minor change: Fix comment, 1mA is 10^-3A. > I have just a minor issue. There could be an inconsistency with units as in my patch I make current_lsb adjustable and I need it to be in uA (it used to be hardcoded as 1 mA so to achieve better precision we need smaller units). So in order to keep calibration register properly scaled, I convert uOhms to mOhms on each set_calibration(). So if both my changes and your changes were applied, on each shunt_resistore_store we would be performing multiplication by 10^6 and then in set_calibration() division by 10^3 which seems odd to me. I guess we could keep it as shunt_resistor_ohms instead of shunt_resistor_uohm. We could avoid performing division on each shunt_resistor_show() and perform multiplication by 10^3 only once in set_calibration() on each shunt_resistore_store(). We could then change the default value and perform division only on probing, when reading the shunt_resistance from device tree. There are many other options. It's not a major issue so maybe we could leave it as it is or you could suggest some changes in my patch. > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > > drivers/iio/adc/ina2xx-adc.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index 361fb4e459d5..1930f853e8c5 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -127,7 +127,7 @@ 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_uohm; > int avg; > int int_time_vbus; /* Bus voltage integration time uS */ > int int_time_vshunt; /* Shunt voltage integration time uS */ > @@ -444,7 +444,7 @@ 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. > + * of 1.0 x10-3. The only remaining parameter is RShunt. > * 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 > @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev, > static int ina2xx_set_calibration(struct ina2xx_chip_info *chip) > { > u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, > - chip->shunt_resistor); > + chip->shunt_resistor_uohm); > > return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); > } > @@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val) > if (val <= 0 || val > chip->config->calibration_factor) > return -EINVAL; > > - chip->shunt_resistor = val; > + chip->shunt_resistor_uohm = val; > > return 0; > } > @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev, > char *buf) > { > struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > + int vals[2] = { chip->shunt_resistor_uohm, 1000000 }; > > - return sprintf(buf, "%d\n", chip->shunt_resistor); > + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals); > } > > static ssize_t ina2xx_shunt_resistor_store(struct device *dev, > @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev, > const char *buf, size_t len) > { > struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > - unsigned long val; > - int ret; > + int val, val_fract, ret; > > - ret = kstrtoul((const char *) buf, 10, &val); > + ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract); > if (ret) > return ret; > > - ret = set_shunt_resistor(chip, val); > + ret = set_shunt_resistor(chip, val * 1000000 + val_fract); > if (ret) > return ret; > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote: > On 10/01/2017 09:48 PM, Stefan Brüns wrote: > > According to the ABI documentation, the shunt resistor value should be > > specificied in Ohm. As this is also used/documented for the MAX9611, > > use the same for the INA2xx driver. > > > > This poses an ABI break for anyone actually altering the shunt value > > through the sysfs interface, it does not alter the default value nor > > a value set from the devicetree. > > > > Minor change: Fix comment, 1mA is 10^-3A. > > I have just a minor issue. There could be an inconsistency with units as in > my patch I make current_lsb adjustable and I need it to be in uA (it used > to be hardcoded as 1 mA so to achieve better precision we need smaller > units). So in order to keep calibration register properly scaled, I convert > uOhms to mOhms on each set_calibration(). So if both my changes and your > changes were applied, on each shunt_resistore_store we would be performing > multiplication by 10^6 and then in set_calibration() division by 10^3 which > seems odd to me. > > I guess we could keep it as shunt_resistor_ohms instead of > shunt_resistor_uohm. We could avoid performing division on each > shunt_resistor_show() and perform multiplication by 10^3 only once in > set_calibration() on each > shunt_resistore_store(). We could then change the default value and perform > division only on probing, when reading the shunt_resistance from device > tree. > > There are many other options. It's not a major issue so maybe we could leave > it as it is or you could suggest some changes in my patch. Sorry it took me so long to answer ... The current fixed current_lsb of 1mA is indeed a bad choice for everything but a shunt resistor value of 10mOhm, as it truncates the current value. So what is a *good* choice? One important point is the current register is merely more than a convenience register. At least for the INA219/220, it provides nothing not achievable in software, and for the INA226 family it only has added value if the current is varying faster than the readout frequency and the averaging is used. The precision of the current register is limited by the precision of the shunt voltage register, and may be reduced by the applied scaling/calibration factor. The precision of the shunt voltage register is fixed at 10uV (INA219) resp. 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the noise and offset, but the lsb value is still fixed. If one wants to carry over the shunt voltage register precision into the current register, its important no (or hardly any) truncation happens. The terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3): INA219: current = shunt_voltage * cal_register / 4096 INA226: current = shunt_voltage * cal_register / 2048 So any cal value smaller than 4096 (2048) will introduce truncation errors, larger values may introduce overflows, if the full input range is used. Now, would it not be wise to always use 4096 (2048) for the calibration value? The raw values from the IIO subsystem are meaningless without their accompanying scale factor. Instead of changing the calibration value, why not just change the reported scale factor? More opinions are very welcome. Kind regards, Stefan
On 10/14/2017 08:27 PM, Stefan Bruens wrote: > On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote: >> On 10/01/2017 09:48 PM, Stefan Brüns wrote: >>> According to the ABI documentation, the shunt resistor value should be >>> specificied in Ohm. As this is also used/documented for the MAX9611, >>> use the same for the INA2xx driver. >>> >>> This poses an ABI break for anyone actually altering the shunt value >>> through the sysfs interface, it does not alter the default value nor >>> a value set from the devicetree. >>> >>> Minor change: Fix comment, 1mA is 10^-3A. >> >> I have just a minor issue. There could be an inconsistency with units as in >> my patch I make current_lsb adjustable and I need it to be in uA (it used >> to be hardcoded as 1 mA so to achieve better precision we need smaller >> units). So in order to keep calibration register properly scaled, I convert >> uOhms to mOhms on each set_calibration(). So if both my changes and your >> changes were applied, on each shunt_resistore_store we would be performing >> multiplication by 10^6 and then in set_calibration() division by 10^3 which >> seems odd to me. >> >> I guess we could keep it as shunt_resistor_ohms instead of >> shunt_resistor_uohm. We could avoid performing division on each >> shunt_resistor_show() and perform multiplication by 10^3 only once in >> set_calibration() on each >> shunt_resistore_store(). We could then change the default value and perform >> division only on probing, when reading the shunt_resistance from device >> tree. >> >> There are many other options. It's not a major issue so maybe we could leave >> it as it is or you could suggest some changes in my patch. > > Sorry it took me so long to answer ... > > The current fixed current_lsb of 1mA is indeed a bad choice for everything but > a shunt resistor value of 10mOhm, as it truncates the current value. So what > is a *good* choice? > > One important point is the current register is merely more than a convenience > register. At least for the INA219/220, it provides nothing not achievable in > software, and for the INA226 family it only has added value if the current is > varying faster than the readout frequency and the averaging is used. > > The precision of the current register is limited by the precision of the shunt > voltage register, and may be reduced by the applied scaling/calibration > factor. > > The precision of the shunt voltage register is fixed at 10uV (INA219) resp. > 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the > noise and offset, but the lsb value is still fixed. > > If one wants to carry over the shunt voltage register precision into the > current register, its important no (or hardly any) truncation happens. The > terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3): > > INA219: current = shunt_voltage * cal_register / 4096 > INA226: current = shunt_voltage * cal_register / 2048 > > So any cal value smaller than 4096 (2048) will introduce truncation errors, > larger values may introduce overflows, if the full input range is used. Now, > would it not be wise to always use 4096 (2048) for the calibration value? > > The raw values from the IIO subsystem are meaningless without their > accompanying scale factor. Instead of changing the calibration value, why not > just change the reported scale factor? > > More opinions are very welcome. > > Kind regards, > > Stefan > Thanks for the reply. I agree that cal_register set to 4096 (2048) allows us to eliminate truncaction error. However according to your suggestion, if we made cal_reg a fixed value, then current_lsb and r_shunt should be also a fixed value, as they are related according to formula 8.5 (1) cal_register = 0.00512 / (current_lsb * r_shunt) Therefore, changing the scale value wouldn't affect the calib_reg value, so it wouldn't give the user any information on the actual current_lsb of the device. The real value is calculated like this by the user: processed_value = raw_value * scale I think that even after changing the scale value processed_value is expected to be approximately the same. Maybe I'm wrong or I didn't precisely understand what you have suggested. I hope that someone will also comment on that. Best regards, Maciej -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote: > On 10/14/2017 08:27 PM, Stefan Bruens wrote: > > On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote: > >> On 10/01/2017 09:48 PM, Stefan Brüns wrote: > >>> According to the ABI documentation, the shunt resistor value should be > >>> specificied in Ohm. As this is also used/documented for the MAX9611, > >>> use the same for the INA2xx driver. > >>> > >>> This poses an ABI break for anyone actually altering the shunt value > >>> through the sysfs interface, it does not alter the default value nor > >>> a value set from the devicetree. > >>> > >>> Minor change: Fix comment, 1mA is 10^-3A. > >> > >> I have just a minor issue. There could be an inconsistency with units as > >> in > >> my patch I make current_lsb adjustable and I need it to be in uA (it used > >> to be hardcoded as 1 mA so to achieve better precision we need smaller > >> units). So in order to keep calibration register properly scaled, I > >> convert > >> uOhms to mOhms on each set_calibration(). So if both my changes and your > >> changes were applied, on each shunt_resistore_store we would be > >> performing > >> multiplication by 10^6 and then in set_calibration() division by 10^3 > >> which > >> seems odd to me. > >> > >> I guess we could keep it as shunt_resistor_ohms instead of > >> shunt_resistor_uohm. We could avoid performing division on each > >> shunt_resistor_show() and perform multiplication by 10^3 only once in > >> set_calibration() on each > >> shunt_resistore_store(). We could then change the default value and > >> perform > >> division only on probing, when reading the shunt_resistance from device > >> tree. > >> > >> There are many other options. It's not a major issue so maybe we could > >> leave it as it is or you could suggest some changes in my patch. > > > > Sorry it took me so long to answer ... > > > > The current fixed current_lsb of 1mA is indeed a bad choice for everything > > but a shunt resistor value of 10mOhm, as it truncates the current value. > > So what is a *good* choice? > > > > One important point is the current register is merely more than a > > convenience register. At least for the INA219/220, it provides nothing > > not achievable in software, and for the INA226 family it only has added > > value if the current is varying faster than the readout frequency and the > > averaging is used. > > > > The precision of the current register is limited by the precision of the > > shunt voltage register, and may be reduced by the applied > > scaling/calibration factor. > > > > The precision of the shunt voltage register is fixed at 10uV (INA219) > > resp. > > 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the > > noise and offset, but the lsb value is still fixed. > > > > If one wants to carry over the shunt voltage register precision into the > > current register, its important no (or hardly any) truncation happens. The > > terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3): > > > > INA219: current = shunt_voltage * cal_register / 4096 > > INA226: current = shunt_voltage * cal_register / 2048 > > > > So any cal value smaller than 4096 (2048) will introduce truncation > > errors, > > larger values may introduce overflows, if the full input range is used. > > Now, would it not be wise to always use 4096 (2048) for the calibration > > value? > > > > The raw values from the IIO subsystem are meaningless without their > > accompanying scale factor. Instead of changing the calibration value, why > > not just change the reported scale factor? > > > > More opinions are very welcome. > > > > Kind regards, > > > > Stefan > > Thanks for the reply. > > I agree that cal_register set to 4096 (2048) allows us to eliminate > truncaction error. However according to your suggestion, if we made cal_reg > a fixed value, then current_lsb and r_shunt should be also a fixed value, > as they are related according to formula 8.5 (1) > > cal_register = 0.00512 / (current_lsb * r_shunt) A fixed cal_register only means the current_lsb is implied by the selected shunt resistor value. If you insert 2048 into the equation above, you get: current_lsb = 2.5 * 1e-6 * r_shunt, and using Ohms law to replace r_shunt, thats exactly the resolution of the shunt_voltage register as specified in the datasheet. The higher the shunt resistor value, the smaller the current_lsb. > Therefore, changing the scale value wouldn't affect the calib_reg value, so > it wouldn't give the user any information on the actual current_lsb of the > device. The real value is calculated like this by the user: > > processed_value = raw_value * scale > > I think that even after changing the scale value processed_value is expected > to be approximately the same. A fixed cal_register means you change the current_lsb by changing the shunt resistor. This exposes the full ADC resolution. The current_lsb *is* the scale value. Kind regards, Stefan
On 11/06/2017 11:21 AM, Stefan Brüns wrote: > On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote: >> On 10/14/2017 08:27 PM, Stefan Bruens wrote: >>> On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote: >>>> On 10/01/2017 09:48 PM, Stefan Brüns wrote: >>>>> According to the ABI documentation, the shunt resistor value should be >>>>> specificied in Ohm. As this is also used/documented for the MAX9611, >>>>> use the same for the INA2xx driver. >>>>> >>>>> This poses an ABI break for anyone actually altering the shunt value >>>>> through the sysfs interface, it does not alter the default value nor >>>>> a value set from the devicetree. >>>>> >>>>> Minor change: Fix comment, 1mA is 10^-3A. >>>> >>>> I have just a minor issue. There could be an inconsistency with units as >>>> in >>>> my patch I make current_lsb adjustable and I need it to be in uA (it used >>>> to be hardcoded as 1 mA so to achieve better precision we need smaller >>>> units). So in order to keep calibration register properly scaled, I >>>> convert >>>> uOhms to mOhms on each set_calibration(). So if both my changes and your >>>> changes were applied, on each shunt_resistore_store we would be >>>> performing >>>> multiplication by 10^6 and then in set_calibration() division by 10^3 >>>> which >>>> seems odd to me. >>>> >>>> I guess we could keep it as shunt_resistor_ohms instead of >>>> shunt_resistor_uohm. We could avoid performing division on each >>>> shunt_resistor_show() and perform multiplication by 10^3 only once in >>>> set_calibration() on each >>>> shunt_resistore_store(). We could then change the default value and >>>> perform >>>> division only on probing, when reading the shunt_resistance from device >>>> tree. >>>> >>>> There are many other options. It's not a major issue so maybe we could >>>> leave it as it is or you could suggest some changes in my patch. >>> >>> Sorry it took me so long to answer ... >>> >>> The current fixed current_lsb of 1mA is indeed a bad choice for everything >>> but a shunt resistor value of 10mOhm, as it truncates the current value. >>> So what is a *good* choice? >>> >>> One important point is the current register is merely more than a >>> convenience register. At least for the INA219/220, it provides nothing >>> not achievable in software, and for the INA226 family it only has added >>> value if the current is varying faster than the readout frequency and the >>> averaging is used. >>> >>> The precision of the current register is limited by the precision of the >>> shunt voltage register, and may be reduced by the applied >>> scaling/calibration factor. >>> >>> The precision of the shunt voltage register is fixed at 10uV (INA219) >>> resp. >>> 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the >>> noise and offset, but the lsb value is still fixed. >>> >>> If one wants to carry over the shunt voltage register precision into the >>> current register, its important no (or hardly any) truncation happens. The >>> terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3): >>> >>> INA219: current = shunt_voltage * cal_register / 4096 >>> INA226: current = shunt_voltage * cal_register / 2048 >>> >>> So any cal value smaller than 4096 (2048) will introduce truncation >>> errors, >>> larger values may introduce overflows, if the full input range is used. >>> Now, would it not be wise to always use 4096 (2048) for the calibration >>> value? >>> >>> The raw values from the IIO subsystem are meaningless without their >>> accompanying scale factor. Instead of changing the calibration value, why >>> not just change the reported scale factor? >>> >>> More opinions are very welcome. >>> >>> Kind regards, >>> >>> Stefan >> >> Thanks for the reply. >> >> I agree that cal_register set to 4096 (2048) allows us to eliminate >> truncaction error. However according to your suggestion, if we made cal_reg >> a fixed value, then current_lsb and r_shunt should be also a fixed value, >> as they are related according to formula 8.5 (1) >> >> cal_register = 0.00512 / (current_lsb * r_shunt) > > A fixed cal_register only means the current_lsb is implied by the selected > shunt resistor value. > > If you insert 2048 into the equation above, you get: > > current_lsb = 2.5 * 1e-6 * r_shunt, > > and using Ohms law to replace r_shunt, thats exactly the resolution of the > shunt_voltage register as specified in the datasheet. The higher the shunt > resistor value, the smaller the current_lsb. > >> Therefore, changing the scale value wouldn't affect the calib_reg value, so >> it wouldn't give the user any information on the actual current_lsb of the >> device. The real value is calculated like this by the user: >> >> processed_value = raw_value * scale >> >> I think that even after changing the scale value processed_value is expected >> to be approximately the same. > > A fixed cal_register means you change the current_lsb by changing the shunt > resistor. This exposes the full ADC resolution. > > The current_lsb *is* the scale value. > > Kind regards, > > Stefan > Thanks for your explanation. I can do this the way you suggest, so the only change with the original driver would be to make current_lsb (which is a scale value) follow changes of shunt_resistance value from userspace. But before I'd like to ask Jonathan for opinion on that. Kind regards, Maciej -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 08 Nov 2017 14:22:08 +0100 Maciej Purski <m.purski@samsung.com> wrote: > On 11/06/2017 11:21 AM, Stefan Brüns wrote: > > On Thursday, November 2, 2017 10:04:01 AM CET Maciej Purski wrote: > >> On 10/14/2017 08:27 PM, Stefan Bruens wrote: > >>> On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote: > >>>> On 10/01/2017 09:48 PM, Stefan Brüns wrote: > >>>>> According to the ABI documentation, the shunt resistor value should be > >>>>> specificied in Ohm. As this is also used/documented for the MAX9611, > >>>>> use the same for the INA2xx driver. > >>>>> > >>>>> This poses an ABI break for anyone actually altering the shunt value > >>>>> through the sysfs interface, it does not alter the default value nor > >>>>> a value set from the devicetree. > >>>>> > >>>>> Minor change: Fix comment, 1mA is 10^-3A. > >>>> > >>>> I have just a minor issue. There could be an inconsistency with units as > >>>> in > >>>> my patch I make current_lsb adjustable and I need it to be in uA (it used > >>>> to be hardcoded as 1 mA so to achieve better precision we need smaller > >>>> units). So in order to keep calibration register properly scaled, I > >>>> convert > >>>> uOhms to mOhms on each set_calibration(). So if both my changes and your > >>>> changes were applied, on each shunt_resistore_store we would be > >>>> performing > >>>> multiplication by 10^6 and then in set_calibration() division by 10^3 > >>>> which > >>>> seems odd to me. > >>>> > >>>> I guess we could keep it as shunt_resistor_ohms instead of > >>>> shunt_resistor_uohm. We could avoid performing division on each > >>>> shunt_resistor_show() and perform multiplication by 10^3 only once in > >>>> set_calibration() on each > >>>> shunt_resistore_store(). We could then change the default value and > >>>> perform > >>>> division only on probing, when reading the shunt_resistance from device > >>>> tree. > >>>> > >>>> There are many other options. It's not a major issue so maybe we could > >>>> leave it as it is or you could suggest some changes in my patch. > >>> > >>> Sorry it took me so long to answer ... > >>> > >>> The current fixed current_lsb of 1mA is indeed a bad choice for everything > >>> but a shunt resistor value of 10mOhm, as it truncates the current value. > >>> So what is a *good* choice? > >>> > >>> One important point is the current register is merely more than a > >>> convenience register. At least for the INA219/220, it provides nothing > >>> not achievable in software, and for the INA226 family it only has added > >>> value if the current is varying faster than the readout frequency and the > >>> averaging is used. > >>> > >>> The precision of the current register is limited by the precision of the > >>> shunt voltage register, and may be reduced by the applied > >>> scaling/calibration factor. > >>> > >>> The precision of the shunt voltage register is fixed at 10uV (INA219) > >>> resp. > >>> 2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the > >>> noise and offset, but the lsb value is still fixed. > >>> > >>> If one wants to carry over the shunt voltage register precision into the > >>> current register, its important no (or hardly any) truncation happens. The > >>> terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3): > >>> > >>> INA219: current = shunt_voltage * cal_register / 4096 > >>> INA226: current = shunt_voltage * cal_register / 2048 > >>> > >>> So any cal value smaller than 4096 (2048) will introduce truncation > >>> errors, > >>> larger values may introduce overflows, if the full input range is used. > >>> Now, would it not be wise to always use 4096 (2048) for the calibration > >>> value? > >>> > >>> The raw values from the IIO subsystem are meaningless without their > >>> accompanying scale factor. Instead of changing the calibration value, why > >>> not just change the reported scale factor? > >>> > >>> More opinions are very welcome. > >>> > >>> Kind regards, > >>> > >>> Stefan > >> > >> Thanks for the reply. > >> > >> I agree that cal_register set to 4096 (2048) allows us to eliminate > >> truncaction error. However according to your suggestion, if we made cal_reg > >> a fixed value, then current_lsb and r_shunt should be also a fixed value, > >> as they are related according to formula 8.5 (1) > >> > >> cal_register = 0.00512 / (current_lsb * r_shunt) > > > > A fixed cal_register only means the current_lsb is implied by the selected > > shunt resistor value. > > > > If you insert 2048 into the equation above, you get: > > > > current_lsb = 2.5 * 1e-6 * r_shunt, > > > > and using Ohms law to replace r_shunt, thats exactly the resolution of the > > shunt_voltage register as specified in the datasheet. The higher the shunt > > resistor value, the smaller the current_lsb. > > > >> Therefore, changing the scale value wouldn't affect the calib_reg value, so > >> it wouldn't give the user any information on the actual current_lsb of the > >> device. The real value is calculated like this by the user: > >> > >> processed_value = raw_value * scale > >> > >> I think that even after changing the scale value processed_value is expected > >> to be approximately the same. > > > > A fixed cal_register means you change the current_lsb by changing the shunt > > resistor. This exposes the full ADC resolution. > > > > The current_lsb *is* the scale value. > > > > Kind regards, > > > > Stefan > > > > Thanks for your explanation. I can do this the way you suggest, so the only > change with the original driver would be to make current_lsb (which is a scale > value) follow changes of shunt_resistance value from userspace. > > But before I'd like to ask Jonathan for opinion on that. This is what I was thinking as well. We basically ensure the scale is right for the shunt_resistance with the ADC operating at it's best possible accuracy and let userspace sort out the mess (as we provide it with the data to do so). > > Kind regards, > > Maciej > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 361fb4e459d5..1930f853e8c5 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -127,7 +127,7 @@ 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_uohm; int avg; int int_time_vbus; /* Bus voltage integration time uS */ int int_time_vshunt; /* Shunt voltage integration time uS */ @@ -444,7 +444,7 @@ 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. + * of 1.0 x10-3. The only remaining parameter is RShunt. * 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 @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev, static int ina2xx_set_calibration(struct ina2xx_chip_info *chip) { u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, - chip->shunt_resistor); + chip->shunt_resistor_uohm); return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); } @@ -463,7 +463,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val) if (val <= 0 || val > chip->config->calibration_factor) return -EINVAL; - chip->shunt_resistor = val; + chip->shunt_resistor_uohm = val; return 0; } @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev, char *buf) { struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); + int vals[2] = { chip->shunt_resistor_uohm, 1000000 }; - return sprintf(buf, "%d\n", chip->shunt_resistor); + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals); } static ssize_t ina2xx_shunt_resistor_store(struct device *dev, @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev, const char *buf, size_t len) { struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); - unsigned long val; - int ret; + int val, val_fract, ret; - ret = kstrtoul((const char *) buf, 10, &val); + ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract); if (ret) return ret; - ret = set_shunt_resistor(chip, val); + ret = set_shunt_resistor(chip, val * 1000000 + val_fract); if (ret) return ret;
According to the ABI documentation, the shunt resistor value should be specificied in Ohm. As this is also used/documented for the MAX9611, use the same for the INA2xx driver. This poses an ABI break for anyone actually altering the shunt value through the sysfs interface, it does not alter the default value nor a value set from the devicetree. Minor change: Fix comment, 1mA is 10^-3A. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/iio/adc/ina2xx-adc.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)