Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 27 Apr 2025 21:14:17 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> Separate filter type and decimation rate from the sampling frequency
> attribute. The new filter type attribute enables sinc3, sinc3+rej60
> and wideband filters, which were previously unavailable.
> 
> Previously, combining decimation and MCLK divider in the sampling
> frequency obscured performance trade-offs. Lower MCLK divider
> settings increase power usage, while lower decimation rates reduce
> precision by decreasing averaging. By creating an oversampling
> attribute, which controls the decimation, users gain finer control
> over performance.
> 
> The addition of those attributes allows a wider range of sampling
> frequencies and more access to the device features. Sampling frequency
> table is updated after every digital filter parameter change.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Co-developed-by: Pop Paul <paul.pop@analog.com>
> Signed-off-by: Pop Paul <paul.pop@analog.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
A few trivial additions from me.

> ---
> v6 Changes:
> * Made sinc3 decimation rate calculation clearer as requested.
> * Renamed some filter functions to clarify the purpose.
> * Other nits.
> 
> v5 Changes:
> * Addressed some nits.
> * Use the new new iio_device_claim/release_direct() functions.
> 
> v4 Changes:
> * Sampling frequency table is dynamically updated after every
>   filter configuration.
> 
> v3 Changes:
> * removed unused variables.
> * included sinc3+rej60 filter type.
> * oversampling_ratio moved to info_mask_shared_by_type.
> * reordered functions to avoid forward declaration.
> * simplified regmap writes.
> * Removed locking.
> * replaced some helper functions for direct regmap_update_bits
>   calls.
> * Addressed other nits.
> 
> v2 Changes:
> * Decimation_rate attribute replaced for oversampling_ratio.
> ---
>  drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++-------
>  1 file changed, 293 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 10791a85d2c5..e2b8f12260a5 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -20,6 +20,8 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/sysfs.h>
>  #include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/util_macros.h>
>  
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> @@ -77,7 +79,7 @@
>  #define AD7768_PWR_PWRMODE(x)		FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
>  
>  /* AD7768_REG_DIGITAL_FILTER */
> -#define AD7768_DIG_FIL_FIL_MSK		GENMASK(6, 4)
> +#define AD7768_DIG_FIL_FIL_MSK		GENMASK(7, 4)

Bug?  If so does this belong in a precursor patch?

>  #define AD7768_DIG_FIL_FIL(x)		FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)

> @@ -404,22 +473,110 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static int ad7768_set_dig_fil(struct ad7768_state *st,
> -			      enum ad7768_dec_rate dec_rate)
> +static int ad7768_set_sinc3_dec_rate(struct ad7768_state *st,
> +				     unsigned int dec_rate)
>  {
> -	unsigned int mode;
> +	unsigned int max_dec_rate;
> +	u8 dec_rate_reg[2];
> +	u16 regval;
>  	int ret;
>  
> -	if (dec_rate == AD7768_DEC_RATE_8 || dec_rate == AD7768_DEC_RATE_16)
> -		mode = AD7768_DIG_FIL_FIL(dec_rate);
> -	else
> -		mode = AD7768_DIG_FIL_DEC_RATE(dec_rate);
> +	/*
> +	 * Maximum dec_rate is limited by the MCLK_DIV value

Oddly short wrap. Go nearer 80 chars.

> +	 * and by the ODR. The edge case is for MCLK_DIV = 2
> +	 * ODR = 50 SPS.
> +	 * max_dec_rate <= MCLK / (2 * 50)
> +	 */

> +static int ad7768_get_filter_type_attr(struct iio_dev *dev,
> +				       const struct iio_chan_spec *chan)
> +{
> +	struct ad7768_state *st = iio_priv(dev);
> +	int ret;
> +	unsigned int mode;
> +
> +	ret = regmap_read(st->regmap, AD7768_REG_DIGITAL_FILTER, &mode);
> +	if (ret)
> +		return ret;
> +
> +	mode = FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode);
> +
> +	/* From the register value, get the corresponding filter type */
> +	return ad7768_filter_regval_to_type[mode];
	return ad7768_filter_regval_to_type[FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode)];

Is fine (only just over 80 chars and I'm getting more relaxed as time moves forwards).

Also avoids the dual meaning of mode which I never like to see.

>  }

>  
> -static int ad7768_write_raw(struct iio_dev *indio_dev,
> -			    struct iio_chan_spec const *chan,
> -			    int val, int val2, long info)
> +static int __ad7768_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long info)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad7768_set_freq(st, val);

Whilst I doubt anyone will notice this looks like a functional change
that should be called out in the patch description.
Previously we allowed frequency changes in buffered mode, now we don't.

> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad7768_configure_dig_fil(indio_dev, st->filter_type, val);
> +		if (ret)
> +			return ret;
> +
> +		/* Update sampling frequency */
> +		return ad7768_set_freq(st, st->samp_freq);
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static int ad7768_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = __ad7768_write_raw(indio_dev, chan, val, val2, info);
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}
;