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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241219161301.3f708302@jic23-huawei>
Date: Thu, 19 Dec 2024 16:13:01 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Trevor Gamblin <tgamblin@...libre.com>
Cc: Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, Lars-Peter
 Clausen <lars@...afoo.de>, Jonathan Corbet <corbet@....net>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad4695: add offload-based oversampling
 support

On Tue, 17 Dec 2024 16:47:28 -0500
Trevor Gamblin <tgamblin@...libre.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@...libre.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.

		


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