linux-kernel - Re: [PATCH v2 5/5] 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: <DA11BDA3-E4E3-4C1A-9E4E-84E92F62A4B3@watter.com>
Date: Sat, 16 Aug 2025 09:12:37 -0400
From: Ben Collins <bcollins@...ter.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/5] iio: mcp9600: Add support for IIR filter


> On Aug 16, 2025, at 5:54 AM, Jonathan Cameron <jic23@...nel.org> wrote:
> 
> On Wed, 13 Aug 2025 17:52:04 -0500
> David Lechner <dlechner@...libre.com> wrote:
> 
>> On 8/13/25 10:15 AM, Ben Collins wrote:
>>> MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
>>> to allow get/set of this value.
>>> 
>>> Signed-off-by: Ben Collins <bcollins@...ter.com>
>>> ---
>>> drivers/iio/temperature/mcp9600.c | 43 +++++++++++++++++++++++++++++++
>>> 1 file changed, 43 insertions(+)
>>> 
>>> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
>>> index 5ead565f1bd8c..5bed3a35ae65e 100644
>>> --- a/drivers/iio/temperature/mcp9600.c
>>> +++ b/drivers/iio/temperature/mcp9600.c
>>> @@ -31,6 +31,7 @@
>>> #define MCP9600_STATUS_ALERT(x) BIT(x)
>>> #define MCP9600_SENSOR_CFG 0x5
>>> #define MCP9600_SENSOR_TYPE_MASK GENMASK(6, 4)
>>> +#define MCP9600_FILTER_MASK GENMASK(2, 0)
>>> #define MCP9600_ALERT_CFG1 0x8
>>> #define MCP9600_ALERT_CFG(x) (MCP9600_ALERT_CFG1 + (x - 1))
>>> #define MCP9600_ALERT_CFG_ENABLE BIT(0)
>>> @@ -111,6 +112,7 @@ static const struct iio_event_spec mcp9600_events[] = {
>>> .address = MCP9600_HOT_JUNCTION,        \
>>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |        \
>>>       BIT(IIO_CHAN_INFO_SCALE) |       \
>>> +       BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |  \
>>>       BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE), \
>>> .event_spec = &mcp9600_events[hj_ev_spec_off],        \
>>> .num_event_specs = hj_num_ev,        \
>>> @@ -149,6 +151,7 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>>> struct mcp9600_data {
>>> struct i2c_client *client;
>>> u32 thermocouple_type;
>>> + u32 filter_level;
>>> };
>>> 
>>> static int mcp9600_read(struct mcp9600_data *data,
>>> @@ -186,6 +189,9 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
>>> case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
>>> *val = mcp9600_tc_types[data->thermocouple_type];
>>> return IIO_VAL_CHAR;
>>> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>>> + *val = data->filter_level;  
>> 
>> We can't just pass the raw value through for this. The ABI is defined
>> in Documentation/ABI/testing/sysfs-bus-iio and states that the value
>> is the frequency in Hz.
>> 
>> So we need to do the math to convert from the register value to
>> the required value.
>> 
>> I'm a bit rusty on my discrete time math, so I had chatgpt help me
>> do the transform of the function from the datasheet to a transfer
>> function and use that to find the frequency response.
>> 
>> It seemed to match what my textbook was telling me, so hopefully
>> it got it right.
>> 
>> Then it spit out the following program that can be used to make
>> a table of 3dB points for a given sampling frequency. If I read the
>> datasheet right, the sampling frequency depends on the number of
>> bits being read.
>> 
>> For example, for 3 Hz sample rate (18-bit samples), I got:
>> 
>>  n  f_3dB (Hz)
>>  1  0.58774
>>  2  0.24939
>>  3  0.12063
>>  4  0.05984
>>  5  0.02986
>>  6  0.01492
>>  7  0.00746
>> 
>> I had to skip n=0 though since that is undefined. Not sure how we
>> handle that since it means no filter. Maybe Jonathan can advise?
> 
> This is always a fun corner case.  Reality is there is always
> some filtering going on due to the analog side of things we
> just have no idea what it is if the nicely defined filter is
> turned off.  I can't remember what we have done in the past,
> but one option would be to just have anything bigger than 0.58774
> defined as being filter off and return a big number. Not elegant
> though.  Or just don't bother supporting it if we think no one
> will ever want to run with not filter at all.
> 
> Hmm. or given this is a digital filter on a sampled signal, can we establish
> an effective frequency that could be detected without aliasing and
> use that?  Not sure - I'm way to rusty on filter theory (and was
> never that good at it!)

I’ve seen another driver use { U64_MAX, U64_MAX } for this case. It
didn’t seem very clean. I thought to use { 999999, 999999 } or even
{ 1, 0 }, but anything other than “off” just felt odd.

ChatGPT suggests this:

    • Clamp to Nyquist frequency
        • For a sample rate f_s, the maximum realizable cutoff is the Nyquist limit f_s/2.
        • At f_s = 3\ \text{Hz}, Nyquist is 1.5\ \text{Hz}.
        • You could encode { 1, 500000 } (1.5 Hz) as the maximum meaningful cutoff.