diff mbox series

[v6,09/13] iio: afe: rescale: fix precision on fractional log scale

Message ID 20210721030613.3105327-10-liambeguin@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: afe: add temperature rescaling support | expand

Commit Message

Liam Beguin July 21, 2021, 3:06 a.m. UTC 
From: Liam Beguin <lvb@xiphos.com>

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@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Peter Rosin July 23, 2021, 9:20 p.m. UTC | #1 
On 2021-07-21 05:06, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> 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@xiphos.com>
> ---
>  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!

I do not think this is an uncontested improvement. You have broken the case
where *val2 is "large" before the scale factor is applied.

Cheers,
Peter

>  	case IIO_VAL_INT_PLUS_NANO:
>  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>  		tmp = div_s64(tmp, rescale->denominator);
>
Liam Beguin July 28, 2021, 12:26 a.m. UTC | #2 
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@xiphos.com>
> > 
> > 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@xiphos.com>
> > ---
> >  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?

Thanks,
Liam

>
> Cheers,
> Peter
>
> >  	case IIO_VAL_INT_PLUS_NANO:
> >  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >  		tmp = div_s64(tmp, rescale->denominator);
> >
Peter Rosin July 28, 2021, 7:58 a.m. UTC | #3 
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@xiphos.com>
>>>
>>> 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@xiphos.com>
>>> ---
>>>  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?

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?

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);
>>>
>
Liam Beguin July 29, 2021, 4:19 p.m. UTC | #4 
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@xiphos.com>
> >>>
> >>> 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@xiphos.com>
> >>> ---
> >>>  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);
> >>>
> >
diff mbox series

Patch

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;
 	case IIO_VAL_INT_PLUS_NANO:
 		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
 		tmp = div_s64(tmp, rescale->denominator);