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

[Trevor Gamblin <tgamblin@baylibre.com>]

On 2025-01-07 16:02, David Lechner wrote:
> 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@baylibre.com> wrote:
>>>
>>>> On 2024-12-19 11:13, Jonathan Cameron wrote:
>>>>> On Tue, 17 Dec 2024 16:47:28 -0500
>>>>> Trevor Gamblin <tgamblin@baylibre.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@baylibre.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. ;-)
My bad.
>
>
>> 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.
Alright, will fix this.
>
>>          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).
And this.
>
> 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.
Should I handle that with an extra call to clamp_t()?