On Wed, 08 Nov 2017 14:22:08 +0100 Maciej Purski <m.purski@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
