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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 6 Mar 2025 18:04:24 -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 paramerter change.
> 
> 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>
> ---
> v4 Changes:
> * Sampling frequency table is dinamically updated after every

Good to spell check. Dynamically

>   filter configuration.

Currently this runs into the potential race conditions we get with
read_avail callbacks.  If we update the avail values in parallel
with consumer code in a kernel driver reading them we can get tearing.
So better if possible to do it all before those interfaces are exposed
and just pick from a set of static arrays.

> +static struct iio_chan_spec_ext_info ad7768_ext_info[] = {
> +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7768_flt_type_iio_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL, &ad7768_flt_type_iio_enum),
> +	{ },

No trailing comma on a terminating entry as we don't want it to be easy
to accidentally add stuff after this.

> +};


> +static int ad7768_configure_dig_fil(struct iio_dev *dev,
> +				    enum ad7768_filter_type filter_type,
> +				    unsigned int dec_rate)
> +{
> +	struct ad7768_state *st = iio_priv(dev);
> +	unsigned int dec_rate_idx, dig_filter_regval;
> +	int ret;
> +
> +	switch (filter_type) {
> +	case AD7768_FILTER_SINC3:
> +		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC3);
> +		break;
> +	case AD7768_FILTER_SINC3_REJ60:
> +		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC3_REJ60);
> +		break;
> +	case AD7768_FILTER_WIDEBAND:
> +		/* Skip decimations 8 and 16, not supported by the wideband filter */
> +		dec_rate_idx = find_closest(dec_rate, &ad7768_dec_rate_values[2],
> +					    ARRAY_SIZE(ad7768_dec_rate_values) - 2);
> +		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_WIDEBAND) |
> +				    AD7768_DIG_FIL_DEC_RATE(dec_rate_idx);
> +		/* Correct the index offset */
> +		dec_rate_idx += 2;
> +		break;
> +	case AD7768_FILTER_SINC5:
> +		dec_rate_idx = find_closest(dec_rate, ad7768_dec_rate_values,
> +					    ARRAY_SIZE(ad7768_dec_rate_values));
> +
> +		/*
> +		 * Decimations 8 (idx 0) and 16 (idx 1) are set in the
> +		 * FILTER[6:4] field. The other decimations are set in the
> +		 * DEC_RATE[2:0] field, and the idx need to be offsetted by two.
> +		 */
> +		if (dec_rate_idx == 0)
> +			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5_X8);
> +		else if (dec_rate_idx == 1)
> +			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5_X16);
> +		else
> +			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5) |
> +					    AD7768_DIG_FIL_DEC_RATE(dec_rate_idx - 2);
> +		break;
> +	}
> +
> +	ret = regmap_write(st->regmap, AD7768_REG_DIGITAL_FILTER, dig_filter_regval);
> +	if (ret)
>  		return ret;
>  
> -	/* A sync-in pulse is required every time the filter dec rate changes */
> +	st->filter_type = filter_type;
> +	/*
> +	 * The decimation for SINC3 filters are configured in different
> +	 * registers
> +	 */
> +	if (filter_type == AD7768_FILTER_SINC3 ||
> +	    filter_type == AD7768_FILTER_SINC3_REJ60) {
> +		ret = ad7768_set_sinc3_dec_rate(st, dec_rate);
> +		if (ret)
> +			return ret;
> +	} else {
> +		st->oversampling_ratio =  ad7768_dec_rate_values[dec_rate_idx];

Looks like an extra space after =

> +	}
> +
> +	ad7768_fill_samp_freq_tbl(st);

This is opens a potentially complex race condition if we have the an
in kernel consumer reading the data in this array as it is being updated
(currently we can't stop that happening though solutions to that problem
have been much discussed). 

There aren't that many oversampling ratios so perhaps it is better
to precalculate all the potential available values as an array indexed
by oversampling ratio.  That way all the data is const, it's just possible
to get stale pointer to the wrong entry which can always happen anyway
if the read vs update happen in different entities.

> +
> +	/* A sync-in pulse is required after every configuration change */
>  	return ad7768_send_sync_pulse(st);
>  }

>  
> +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);

update to use if (!iio_device_claim_direct())

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