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

[Jonathan Cameron <jic23@kernel.org>]

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