Re: [PATCH v1 1/1] iio: adc: ad7192: Refactor filter config

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 25 Apr 2025 10:43:29 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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.

I'm curious.  Why isn't it useful?

> > 
> > 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@analog.com>

> > +static const char *const ad7192_filter_modes_str[] = {
> > +	[AD7192_FILTER_SINC4] =			"sinc4",
> > +	[AD7192_FILTER_SINC3] =			"sinc3",
> > +	[AD7192_FILTER_SINC4_CHOP] =		"sinc4+chop",

Is chop really a filter? I had to look it up and to me at least it
seems like it isn't even though one thing it does is remove
some types of noise.  It also removes linear offsets (some types
of filter kind of do that, but the affect of chop smells more like
a calibration tweak than a filter)  

Maybe we need a separate control for chop, rather than trying to
force it through our already complex filter type attributes?


> > +	[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

hmm. That definition is odd.

> "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.

Tricky bit is whether the device changes the output rate (as needed for oversampling)
or whether it is applying the filter but retaining full output data rate.

Not sure which is happening here.  Given we previously had oversampling I guess
the datarate was affected?


> 
> 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.

Not a lot.  Too long since I last went anywhere near filters, so beyond agreeing
that we should stick to existing ABI where possible I don't really have any
useful guidance here.