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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 16 Aug 2025 12:16:44 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/16/25 10:33 AM, Ben Collins wrote:
> >   
> >> On Aug 16, 2025, at 11:19 AM, Ben Collins <bcollins@watter.com> wrote:
> >>  
> >>>
> >>> 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.  
> > 
> > Clarification: An EMA is a 1-pole IIR filter, while IIR filters can be
> > many other types besides 1-pole.
> >   
> 
> We already have a "sinc5+avg" filter, so I would call this one "avg".
> 
> I don't think we need to get too specific. The main point of the names
> is that for chips with more than one filter, it is obvious which one
> is which. For a chip with only "none" and "avg" is will be obvious
> that "avg" turns the filter on.
> 
I think average tends to be a box filter rather than an IIR one.  It's
kind of oversampling but you get a read out for each sample.

Jonathan