Message-ID: <20250816160835.3b44a4cd@jic23-huawei>
Date: Sat, 16 Aug 2025 16:08:35 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ben Collins <bcollins@...ter.com>
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 Sat, 16 Aug 2025 09:12:37 -0400
Ben Collins <bcollins@...ter.com> wrote:

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

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

Jonathan

Powered by blists - more mailing lists