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

[Trevor Gamblin]

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@xxxxxxxxxxxx> wrote:
> > >
> > > > On 2024-12-19 11:13, Jonathan Cameron wrote:
> > > > > On Tue, 17 Dec 2024 16:47:28 -0500
> > > > > Trevor Gamblin <tgamblin@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ...
>
> > > 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()?