From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752601AbdKHNWQ (ORCPT ); Wed, 8 Nov 2017 08:22:16 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:38810 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752397AbdKHNWN (ORCPT ); Wed, 8 Nov 2017 08:22:13 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20171108132210euoutp01a8bd530635884ada15bccf7604299b3d~1Hv8RlocH1590815908euoutp017 X-AuditID: cbfec7f1-f793a6d00000326b-e6-5a030501f996 MIME-version: 1.0 Content-type: text/plain; charset="windows-1252"; format="flowed" From: Maciej Purski Subject: Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm To: =?UTF-8?Q?Stefan_Br=c3=bcns?= Cc: linux-iio@vger.kernel.org, Peter Meerwald-Stadler , linux-kernel@vger.kernel.org, "Andrew F . Davis" , Javier Martinez Canillas , Lars-Peter Clausen , Jonathan Cameron , Hartmut Knaack Message-id: <8d828867-7f8b-9dce-a331-3c68dbd3c842@samsung.com> Date: Wed, 08 Nov 2017 14:22:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 In-reply-to: <1952714.EgNKj5xy7d@pebbles> Content-language: en-US Content-transfer-encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMKsWRmVeSWpSXmKPExsWy7djP87qMrMxRBlf3W1i8PzWR3eLN2zVM Fg+aVjFZ7Pr/htliyeT5rBbzjrxjsbi8aw6bxe9dx9gtGk/NYXXg9PjwMc5j06pONo8lbw6x emzpv8vucb75CKPH+Xtv2TyO39jO5PF5k1wARxSXTUpqTmZZapG+XQJXxsOjnxkLlmpX9Oy+ wNbA+FSpi5GTQ0LAROL9nWtMELaYxIV769m6GLk4hASWMkqcetnEAuF8ZpRo3ruMHabjWO88 FhBbSGAZo8SS6bogNq+AoMSPyffA4swCjhIPFu1khWh+xigxb/F9IIeDg01AS2JNezxIjbBA hMS9nbuZQMIiAg4Sy1bxgJQzC2xgkvjQvYwVYqadxLMNPWCtLAKqEq9WBYCERYFaL2z6CXY0 p4CmRPfk1cwQa8UlmltvQp0gL3HwynOw+yUEvrNJ3H08AepLF4m+i/OgbGGJV8e3QP0lI9HZ cRAqXi1x8esuNgi7RqLx9gaoGmuJz5O2QC3jk5i0bTozyG0SArwSHW1CECUeEi++f4Yqd5S4 /eAbNBimMklcb+5gnMAoPwspuGYhBdcsJD/MQvLDAkaWVYwiqaXFuempxUZ6xYm5xaV56XrJ +bmbGIFJ6fS/4x93ML4/YXWIUYCDUYmHV0ORKUqINbGsuDL3EKMEB7OSCO+5s0Ah3pTEyqrU ovz4otKc1OJDjNIcLErivLZRbZFCAumJJanZqakFqUUwWSYOTqkGRt3NvSdUr+jVRCv7S5Z3 pF++nZoXpPklVfDS73ZZ32c7tNdUnpz25JGXtrfnzIWpitNZBZ9+XqOi6PJ58qIfRhE73j/T 4d605L3mr3mnJm96+0mnnvGjtPdRa03L3bx7bC619Os06vXJH/B+pdhUEyQb9fQWT4Sm0Cmv I03PjIMOSi7lm3fnrBJLcUaioRZzUXEiACIoTQxGAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupgkeLIzCtJLcpLzFFi42I5/e/4NV1GVuYog2nXxCzen5rIbvHm7Rom iwdNq5gsdv1/w2yxZPJ8Vot5R96xWFzeNYfN4veuY+wWjafmsDpwenz4GOexaVUnm8eSN4dY Pbb032X3ON98hNHj/L23bB7Hb2xn8vi8SS6AI4rLJiU1J7MstUjfLoEr4+HRz4wFS7UrenZf YGtgfKrUxcjJISFgInGsdx4LhC0mceHeejYQW0hgCaPE3VeFIDavgKDEj8n3wGqYBWwlFrxf B2RzAdU8Y5S4t+APYxcjBwebgJbEmvZ4kBphgQiJ/R9fs4CERQQcJJat4gEpZxbYwCSx8mET G0TvVCaJrdPOskMssJN4tqGHFaSBRUBV4tWqABBTFGjOho38IBWcApoS3ZNXM0OcIC7R3HoT 6hx5iYNXnrNMYBScheTSWUgunYWkZRaSlgWMLKsYRVJLi3PTc4sN9YoTc4tL89L1kvNzNzEC 42fbsZ+bdzBe2hh8iFGAg1GJh1dDkSlKiDWxrLgy9xCjBAezkgjvubNAId6UxMqq1KL8+KLS nNTiQ4zSHCxK4ry9e1ZHCgmkJ5akZqemFqQWwWSZODilGhiDbXp0D005biA15X9avN3bat3Z 4sc+RKuIXf64ws+5grm49vqrJ/Ouns25kXFO+qK4hM/lo9ab2cJsCnPnVqc2cN+YOvkzm+eq BTxTV7Ym50/SY19q4fDemH1L7adTK9xcnMtfvZvb/tKan7VmSqT4r7Ylz7wm5S921604uEhJ u2tLp/mEj6JKLMUZiYZazEXFiQC9q12UmwIAAA== X-CMS-MailID: 20171108132209eucas1p261985491eaf13aa3076b0dc8f03375ff X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20171106102118epcas1p447fb44a5939a827e6bc367841c0564d5 X-RootMTR: 20171106102118epcas1p447fb44a5939a827e6bc367841c0564d5 References: <7ee955b2-a58f-45e0-985d-1f7508d41ae7@rwthex-w2-a.rwth-ad.de> <2075928.pSFUuclJ1z@pebbles.site> <678f7605-3fc8-cc28-cb16-45381d75bad7@samsung.com> <1952714.EgNKj5xy7d@pebbles> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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