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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 31 Mar 2025 21:18:16 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> 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.

Ah, understood now.  Another case where I guess we need to fix up in the long
term.  Short term not a problem as I don't think any consumers yet read the filter
parameters anyway.

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