Re: [PATCH RESEND v3 16/17] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 12 Feb 2025 15:18:59 -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.
> 
> 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>
As below. We should aim to 'pre bake' the value arrays for
get_available() to avoid the potential race conditions of a consumer
seeing a partly updated set a parameters change.

Better to see a consistent but stale one.

Jonathan

> ---
> v3 Changes:
> * removed unsed variables.
> * included sinc3+rej60 filter type.
> * oversampling_ratio moved to info_mask_shared_by_type.
> * reordered functions to avoid foward 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 | 359 ++++++++++++++++++++++++++++++-------
>  1 file changed, 290 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 8aea38c154fe..18f1ea0bf66d 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c

> +
> +/* Decimation Rate range for each filter type */
> +static const int ad7768_dec_rate_range[][3] = {
> +	[AD7768_FILTER_SINC5] = { 8, 8, 1024 },
> +	[AD7768_FILTER_SINC3] = { 32, 32, 163840 },
> +	[AD7768_FILTER_WIDEBAND] = { 32, 32, 1024 },
> +	[AD7768_FILTER_SINC3_REJ60] = { 32, 32, 163840 },
> +};
> +
> +/*
> + * The AD7768-1 supports three primary filter types:
> + * Sinc5, Sinc3, and Wideband.
> + * However, the filter register values can also encode
wrap at 80 chars.
> + * additional parameters such as decimation rates and
> + * 60Hz rejection. This utility function separates the
> + * filter type from these parameters.
> + */

>  
> -	return 0;
> +static int ad7768_get_fil_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.

Very short line wrap.  Stick to 80 chars.

> +	 */
> +	return ad7768_filter_regval_to_type[mode];
>  }

>  
> @@ -619,16 +798,25 @@ static int ad7768_read_avail(struct iio_dev *indio_dev,
>  			     long info)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> -	int i;
> +	int i, freq_filtered, len = 0;
>  
>  	switch (info) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = (int *)ad7768_dec_rate_range[st->filter_type];
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++)
> -			st->samp_freq_avail[i] = DIV_ROUND_CLOSEST(st->mclk_freq,
> -								   ad7768_clk_config[i].clk_div);
> +		freq_filtered = DIV_ROUND_CLOSEST(st->mclk_freq, st->oversampling_ratio);

Ah. So now it is dynamic.  This hits the previously mentioned race.
A consumer can be holding a copy of this and acting on it whilst holding no
locks on this device - thus it can see a mixture of values as this update
occurs. To avoid that you need to precompute the combinations +
store the lot in arrays.  Then this code should simply be selecting the arrays.
A consumer holding a stale one will get a consistent (if wrong) set.

The < 50 check makes this more complex than normal but they are still static
choices I think as long as the input clock doesn't change.

> +		for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) {
> +			st->samp_freq_avail[len] = DIV_ROUND_CLOSEST(freq_filtered,
> +								     ad7768_mclk_div_rates[i]);
> +			/* Sampling frequency cannot be lower than the minimum of 50 SPS */
> +			if (st->samp_freq_avail[len] >= 50)
> +				len++;
> +		}
>  
>  		*vals = (int *)st->samp_freq_avail;
> -		*length = ARRAY_SIZE(ad7768_clk_config);
> +		*length = len;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
> @@ -636,20 +824,45 @@ static int ad7768_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -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);
> +
> +	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;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
Previously we didn't claim this to set the sampling frequency.
That change looks like a potential ABI issue.  I'm fine with it
if we should always have this protected.

If you are just using it to avoid racing between setting sampling
frequency and oversampling ratio then don't use that, use a local
lock where the scope can be clearly described.

> +	if (ret)
> +		return ret;
> +
> +	ret = __ad7768_write_raw(indio_dev, chan, val, val2, info);
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
> +}