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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e00a420-0db7-4df4-8164-a48dffd864e7@baylibre.com>
Date: Tue, 19 Aug 2025 09:43:48 -0500
From: David Lechner <dlechner@...libre.com>
To: 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 6/6] iio: mcp9600: Add support for IIR filter

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

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