Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Peter Rosin]

On 2021-07-28 02:21, Liam Beguin wrote:
> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
>> On 2021-07-21 05:06, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xxxxxxxxxx>
>>>
>>> Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types.
>>> Add support for these to allow using the iio-rescaler with them.
>>>
>>> Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index d0669fd8eac5..2b73047365cc 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>  		do_div(tmp, 1000000000LL);
>>>  		*val = tmp;
>>>  		return scale_type;
>>> +	case IIO_VAL_INT_PLUS_NANO:
>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>> +		tmp = div_s64(tmp, rescale->denominator);
>>> +
>>> +		*val = div_s64(tmp, 1000000000LL);
>>> +		*val2 = tmp - *val * 1000000000LL;
>>> +		return scale_type;
> 
> Hi Peter,
> 
>>
>> Hi!
>>
>> My objection from v5 still stands. Did you forget or did you simply send
>> the
>> wrong patch?
> 
> Apologies, again I didn't mean to make it seem like I ignored your comments.
> I tried your suggestion, but had issues when *val2 would overflow into
> the integer part.

Not saying anything about it not working does indeed make it seem like you
ignored it :-)  Or did I just miss where you said this? Anyway, no problem,
it can be a mess dealing with a string of commits when there are numerous
things to take care of between each iteration. And it's very easy to burn
out and just back away. Please don't do that!

> Even though what I has was more prone to integer overflow with the first
> multiplication, I thought it was still a valid solution as it passed the
> tests.

I did state that you'd need to add overflow handling from the fraction
calculation and handling for negative values, so it was no surprise that
my original sketchy suggestion didn't work as-is.

> 
>>
>> Untested suggestion, this time handling negative values and
>> canonicalizing any
>> overflow from the fraction calculation.
>>
>> neg = *val < 0 || *val2 < 0;
>> tmp = (s64)abs(*val) * rescale->numerator;
>> rem = do_div(tmp, rescale->denominator);
>> *val = tmp;
>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
>> do_div(tmp, rescale->denominator);
>> *val2 = do_div(tmp, 1000000000LL);
>> *val += tmp;
>> if (neg) {
>> if (*val < 0)
>> *val = -*val;
>> else
>> *val2 = -*val;

This last line should of course be *val2 = -*val2;
Sorry.

> 
> I'll look into this suggestion.

Thanks!

> 
>> }
>>
>>> +	case IIO_VAL_INT_PLUS_MICRO:
>>> +		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
>>> +		tmp = div_s64(tmp, rescale->denominator);
>>> +
>>> +		*val = div_s64(tmp, 1000000);
>>
>> Why do you not have the LL suffix here?
> 
> Doesnt' LL make it into a 64 bit integer?
> I left it out because the second parameter of div_s64() should be s32.

It just looked really odd with 1000000000LL for all instances, but then
1000000LL only for some. The lack of symmetry bothered me.

To me, it seems as if we either need to support old/small crap with
int being 16-bit, or we don't. If we don't need support for 16-bit,
then we don't need any LL suffix, since 1000000000 fits just fine in
32-bit. If we do need 16-bit support, then we need LL (or something)
all over since neither 1000000 nor 1000000000 fit in 16-bit.

I think the compiler looks at the value of the constant and not the
size of its type when selecting how big values the mul/add/whatever
needs handle. So, adding LL feels like the safe option. Further, I
guesstimate that the runtime cost of adding LL is zero and that the
compile time cost is negligible.

But maybe I'm missing something?

Cheers,
Peter

> 
> Thanks,
> Liam
> 
>>
>> Cheers,
>> Peter
>>
>>> +		*val2 = tmp - *val * 1000000;
>>> +		return scale_type;
>>>  	default:
>>>  		return -EOPNOTSUPP;
>>>  	}
>>>
>