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

[Jonathan Santos <jonath4nns@gmail.com>]

On 03/08, Jonathan Cameron wrote:
> 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
F> > 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.
> 
I understand the problem, but the number of possible sampling
frequencies is quite large because of the decimation/OSR:

-> For wideband there are 6 decimations available.
-> For Sinc5 there are 8 decimations.
-> For sinc3 (here's the problem) we have up to 5119 decimation options.
From x32 to x163,840 (mclk_div = 2) with a 32 step.

BTW, that's why we use ranges for `oversampling_ratio_available`
attribute.

To reflect all sampling frequencies combinations (fref/(mclk_div *
OSR)), we would need a considerably large array. We did not have this
problem before because we were not supporting the sinc3 filter.

> > +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 =
> 

Sorry about that

> > +	}
> > +
> > +	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

As I said above, unfortunately there are many OSR options.

> 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())
> 

OK!

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