| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z+swyCusIwl8zixt@JSANTO12-L01.ad.analog.com>
Date: Mon, 31 Mar 2025 21:18:16 -0300
From: Jonathan Santos <jonath4nns@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, lars@...afoo.de,
Michael.Hennerich@...log.com, marcelo.schmitt@...log.com,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
linus.walleij@...aro.org, brgl@...ev.pl, lgirdwood@...il.com,
broonie@...nel.org, dlechner@...libre.com,
marcelo.schmitt1@...il.com, Pop Paul <paul.pop@...log.com>
Subject: Re: [PATCH v4 16/17] iio: adc: ad7768-1: add filter type and
oversampling ratio attributes
On 03/08, Jonathan Cameron wrote:
> On Thu, 6 Mar 2025 18:04:24 -0300
> Jonathan Santos <Jonathan.Santos@...log.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@...log.com>
> > Signed-off-by: Pop Paul <paul.pop@...log.com>
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.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;
> > +}
>
Powered by blists - more mailing lists