Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Maciej Purski]

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