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

[Ben Collins <bcollins@watter.com>]

> On Aug 16, 2025, at 11:08 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> On Sat, 16 Aug 2025 09:12:37 -0400
> Ben Collins <bcollins@watter.com> wrote:
> 
>>> On Aug 16, 2025, at 5:54 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> 
>>> On Wed, 13 Aug 2025 17:52:04 -0500
>>> David Lechner <dlechner@baylibre.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@watter.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.
> Ah.  Could we use filter_type? (additional attribute)
> 
> That already has a 'none' option.  Nothing there yet that works for the 'on'
> option here.  These are always tricky to name unless they are a very
> well known class of filter.   The datasheet calls this one an Exponential
> Moving Average filter. Not a term I'd encountered before, but google did
> find me some references.  so maybe ema as a filter type?

In the docs I have, it says:

	In addition, this device integrates a first order recursive
	Infinite Impulse Response (IIR) filter, also known as
	Exponential Moving Average (EMA).

The EMA formula I’ve used for an adc-attached thermistor was the same
formula I’ve seen used in IIR, so I think they are generally the same.

>> 
>> 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.
> 
> Hmm. Whilst kind of backwards as that's where you'll see aliasing it does make more sense
> I think than just a magic large number.
> 
> I think I prefer the filter type route though now your comment on 'off' has lead me to it.
> 
> Make sure to add ABI docs for the new filter type if you do go that way.

I was considering a new “filter_enable” attribute and only list the
other values in the 3db filter available. This seems more robust and
doesn’t require any sort of agreed on magic number.