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

[Jonathan Santos <jonath4nns@gmail.com>]

On 02/16, Jonathan Cameron wrote:
> 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.
>
Yes, before the available frequencies were static, but now they depend
on the oversampling ratio. I can create a helper function to precomput
and fill this array and call it after configuring the digital filter, 
where the filter type and OSR is set.

For the previous static case, i call this function in the probe.
> 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.
>

I will include this in the helper function.

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