| 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: <evxvva62zqwusoizcoeds6bojjdc4z5chtdeaikaxuliftvfde@xuctkxp45437>
Date: Fri, 25 Jul 2025 09:44:48 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] iio: adc: ad7173: support changing filter type
On Thu, Jul 10, 2025 at 05:39:52PM -0500, David Lechner wrote:
> Add support for changing the filter type to the ad7173 driver.
>
> This family of chips by default uses a sinc5+sinc1 filter. There are
> also optional post-filters that can be added in this configuration for
> various 50/60Hz rejection purposes. The sinc3 filter doesn't have any
> post-filters and handles the output data rate (ODR) a bit differently.
> Here, we've opted to use SINC3_MAPx to get the maximum possible
> sampling frequencies with the SINC3 filter.
>
> Adding support consists of adding the filter_type and
> filter_type_available attributes, making the sampling_frequency
> attribute aware of the filter type, and programming the filter
> parameters when we configure the channel of the ADC for reading
> a sample.
>
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> drivers/iio/adc/ad7173.c | 186 +++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 181 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 01d78d531d6c024dd92fff21b8b2afb750092b66..c235096fbad4aeb77a7385001a16bc0ecd603f46 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -8,6 +8,7 @@
> * AD7175-8/AD7176-2/AD7177-2
> *
> * Copyright (C) 2015, 2024 Analog Devices, Inc.
> + * Copyright (C) 2025 BayLibre, SAS
> */
>
> #include <linux/array_size.h>
> @@ -149,7 +150,12 @@
> (pin2) < st->info->num_voltage_in && \
> (pin2) >= st->info->num_voltage_in_div)
>
> -#define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
> +#define AD7173_FILTER_SINC3_MAP BIT(15)
> +#define AD7173_FILTER_SINC3_MAP_DIV GENMASK(14, 0)
> +#define AD7173_FILTER_ENHFILTEN BIT(11)
> +#define AD7173_FILTER_ENHFILT_MASK GENMASK(10, 8)
> +#define AD7173_FILTER_ORDER BIT(6)
> +#define AD7173_FILTER_ODR_MASK GENMASK(5, 0)
> #define AD7173_MAX_CONFIGS 8
> #define AD4111_OW_DET_THRSH_MV 300
>
> @@ -190,6 +196,15 @@ struct ad7173_device_info {
> u8 num_gpios;
> };
>
> +enum ad7173_filter_type {
> + AD7173_FILTER_SINC3,
> + AD7173_FILTER_SINC5_SINC1,
> + AD7173_FILTER_SINC5_SINC1_PF1,
> + AD7173_FILTER_SINC5_SINC1_PF2,
> + AD7173_FILTER_SINC5_SINC1_PF3,
> + AD7173_FILTER_SINC5_SINC1_PF4,
> +};
> +
> struct ad7173_channel_config {
> /* Openwire detection threshold */
> unsigned int openwire_thrsh_raw;
> @@ -205,8 +220,10 @@ struct ad7173_channel_config {
> struct_group(config_props,
> bool bipolar;
> bool input_buf;
> + u16 sinc3_ord_div;
typo? ord -> odr?
> u8 sinc5_odr_index;
> u8 ref_sel;
> + enum ad7173_filter_type filter_type;
> );
> };
>
> @@ -266,6 +283,24 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
> 5000, /* 20 */
> };
>
> +/**
> + * ad7173_sinc3_ord_div_from_odr() - Convert ODR to divider value
> + * @odr_millihz: ODR (sampling_frequency) in milliHz
> + * Returns: Divider value for SINC3 filter to pass.
> + */
> +static u16 ad7173_sinc3_ord_div_from_odr(u32 odr_millihz)
same typo in the func name?
> +{
> + /*
> + * Divider is f_MOD (1 MHz) / 32 / ODR. ODR freq is in milliHz, so
> + * we need to convert f_MOD to the same units. When SING_CYC=1 or
> + * multiple channels are enabled (currently always the case), there
> + * is an additional factor of 3.
> + */
> + u32 div = DIV_ROUND_CLOSEST(MEGA * MILLI, odr_millihz * 32 * 3);
> + /* Avoid divide by 0 and limit to register field size. */
> + return clamp(div, 1U, AD7173_FILTER_SINC3_MAP_DIV);
> +}
> +
> static unsigned int ad4111_current_channel_config[] = {
> /* Ain sel: pos neg */
> 0x1E8, /* 15:IIN0+ 8:IIN0− */
> @@ -369,6 +404,47 @@ static const struct iio_enum ad7173_syscalib_mode_enum = {
> .get = ad7173_get_syscalib_mode
> };
>
> +static const char * const ad7173_filter_types_str[] = {
> + [AD7173_FILTER_SINC3] = "sinc3",
> + [AD7173_FILTER_SINC5_SINC1] = "sinc5+sinc1",
> + [AD7173_FILTER_SINC5_SINC1_PF1] = "sinc5+sinc1+pf1",
> + [AD7173_FILTER_SINC5_SINC1_PF2] = "sinc5+sinc1+pf2",
> + [AD7173_FILTER_SINC5_SINC1_PF3] = "sinc5+sinc1+pf3",
> + [AD7173_FILTER_SINC5_SINC1_PF4] = "sinc5+sinc1+pf4",
> +};
> +
> +static int ad7173_set_filter_type(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int val)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + st->channels[chan->address].cfg.filter_type = val;
> + st->channels[chan->address].cfg.live = false;
> +
> + iio_device_release_direct(indio_dev);
> +
> + return 0;
> +}
> +
> +static int ad7173_get_filter_type(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> +
> + return st->channels[chan->address].cfg.filter_type;
> +}
I know, I'm usual picky with this but FWIW, the above is a possible data
race. Anyways, still up to you...
- Nuno Sá
> +
> +static const struct iio_enum ad7173_filter_type_enum = {
> + .items = ad7173_filter_types_str,
> + .num_items = ARRAY_SIZE(ad7173_filter_types_str),
> + .set = ad7173_set_filter_type,
> + .get = ad7173_get_filter_type,
> +};
> +
> static const struct iio_chan_spec_ext_info ad7173_chan_spec_ext_info[] = {
> {
> .name = "sys_calibration",
> @@ -379,6 +455,16 @@ static const struct iio_chan_spec_ext_info ad7173_chan_spec_ext_info[] = {
> &ad7173_syscalib_mode_enum),
> IIO_ENUM_AVAILABLE("sys_calibration_mode", IIO_SHARED_BY_TYPE,
> &ad7173_syscalib_mode_enum),
> + IIO_ENUM("filter_type", IIO_SEPARATE, &ad7173_filter_type_enum),
> + IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_TYPE,
> + &ad7173_filter_type_enum),
> + { }
> +};
> +
> +static const struct iio_chan_spec_ext_info ad7173_temp_chan_spec_ext_info[] = {
> + IIO_ENUM("filter_type", IIO_SEPARATE, &ad7173_filter_type_enum),
> + IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_TYPE,
> + &ad7173_filter_type_enum),
> { }
> };
>
> @@ -582,14 +668,18 @@ static bool ad7173_setup_equal(const struct ad7173_channel_config *cfg1,
> sizeof(struct {
> bool bipolar;
> bool input_buf;
> + u16 sinc3_ord_div;
> u8 sinc5_odr_index;
> u8 ref_sel;
> + enum ad7173_filter_type filter_type;
> }));
>
> return cfg1->bipolar == cfg2->bipolar &&
> cfg1->input_buf == cfg2->input_buf &&
> + cfg1->sinc3_ord_div == cfg2->sinc3_ord_div &&
> cfg1->sinc5_odr_index == cfg2->sinc5_odr_index &&
> - cfg1->ref_sel == cfg2->ref_sel;
> + cfg1->ref_sel == cfg2->ref_sel &&
> + cfg1->filter_type == cfg2->filter_type;
> }
>
> static struct ad7173_channel_config *
> @@ -630,6 +720,7 @@ static int ad7173_load_config(struct ad7173_state *st,
> {
> unsigned int config;
> int free_cfg_slot, ret;
> + u8 post_filter_enable, post_filter_select;
>
> free_cfg_slot = ida_alloc_range(&st->cfg_slots_status, 0,
> st->info->num_configs - 1, GFP_KERNEL);
> @@ -649,8 +740,49 @@ static int ad7173_load_config(struct ad7173_state *st,
> if (ret)
> return ret;
>
> + /*
> + * When SINC3_MAP flag is enabled, the rest of the register has a
> + * different meaning. We are using this option to allow the most
> + * possible sampling frequencies with SINC3 filter.
> + */
> + if (cfg->filter_type == AD7173_FILTER_SINC3)
> + return ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(free_cfg_slot), 2,
> + FIELD_PREP(AD7173_FILTER_SINC3_MAP, 1) |
> + FIELD_PREP(AD7173_FILTER_SINC3_MAP_DIV,
> + cfg->sinc3_ord_div));
> +
> + switch (cfg->filter_type) {
> + case AD7173_FILTER_SINC5_SINC1_PF1:
> + post_filter_enable = 1;
> + post_filter_select = 2;
> + break;
> + case AD7173_FILTER_SINC5_SINC1_PF2:
> + post_filter_enable = 1;
> + post_filter_select = 3;
> + break;
> + case AD7173_FILTER_SINC5_SINC1_PF3:
> + post_filter_enable = 1;
> + post_filter_select = 5;
> + break;
> + case AD7173_FILTER_SINC5_SINC1_PF4:
> + post_filter_enable = 1;
> + post_filter_select = 6;
> + break;
> + default:
> + post_filter_enable = 0;
> + post_filter_select = 0;
> + break;
> + }
> +
> return ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(free_cfg_slot), 2,
> - AD7173_FILTER_ODR0_MASK & cfg->sinc5_odr_index);
> + FIELD_PREP(AD7173_FILTER_SINC3_MAP, 0) |
> + FIELD_PREP(AD7173_FILTER_ENHFILT_MASK,
> + post_filter_enable) |
> + FIELD_PREP(AD7173_FILTER_ENHFILTEN,
> + post_filter_select) |
> + FIELD_PREP(AD7173_FILTER_ORDER, 0) |
> + FIELD_PREP(AD7173_FILTER_ODR_MASK,
> + cfg->sinc5_odr_index));
> }
>
> static int ad7173_config_channel(struct ad7173_state *st, int addr)
> @@ -1183,6 +1315,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
> case IIO_CHAN_INFO_SAMP_FREQ:
> + if (st->channels[chan->address].cfg.filter_type == AD7173_FILTER_SINC3) {
> + /* Inverse operation of ad7173_sinc3_ord_div_from_odr() */
> + *val = MEGA;
> + *val2 = 3 * 32 * st->channels[chan->address].cfg.sinc3_ord_div;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> reg = st->channels[chan->address].cfg.sinc5_odr_index;
>
> *val = st->info->sinc5_data_rates[reg] / MILLI;
> @@ -1221,6 +1360,10 @@ static int ad7173_write_raw(struct iio_dev *indio_dev,
> *
> * This will cause the reading of CH1 to be actually done once every
> * 200.16ms, an effective rate of 4.99sps.
> + *
> + * Both the sinc5 and sinc3 rates are set here so that if the filter
> + * type is changed, the requested rate will still be set (aside from
> + * rounding differences).
> */
> case IIO_CHAN_INFO_SAMP_FREQ:
> freq = val * MILLI + val2 / MILLI;
> @@ -1230,6 +1373,7 @@ static int ad7173_write_raw(struct iio_dev *indio_dev,
>
> cfg = &st->channels[chan->address].cfg;
> cfg->sinc5_odr_index = i;
> + cfg->sinc3_ord_div = ad7173_sinc3_ord_div_from_odr(freq);
> cfg->live = false;
> break;
>
> @@ -1246,17 +1390,40 @@ static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
> const unsigned long *scan_mask)
> {
> struct ad7173_state *st = iio_priv(indio_dev);
> + u16 sinc3_count = 0;
> + u16 sinc3_div = 0;
> int i, j, k, ret;
>
> for (i = 0; i < indio_dev->num_channels; i++) {
> - if (test_bit(i, scan_mask))
> + const struct ad7173_channel_config *cfg = &st->channels[i].cfg;
> +
> + if (test_bit(i, scan_mask)) {
> + if (cfg->filter_type == AD7173_FILTER_SINC3) {
> + sinc3_count++;
> +
> + if (sinc3_div == 0) {
> + sinc3_div = cfg->sinc3_ord_div;
> + } else if (sinc3_div != cfg->sinc3_ord_div) {
> + dev_err(&st->sd.spi->dev,
> + "All enabled channels must have the same sampling_frequency for sinc3 filter_type\n");
> + return -EINVAL;
> + }
> + }
> +
> ret = ad7173_set_channel(&st->sd, i);
> - else
> + } else {
> ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
> + }
> if (ret < 0)
> return ret;
> }
>
> + if (sinc3_count && sinc3_count < bitmap_weight(scan_mask, indio_dev->num_channels)) {
> + dev_err(&st->sd.spi->dev,
> + "All enabled channels must have sinc3 filter_type\n");
> + return -EINVAL;
> + }
> +
> /*
> * On some chips, there are more channels that setups, so if there were
> * more unique setups requested than the number of available slots,
> @@ -1415,6 +1582,7 @@ static const struct iio_chan_spec ad7173_temp_iio_channel_template = {
> .storagebits = 32,
> .endianness = IIO_BE,
> },
> + .ext_info = ad7173_temp_chan_spec_ext_info,
> };
>
> static void ad7173_disable_regulators(void *data)
> @@ -1655,7 +1823,11 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> chan_st_priv->cfg.bipolar = false;
> chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> + chan_st_priv->cfg.sinc3_ord_div = ad7173_sinc3_ord_div_from_odr(
> + st->info->sinc5_data_rates[st->info->odr_start_value]
> + );
> chan_st_priv->cfg.sinc5_odr_index = st->info->odr_start_value;
> + chan_st_priv->cfg.filter_type = AD7173_FILTER_SINC5_SINC1;
> chan_st_priv->cfg.openwire_comp_chan = -1;
> st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> if (st->info->data_reg_only_16bit)
> @@ -1727,7 +1899,11 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> chan->scan_index = chan_index;
> chan->channel = ain[0];
> chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> + chan_st_priv->cfg.sinc3_ord_div = ad7173_sinc3_ord_div_from_odr(
> + st->info->sinc5_data_rates[st->info->odr_start_value]
> + );
> chan_st_priv->cfg.sinc5_odr_index = st->info->odr_start_value;
> + chan_st_priv->cfg.filter_type = AD7173_FILTER_SINC5_SINC1;
> chan_st_priv->cfg.openwire_comp_chan = -1;
>
> chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
>
> --
> 2.43.0
>
Powered by blists - more mailing lists