On 2021-07-30 08:49, Peter Rosin wrote:

On 2021-07-29 17:56, Liam Beguin wrote:

quoted

On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:

quoted

On 2021-07-28 02:21, Liam Beguin wrote:

quoted

On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:

quoted

On 2021-07-21 05:06, Liam Beguin wrote:

quoted

From: Liam Beguin <redacted>

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 <redacted>
---
 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,

quoted

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.

Hi Peter,

quoted

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!

It was my mistake. Thanks for the encouragement :-)

quoted

quoted

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.

quoted

quoted

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.

quoted

I'll look into this suggestion.

Thanks!

Starting from what you suggested, here's what I came up with.
I also added a few test cases to cover corner cases.

	if (scale_type == IIO_VAL_INT_PLUS_NANO)
		mult = 1000000000LL;
	else
		mult = 1000000LL;
	/*
	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
	 * *val2 is negative the schan scale is negative
	 */
	neg = *val < 0 || *val2 < 0;

	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);

Small nit, but I think abs() returns a signed type compatible
with the argument type. I.e. (s32)abs(rescale->...) where both
numerator and denominator are already s32 could just as well
be written without the cast as plain old abs(rescale->...)

quoted

	*val = div_s64_rem(tmp, (s32)abs(rescale->denominator), &rem);

	tmp = (s64)rem * mult +
		(s64)abs(*val2) * (s32)abs(rescale->numerator);
	tmp = div_s64(tmp, (s32)abs(rescale->denominator));

	*val += div_s64_rem(tmp, mult, val2);

	/*
	 * If the schan scale or only one of the rescaler elements is
	 * negative, the combined scale is negative.
	 */
	if (neg || ((rescale->numerator < 0) ^ (rescale->denominator < 0)))

Hang on, that's not right. If the value and only one of the rescaler
elements is negative, the result is positive. || is not the correct
logical operation.

quoted

		*val = -*val;

Unconditionally negating *val doesn't negate the combined value when
*val is zero and *val2 isn't. My test "if (*val < 0)" above attempting
to take care of this case is clearly not right. It should of course be
"if (*val > 0)" since *val is not yet negated. Duh!

In fact, I think a few tests scaling to/from the [-1,1] interval
would be benefitial for this exact reason.

So, with both these issues taken care of:

 	if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
		if (*val > 0)
			*val = -*val;
		else
			*val2 = -*val2;
	}

(bitwise ^ is safe since all operands come from logical operations, i.e.
they are either zero or one and nothing else)

Cheers,
Peter