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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1c641b37-475a-4153-bcfc-e0e72d79fa76@baylibre.com>
Date: Thu, 2 Jan 2025 13:19:19 -0500
From: Trevor Gamblin <tgamblin@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
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 2024-12-19 11:13, Jonathan Cameron wrote:
> 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.

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.

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