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

[Trevor Gamblin <tgamblin@baylibre.com>]

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