On 02/16, Jonathan Cameron wrote:
> On Wed, 12 Feb 2025 15:18:59 -0300
> Jonathan Santos <Jonathan.Santos@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Pop Paul <paul.pop@xxxxxxxxxx>
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> 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;
> > +}
