On Wed Jul 28, 2021 at 3:58 AM EDT, Peter Rosin wrote: > On 2021-07-28 02:26, Liam Beguin wrote: > > On Fri Jul 23, 2021 at 5:20 PM EDT, Peter Rosin wrote: > >> On 2021-07-21 05:06, Liam Beguin wrote: > >>> From: Liam Beguin <lvb@xxxxxxxxxx> > >>> > >>> The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected > >>> scale. Update the case so that the rescaler returns a fractional type > >>> and a more precise scale. > >>> > >>> Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx> > >>> --- > >>> drivers/iio/afe/iio-rescale.c | 9 +++------ > >>> 1 file changed, 3 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > >>> index 35fa3b4e53e0..47cd4a6d9aca 100644 > >>> --- a/drivers/iio/afe/iio-rescale.c > >>> +++ b/drivers/iio/afe/iio-rescale.c > >>> @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > >>> *val2 = rescale->denominator; > >>> return IIO_VAL_FRACTIONAL; > >>> case IIO_VAL_FRACTIONAL_LOG2: > >>> - tmp = *val * 1000000000LL; > >>> - do_div(tmp, rescale->denominator); > >>> - tmp *= rescale->numerator; > >>> - do_div(tmp, 1000000000LL); > >>> - *val = tmp; > >>> - return scale_type; > >>> + *val = rescale->numerator * *val; > >>> + *val2 = rescale->denominator * (1 << *val2); > >>> + return IIO_VAL_FRACTIONAL; > >> > >> Hi! > > > > Hi Peter, > > > >> > >> I do not think this is an uncontested improvement. You have broken the > >> case > >> where *val2 is "large" before the scale factor is applied. > > > > I was a little reluctant to add this change as I keep increasing the > > scope of this series, but since I added tests for all cases, I didn't > > want to leave this one out. > > > Would you rather I drop this patch and the test cases associated to it? > > Why drop the tests? Are they doing any harm? Or are they testing exactly > the problem situation that fail without this patch? They are testing this problem and fail without the patch. > > In that case, I guess fix the tests to pass and preferably add tests > for the *val2 is "large" situation (that this patch breaks) so that the > next person trying to improve precision is made aware of the overflow > problem. Does that make sense? To handle large values of *val2, I could use the same logic as in IIO_VAL_FRACTIONAL with check_mul_overflow() and gcd(). would that be okay? Thanks, Liam > > Cheers, > Peter > > > Thanks, > > Liam > > > >> > >> Cheers, > >> Peter > >> > >>> case IIO_VAL_INT_PLUS_NANO: > >>> tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator; > >>> tmp = div_s64(tmp, rescale->denominator); > >>> > >
