Messages in this thread |  | | Date | Mon, 22 Jan 2024 20:28:22 +1030 | | Subject | Re: [PATCH] iio: gts-helper: Fix division loop | | From | Subhajit Ghosh <> |
| |
On 22/1/24 17:20, Matti Vaittinen wrote: > On 1/19/24 13:56, Subhajit Ghosh wrote: >> On 8/1/24 02:52, Jonathan Cameron wrote: >>> On Thu, 4 Jan 2024 11:34:28 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> The loop based 64bit division may run for a long time when dividend is a >>>> lot bigger than the divider. Replace the division loop by the >>>> div64_u64() which implementation may be significantly faster. >>>> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>> >>> Hmm. Fix or not perf improvement? I'm going to take the middle ground >>> and leave the fixes tag, but not rush this in. >>> >>> So applied to the togreg branch of iio.git and for now just pushed out >>> as testing for 0-day etc to take a look before I rebase that tree after >>> rc1. >>> >>> >>> >>>> --- >>>> >>>> I've implemented also a fixup series for supporting rounding of >>>> gains/scales: >>>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/ >>>> >>>> That series does also remove the offending loop. >>>> >>>> We don't currently have any in-tree users of GTS helpers which would >>>> need the rounding support so pushing the rounding is not urgent (and I >>>> haven't heard of Subjahit whose driver required the rounding). Hence, we >>>> may want to only take this loop fix in for now (?) and reconsider >>>> rounding when someone need that. >>>> >>>> Jonathan, what's your take on this? >>> Agreed - let us wait for the rounding to have a user, but makes sense >>> to tidy this corner up in the meantime. >>> >>> Thanks, >>> >>> Jonathan >>> >>>> >>>> drivers/iio/industrialio-gts-helper.c | 5 ++--- >>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c >>>> index 7653261d2dc2..abcab2d38589 100644 >>>> --- a/drivers/iio/industrialio-gts-helper.c >>>> +++ b/drivers/iio/industrialio-gts-helper.c >>>> @@ -34,7 +34,7 @@ >>>> static int iio_gts_get_gain(const u64 max, const u64 scale) >>>> { >>>> u64 full = max; >>>> - int tmp = 1; >>>> + int tmp = 0; >>>> if (scale > full || !scale) >>>> return -EINVAL; >>>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale) >>>> tmp++; >>>> } >>>> - while (full > scale * (u64)tmp) >>>> - tmp++; >>>> + tmp += div64_u64(full, scale); >>>> return tmp; >>>> } >>>> >>>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab >>> >>> >> Hi Matti, >> >> Your fix works beautifully with the latest version of apds9306 driver which I am working on. >> All available scale values can be set without any errors. Thank you. > > Thanks for testing Subhajit! Just to ensure we have no miscommunication - did you test just this division fix, or the rounding fix here: > https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/ You are most welcome. I did not check the above rounding fix pointed out by the link. I will be happy to check it and let you know by the end of this month. I checked this division fix.
> >> Moving to a new city with a new full time job with the assumption of getting more time >> for my list of opensource projects and contributions proved to be utterly wrong! > > Well, I can't blame you :) Being in a new work at new city sounds like you have a lot on your plate right now. Give it half a year and things will stabilize though :) Oh, and falsely assuming that "when XXX, I will have the time to do YYY" - been there done that :) > > Good luck on the new work and city! Thank you Matti. > > Yours, > -- Matti > Regards, Subhajit Ghosh
|  |