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

[Trevor Gamblin <tgamblin@baylibre.com>]

On 2025-01-13 09:35, Nuno Sá wrote:
> On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin 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>
>> ---
> LGTM, just one small thing inline... Either way:
>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
>>   drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
>> -
>>   1 file changed, 303 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>> index c8cd73d19e86..0caaeaa310ed 100644
>> --- a/drivers/iio/adc/ad4695.c
>> +++ b/drivers/iio/adc/ad4695.c
>> @@ -79,6 +79,7 @@
>>   #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
>>   #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
>>   #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
>> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
>>   #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
>>   #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
>>   #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
>> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
>>   	bool bipolar;
>>   	enum ad4695_in_pair pin_pairing;
>>   	unsigned int common_mode_mv;
>> +	unsigned int oversampling_ratio;
>>   };
>>   
> ...
>
>> +
>> +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;
>> +	}
>> +
>> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
>> +
> Why not clamp()? AFAICS, we have the same type on all the arguments. I also
> think clamp*() macros got the same improvements as min/max() ones which means
> that using the ones with explicit casts are not so often needed anymore. My
> understanding is also that those macros are not that encouraged as it's easy to
> go wrong with the casts.
I have no preference, this is just a recent habitual use of clamp_t(). If
clamp() is preferred I can send a v3. Or maybe Jonathan can tweak it 
when it is

eventually applied?

- Trevor

>
> - Nuno Sá