Message ID | ZUDN9n8iXoNwzifQ@dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b90f:0:b0:403:3b70:6f57 with SMTP id t15csp120000vqg; Tue, 31 Oct 2023 02:52:55 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH/bgXVCqlHn0inP2s8YyJbe227LiroOwQ8y1M9iKz4xiOD7fmkCrieVg3ifghUk5B5P+kI X-Received: by 2002:a17:90b:814:b0:27e:3342:5c1f with SMTP id bk20-20020a17090b081400b0027e33425c1fmr9117217pjb.43.1698745975493; Tue, 31 Oct 2023 02:52:55 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698745975; cv=none; d=google.com; s=arc-20160816; b=fArd4uOWcpjT5oEjh7VtrcgXyLFr/Y7stOwoxR4tksdciwSEhy3CiFBWvOUu5nSLMD 7k4ubenzhrRpaoD11P2RPr2aZSelyxDLTgGKZhL7xTZpJo2wGYe2uH2gRCAZ6PGWhIkw vhC5FftqEPOJpf0AUw4a80oaoCj3Ux4R0HcCtDpZKnxsrHxpCFNTb7Uz+YmY4nD4dA0R 4h3L1IgW1UpCWdkUN5/k92qa1CRJrxkXmCrIE/RicuVv9nlOcGrCwDLR8dC+fFe4L9CE SGuEB8vLW7Rl/WmmCFzculPxku7zqkIUyhhLgh/ljkMyRnoQngxmkW5594uusg8Nn/9y JL+A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=pm0E+qHfJ96Z5WGc1StxiY8bt/dlGJYxdkUl7eJLbGA=; fh=r1UR4v2XzeTofpxhZMlbg/c2zFMlnw6DXBP6AA4bgy0=; b=vyCCfBmAGOTwiPbIPNglPdSnaVPqt4Itk1MJ/aFFzVAAyNx8KvEeJRVX/3VaCSCdqX wkBD13duzBSIzvq/vUoJBKBTapGPbsknr912raBz4tidKMNTkqeD2JRwlcMCk789xNYn U1fa6IoxiavEPRaNNb3wH4GXaVIWXqtUntnT+0V4HXyCOlgCeqMV4jSWtYQe9wfqZD0i IyV7onCGfb9yL/rQD0E9TrrHhBqdlGJFJtEdsofp9bz/W/0A4D4QANu8dhOKwxhbpcyF TKMyMkQ+/4aYSk7Zh6yXz4lSDrn2Tx5+M95lO74XkWBLakNIJUJ82hacAe3bzdYEVcTV YbNg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=WeLLHKRZ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id a1-20020a17090aa50100b002800ab6f85asi738425pjq.119.2023.10.31.02.52.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Oct 2023 02:52:55 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=WeLLHKRZ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id D5527802B540; Tue, 31 Oct 2023 02:52:44 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236159AbjJaJwZ (ORCPT <rfc822;gah0developer@gmail.com> + 34 others); Tue, 31 Oct 2023 05:52:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43012 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229658AbjJaJwD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 31 Oct 2023 05:52:03 -0400 Received: from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com [IPv6:2a00:1450:4864:20::22a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E647710F2; Tue, 31 Oct 2023 02:50:58 -0700 (PDT) Received: by mail-lj1-x22a.google.com with SMTP id 38308e7fff4ca-2c5071165d5so29954241fa.0; Tue, 31 Oct 2023 02:50:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698745856; x=1699350656; darn=vger.kernel.org; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :from:to:cc:subject:date:message-id:reply-to; bh=pm0E+qHfJ96Z5WGc1StxiY8bt/dlGJYxdkUl7eJLbGA=; b=WeLLHKRZqlFsRTWn8NSO7ls0efZ1CGhoeDRxZHhDJVkeyyQmhD/+3zl8RwJYEbIuwm g3m2L89otpss7EUQ0halGAQmu6Uj8F6TRrdMBResVshDG1RP1Awu7A1XoEin0idy9cmc Z2ac6Rn2OSJsZo5i/WQU4Ms41Rskwo0WmzaUkCI7O2BxL8bmdhIJHyHqarPrm89MYbfi FJh+cTIVuiPAkfCe/6LfUgQHkqmRIK3/uMcTeY5nmbfWoJuOYBO7bC7wD/8i+Ib0VVa9 FvXIoeBjjVcA4aShsrI7GZuY184XrWtkxW2D65dfmRlmcKNq9BTrkDtvU7p6kUKbMEWB /bwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698745856; x=1699350656; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pm0E+qHfJ96Z5WGc1StxiY8bt/dlGJYxdkUl7eJLbGA=; b=ZwB/TG9O+cTbcq4J7TJKWQRM21l/L6qXERXkC6jd5ZoiMAzEUsAMSPA1Jeyt2d90fo 3peSccMg5UkiovCPef5kg7sQnzqiOcXi52JwnfsTYICVmKVgwF1GY7vevdPZZuo7ExnY 4e72jtvXiNkZcSZFKMxNkix5M6yXGW9WAilYU2elP83Y4ph3Q+zFJJEzFqavckVBaETX zKFbUOobZgNcZ5Qlum/IZd8T+PPIs3jEu8IpdftZjtl9+nS270ivzVy6buuR5V5zeCH3 RiovQU7o74ebSw0zS3ouo7Y1rA1ISbVm03KDtfSMQcXg0TCyJ+tB5B68tHkRVXAVt4Wd O3dw== X-Gm-Message-State: AOJu0YyVtbHz5LuPx6uLaXM/jpXIiFEB+MUM63+7+Vm2gh0TMr8pSf8W TTIASQK/pw9HimN6PtkyXnc= X-Received: by 2002:a2e:bb86:0:b0:2b6:ea3b:f082 with SMTP id y6-20020a2ebb86000000b002b6ea3bf082mr7634854lje.38.1698745856331; Tue, 31 Oct 2023 02:50:56 -0700 (PDT) Received: from dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi (dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi. [2001:14ba:16f8:1500::1]) by smtp.gmail.com with ESMTPSA id s25-20020a2e2c19000000b002b70a8478ddsm143509ljs.44.2023.10.31.02.50.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Oct 2023 02:50:55 -0700 (PDT) Date: Tue, 31 Oct 2023 11:50:46 +0200 From: Matti Vaittinen <mazziesaccount@gmail.com> To: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>, Matti Vaittinen <mazziesaccount@gmail.com> Cc: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] iio: gts-helpers: Round gains and scales Message-ID: <ZUDN9n8iXoNwzifQ@dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ZJOGLzL4y7uQJBvg" Content-Disposition: inline X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Tue, 31 Oct 2023 02:52:44 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781264259920289057 X-GMAIL-MSGID: 1781264259920289057 |
Series |
iio: gts-helpers: Round gains and scales
|
|
Commit Message
Matti Vaittinen
Oct. 31, 2023, 9:50 a.m. UTC
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@gmail.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(-)
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
Comments
On Tue, 31 Oct 2023 11:50:46 +0200 Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ > > Do rounding when computing scales and gains. > > Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Hi Matti, A few questions inline about the maths. > > --- > 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? > + 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? > 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; Why divide by 4? Looks like classic issue with using shifts for division causing confusion. > + > + 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? > + 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. */ Grumble - unrelated change. > 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
On 11/26/23 19:26, Jonathan Cameron wrote: > On Tue, 31 Oct 2023 11:50:46 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ >> >> Do rounding when computing scales and gains. >> >> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Hi Matti, > > A few questions inline about the maths. I appreciate the questions :) Thanks! > >> >> --- >> 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? > >> + 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. > >> 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; > > Why divide by 4? Looks like classic issue with using shifts for division > causing confusion. Yes. Looks like a brainfart to me. I need to fire-up my tests and revise this (and the check you asked about above). It seems to take a while from me to wrap my head around this again... Thanks for pointing this out! > >> + >> + 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. > >> + 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. */ > > Grumble - unrelated change. Yes. I'll drop this. > >> 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 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 :) Yours, -- Matti
On 11/27/23 09:48, Matti Vaittinen wrote: > On 11/26/23 19:26, Jonathan Cameron wrote: >> On Tue, 31 Oct 2023 11:50:46 +0200 >> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ >>> >>> Do rounding when computing scales and gains. >>> >>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> >> Hi Matti, >> >> A few questions inline about the maths. > > I appreciate the questions :) Thanks! >> >>> >>> --- >>> 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? > >> >>> + 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. > >> >>> 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; >> >> Why divide by 4? Looks like classic issue with using shifts for division >> causing confusion. > > Yes. Looks like a brainfart to me. I need to fire-up my tests and revise > this (and the check you asked about above). It seems to take a while > from me to wrap my head around this again... > > Thanks for pointing this out! > >> >>> + >>> + while (full + half_div >= scale * (u64)tmp) >>> tmp++; Oh. This is a problem. Adding half_div to full here can cause the scale * (u64)tmp to overflow. The overflow-prevention above only ensures full is smaller than the U64_MAX - scale. Here we should ensure full + half_div is less than U64_MAX - scale to ensure the loop always stops. All in all, this is horrible. Just ran a quick and dirty test on my laptop, and using 0xFFFF FFFF FFFF FFFF as full and 0x1 0000 0000 as scale (without the half_div addition) ran this loop for several seconds. Sigh. My brains jammed. I know this can not be an unique problem. I am sure there exists a better solution somewhere - any pointers would be appreciated :) >>> - return tmp; >>> + return tmp - 1; >>> } >>> /** Yours, -- Matti
On 11/28/23 13:56, Matti Vaittinen wrote: > On 11/27/23 09:48, Matti Vaittinen wrote: >> On 11/26/23 19:26, Jonathan Cameron wrote: >>> On Tue, 31 Oct 2023 11:50:46 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ >>>> >>>> Do rounding when computing scales and gains. >>>> >>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> ... >>>> + 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; >>> >>> Why divide by 4? Looks like classic issue with using shifts for >>> division >>> causing confusion. >> >> Yes. Looks like a brainfart to me. I need to fire-up my tests and >> revise this (and the check you asked about above). It seems to take a >> while from me to wrap my head around this again... >> >> Thanks for pointing this out! >> >>> >>>> + >>>> + while (full + half_div >= scale * (u64)tmp) >>>> tmp++; > > Oh. This is a problem. Adding half_div to full here can cause the scale > * (u64)tmp to overflow. The overflow-prevention above only ensures full > is smaller than the U64_MAX - scale. Here we should ensure full + > half_div is less than U64_MAX - scale to ensure the loop always stops. > > All in all, this is horrible. Just ran a quick and dirty test on my > laptop, and using 0xFFFF FFFF FFFF FFFF as full and 0x1 0000 0000 as > scale (without the half_div addition) ran this loop for several seconds. > > Sigh. My brains jammed. I know this can not be an unique problem. I am > sure there exists a better solution somewhere - any pointers would be > appreciated :) > And as a reply to myself - is there something wrong with using the div64_u64()? Sorry for the noise...
On Mon, 27 Nov 2023 09:48:08 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 11/26/23 19:26, Jonathan Cameron wrote: > > On Tue, 31 Oct 2023 11:50:46 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ > >> > >> Do rounding when computing scales and gains. > >> > >> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > > > Hi Matti, > > > > A few questions inline about the maths. > > I appreciate the questions :) Thanks! I found some emails hiding so late replies... > > > >> > >> --- > >> 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. > > > > >> + 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. > > > >> + > >> + 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. > > > > >> + 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. */ > > > > Grumble - unrelated change. > > Yes. I'll drop this. > > > > >> 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 > > 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! Jonathan > > Yours, > -- Matti > >
On 12/4/23 16:30, Jonathan Cameron wrote: > On Mon, 27 Nov 2023 09:48:08 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 11/26/23 19:26, Jonathan Cameron wrote: >>> On Tue, 31 Oct 2023 11:50:46 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com/ >>>> >>>> Do rounding when computing scales and gains. >>>> >>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> >>> 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
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],