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

["Nuno Sá" <noname.nuno@gmail.com>]

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.

- Nuno Sá
>