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

[David Lechner]

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@xxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>> ---
>>>>> +		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.

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