| 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: <20250505170950.1d7941d0@jic23-huawei>
Date: Mon, 5 May 2025 17:09:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
<andy@...nel.org>, <nuno.sa@...log.com>, <Michael.Hennerich@...log.com>,
<marcelo.schmitt@...log.com>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <marcelo.schmitt1@...il.com>,
<linus.walleij@...aro.org>, <brgl@...ev.pl>, <lgirdwood@...il.com>,
<broonie@...nel.org>, <jonath4nns@...il.com>, <dlechner@...libre.com>, "Pop
Paul" <paul.pop@...log.com>
Subject: Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and
oversampling ratio attributes
On Sun, 27 Apr 2025 21:14:17 -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
> 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 parameter change.
>
> Reviewed-by: David Lechner <dlechner@...libre.com>
> 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>
A few trivial additions from me.
> ---
> v6 Changes:
> * Made sinc3 decimation rate calculation clearer as requested.
> * Renamed some filter functions to clarify the purpose.
> * Other nits.
>
> v5 Changes:
> * Addressed some nits.
> * Use the new new iio_device_claim/release_direct() functions.
>
> v4 Changes:
> * Sampling frequency table is dynamically updated after every
> filter configuration.
>
> v3 Changes:
> * removed unused variables.
> * included sinc3+rej60 filter type.
> * oversampling_ratio moved to info_mask_shared_by_type.
> * reordered functions to avoid forward 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 | 363 ++++++++++++++++++++++++++++++-------
> 1 file changed, 293 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 10791a85d2c5..e2b8f12260a5 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -20,6 +20,8 @@
> #include <linux/regulator/driver.h>
> #include <linux/sysfs.h>
> #include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/util_macros.h>
>
> #include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> @@ -77,7 +79,7 @@
> #define AD7768_PWR_PWRMODE(x) FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
>
> /* AD7768_REG_DIGITAL_FILTER */
> -#define AD7768_DIG_FIL_FIL_MSK GENMASK(6, 4)
> +#define AD7768_DIG_FIL_FIL_MSK GENMASK(7, 4)
Bug? If so does this belong in a precursor patch?
> #define AD7768_DIG_FIL_FIL(x) FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)
> @@ -404,22 +473,110 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
> return ret;
> }
>
> -static int ad7768_set_dig_fil(struct ad7768_state *st,
> - enum ad7768_dec_rate dec_rate)
> +static int ad7768_set_sinc3_dec_rate(struct ad7768_state *st,
> + unsigned int dec_rate)
> {
> - unsigned int mode;
> + unsigned int max_dec_rate;
> + u8 dec_rate_reg[2];
> + u16 regval;
> int ret;
>
> - if (dec_rate == AD7768_DEC_RATE_8 || dec_rate == AD7768_DEC_RATE_16)
> - mode = AD7768_DIG_FIL_FIL(dec_rate);
> - else
> - mode = AD7768_DIG_FIL_DEC_RATE(dec_rate);
> + /*
> + * Maximum dec_rate is limited by the MCLK_DIV value
Oddly short wrap. Go nearer 80 chars.
> + * and by the ODR. The edge case is for MCLK_DIV = 2
> + * ODR = 50 SPS.
> + * max_dec_rate <= MCLK / (2 * 50)
> + */
> +static int ad7768_get_filter_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 */
> + return ad7768_filter_regval_to_type[mode];
return ad7768_filter_regval_to_type[FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode)];
Is fine (only just over 80 chars and I'm getting more relaxed as time moves forwards).
Also avoids the dual meaning of mode which I never like to see.
> }
>
> -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);
Whilst I doubt anyone will notice this looks like a functional change
that should be called out in the patch description.
Previously we allowed frequency changes in buffered mode, now we don't.
> +
> + 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;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + ret = __ad7768_write_raw(indio_dev, chan, val, val2, info);
> + iio_device_release_direct(indio_dev);
> +
> + return ret;
> +}
;
Powered by blists - more mailing lists