Message-ID: <6d0ff620-ec1a-4b17-9b5d-b9c48078271a@baylibre.com>
Date: Fri, 25 Apr 2025 10:43:29 -0500
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Alisa-Dariana Roman <alisa.roman@...log.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Subject: Re: [PATCH v1 1/1] iio: adc: ad7192: Refactor filter config

On 4/25/25 8:20 AM, Alisa-Dariana Roman wrote:
> It is not useful for users to set the 3db filter frequency or the
> oversampling value. Remove the option for these to be set by the user.
> 
> The available arrays for 3db filter frequency and oversampling value are
> not removed for backward compatibility.
> 
> The available array for 3db filter frequency is dynamic now, since some
> chips have 4 filter modes and others have 16.

The available array only makes sense if the matching attribute is writeable.
As mentioned in my reply to the cover letter, I think we should keep it
writeable for backwards compatibility. But we don't need to extend it to allow
writing new options, so keeping the previous available array seems fine to me.

> 
> Expose the filter mode to user, providing an intuitive way to select
> filter behaviour.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@...log.com>
> ---
>  drivers/iio/adc/ad7192.c | 455 +++++++++++++++++++++++++--------------
>  1 file changed, 288 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 530e1d307860..1846dc4e90b0 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -165,9 +165,9 @@
>  #define AD7192_EXT_FREQ_MHZ_MAX	5120000
>  #define AD7192_INT_FREQ_MHZ	4915200
>  
> -#define AD7192_NO_SYNC_FILTER	1
> -#define AD7192_SYNC3_FILTER	3
> -#define AD7192_SYNC4_FILTER	4
> +#define AD7192_FILTER_DIV	1024
> +#define AD7192_FS_MIN		1
> +#define AD7192_FS_MAX		1023
>  
>  /* NOTE:
>   * The AD7190/2/5 features a dual use data out ready DOUT/RDY output.
> @@ -182,6 +182,25 @@ enum {
>  	AD7192_SYSCALIB_FULL_SCALE,
>  };
>  
> +enum ad7192_filter_mode {
> +	AD7192_FILTER_SINC4,
> +	AD7192_FILTER_SINC3,
> +	AD7192_FILTER_SINC4_CHOP,
> +	AD7192_FILTER_SINC3_CHOP,
> +	AD7192_FILTER_SINC4_AVG2,
> +	AD7192_FILTER_SINC3_AVG2,
> +	AD7192_FILTER_SINC4_CHOP_AVG2,
> +	AD7192_FILTER_SINC3_CHOP_AVG2,
> +	AD7192_FILTER_SINC4_AVG8,
> +	AD7192_FILTER_SINC3_AVG8,
> +	AD7192_FILTER_SINC4_CHOP_AVG8,
> +	AD7192_FILTER_SINC3_CHOP_AVG8,
> +	AD7192_FILTER_SINC4_AVG16,
> +	AD7192_FILTER_SINC3_AVG16,
> +	AD7192_FILTER_SINC4_CHOP_AVG16,
> +	AD7192_FILTER_SINC3_CHOP_AVG16,
> +};
> +
>  enum {
>  	ID_AD7190,
>  	ID_AD7192,
> @@ -190,11 +209,24 @@ enum {
>  	ID_AD7195,
>  };
>  
> +struct ad7192_filter_config {
> +	enum ad7192_filter_mode		filter_mode;
> +	unsigned int			f_order;

I assume f means filter? Can we spell that out everywhere we have f_order (in
function names too)?

> +	u8				sinc3_en;
> +	u8				chop_en;
> +	u8				avg_val;
> +	enum iio_available_type		samp_freq_avail_type;

If this is always IIO_AVAIL_RANGE, then we don't need this field.

> +	int				samp_freq_avail_len;

If this is always 2, we don't need this field either.

> +	int				samp_freq_avail[2][2];

Range has 3 elements, start, step, stop. This only has 2.

> +};
> +
>  struct ad7192_chip_info {
>  	unsigned int			chip_id;
>  	const char			*name;
>  	const struct iio_chan_spec	*channels;
>  	u8				num_channels;
> +	const struct ad7192_filter_config	*filter_configs;
> +	u8					num_filter_modes;
>  	const struct ad_sigma_delta_info	*sigma_delta_info;
>  	const struct iio_info		*info;
>  	int (*parse_channels)(struct iio_dev *indio_dev);
> @@ -210,13 +242,16 @@ struct ad7192_state {
>  	u32				mode;
>  	u32				conf;
>  	u32				scale_avail[8][2];
> -	u32				filter_freq_avail[4][2];
> +	u32				(*filter_freq_avail)[2];
> +	u8				num_filter_modes;
>  	u32				oversampling_ratio_avail[4];
>  	u8				gpocon;
>  	u8				clock_sel;
>  	struct mutex			lock;	/* protect sensor state */
>  	u8				syscalib_mode[8];
>  
> +	enum ad7192_filter_mode		filter_mode;
> +
>  	struct ad_sigma_delta		sd;
>  };
>  
> @@ -282,7 +317,200 @@ static const struct iio_enum ad7192_syscalib_mode_enum = {
>  	.get = ad7192_get_syscalib_mode
>  };
>  
> -static const struct iio_chan_spec_ext_info ad7192_calibsys_ext_info[] = {
> +#define AD7192_FILTER_CONFIG(_filter_mode, _f_order, _sinc3_en, _chop_en, _avg_val)	\
> +{									\
> +		.filter_mode = (_filter_mode),				\
> +		.f_order = (_f_order),					\
> +		.sinc3_en = (_sinc3_en),				\
> +		.chop_en = (_chop_en),					\
> +		.avg_val = (_avg_val),					\
> +		.samp_freq_avail_type = IIO_AVAIL_RANGE,		\
> +		.samp_freq_avail_len = 2,		\
> +		.samp_freq_avail = {					\
> +			{ AD7192_INT_FREQ_MHZ,				\
> +				(_f_order) * AD7192_FILTER_DIV * AD7192_FS_MAX },	\
> +			{ AD7192_INT_FREQ_MHZ,				\
> +				(_f_order) * AD7192_FILTER_DIV * AD7192_FS_MIN },	\

These ranges don't make sense to me. Shouldn't we be calculating it during probe
based on the actual clock rate?

> +		},							\
> +}
> +
> +static const struct ad7192_filter_config ad7192_filter_configs[] = {
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4,		1, 0, 0, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3,		1, 1, 0, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_CHOP,		4, 0, 1, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_CHOP,		3, 1, 1, 1),
> +};
> +
> +static const struct ad7192_filter_config ad7192_filter_configs_avg[] = {
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4,		1, 0, 0, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3,		1, 1, 0, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_CHOP,		4, 0, 1, 1),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_CHOP,		3, 1, 1, 1),
> +
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_AVG2,		5, 0, 0, 2),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_AVG2,		4, 1, 0, 2),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_CHOP_AVG2,	5, 0, 1, 2),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_CHOP_AVG2,	4, 1, 1, 2),
> +
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_AVG8,		11, 0, 0, 8),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_AVG8,		10, 1, 0, 8),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_CHOP_AVG8,	11, 0, 1, 8),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_CHOP_AVG8,	10, 1, 1, 8),
> +
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_AVG16,		19, 0, 0, 16),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_AVG16,		18, 1, 0, 16),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC4_CHOP_AVG16,	19, 0, 1, 16),
> +	AD7192_FILTER_CONFIG(AD7192_FILTER_SINC3_CHOP_AVG16,	18, 1, 1, 16),
> +};
> +
> +static const char *const ad7192_filter_modes_str[] = {
> +	[AD7192_FILTER_SINC4] =			"sinc4",
> +	[AD7192_FILTER_SINC3] =			"sinc3",
> +	[AD7192_FILTER_SINC4_CHOP] =		"sinc4+chop",
> +	[AD7192_FILTER_SINC3_CHOP] =		"sinc3+chop",
> +	[AD7192_FILTER_SINC4_AVG2] =		"sinc4+avg2",
> +	[AD7192_FILTER_SINC3_AVG2] =		"sinc3+avg2",
> +	[AD7192_FILTER_SINC4_CHOP_AVG2] =	"sinc4+chop+avg2",
> +	[AD7192_FILTER_SINC3_CHOP_AVG2] =	"sinc3+chop+avg2",
> +	[AD7192_FILTER_SINC4_AVG8] =		"sinc4+avg8",
> +	[AD7192_FILTER_SINC3_AVG8] =		"sinc3+avg8",
> +	[AD7192_FILTER_SINC4_CHOP_AVG8] =	"sinc4+chop+avg8",
> +	[AD7192_FILTER_SINC3_CHOP_AVG8] =	"sinc3+chop+avg8",
> +	[AD7192_FILTER_SINC4_AVG16] =		"sinc4+avg16",
> +	[AD7192_FILTER_SINC3_AVG16] =		"sinc3+avg16",
> +	[AD7192_FILTER_SINC4_CHOP_AVG16] =	"sinc4+chop+avg16",
> +	[AD7192_FILTER_SINC3_CHOP_AVG16] =	"sinc3+chop+avg16",
> +};

We need to make these match the values already defined in the ABI docs as much
as we can.

I see in the datasheets that there is a REJ60 bit in the MODE register, so I
would expect to see "sinc3+rej60" in this list as well.

We already have "sinc3+sinc1" that is defined as 'Sinc3 + averaging by 8' so
"sinc3+avg8" would be redunant. And given that this driver already uses
the oversampling_ratio attribute to control the avg2/8/16, I'm wondering if we
can keep that instead of introducing more filter types.

I also wonder if "sinc3+pf1" could be used for "sinc3+chop" since it is defined
as a device-specific post filter. Or make the case that "chop" is common enough
that it deseres it's own name.

I'm not the best expert on filters though, so I'm sure Jonathan will have some
better wisdom to share here.

Here is the existing list. If we have any filter types that don't fit into the
existing ones, we will need to have a separate patch to add those to the
documentation too.


What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available
What:		/sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_type_available
KernelVersion:	6.1
Contact:	linux-iio@...r.kernel.org
Description:
		Reading returns a list with the possible filter modes. Options
		for the attribute:

		* "sinc3" - The digital sinc3 filter. Moderate 1st
		  conversion time. Good noise performance.
		* "sinc4" - Sinc 4. Excellent noise performance. Long
		  1st conversion time.
		* "sinc5" - The digital sinc5 filter. Excellent noise
		  performance
		* "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion
		  time.
		* "sinc3+rej60" - Sinc3 + 60Hz rejection.
		* "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion
		  time.
		* "sinc3+pf1" - Sinc3 + device specific Post Filter 1.
		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
		* "wideband" - filter with wideband low ripple passband
		  and sharp transition band.



> +static int ad7192_set_filter_mode(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int val)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	const struct ad7192_filter_config *filter_config = &st->chip_info->filter_configs[val];

Seems dangerous to access an array without checking that val is in range first,
especially since it comes from userspace.

> +	u16 old_samp_freq, div;
> +	int i, ret;

Probably should have iio_device_claim_direct() here to make sure we can't change
the filter mode in the middle of a buffer read.

> +
> +	old_samp_freq = ad7192_get_f_adc(st);
> +
> +	st->filter_mode = val;
> +
> +	div = DIV_ROUND_CLOSEST(st->fclk, ad7192_get_f_order(st) * old_samp_freq);
> +	if (div < AD7192_FS_MIN || div > AD7192_FS_MAX)
> +		return -EINVAL;
> +
> +	st->mode &= ~AD7192_MODE_RATE_MASK;
> +	st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
> +
> +	st->mode &= ~AD7192_MODE_SINC3;
> +	st->mode |= FIELD_PREP(AD7192_MODE_SINC3, filter_config->sinc3_en);
> +
> +	st->conf &= ~AD7192_CONF_CHOP;
> +	st->conf |= FIELD_PREP(AD7192_CONF_CHOP, filter_config->chop_en);
> +
> +	for (i = 0; i < ARRAY_SIZE(st->oversampling_ratio_avail); i++) {
> +		if (filter_config->avg_val != st->oversampling_ratio_avail[i])
> +			continue;
> +
> +		st->mode &= ~AD7192_MODE_AVG_MASK;
> +		st->mode |= FIELD_PREP(AD7192_MODE_AVG_MASK, i);
> +	}
> +
> +	ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad_sd_write_reg(&st->sd, AD7192_REG_CONF, 3, st->conf);
> +	if (ret < 0)
> +		return ret;
> +
> +	ad7192_update_filter_freq_avail(st);
> +
> +	return 0;
> +}
> +
> +static int ad7192_get_filter_mode(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	return st->filter_mode;
> +}
> +
> +static const struct iio_enum ad7192_filter_mode_enum = {
> +	.items = ad7192_filter_modes_str,
> +	.num_items = ARRAY_SIZE(ad7192_filter_modes_str),
> +	.set = ad7192_set_filter_mode,
> +	.get = ad7192_get_filter_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info ad7192_ext_info[] = {
>  	{
>  		.name = "sys_calibration",
>  		.write = ad7192_write_syscalib,
> @@ -292,6 +520,9 @@ static const struct iio_chan_spec_ext_info ad7192_calibsys_ext_info[] = {
>  		 &ad7192_syscalib_mode_enum),
>  	IIO_ENUM_AVAILABLE("sys_calibration_mode", IIO_SHARED_BY_TYPE,
>  			   &ad7192_syscalib_mode_enum),
> +	IIO_ENUM("filter_mode", IIO_SHARED_BY_ALL, &ad7192_filter_mode_enum),
> +	IIO_ENUM_AVAILABLE("filter_mode", IIO_SHARED_BY_ALL,
> +			   &ad7192_filter_mode_enum),

As in the ABI docs above, this needs to be "filter_type", not "filter_mode". It
would make sense to rename the functions and variables as well. (We recently
standardized this across all existing drivers.)

>  	{ }
>  };

Since some chips don't support averaging, we will need two different ext_info
now so that filter_type_available is correct for those chips.

>  
> @@ -650,15 +881,22 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
>  	st->oversampling_ratio_avail[2] = 8;
>  	st->oversampling_ratio_avail[3] = 16;
>  
> -	st->filter_freq_avail[0][0] = 600;
> -	st->filter_freq_avail[1][0] = 800;
> -	st->filter_freq_avail[2][0] = 2300;
> -	st->filter_freq_avail[3][0] = 2720;
> +	return 0;
> +}
>  
> -	st->filter_freq_avail[0][1] = 1000;
> -	st->filter_freq_avail[1][1] = 1000;
> -	st->filter_freq_avail[2][1] = 1000;
> -	st->filter_freq_avail[3][1] = 1000;
> +static int ad7192_filter_setup(struct ad7192_state *st)
> +{
> +	unsigned int num_modes = st->chip_info->num_filter_modes;
> +
> +	st->filter_freq_avail = devm_kcalloc(&st->sd.spi->dev, num_modes,
> +					     sizeof(*st->filter_freq_avail),
> +					     GFP_KERNEL);
> +	if (!st->filter_freq_avail)
> +		return -ENOMEM;
> +
> +	st->num_filter_modes = num_modes;
> +
> +	ad7192_update_filter_freq_avail(st);

As mentioned elsewhere, I would just keep the existing implementation for the
3db_frequency_available attribute and add some comments that it is for backwards
compatibility only.



> @@ -936,7 +1050,7 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), 1024);
> +		*val = DIV_ROUND_CLOSEST(ad7192_get_f_adc(st), AD7192_FILTER_DIV);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		*val = ad7192_get_3db_filter_freq(st);
> @@ -957,7 +1071,7 @@ static int __ad7192_write_raw(struct iio_dev *indio_dev,
>  			      long mask)
>  {
>  	struct ad7192_state *st = iio_priv(indio_dev);
> -	int i, div;
> +	int i, div, ret;
>  	unsigned int tmp;
>  
>  	guard(mutex)(&st->lock);
> @@ -982,32 +1096,20 @@ static int __ad7192_write_raw(struct iio_dev *indio_dev,
>  		if (!val)
>  			return -EINVAL;
>  
> -		div = st->fclk / (val * ad7192_get_f_order(st) * 1024);
> -		if (div < 1 || div > 1023)
> +		div = st->fclk / (val * ad7192_get_f_order(st) * AD7192_FILTER_DIV);
> +		if (div < AD7192_FS_MIN || div > AD7192_FS_MAX)
>  			return -EINVAL;
>  
>  		st->mode &= ~AD7192_MODE_RATE_MASK;
>  		st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
> -		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +
> +		ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +		if (ret)
> +			return ret;

This looks like an unrelated fixup, so please put that in a separate patch.
Actually 2 patches, one for adding the macros and one for checking the return
value.

Powered by blists - more mailing lists