linux-kernel - Re: [PATCH 1/2] iio: adc: ad4695: add offload-based oversampling support

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f801aecd-be1c-45f5-9ea0-081162dd74e2@baylibre.com>
Date: Tue, 7 Jan 2025 15:02:13 -0600
From: David Lechner <dlechner@...libre.com>
To: Trevor Gamblin <tgamblin@...libre.com>,
 Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.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 1/7/25 2:21 PM, Trevor Gamblin wrote:
> 
> 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>

...

>> 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.
> 

Mail is easier to read when wrapped to 80 chars. ;-)


> 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 */

This comment doesn't seem 100% accurate. val2 range is (-MICRO, MICRO) if
val == 0 or [0, MICRO) if val != 0.

>         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);

Could also write this as `val2 * scale / (int)MICRO` lest someone try to remove
the double negative and break it (because MICRO is unsigned).

This also calls into question if MICRO and similar macros should actually be
unsigned because it can lead to subtle bugs since it is perfectly reasonable
to expect -1 * MICRO to be -1000000, but it isn't.

>         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.

Probably not a big deal, but there is unhanded overflow when val is near S32_MAX
or S32_MIN.