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