| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9128ecae-73e9-4a66-8cd0-4d98c14ff05f@baylibre.com>
Date: Tue, 7 Jan 2025 15:21:34 -0500
From: Trevor Gamblin <tgamblin@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
David Lechner <dlechner@...libre.com>, Lars-Peter Clausen <lars@...afoo.de>,
Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad4695: add offload-based oversampling
support
On 2025-01-04 07:30, Jonathan Cameron wrote:
> On Thu, 2 Jan 2025 13:19:19 -0500
> Trevor Gamblin <tgamblin@...libre.com> wrote:
>
>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>> On Tue, 17 Dec 2024 16:47:28 -0500
>>> Trevor Gamblin <tgamblin@...libre.com> wrote:
>>>
>>>> Add support for the ad4695's oversampling feature when SPI offload is
>>>> available. This allows the ad4695 to set oversampling ratios on a
>>>> per-channel basis, raising the effective-number-of-bits from 16
>>>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
>>>> full cycle through the auto-sequencer). The logic for reading and
>>>> writing sampling frequency for a given channel is also adjusted based on
>>>> the current oversampling ratio.
>>>>
>>>> The non-offload case isn't supported as there isn't a good way to
>>>> trigger the CNV pin in this mode. Support could be added in the future
>>>> if a use-case arises.
>>>>
>>>> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
>>> Hi Trevor,
>>>
>>> The clamping fun of get_calibbias seems overkill. If this isn't going to ever
>>> overflow an s64 maybe just use the high precision to do it the easy way.
>>> I'm not sure you can't just fit it in an s32 for that matter. I've just
>>> not done the maths to check.
>>>
>>> Jonathan
>>>
>>>
>>>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
>>>> +{
>>>> + unsigned int reg_val;
>>>> +
>>>> + switch (osr) {
>>>> + case 4:
>>>> + if (val2 >= 0 && val > S16_MAX / 2)
>>>> + reg_val = S16_MAX;
>>>> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
>>> It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise
>>> we carry the sign in the val part. Sometimes we generalize that to
>>> make life easier for driver writers but I think you can use that here
>>> to simplify things.
>>>
>>> (for background look at __iio_str_to_fixpoint() - it's a bit of a hack
>>> to deal with integers have no negative 0)
>>>
>>> if (val > S16_MAX / 2)
>>> ...
>>> else if (val < S16_MIN / 2)
>>> ...
>>> else if (val2 < 0) etc
>>>
>>> You may feel it is better to keep the code considering the val2 < 0 when
>>> val != 0 case and I don't mind that as it's not wrong, just overly complex!
>>>
>>> If you can easily clamp the overall range you can just do some maths
>>> with enough precision to get one number (probably a s64) and clamp that.
>>> Easy to sanity check for overflow based on val to ensure no overflows.
>> Hi Jonathan,
>>
>> I'm reviewing this again but I'm not entirely clear what you mean.
>>
>> Are you suggesting that the entire switch block could be simplified
>> (i.e. eliminating the previous simplification for the val2 < 0 case in
>> the process), or that the calls to clamp_t can be combined?
>>
>> I've tested out simplifying the val2 < 0 case locally and driver
>> functionality still seems OK. Maybe I'm missing a third option.
Hi Jonathan,
> The extra info we can use is that val2 is always positive
> if val != 0 and it never takes a value beyond +- MICRO because
> otherwise val would be non 0 instead.
>
>
> Taking original code and ruling out cases.
> + case 4:
> + if (val2 >= 0 && val > S16_MAX / 2)
> // If val is non 0 then val2 is postive, so
> // if (val > S16_MAX / 2)
> // reg_val = S16_MAX;
>
> + reg_val = S16_MAX;
> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2)
>
> // If val2 < 0 then val == 0 which is never less than S16_MIN / 2
> // So this condition never happens.
Thanks for catching these.
>
> + reg_val = S16_MIN;
> + else if (val2 < 0)
> // likewise, this is actually clamping val2 * 2 / MICRO which
> // is never going to be anywhere near S16_MIN or S16_MAX as I think
> // it is always between +1 and -1 as val2 itself is limited to -MICRO to MICRO
>
> + reg_val = clamp_t(int,
> + -(val * 2 + -val2 * 2 / MICRO),
> + S16_MIN, S16_MAX);
> + else if (val < 0)
> //This one is fine.
> + reg_val = clamp_t(int,
> + val * 2 - val2 * 2 / MICRO,
> + S16_MIN, S16_MAX);
> + else
> //As is this one
> + reg_val = clamp_t(int,
> + val * 2 + val2 * 2 / MICRO,
> + S16_MIN, S16_MAX);
> + return reg_val;
>
> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer.
> int val_calc;
> if (val > 0)
> val_calc = val * 2 + val2 * 2 / MICRO;
> else if (val < 0)
> val_calc = -(val * 2 - val2 * 2 / MICRO);
> else /* Only now does val2 sign matter as val == 0 */
> val_calc = val2 * 2 / MICRO;
I've been testing out these simplifications (but using the scaling
suggestion from below, which is great - for some reason I had it in my
head that doing so wasn't an option).
These seem to have some issues with signs for particularly small
calibbias values. I think it's because while my (val2 < 0) case was
doing unnecessary clamping, the math itself was OK.
I did some more experimenting, and came up with a new version of the
function that looks like this:
static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
{
int val_calc, scale;
switch (osr) {
case 4:
scale = 4;
break;
case 16:
scale = 2;
break;
case 64:
scale = 1;
break;
default:
scale = 8;
break;
}
/* Note that val2 > 0 if val != 0 and val2 range +- MICRO */
if (val < 0)
val_calc = val * scale - val2 * scale / MICRO;
else if (val2 < 0)
/* if val2 < 0 then val == 0 */
val_calc = -(-val2 * scale / MICRO);
else
val_calc = val * scale + val2 * scale / MICRO;
val_calc /= 2;
return clamp_t(int, val_calc, S16_MIN, S16_MAX);
}
This seems to match all of the expected outputs for the
pre-simplification version in this patch series when I test it. If there
are no issues with it, I'll send a v2.
>
> Which can simplify because we know val is 0 for last case.
> Whether this is worth doing depends on trade off between
> docs needed to explain the code and shorter code.
>
> /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */
> if (val < 0)
> val_calc = val * 2 - val2 * 2 / MICRO;
> else
> val_calc = val * 2 + val2 * 2 / MICRO;
>
> reg_val = clamp_t(int, val_calc, S16_MIN, S16_MAX);
>
> One trivial additional simplication below.
>
> You might also be able to scale temporary up by 2 and ust
> have the switch statement set a scaling value.
>
> In this case scale == 4 in other cases below, 2, 1, and 8 for the default
>
>
> if (val < 0)
> val_calc = val * scale - val2 * scale / MICRO;
> else
> val_calc = val * scale + val2 * scale / MICRO;
>
> val_calc /= 2; /* to remove the factor of 2 */
>
> reg_val = clamp_t (int, val_calc, S16_MIN, S16_MAX);
> after the switch statement with comments when setting scale on the * 2
> multiplier to avoid the / 2 for case 64.
>
>> - Trevor
>>
>>>
>>>
>>>
>>>> + reg_val = S16_MIN;
>>>> + else if (val2 < 0)
>>>> + reg_val = clamp_t(int,
>>>> + -(val * 2 + -val2 * 2 / MICRO),
>>>> + S16_MIN, S16_MAX);
>>>> + else if (val < 0)
>>>> + reg_val = clamp_t(int,
>>>> + val * 2 - val2 * 2 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + else
>>>> + reg_val = clamp_t(int,
>>>> + val * 2 + val2 * 2 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + return reg_val;
>>>> + case 16:
>>>> + if (val2 >= 0 && val > S16_MAX)
>>>> + reg_val = S16_MAX;
>>>> + else if ((val2 < 0 ? -val : val) < S16_MIN)
>>>> + reg_val = S16_MIN;
>>>> + else if (val2 < 0)
>>>> + reg_val = clamp_t(int,
>>>> + -(val + -val2 / MICRO),
>>>> + S16_MIN, S16_MAX);
>>>> + else if (val < 0)
>>>> + reg_val = clamp_t(int,
>>>> + val - val2 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + else
>>>> + reg_val = clamp_t(int,
>>>> + val + val2 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + return reg_val;
>>>> + case 64:
>>>> + if (val2 >= 0 && val > S16_MAX * 2)
>>>> + reg_val = S16_MAX;
>>>> + else if ((val2 < 0 ? -val : val) < S16_MIN * 2)
>>>> + reg_val = S16_MIN;
>>>> + else if (val2 < 0)
>>>> + reg_val = clamp_t(int,
>>>> + -(val / 2 + -val2 / 2 / MICRO),
>>>> + S16_MIN, S16_MAX);
>>>> + else if (val < 0)
>>>> + reg_val = clamp_t(int,
>>>> + val / 2 - val2 / 2 / MICRO,
> For these val2 / 2 / MICRO always 0 so value of val2 never matters.
>
>>>> + S16_MIN, S16_MAX);
>>>> + else
>>>> + reg_val = clamp_t(int,
>>>> + val / 2 + val2 / 2 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + return reg_val;
>>>> + default:
>>>> + if (val2 >= 0 && val > S16_MAX / 4)
>>>> + reg_val = S16_MAX;
>>>> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
>>>> + reg_val = S16_MIN;
>>>> + else if (val2 < 0)
>>>> + reg_val = clamp_t(int,
>>>> + -(val * 4 + -val2 * 4 / MICRO),
>>>> + S16_MIN, S16_MAX);
>>>> + else if (val < 0)
>>>> + reg_val = clamp_t(int,
>>>> + val * 4 - val2 * 4 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + else
>>>> + reg_val = clamp_t(int,
>>>> + val * 4 + val2 * 4 / MICRO,
>>>> + S16_MIN, S16_MAX);
>>>> + return reg_val;
>>>> + }
>>>> +}
>>>> +
Powered by blists - more mailing lists