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>
> > 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.
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.
+ 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;
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;
> >> + }
> >> +}
> >> +
