[PATCH v7 0/5] iio: mcp9600: Implement IIR feature and add iio core support

[PATCH v7 0/5] iio: mcp9600: Implement IIR feature and add iio core support

[Ben Collins]

ChangeLog:
v6 -> v7:
  - Remove extra space before trailing \
  - Don't add extra white-space
  - Remove mcp9600_write_raw_get_fmt
  - Separate out the mcp9600 IIR series into its own series as there is
    a lot of conversation around implementation.
  - Add rework of ad4080 to match filter_type "none" logic

v5 -> v6:
  - Cleanup tabs that were converted to spaces

v4 -> v5:
  - Missed a one line fix to IIR patch (5/5)

v3 -> v4:
  - Based on lots of feedback, use frequency values for IIR, and use
    filter_type[none, ema] to enable or disable.

v2 -> v3:
  - Improve changelogs in each patch
  - Based on feedback from Andy Shevchenko <andy.shevchenko@gmail.com>
    * Fix typos
    * FIELD_PREP -> FIELD_MODIFY
    * Remove explicit setting of 0 value in filter_level
  - Based on feedback from David Lechner <dlechner@baylibre.com>
    * Rework IIR values exposed to sysfs. Using the ratios, there was no
      way to represent "disabled" (i.e. infinity). Based on the bmp280
      driver I went with using the power coefficients (e.g. 1, 2, 4, 8,
      ...) where 1 is disabled (n=0).

v1 -> v2:
  - Break into individual patches

v1:
  - Initial patch to enable IIR and thermocouple-type
  - Recognize mcp9601

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
Ben Collins (5):
      iio: core: Add IIO_VAL_EMPTY type
      ABI: sysfs-bus-iio: Disambiguate usage for filter_type "none"
      ABI: sysfs-bus-iio: Document "ema" filter_type
      iio: mcp9600: Add support for IIR filter
      iio: ad4080: Rework filter_type "none" logic

 Documentation/ABI/testing/sysfs-bus-iio |   8 +-
 drivers/iio/adc/ad4080.c                |  23 +++--
 drivers/iio/industrialio-core.c         |   1 +
 drivers/iio/temperature/mcp9600.c       | 147 ++++++++++++++++++++++++++++++++
 include/linux/iio/types.h               |   1 +
 5 files changed, 166 insertions(+), 14 deletions(-)
---
base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
change-id: 20250819-mcp9600-iir-8f7ff1ad0804
prerequisite-change-id: 20250819-upstream-changes-c89af86743fa:v8
prerequisite-patch-id: 92882274615d59f2e89c189ce0859297fca88772
prerequisite-patch-id: d2c1fd9da2dee3ad5dc240f34cc108d02980a4c5
prerequisite-patch-id: 72368205aaa96b053ba78ffe6548d0895e039753
prerequisite-patch-id: 98a8a8ee92fc0a9836975d5b216d41702860019a
prerequisite-patch-id: c24cb6dd5b2b385c00bffdbf1f7a61e4d1532f49

Best regards,
-- 
Ben Collins <bcollins@kernel.org>

[PATCH v7 1/5] iio: core: Add IIO_VAL_EMPTY type

[Ben Collins]

In certain situations it may be necessary to return nothing when reading
an attribute.

For example, when a driver has a filter_type of "none" it should not
print any information for frequency or available frequencies.

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
 drivers/iio/industrialio-core.c | 1 +
 include/linux/iio/types.h       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 159d6c5ca3cec3f5c37ee9b85ef1681cca36f5c7..e4ff5b940223ab58bf61b394cc9357cd3674cfda 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -702,6 +702,7 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
 	case IIO_VAL_INT_64:
 		tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
 		return sysfs_emit_at(buf, offset, "%lld", tmp2);
+	case IIO_VAL_EMPTY:
 	default:
 		return 0;
 	}
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index ad2761efcc8315e1f9907d2a7159447fb463333e..261745c2d94e582bcca1a2abb297436e8314c930 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -32,6 +32,7 @@ enum iio_event_info {
 #define IIO_VAL_FRACTIONAL 10
 #define IIO_VAL_FRACTIONAL_LOG2 11
 #define IIO_VAL_CHAR 12
+#define IIO_VAL_EMPTY 13
 
 enum iio_available_type {
 	IIO_AVAIL_LIST,

-- 
2.39.5

Re: [PATCH v7 1/5] iio: core: Add IIO_VAL_EMPTY type

[David Lechner]

On 8/25/25 7:10 PM, Ben Collins wrote:
> In certain situations it may be necessary to return nothing when reading
> an attribute.
> 
> For example, when a driver has a filter_type of "none" it should not
> print any information for frequency or available frequencies.
> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> ---
>  drivers/iio/industrialio-core.c | 1 +
>  include/linux/iio/types.h       | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 159d6c5ca3cec3f5c37ee9b85ef1681cca36f5c7..e4ff5b940223ab58bf61b394cc9357cd3674cfda 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -702,6 +702,7 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>  	case IIO_VAL_INT_64:
>  		tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
>  		return sysfs_emit_at(buf, offset, "%lld", tmp2);
> +	case IIO_VAL_EMPTY:
>  	default:
>  		return 0;
>  	}
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index ad2761efcc8315e1f9907d2a7159447fb463333e..261745c2d94e582bcca1a2abb297436e8314c930 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -32,6 +32,7 @@ enum iio_event_info {
>  #define IIO_VAL_FRACTIONAL 10
>  #define IIO_VAL_FRACTIONAL_LOG2 11
>  #define IIO_VAL_CHAR 12
> +#define IIO_VAL_EMPTY 13
>  
>  enum iio_available_type {
>  	IIO_AVAIL_LIST,
> 

This is an interesting idea, but I think it would be a lot of work
to teach existing userspace tools to handle this new possibility.

On top of that, I'm not quite convinced it is necessary. If a numeric
value is undefined, then there is already a well known expression for
that: "nan". Or in the case of this series, the 3dB point when the
filter is disable could also be considered "inf". Using these would have
a better chance of working with existing userspace tools since things
like `scanf()` can already handle these.

Re: [PATCH v7 1/5] iio: core: Add IIO_VAL_EMPTY type

[Ben Collins]

On Tue, Aug 26, 2025 at 12:00:05PM -0500, David Lechner wrote:
> On 8/25/25 7:10 PM, Ben Collins wrote:
> > In certain situations it may be necessary to return nothing when reading
> > an attribute.
> > 
> > For example, when a driver has a filter_type of "none" it should not
> > print any information for frequency or available frequencies.
> > 
> > Signed-off-by: Ben Collins <bcollins@kernel.org>
> > ---
> >  drivers/iio/industrialio-core.c | 1 +
> >  include/linux/iio/types.h       | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 159d6c5ca3cec3f5c37ee9b85ef1681cca36f5c7..e4ff5b940223ab58bf61b394cc9357cd3674cfda 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -702,6 +702,7 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> >  	case IIO_VAL_INT_64:
> >  		tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> >  		return sysfs_emit_at(buf, offset, "%lld", tmp2);
> > +	case IIO_VAL_EMPTY:
> >  	default:
> >  		return 0;
> >  	}
> > diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> > index ad2761efcc8315e1f9907d2a7159447fb463333e..261745c2d94e582bcca1a2abb297436e8314c930 100644
> > --- a/include/linux/iio/types.h
> > +++ b/include/linux/iio/types.h
> > @@ -32,6 +32,7 @@ enum iio_event_info {
> >  #define IIO_VAL_FRACTIONAL 10
> >  #define IIO_VAL_FRACTIONAL_LOG2 11
> >  #define IIO_VAL_CHAR 12
> > +#define IIO_VAL_EMPTY 13
> >  
> >  enum iio_available_type {
> >  	IIO_AVAIL_LIST,
> > 
> 
> This is an interesting idea, but I think it would be a lot of work
> to teach existing userspace tools to handle this new possibility.
> 
> On top of that, I'm not quite convinced it is necessary. If a numeric
> value is undefined, then there is already a well known expression for
> that: "nan". Or in the case of this series, the 3dB point when the
> filter is disable could also be considered "inf". Using these would have
> a better chance of working with existing userspace tools since things
> like `scanf()` can already handle these.

I'm ok with "inf", but then would there also be an "inf" in available
frequencies?

This would take us all the way back to where I could just not even need
a filter_type==none and make the 3db available values:

{ inf, 0.5xxx, ... }

And inf would just be the filter is disabled.

-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

Re: [PATCH v7 1/5] iio: core: Add IIO_VAL_EMPTY type

[Jonathan Cameron]

On Tue, 26 Aug 2025 14:52:27 -0400
Ben Collins <bcollins@kernel.org> wrote:

> On Tue, Aug 26, 2025 at 12:00:05PM -0500, David Lechner wrote:
> > On 8/25/25 7:10 PM, Ben Collins wrote:  
> > > In certain situations it may be necessary to return nothing when reading
> > > an attribute.
> > > 
> > > For example, when a driver has a filter_type of "none" it should not
> > > print any information for frequency or available frequencies.
> > > 
> > > Signed-off-by: Ben Collins <bcollins@kernel.org>
> > > ---
> > >  drivers/iio/industrialio-core.c | 1 +
> > >  include/linux/iio/types.h       | 1 +
> > >  2 files changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index 159d6c5ca3cec3f5c37ee9b85ef1681cca36f5c7..e4ff5b940223ab58bf61b394cc9357cd3674cfda 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -702,6 +702,7 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> > >  	case IIO_VAL_INT_64:
> > >  		tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> > >  		return sysfs_emit_at(buf, offset, "%lld", tmp2);
> > > +	case IIO_VAL_EMPTY:
> > >  	default:
> > >  		return 0;
> > >  	}
> > > diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> > > index ad2761efcc8315e1f9907d2a7159447fb463333e..261745c2d94e582bcca1a2abb297436e8314c930 100644
> > > --- a/include/linux/iio/types.h
> > > +++ b/include/linux/iio/types.h
> > > @@ -32,6 +32,7 @@ enum iio_event_info {
> > >  #define IIO_VAL_FRACTIONAL 10
> > >  #define IIO_VAL_FRACTIONAL_LOG2 11
> > >  #define IIO_VAL_CHAR 12
> > > +#define IIO_VAL_EMPTY 13
> > >  
> > >  enum iio_available_type {
> > >  	IIO_AVAIL_LIST,
> > >   
> > 
> > This is an interesting idea, but I think it would be a lot of work
> > to teach existing userspace tools to handle this new possibility.
> > 
> > On top of that, I'm not quite convinced it is necessary. If a numeric
> > value is undefined, then there is already a well known expression for
> > that: "nan". Or in the case of this series, the 3dB point when the
> > filter is disable could also be considered "inf". Using these would have
> > a better chance of working with existing userspace tools since things
> > like `scanf()` can already handle these.  
> 
> I'm ok with "inf", but then would there also be an "inf" in available
> frequencies?
> 
> This would take us all the way back to where I could just not even need
> a filter_type==none and make the 3db available values:
> 
> { inf, 0.5xxx, ... }
> 
> And inf would just be the filter is disabled.
> 

Definitely an interesting concept and with appropriate documentation
additions I rather like it.  What particular formating of INF do
fscanf and friends accept?  looks like it's the strtod() man page
which says INF of INFINITY (disregarding case).  So indeed inf seems
like a valid choice.

Jonathan

[PATCH v7 2/5] ABI: sysfs-bus-iio: Disambiguate usage for filter_type "none"

[Ben Collins]

When a filter_type is "none", there really is no useful information that
can be presented from sampling and frequency attributes.

This is especially true when the driver supports more than one
filter_type, since the information would be ambiguous.

Suggest returning IIO_VAL_EMPTY for this case.

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
 Documentation/ABI/testing/sysfs-bus-iio | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 7e31b8cd49b32ea5b58bd99afc2e8105314d7a39..df42a4040530ee96096b998bebc8a08b4fb2d78f 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2276,7 +2276,11 @@ Description:
 		Reading returns a list with the possible filter modes. Options
 		for the attribute:
 
-		* "none" - Filter is disabled/bypassed.
+		* "none" - Filter is disabled/bypassed. When set in the
+		  filter_type attribute, a driver should return IIO_VAL_EMPTY
+		  when reading sampling and frequency related attributes of
+		  this filter, and return -EINVAL when trying to write these
+		  attributes.
 		* "sinc1" - The digital sinc1 filter. Fast 1st
 		  conversion time. Poor noise performance.
 		* "sinc3" - The digital sinc3 filter. Moderate 1st

-- 
2.39.5

[PATCH v7 3/5] ABI: sysfs-bus-iio: Document "ema" filter_type

[Ben Collins]

Exponential Moving Average (EMA) is a first-order IIR filter.

This is a common IIR algorithm used in mcp9600 and others.

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
 Documentation/ABI/testing/sysfs-bus-iio | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index df42a4040530ee96096b998bebc8a08b4fb2d78f..4e30f1db0a930f021cf07e5553d35a3feffddcc2 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2281,6 +2281,8 @@ Description:
 		  when reading sampling and frequency related attributes of
 		  this filter, and return -EINVAL when trying to write these
 		  attributes.
+		* "ema" - Exponential Moving Average (EMA) is a first-order
+		  Infinite Impulse Response (IIR) filter.
 		* "sinc1" - The digital sinc1 filter. Fast 1st
 		  conversion time. Poor noise performance.
 		* "sinc3" - The digital sinc3 filter. Moderate 1st

-- 
2.39.5

[PATCH v7 4/5] iio: mcp9600: Add support for IIR filter

[Ben Collins]

MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
to allow get/set of this value.

Use filter_type[none, ema] for enabling the IIR filter.

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
 drivers/iio/temperature/mcp9600.c | 147 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index aa42c2b1a369edbd36e0d6d6d1738ed0069fd990..d3309e30628ae5cdc74378403952ba285990f4c0 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		0x05
 #define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
+#define MCP9600_FILTER_MASK		GENMASK(2, 0)
 #define MCP9600_ALERT_CFG1		0x08
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
@@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
 	[THERMOCOUPLE_TYPE_R] = 'R',
 };
 
+enum mcp9600_filter {
+	MCP9600_FILTER_TYPE_NONE,
+	MCP9600_FILTER_TYPE_EMA,
+};
+
+static const char * const mcp9600_filter_type[] = {
+	[MCP9600_FILTER_TYPE_NONE] = "none",
+	[MCP9600_FILTER_TYPE_EMA] = "ema",
+};
+
+static const int mcp_iir_coefficients_avail[7][2] = {
+	/* Level 0 is no filter */
+	{ 0, 524549 },
+	{ 0, 243901 },
+	{ 0, 119994 },
+	{ 0,  59761 },
+	{ 0,  29851 },
+	{ 0,  14922 },
+	{ 0,   7461 },
+};
+
 static const struct iio_event_spec mcp9600_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -119,6 +141,8 @@ struct mcp_chip_info {
 struct mcp9600_data {
 	struct i2c_client *client;
 	u32 thermocouple_type;
+	int filter_level;
+	int filter_enabled;
 };
 
 static int mcp9600_config(struct mcp9600_data *data)
@@ -129,6 +153,9 @@ static int mcp9600_config(struct mcp9600_data *data)
 
 	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
 			  mcp9600_type_map[data->thermocouple_type]);
+	/* The chip understands 0 as "none", and 1-7 as ema filter levels. */
+	if (data->filter_enabled)
+		FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level + 1);
 
 	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
 	if (ret < 0) {
@@ -146,7 +173,11 @@ static int mcp9600_config(struct mcp9600_data *data)
 			.address = MCP9600_HOT_JUNCTION,		       \
 			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
 					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
+					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
 					      BIT(IIO_CHAN_INFO_SCALE),	       \
+			.info_mask_separate_available =                        \
+					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
+			.ext_info = mcp9600_ext_filter,			       \
 			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
 			.num_event_specs = hj_num_ev,			       \
 		},							       \
@@ -162,6 +193,57 @@ static int mcp9600_config(struct mcp9600_data *data)
 		},							       \
 	}
 
+static int mcp9600_get_filter(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+
+	return data->filter_enabled ? MCP9600_FILTER_TYPE_EMA :
+		MCP9600_FILTER_TYPE_NONE;
+}
+
+static int mcp9600_set_filter(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      unsigned int mode)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int new_type;
+
+	switch (mode) {
+	case MCP9600_FILTER_TYPE_NONE:
+		new_type = 0;
+		break;
+
+	case MCP9600_FILTER_TYPE_EMA:
+		new_type = 1;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	/* Do not reset the filter if we don't need to. */
+	if (data->filter_enabled == new_type)
+		return 0;
+
+	data->filter_enabled = new_type;
+	return mcp9600_config(data);
+}
+
+static const struct iio_enum mcp9600_filter_enum = {
+	.items = mcp9600_filter_type,
+	.num_items = ARRAY_SIZE(mcp9600_filter_type),
+	.get = mcp9600_get_filter,
+	.set = mcp9600_set_filter,
+};
+
+static const struct iio_chan_spec_ext_info mcp9600_ext_filter[] = {
+	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &mcp9600_filter_enum),
+	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
+			   &mcp9600_filter_enum),
+	{ }
+};
+
 static const struct iio_chan_spec mcp9600_channels[][2] = {
 	MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
 	MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
@@ -216,6 +298,69 @@ 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:
+		if (!data->filter_enabled)
+			return IIO_VAL_EMPTY;
+
+		*val = mcp_iir_coefficients_avail[data->filter_level][0];
+		*val2 = mcp_iir_coefficients_avail[data->filter_level][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9600_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		if (!data->filter_enabled) {
+			*type = IIO_VAL_EMPTY;
+			return IIO_AVAIL_LIST;
+		}
+
+		*vals = (int *)mcp_iir_coefficients_avail;
+		*length = 2 * ARRAY_SIZE(mcp_iir_coefficients_avail);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9600_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		if (!data->filter_enabled)
+			return -EINVAL;
+
+		for (i = 0; i < ARRAY_SIZE(mcp_iir_coefficients_avail); i++) {
+			if (mcp_iir_coefficients_avail[i][0] == val &&
+			    mcp_iir_coefficients_avail[i][1] == val2)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(mcp_iir_coefficients_avail))
+			return -EINVAL;
+
+		/* Do not reset the filter if there's no change. */
+		if (data->filter_level == i)
+			return 0;
+
+		data->filter_level = i;
+		return mcp9600_config(data);
+
 	default:
 		return -EINVAL;
 	}
@@ -358,6 +503,8 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
 
 static const struct iio_info mcp9600_info = {
 	.read_raw = mcp9600_read_raw,
+	.read_avail = mcp9600_read_avail,
+	.write_raw = mcp9600_write_raw,
 	.read_event_config = mcp9600_read_event_config,
 	.write_event_config = mcp9600_write_event_config,
 	.read_event_value = mcp9600_read_thresh,

-- 
2.39.5

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

[David Lechner]

On 8/25/25 7:10 PM, Ben Collins wrote:
> MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> to allow get/set of this value.
> 
> Use filter_type[none, ema] for enabling the IIR filter.
> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> ---
>  drivers/iio/temperature/mcp9600.c | 147 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index aa42c2b1a369edbd36e0d6d6d1738ed0069fd990..d3309e30628ae5cdc74378403952ba285990f4c0 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		0x05
>  #define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
> +#define MCP9600_FILTER_MASK		GENMASK(2, 0)
>  #define MCP9600_ALERT_CFG1		0x08
>  #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
>  #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> @@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
>  	[THERMOCOUPLE_TYPE_R] = 'R',
>  };
>  
> +enum mcp9600_filter {
> +	MCP9600_FILTER_TYPE_NONE,
> +	MCP9600_FILTER_TYPE_EMA,
> +};
> +
> +static const char * const mcp9600_filter_type[] = {
> +	[MCP9600_FILTER_TYPE_NONE] = "none",
> +	[MCP9600_FILTER_TYPE_EMA] = "ema",
> +};
> +
> +static const int mcp_iir_coefficients_avail[7][2] = {
> +	/* Level 0 is no filter */
> +	{ 0, 524549 },
> +	{ 0, 243901 },
> +	{ 0, 119994 },
> +	{ 0,  59761 },
> +	{ 0,  29851 },
> +	{ 0,  14922 },
> +	{ 0,   7461 },
> +};
> +
>  static const struct iio_event_spec mcp9600_events[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -119,6 +141,8 @@ struct mcp_chip_info {
>  struct mcp9600_data {
>  	struct i2c_client *client;
>  	u32 thermocouple_type;
> +	int filter_level;
> +	int filter_enabled;
>  };
>  
>  static int mcp9600_config(struct mcp9600_data *data)
> @@ -129,6 +153,9 @@ static int mcp9600_config(struct mcp9600_data *data)
>  
>  	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
>  			  mcp9600_type_map[data->thermocouple_type]);
> +	/* The chip understands 0 as "none", and 1-7 as ema filter levels. */
> +	if (data->filter_enabled)
> +		FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level + 1);
>  
>  	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
>  	if (ret < 0) {
> @@ -146,7 +173,11 @@ static int mcp9600_config(struct mcp9600_data *data)
>  			.address = MCP9600_HOT_JUNCTION,		       \
>  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
>  					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> +					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \

Does the filter actually only apply to the hot junction and not the
cold junction? There is a mismatch between this being info_mask_separate
and the filter_type being IIO_SHARED_BY_ALL.

Not related to this patch, but the comment same applies to
IIO_CHAN_INFO_THERMOCOUPLE_TYPE - I missed that in previous reviews.

>  					      BIT(IIO_CHAN_INFO_SCALE),	       \
> +			.info_mask_separate_available =                        \
> +					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
> +			.ext_info = mcp9600_ext_filter,			       \
>  			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
>  			.num_event_specs = hj_num_ev,			       \
>  		},							       \
> @@ -162,6 +193,57 @@ static int mcp9600_config(struct mcp9600_data *data)
>  		},							       \
>  	}
>  
> +static int mcp9600_get_filter(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +
> +	return data->filter_enabled ? MCP9600_FILTER_TYPE_EMA :
> +		MCP9600_FILTER_TYPE_NONE;
> +}
> +
> +static int mcp9600_set_filter(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      unsigned int mode)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	int new_type;

This variable name is a little confusing. It looks like it should
rather be:

	bool new_filter_enabled;

> +
> +	switch (mode) {
> +	case MCP9600_FILTER_TYPE_NONE:
> +		new_type = 0;
> +		break;
> +
> +	case MCP9600_FILTER_TYPE_EMA:
> +		new_type = 1;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Do not reset the filter if we don't need to. */
> +	if (data->filter_enabled == new_type)
> +		return 0;
> +
> +	data->filter_enabled = new_type;
> +	return mcp9600_config(data);
> +}
> +
> +static const struct iio_enum mcp9600_filter_enum = {
> +	.items = mcp9600_filter_type,
> +	.num_items = ARRAY_SIZE(mcp9600_filter_type),
> +	.get = mcp9600_get_filter,
> +	.set = mcp9600_set_filter,
> +};
> +
> +static const struct iio_chan_spec_ext_info mcp9600_ext_filter[] = {
> +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &mcp9600_filter_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> +			   &mcp9600_filter_enum),
> +	{ }
> +};
> +
>  static const struct iio_chan_spec mcp9600_channels[][2] = {
>  	MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
>  	MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
> @@ -216,6 +298,69 @@ 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:
> +		if (!data->filter_enabled)
> +			return IIO_VAL_EMPTY;

This brings us back to the earlier discussion of what should this
be when the filter is disabled. Mathematically, it would be infinite.

I wonder if it would be reasonable to return "inf" here since many
floating point parsers will already handle that as a valid value.

> +
> +		*val = mcp_iir_coefficients_avail[data->filter_level][0];
> +		*val2 = mcp_iir_coefficients_avail[data->filter_level][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

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

[Ben Collins]

On Tue, Aug 26, 2025 at 12:20:39PM -0500, David Lechner wrote:
> On 8/25/25 7:10 PM, Ben Collins wrote:
> > MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> > to allow get/set of this value.
> > 
> > Use filter_type[none, ema] for enabling the IIR filter.
> > 
> > Signed-off-by: Ben Collins <bcollins@kernel.org>
> > ---
> >  drivers/iio/temperature/mcp9600.c | 147 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 147 insertions(+)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index aa42c2b1a369edbd36e0d6d6d1738ed0069fd990..d3309e30628ae5cdc74378403952ba285990f4c0 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		0x05
> >  #define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
> > +#define MCP9600_FILTER_MASK		GENMASK(2, 0)
> >  #define MCP9600_ALERT_CFG1		0x08
> >  #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
> >  #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> > @@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
> >  	[THERMOCOUPLE_TYPE_R] = 'R',
> >  };
> >  
> > +enum mcp9600_filter {
> > +	MCP9600_FILTER_TYPE_NONE,
> > +	MCP9600_FILTER_TYPE_EMA,
> > +};
> > +
> > +static const char * const mcp9600_filter_type[] = {
> > +	[MCP9600_FILTER_TYPE_NONE] = "none",
> > +	[MCP9600_FILTER_TYPE_EMA] = "ema",
> > +};
> > +
> > +static const int mcp_iir_coefficients_avail[7][2] = {
> > +	/* Level 0 is no filter */
> > +	{ 0, 524549 },
> > +	{ 0, 243901 },
> > +	{ 0, 119994 },
> > +	{ 0,  59761 },
> > +	{ 0,  29851 },
> > +	{ 0,  14922 },
> > +	{ 0,   7461 },
> > +};
> > +
> >  static const struct iio_event_spec mcp9600_events[] = {
> >  	{
> >  		.type = IIO_EV_TYPE_THRESH,
> > @@ -119,6 +141,8 @@ struct mcp_chip_info {
> >  struct mcp9600_data {
> >  	struct i2c_client *client;
> >  	u32 thermocouple_type;
> > +	int filter_level;
> > +	int filter_enabled;
> >  };
> >  
> >  static int mcp9600_config(struct mcp9600_data *data)
> > @@ -129,6 +153,9 @@ static int mcp9600_config(struct mcp9600_data *data)
> >  
> >  	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
> >  			  mcp9600_type_map[data->thermocouple_type]);
> > +	/* The chip understands 0 as "none", and 1-7 as ema filter levels. */
> > +	if (data->filter_enabled)
> > +		FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level + 1);
> >  
> >  	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
> >  	if (ret < 0) {
> > @@ -146,7 +173,11 @@ static int mcp9600_config(struct mcp9600_data *data)
> >  			.address = MCP9600_HOT_JUNCTION,		       \
> >  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
> >  					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> > +					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> 
> Does the filter actually only apply to the hot junction and not the
> cold junction? There is a mismatch between this being info_mask_separate
> and the filter_type being IIO_SHARED_BY_ALL.
> 
> Not related to this patch, but the comment same applies to
> IIO_CHAN_INFO_THERMOCOUPLE_TYPE - I missed that in previous reviews.

The Thermocouple Sensor Configuration register only applies to the
thermocouple (hot-junction), which is what sets the filter and
thermocouple configuration.

More specifically, the filter formula applies to T-delta (thermocouple
hot-junction).

So these are correct.

> >  					      BIT(IIO_CHAN_INFO_SCALE),	       \
> > +			.info_mask_separate_available =                        \
> > +					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
> > +			.ext_info = mcp9600_ext_filter,			       \
> >  			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
> >  			.num_event_specs = hj_num_ev,			       \
> >  		},							       \
> > @@ -162,6 +193,57 @@ static int mcp9600_config(struct mcp9600_data *data)
> >  		},							       \
> >  	}
> >  
> > +static int mcp9600_get_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +
> > +	return data->filter_enabled ? MCP9600_FILTER_TYPE_EMA :
> > +		MCP9600_FILTER_TYPE_NONE;
> > +}
> > +
> > +static int mcp9600_set_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      unsigned int mode)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	int new_type;
> 
> This variable name is a little confusing. It looks like it should
> rather be:
> 
> 	bool new_filter_enabled;

I can change that.

> > +
> > +	switch (mode) {
> > +	case MCP9600_FILTER_TYPE_NONE:
> > +		new_type = 0;
> > +		break;
> > +
> > +	case MCP9600_FILTER_TYPE_EMA:
> > +		new_type = 1;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Do not reset the filter if we don't need to. */
> > +	if (data->filter_enabled == new_type)
> > +		return 0;
> > +
> > +	data->filter_enabled = new_type;
> > +	return mcp9600_config(data);
> > +}
> > +
> > +static const struct iio_enum mcp9600_filter_enum = {
> > +	.items = mcp9600_filter_type,
> > +	.num_items = ARRAY_SIZE(mcp9600_filter_type),
> > +	.get = mcp9600_get_filter,
> > +	.set = mcp9600_set_filter,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info mcp9600_ext_filter[] = {
> > +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &mcp9600_filter_enum),
> > +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> > +			   &mcp9600_filter_enum),
> > +	{ }
> > +};
> > +

I guess I need to make this IIO_SEPARATE/IIO_SHARED_BY_TYPE, but I need
to make sure it's only for the in_temp_raw and not in_temp_ambient_raw.

> >  static const struct iio_chan_spec mcp9600_channels[][2] = {
> >  	MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
> >  	MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
> > @@ -216,6 +298,69 @@ 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:
> > +		if (!data->filter_enabled)
> > +			return IIO_VAL_EMPTY;
> 
> This brings us back to the earlier discussion of what should this
> be when the filter is disabled. Mathematically, it would be infinite.
> 
> I wonder if it would be reasonable to return "inf" here since many
> floating point parsers will already handle that as a valid value.

I'll defer to the other thread for further discussion on this.

-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

[PATCH v7 5/5] iio: ad4080: Rework filter_type "none" logic

[Ben Collins]

The filter_type logic for "none" needed to be reworked to be more
general.

As documented, return IIO_VAL_EMPTY for sampling rates in "none" type
and EINVAL when there's an attempt to write a rate for "none" type.

Signed-off-by: Ben Collins <bcollins@kernel.org>
---
 drivers/iio/adc/ad4080.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
index 6e61787ed3213fe4332bd92b938a7a717dada99f..c7408b9703731ee5d4229a85ffa91ea64b233cd9 100644
--- a/drivers/iio/adc/ad4080.c
+++ b/drivers/iio/adc/ad4080.c
@@ -154,8 +154,6 @@ static const int ad4080_dec_rate_avail[] = {
 	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024,
 };
 
-static const int ad4080_dec_rate_none[] = { 1 };
-
 static const char * const ad4080_power_supplies[] = {
 	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
 };
@@ -268,13 +266,13 @@ static int ad4080_read_raw(struct iio_dev *indio_dev,
 			*val = st->clk_rate;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		if (st->filter_type == FILTER_NONE) {
-			*val = 1;
-		} else {
-			*val = ad4080_get_dec_rate(indio_dev, chan);
-			if (*val < 0)
-				return *val;
-		}
+		if (st->filter_type == FILTER_NONE)
+			return IIO_VAL_EMPTY;
+
+		*val = ad4080_get_dec_rate(indio_dev, chan);
+		if (*val < 0)
+			return *val;
+
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -289,7 +287,7 @@ static int ad4080_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		if (st->filter_type == FILTER_NONE && val > 1)
+		if (st->filter_type == FILTER_NONE)
 			return -EINVAL;
 
 		return ad4080_set_dec_rate(indio_dev, chan, val);
@@ -376,17 +374,16 @@ static int ad4080_read_avail(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (st->filter_type) {
 		case FILTER_NONE:
-			*vals = ad4080_dec_rate_none;
-			*length = ARRAY_SIZE(ad4080_dec_rate_none);
+			*type = IIO_VAL_EMPTY;
 			break;
 		default:
 			*vals = ad4080_dec_rate_avail;
 			*length = st->filter_type >= SINC_5 ?
 				  (ARRAY_SIZE(ad4080_dec_rate_avail) - 2) :
 				  ARRAY_SIZE(ad4080_dec_rate_avail);
+			*type = IIO_VAL_INT;
 			break;
 		}
-		*type = IIO_VAL_INT;
 		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;

-- 
2.39.5

Re: [PATCH v7 5/5] iio: ad4080: Rework filter_type "none" logic

[David Lechner]

On 8/25/25 7:10 PM, Ben Collins wrote:
> The filter_type logic for "none" needed to be reworked to be more
> general.
> 
> As documented, return IIO_VAL_EMPTY for sampling rates in "none" type
> and EINVAL when there's an attempt to write a rate for "none" type.

This patch breaks usespace, which is something we always must avoid.

> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> ---
>  drivers/iio/adc/ad4080.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> index 6e61787ed3213fe4332bd92b938a7a717dada99f..c7408b9703731ee5d4229a85ffa91ea64b233cd9 100644
> --- a/drivers/iio/adc/ad4080.c
> +++ b/drivers/iio/adc/ad4080.c
> @@ -154,8 +154,6 @@ static const int ad4080_dec_rate_avail[] = {
>  	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024,
>  };
>  
> -static const int ad4080_dec_rate_none[] = { 1 };
> -
>  static const char * const ad4080_power_supplies[] = {
>  	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
>  };
> @@ -268,13 +266,13 @@ static int ad4080_read_raw(struct iio_dev *indio_dev,
>  			*val = st->clk_rate;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		if (st->filter_type == FILTER_NONE) {
> -			*val = 1;
> -		} else {
> -			*val = ad4080_get_dec_rate(indio_dev, chan);
> -			if (*val < 0)
> -				return *val;
> -		}
> +		if (st->filter_type == FILTER_NONE)
> +			return IIO_VAL_EMPTY;
> +
> +		*val = ad4080_get_dec_rate(indio_dev, chan);
> +		if (*val < 0)
> +			return *val;
> +
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -289,7 +287,7 @@ static int ad4080_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		if (st->filter_type == FILTER_NONE && val > 1)
> +		if (st->filter_type == FILTER_NONE)
>  			return -EINVAL;
>  
>  		return ad4080_set_dec_rate(indio_dev, chan, val);
> @@ -376,17 +374,16 @@ static int ad4080_read_avail(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		switch (st->filter_type) {
>  		case FILTER_NONE:
> -			*vals = ad4080_dec_rate_none;
> -			*length = ARRAY_SIZE(ad4080_dec_rate_none);
> +			*type = IIO_VAL_EMPTY;
>  			break;
>  		default:
>  			*vals = ad4080_dec_rate_avail;
>  			*length = st->filter_type >= SINC_5 ?
>  				  (ARRAY_SIZE(ad4080_dec_rate_avail) - 2) :
>  				  ARRAY_SIZE(ad4080_dec_rate_avail);
> +			*type = IIO_VAL_INT;
>  			break;
>  		}
> -		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;
> 

Returning a value of 1 for the oversampling ratio when there is no
oversampling going on is perfectly reasonable and mathematically correct.
So I don't consider this change an improvement.

Re: [PATCH v7 5/5] iio: ad4080: Rework filter_type "none" logic

[Ben Collins]

On Tue, Aug 26, 2025 at 11:51:56AM -0500, David Lechner wrote:
> On 8/25/25 7:10 PM, Ben Collins wrote:
> > The filter_type logic for "none" needed to be reworked to be more
> > general.
> > 
> > As documented, return IIO_VAL_EMPTY for sampling rates in "none" type
> > and EINVAL when there's an attempt to write a rate for "none" type.
> 
> This patch breaks usespace, which is something we always must avoid.

I was under the impression there was a need to make the use of
filter_type "none" more consistent.

I don't disagree with not breaking userspace, but it does create
ambiguity for other implementations.

> > 
> > Signed-off-by: Ben Collins <bcollins@kernel.org>
> > ---
> >  drivers/iio/adc/ad4080.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> > index 6e61787ed3213fe4332bd92b938a7a717dada99f..c7408b9703731ee5d4229a85ffa91ea64b233cd9 100644
> > --- a/drivers/iio/adc/ad4080.c
> > +++ b/drivers/iio/adc/ad4080.c
> > @@ -154,8 +154,6 @@ static const int ad4080_dec_rate_avail[] = {
> >  	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024,
> >  };
> >  
> > -static const int ad4080_dec_rate_none[] = { 1 };
> > -
> >  static const char * const ad4080_power_supplies[] = {
> >  	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
> >  };
> > @@ -268,13 +266,13 @@ static int ad4080_read_raw(struct iio_dev *indio_dev,
> >  			*val = st->clk_rate;
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		if (st->filter_type == FILTER_NONE) {
> > -			*val = 1;
> > -		} else {
> > -			*val = ad4080_get_dec_rate(indio_dev, chan);
> > -			if (*val < 0)
> > -				return *val;
> > -		}
> > +		if (st->filter_type == FILTER_NONE)
> > +			return IIO_VAL_EMPTY;
> > +
> > +		*val = ad4080_get_dec_rate(indio_dev, chan);
> > +		if (*val < 0)
> > +			return *val;
> > +
> >  		return IIO_VAL_INT;
> >  	default:
> >  		return -EINVAL;
> > @@ -289,7 +287,7 @@ static int ad4080_write_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		if (st->filter_type == FILTER_NONE && val > 1)
> > +		if (st->filter_type == FILTER_NONE)
> >  			return -EINVAL;
> >  
> >  		return ad4080_set_dec_rate(indio_dev, chan, val);
> > @@ -376,17 +374,16 @@ static int ad4080_read_avail(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >  		switch (st->filter_type) {
> >  		case FILTER_NONE:
> > -			*vals = ad4080_dec_rate_none;
> > -			*length = ARRAY_SIZE(ad4080_dec_rate_none);
> > +			*type = IIO_VAL_EMPTY;
> >  			break;
> >  		default:
> >  			*vals = ad4080_dec_rate_avail;
> >  			*length = st->filter_type >= SINC_5 ?
> >  				  (ARRAY_SIZE(ad4080_dec_rate_avail) - 2) :
> >  				  ARRAY_SIZE(ad4080_dec_rate_avail);
> > +			*type = IIO_VAL_INT;
> >  			break;
> >  		}
> > -		*type = IIO_VAL_INT;
> >  		return IIO_AVAIL_LIST;
> >  	default:
> >  		return -EINVAL;
> > 
> 
> Returning a value of 1 for the oversampling ratio when there is no
> oversampling going on is perfectly reasonable and mathematically correct.
> So I don't consider this change an improvement.

-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

Re: [PATCH v7 5/5] iio: ad4080: Rework filter_type "none" logic

[Jonathan Cameron]

On Tue, 26 Aug 2025 15:11:14 -0400
Ben Collins <bcollins@kernel.org> wrote:

> On Tue, Aug 26, 2025 at 11:51:56AM -0500, David Lechner wrote:
> > On 8/25/25 7:10 PM, Ben Collins wrote:  
> > > The filter_type logic for "none" needed to be reworked to be more
> > > general.
> > > 
> > > As documented, return IIO_VAL_EMPTY for sampling rates in "none" type
> > > and EINVAL when there's an attempt to write a rate for "none" type.  
> > 
> > This patch breaks usespace, which is something we always must avoid.  
> 
> I was under the impression there was a need to make the use of
> filter_type "none" more consistent.
> 
> I don't disagree with not breaking userspace, but it does create
> ambiguity for other implementations.

For oversampling the value of 1 has long been used for
'not oversampling'. So I'm not seeing an ambiguity there.


Jonathan


> 
> > > 
> > > Signed-off-by: Ben Collins <bcollins@kernel.org>
> > > ---
> > >  drivers/iio/adc/ad4080.c | 23 ++++++++++-------------
> > >  1 file changed, 10 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> > > index 6e61787ed3213fe4332bd92b938a7a717dada99f..c7408b9703731ee5d4229a85ffa91ea64b233cd9 100644
> > > --- a/drivers/iio/adc/ad4080.c
> > > +++ b/drivers/iio/adc/ad4080.c
> > > @@ -154,8 +154,6 @@ static const int ad4080_dec_rate_avail[] = {
> > >  	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024,
> > >  };
> > >  
> > > -static const int ad4080_dec_rate_none[] = { 1 };
> > > -
> > >  static const char * const ad4080_power_supplies[] = {
> > >  	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
> > >  };
> > > @@ -268,13 +266,13 @@ static int ad4080_read_raw(struct iio_dev *indio_dev,
> > >  			*val = st->clk_rate;
> > >  		return IIO_VAL_INT;
> > >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > > -		if (st->filter_type == FILTER_NONE) {
> > > -			*val = 1;
> > > -		} else {
> > > -			*val = ad4080_get_dec_rate(indio_dev, chan);
> > > -			if (*val < 0)
> > > -				return *val;
> > > -		}
> > > +		if (st->filter_type == FILTER_NONE)
> > > +			return IIO_VAL_EMPTY;
> > > +
> > > +		*val = ad4080_get_dec_rate(indio_dev, chan);
> > > +		if (*val < 0)
> > > +			return *val;
> > > +
> > >  		return IIO_VAL_INT;
> > >  	default:
> > >  		return -EINVAL;
> > > @@ -289,7 +287,7 @@ static int ad4080_write_raw(struct iio_dev *indio_dev,
> > >  
> > >  	switch (mask) {
> > >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > > -		if (st->filter_type == FILTER_NONE && val > 1)
> > > +		if (st->filter_type == FILTER_NONE)
> > >  			return -EINVAL;
> > >  
> > >  		return ad4080_set_dec_rate(indio_dev, chan, val);
> > > @@ -376,17 +374,16 @@ static int ad4080_read_avail(struct iio_dev *indio_dev,
> > >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > >  		switch (st->filter_type) {
> > >  		case FILTER_NONE:
> > > -			*vals = ad4080_dec_rate_none;
> > > -			*length = ARRAY_SIZE(ad4080_dec_rate_none);
> > > +			*type = IIO_VAL_EMPTY;
> > >  			break;
> > >  		default:
> > >  			*vals = ad4080_dec_rate_avail;
> > >  			*length = st->filter_type >= SINC_5 ?
> > >  				  (ARRAY_SIZE(ad4080_dec_rate_avail) - 2) :
> > >  				  ARRAY_SIZE(ad4080_dec_rate_avail);
> > > +			*type = IIO_VAL_INT;
> > >  			break;
> > >  		}
> > > -		*type = IIO_VAL_INT;
> > >  		return IIO_AVAIL_LIST;
> > >  	default:
> > >  		return -EINVAL;
> > >   
> > 
> > Returning a value of 1 for the oversampling ratio when there is no
> > oversampling going on is perfectly reasonable and mathematically correct.
> > So I don't consider this change an improvement.  
> 

Re: [PATCH v7 0/5] iio: mcp9600: Implement IIR feature and add iio core support

[Andy Shevchenko]

On Tue, Aug 26, 2025 at 3:10 AM Ben Collins <bcollins@kernel.org> wrote:
>
> ChangeLog:
> v6 -> v7:
>   - Remove extra space before trailing \
>   - Don't add extra white-space
>   - Remove mcp9600_write_raw_get_fmt
>   - Separate out the mcp9600 IIR series into its own series as there is
>     a lot of conversation around implementation.
>   - Add rework of ad4080 to match filter_type "none" logic

Wasn't v6 already applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 0/5] iio: mcp9600: Implement IIR feature and add iio core support

[Ben Collins]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Tue, Aug 26, 2025 at 11:14:58AM -0500, Andy Shevchenko wrote:
> On Tue, Aug 26, 2025 at 3:10 AM Ben Collins <bcollins@kernel.org> wrote:
> >
> > ChangeLog:
> > v6 -> v7:
> >   - Remove extra space before trailing \
> >   - Don't add extra white-space
> >   - Remove mcp9600_write_raw_get_fmt
> >   - Separate out the mcp9600 IIR series into its own series as there is
> >     a lot of conversation around implementation.
> >   - Add rework of ad4080 to match filter_type "none" logic
> 
> Wasn't v6 already applied?

v8 of the mcp9600 feature patch series was. This is the IIR series, of
which, none of it has been accepted, yet.

-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]