Message-ID: <ZUDN9n8iXoNwzifQ@dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi>
Date:   Tue, 31 Oct 2023 11:50:46 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Subhajit Ghosh <subhajit.ghosh@...aklogic.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH] iio: gts-helpers: Round gains and scales

The GTS helpers do flooring of scale when calculating available scales.
This results available-scales to be reported smaller than they should
when the division in scale computation resulted remainder greater than
half of the divider. (decimal part of result > 0.5)

Furthermore, when gains are computed based on scale, the gain resulting
from the scale computation is also floored. As a consequence the
floored scales reported by available scales may not match the gains that
can be set.

The related discussion can be found from:
https://lore.kernel.org/all/84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com/

Do rounding when computing scales and gains.

Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>

---
Subjahit, is there any chance you test this patch with your driver? Can
you drop the:
	if (val2 % 10)
		val2 += 1;
from scale setting and do you see written and read scales matching?

I did run a few Kunit tests on this change - but I'm still a bit jumpy
on it... Reviewing/testing is highly appreciated!

Just in case someone is interested in seeing the Kunit tests, they're
somewhat unpolished & crude and can emit noisy debug prints - but can
anyways be found from:
https://github.com/M-Vaittinen/linux/commits/iio-gts-helpers-test-v6.6

---
 drivers/iio/industrialio-gts-helper.c | 58 +++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 7653261d2dc2..7dc144ac10c8 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -18,6 +18,32 @@
 #include <linux/iio/iio-gts-helper.h>
 #include <linux/iio/types.h>
 
+static int iio_gts_get_gain_32(u64 full, unsigned int scale)
+{
+	unsigned int full32 = (unsigned int) full;
+	unsigned int rem;
+	int result;
+
+	if (full == (u64)full32) {
+		unsigned int rem;
+
+		result = full32 / scale;
+		rem = full32 - scale * result;
+		if (rem >= scale / 2)
+			result++;
+
+		return result;
+	}
+
+	rem = do_div(full, scale);
+	if ((u64)rem >= scale / 2)
+		result = full + 1;
+	else
+		result = full;
+
+	return result;
+}
+
 /**
  * iio_gts_get_gain - Convert scale to total gain
  *
@@ -28,30 +54,42 @@
  *		scale is 64 100 000 000.
  * @scale:	Linearized scale to compute the gain for.
  *
- * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
+ * Return:	(rounded) gain corresponding to the scale. -EINVAL if scale
  *		is invalid.
  */
 static int iio_gts_get_gain(const u64 max, const u64 scale)
 {
-	u64 full = max;
+	u64 full = max, half_div;
+	unsigned int scale32 = (unsigned int) scale;
 	int tmp = 1;
 
-	if (scale > full || !scale)
+	if (scale / 2 > full || !scale)
 		return -EINVAL;
 
+	/*
+	 * The loop-based implementation below will potentially run _long_
+	 * if we have a small scale and large 'max' - which may be needed when
+	 * GTS is used for channels returning specific units. Luckily we can
+	 * avoid the loop when scale is small and fits in 32 bits.
+	 */
+	if ((u64)scale32 == scale)
+		return iio_gts_get_gain_32(full, scale32);
+
 	if (U64_MAX - full < scale) {
 		/* Risk of overflow */
-		if (full - scale < scale)
+		if (full - scale / 2 < scale)
 			return 1;
 
 		full -= scale;
 		tmp++;
 	}
 
-	while (full > scale * (u64)tmp)
+	half_div = scale >> 2;
+
+	while (full + half_div >= scale * (u64)tmp)
 		tmp++;
 
-	return tmp;
+	return tmp - 1;
 }
 
 /**
@@ -133,6 +171,7 @@ static int iio_gts_linearize(int scale_whole, int scale_nano,
  * Convert the total gain value to scale. NOTE: This does not separate gain
  * generated by HW-gain or integration time. It is up to caller to decide what
  * part of the total gain is due to integration time and what due to HW-gain.
+ * Computed gain is rounded to nearest integer.
  *
  * Return: 0 on success. Negative errno on failure.
  */
@@ -140,10 +179,13 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
 				int *scale_int, int *scale_nano)
 {
 	u64 tmp;
+	int rem;
 
 	tmp = gts->max_scale;
 
-	do_div(tmp, total_gain);
+	rem = do_div(tmp, total_gain);
+	if (total_gain > 1 && rem >= total_gain / 2)
+		tmp += 1ULL;
 
 	return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
 }
@@ -192,7 +234,7 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
 		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
 		     NULL);
 
-		/* Convert gains to scales */
+		/* Convert gains to scales. */
 		for (j = 0; j < gts->num_hwgain; j++) {
 			ret = iio_gts_total_gain_to_scale(gts, gains[i][j],
 							  &scales[i][2 * j],

base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists