Re: [PATCH 6/6] iio: mcp9600: Add support for IIR filter

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 19 Aug 2025 09:43:48 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/19/25 9:32 AM, Ben Collins wrote:
> > On Tue, Aug 19, 2025 at 09:15:23AM -0500, David Lechner wrote:  
> >> On 8/19/25 9:11 AM, Ben Collins wrote:  
> >>> On Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote:  
> >>>> On 8/18/25 1:32 PM, Ben Collins wrote:  
> >>>>> From: Ben Collins <bcollins@watter.com>
> >>>>>
> >>>>> MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> >>>>> to allow get/set of this value.
> >>>>>
> >>>>> Use a filter_type[none, ema] for enabling the IIR filter.
> >>>>>
> >>>>> Signed-off-by: Ben Collins <bcollins@watter.com>
> >>>>> ---
> >>>>> +		if (data->filter_level == 0)
> >>>>> +			return -EINVAL;  
> >>>>
> >>>> To implement Jonathan's request from v5, drop this error return.
> >>>> We'll also need a separate bool data->is_filter_enabled field so
> >>>> that we can keep the last set filter_level even when the filter
> >>>> is disabled. (i.e. data->filter_level is never == 0).
> >>>>
> >>>> This way, if you set the filter level, you can enable and disable
> >>>> the filter via filter_type and still have the same filter level.
> >>>>  
> >>>
> >>> Thanks, David. This is exactly what I've implemented, plus the
> >>> filter_enable attribute.
> >>>
> >>> Adding the ABI doc updates as well.
> >>>  
> >>
> >>
> >> Don't add the filter_enable attribute. The filter_type attribute
> >> already does the job.  
> > 
> > That doesn't solve the problem at hand. An example:
> > 
> > - Driver has 3 possible filter_type's, plus "none"
> > - User cats filter_type_available and sees [none, sinc4, sinc5, sinc5+avg]
> > - User cats filter_type and sees "none"
> > - User cats frequency_available: What do they see?
> > - User cats frequency: What do they see?  
> 
> The ones for the last selected filter before it was changed to "none".
> If the driver starts in the "none" state a probe, just pick sinc4
> as the default.

That works, or presenting no available frequencies if "none"
- empty list.  Though check the standard wrapper for available works
with a list of size 0. Not something we've done before.  Maybe a risk
of tripping up some userspace code?

Unlike some attribute/controls this one is unlikely to ever be destructive if we
have to pass through unusual states. Might have a slightly slower
transition to steady state if we are going through something inappropriate
briefly.

> 
> > 
> > Without filter_enable, [none, ema] driver works just fine. But the
> > above driver does not.
> >   
> 
> We can wait and see what Jonathan thinks. But if we introduce a
> new filter_enable attribute, then we need to think about what to
> do about the ad4080 driver since it was the one that recently
> introduced the filter_type = "none". Ideally we would change it
> to work the same so that we are consistent between drivers. But
> there is always the consideration that we can't go breaking existing
> ABI.
I was thinking we could paper over it (hence the email I sent
30 seconds before opening this) with a bonus attribute and basing
both filter_enable and none setting on the same underlying state.
Not that intuitive though as I think more about it. 

Setting off none would enable the filter when maybe a user was just
expecting to be able to see what was available (as will work for
any driver not implementing the 'none' value).  The ABI has always
allowed for interactions like this as sometimes we can't avoid them
but here maybe we can.

So ignore email of 2 mins ago, David's suggestion works better
(with modification for not showing any frequencies when on none possibly?)

Anyhow, lets take a little more time on this. I for one shouldn't
make any decisions quickly as is clear here!

Jonathan