On 12/4/23 16:30, Jonathan Cameron wrote:
On Mon, 27 Nov 2023 09:48:08 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
On 11/26/23 19:26, Jonathan Cameron wrote:
On Tue, 31 Oct 2023 11:50:46 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
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@xxxxxxxxx/
Do rounding when computing scales and gains.
Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Hi Matti,
A few questions inline about the maths.
I appreciate the questions :) Thanks!
I found some emails hiding so late replies...
Better late than never :)
To tell the truth, delays have been Ok. I think Subhajit has not needed
this urgently and the darkness of the winter in Finland has hindered my
energy and activity to very low levels.
---
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);
As below, can we just add scale/2 to full in the do_div?
The rationale for doing is it in this way is to prevent (theoretical?)
overflow when adding scale/2 to full. Maybe this warrants adding a comment?
Hmm. Chances are very low of hitting that. I'd just go with adding scale/2
before the div. If you really want to worry about being right at the edge
of available precision, then add a check for that.
I think the v2 will ditch this function.
+ 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)
Seems odd. Why are we checking scale / 2 here?
I am pretty sure I have been thinking of rounding 0.5 to 1.
Not sure I follow - but maybe it'll be clear in v2.
Basically, when scale is greater than max, the division yields values
smaller than 1. So, when we do rounding, everything equal to or greater
than 0.5 and smaller than 1 should be rounded upwards. Eg, purely from
computational perspective, when the "full" is half of the scale,
division returns 0.5. Thus the check.
But I think your question is very much a valid one. By design the driver
gives the max value - and I think that scale exceeding this maximum can
indeed be considered to be invalid. Not that I feel 100% certain on that
Today :)
+
+ 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);
can we do usual trick of
do_div(tmp + total_gain/2, total_gain)
to get the same rounding effect?
Only if we don't care about the case where tmp + total_gain/2 overflows.
As above. The cases where that happens are pretty narrow. I'd not worry about it
or I'd check for that overflow.
part of me says you're right while part of me screams that
1) a _division_ causing overflow is against all that is well and good.
2) if we can cope with the overflow, then we should cope with it.
I am very much undecided what is the best approach here. I'll see how
much clarity there is in the v2 code, what comments can do and then I'll
throw it for you to judge :)
All in all, I am still not 100% sure if rounding is the right ambition.
Do we cause hidden accuracy issues by doing the rounding under the hood?
I feel I need bigger brains :)
Don't we all!
Well, luckily the software development can be seen as an iterative
process :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
