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: <20250816105410.70e47dac@jic23-huawei>
Date: Sat, 16 Aug 2025 10:54:10 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Ben Collins <bcollins@...ter.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 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!)

Jonathan