Date:   Tue, 30 Nov 2021 13:52:01 -0500
From:   Liam Beguin <liambeguin@gmail.com>
To:     Peter Rosin <peda@axentia.se>
Cc:     jic23@kernel.org, lars@metafoo.de, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        robh+dt@kernel.org
Subject: Re: [PATCH v9 00/14] iio: afe: add temperature rescaling support
Message-ID: <YaZy0Wk4Kzptt0SX@shaak>
References: <20211115034334.1713050-1-liambeguin@gmail.com>
 <156bc2fa-6754-2350-4a12-ff25b23ae8a2@axentia.se>
 <YaKUmha11ft6gip2@shaak>
 <b9e1e804-fb59-660f-a9b8-ad6e20dd41aa@axentia.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b9e1e804-fb59-660f-a9b8-ad6e20dd41aa@axentia.se>
Precedence: bulk

Hi Peter,

On Sun, Nov 28, 2021 at 10:17:50AM +0100, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-27 21:27, Liam Beguin wrote:
> > Hi Peter,
> > 
> > On Mon, Nov 22, 2021 at 01:53:44AM +0100, Peter Rosin wrote:
> >> Hi Liam!
> >>
> >> On 2021-11-15 04:43, Liam Beguin wrote:
> >>> Hi Jonathan, Peter,
> 
> snip
> 
> >>> - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed
> >>>   point
> >>
> >> This is not what is going on. Patch 9/14 will convert all fractional
> >> scales to fixed point. But I would really like if you in the "reduce
> >> risk of integer overflow" patch (8/14) would hold true to the above
> >> and keep the fractional scale when possible and only fall back to
> >> the less precise fractional-log case if any of the multiplications
> >> needed for an exact fractional scale causes overflow.
> > 
> > Thanks for looking at these patches again.
> > 
> >> The v8 discussion concluded that this was a valid approach, right?
> > 
> > Yes, I remember you saying that you'd be more comfortable keeping the
> > IIO_VAL_FRACTIONAL.
> > 
> >> I know you also said that the core exposes the scale with nano
> >> precision in sysfs anyway, but that is not true for in-kernel
> >> consumers. They have an easier time reading the "real" scale value
> >> compared to going via the string representation of fixed point
> >> returned from iio_format_value. At least the rescaler itself does so,
> >> which means that chaining rescalers might suffer needless accuracy
> >> degradation.
> > 
> > Agreed, that makes total sense.
> > 
> > If I'm not mistaken, the first condition in the case, if (!rem), will
> > return IIO_VAL_FRACTIONAL if the division is exact, keeping all the
> > precision. No?
> 
> Only if the resulting scale fits in nine decimals. That's never the
> case if you have primes other than 2 and 5 in the denominator (after
> eliminating gcd of course). Which mean that if you chain one rescaler
> doing 1/3 and one doing 3/1, you would get a combined scale of
> 0.999999999 instead of 3/3 if we take the approach of these patches.
> 
> So, what I'm after is that - for IIO_VAL_FRACTIONAL - not take the
> multiply-by-1e9 code path /unless/ the existing fractional approach
> overflows in either numerator or denominator (or both).

Understood, I'll update based on this.

> Side note: The same could be done for IIO_VAL_INT when the numerator
> overflows (since the denominator cannot overflow), but I guess that
> can be done later.

Agreed, I don't mind working on this later but I'd like to focus on
getting the current changes in first.

Thanks,
Liam

> Cheers,
> Peter