[PATCH v2 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[PATCH v2 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[Ben Collins]

MCP9601 is a superset of MCP9600 and is supported by the driver.

Signed-off-by: Ben Collins <bcollins@watter.com>
---
 .../bindings/iio/temperature/microchip,mcp9600.yaml         | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
index d2cafa38a5442..d8af0912ce886 100644
--- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Microchip MCP9600 thermocouple EMF converter
+title: Microchip MCP9600 and similar thermocouple EMF converters
 
 maintainers:
   - Andrew Hepp <andrew.hepp@ahepp.dev>
@@ -14,7 +14,9 @@ description:
 
 properties:
   compatible:
-    const: microchip,mcp9600
+    enum:
+      - microchip,mcp9600
+      - microchip,mcp9601
 
   reg:
     maxItems: 1
-- 
2.50.1

Re: [PATCH v2 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[Krzysztof Kozlowski]

On 13/08/2025 17:15, Ben Collins wrote:
> MCP9601 is a superset of MCP9600 and is supported by the driver.

There is no driver patch, so I cannot verify last part - about driver -
and I don't understand it. If it is already supported, doesn't it make
this patch redundant? Or did you wanted to say you are documenting
compatible being already used?


> 
> Signed-off-by: Ben Collins <bcollins@watter.com>


Where is the changelog? That's a v2.

No cover letter either...

> ---
>  .../bindings/iio/temperature/microchip,mcp9600.yaml         | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> index d2cafa38a5442..d8af0912ce886 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Microchip MCP9600 thermocouple EMF converter
> +title: Microchip MCP9600 and similar thermocouple EMF converters
>  
>  maintainers:
>    - Andrew Hepp <andrew.hepp@ahepp.dev>
> @@ -14,7 +14,9 @@ description:
>  
>  properties:
>    compatible:
> -    const: microchip,mcp9600
> +    enum:
> +      - microchip,mcp9600
> +      - microchip,mcp9601

If it is superset, why isn't this expressed with compatibility and fallback?

>  
>    reg:
>      maxItems: 1


Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[David Lechner]

On 8/13/25 10:15 AM, Ben Collins wrote:
> MCP9601 is a superset of MCP9600 and is supported by the driver.
> 
> Signed-off-by: Ben Collins <bcollins@watter.com>
> ---


Please include a cover letter with a changelog in v3.


>  .../bindings/iio/temperature/microchip,mcp9600.yaml         | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> index d2cafa38a5442..d8af0912ce886 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Microchip MCP9600 thermocouple EMF converter
> +title: Microchip MCP9600 and similar thermocouple EMF converters
>  
>  maintainers:
>    - Andrew Hepp <andrew.hepp@ahepp.dev>
> @@ -14,7 +14,9 @@ description:
>  
>  properties:
>    compatible:
> -    const: microchip,mcp9600
> +    enum:
> +      - microchip,mcp9600
> +      - microchip,mcp9601
>  

It sounds like it would be useful to have a fallback in this case:

properties:
  compatible:
    oneOf:
      - const: microchip,mcp9600
      - items:
          - - microchip,mcp9600
          - microchip,mcp9600

>    reg:
>      maxItems: 1

Usage would then be:

	compatible = "microchip,mcp9601", "microchip,mcp9600";

Re: [PATCH v2 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[David Lechner]

On 8/13/25 7:04 PM, Ben Collins wrote:
> On Wed, Aug 13, 2025 at 04:11:59PM -0500, David Lechner wrote:
>> On 8/13/25 10:15 AM, Ben Collins wrote:
>>> MCP9601 is a superset of MCP9600 and is supported by the driver.
>>>
>>> Signed-off-by: Ben Collins <bcollins@watter.com>
>>> ---


Looks like reply-all was missed on this one and it ended up in my spam,
so I'm just now seeing the reply for the first time. Adding back the
others since I don't think that was intentional.

>>
>>
>> Please include a cover letter with a changelog in v3.
> 
> I had one, but I'm not sure why it didn't get Cc'd around. I'll check on
> that in the v3.
> 
>>>  .../bindings/iio/temperature/microchip,mcp9600.yaml         | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
>>> index d2cafa38a5442..d8af0912ce886 100644
>>> --- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
>>> @@ -4,7 +4,7 @@
>>>  $id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml#
>>>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>  
>>> -title: Microchip MCP9600 thermocouple EMF converter
>>> +title: Microchip MCP9600 and similar thermocouple EMF converters
>>>  
>>>  maintainers:
>>>    - Andrew Hepp <andrew.hepp@ahepp.dev>
>>> @@ -14,7 +14,9 @@ description:
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: microchip,mcp9600
>>> +    enum:
>>> +      - microchip,mcp9600
>>> +      - microchip,mcp9601
>>>  
>>
>> It sounds like it would be useful to have a fallback in this case:
>>
>> properties:
>>   compatible:
>>     oneOf:
>>       - const: microchip,mcp9600
>>       - items:
>>           - - microchip,mcp9600
>>           - microchip,mcp9600
>>
>>>    reg:
>>>      maxItems: 1
>>
>> Usage would then be:
>>
>> 	compatible = "microchip,mcp9601", "microchip,mcp9600";
> 
> The main reason for the compatible is so I can designate the difference
> between 9600 and 9601 for the next patch I am working on which supports
> open-circuit and short-circuit detection. This is a feature in the 9601
> variant.
> 
> The feature depends on the chip being wired to support it, which means
> there will need to be a way to let the driver know that reading
> the OC and SC register bits will produce useful information. I'm leaning
> toward device-tree props to enable this and limiting that for only when
> the driver is told it should assume a 9601.
> 
> Given this info, what seems like the best approach here?
> 
> Thanks
> 


The devicetree already has open-circuit and short-circuit interrupts
for the corresponding pins. So it looks like the binding was written
for both chips already.

If those aren't wired up, falling back to reading registers to get
the status is fine.

I also see there is a V_SENSE pin. So I think it would make sense
to add a microchip,vsense boolean property ($ref: /schemas/types.yaml#/definitions/flag)
that indicates that the V_SENSE pin is wired up.

The driver could then use that to know if it can actually provide
events short/open circuit events or not.

[PATCH v2 2/5] iio: mcp9600: White space cleanup for tab alignment

[Ben Collins]

Purely to align tabs for #defines.

Signed-off-by: Ben Collins <bcollins@watter.com>
---
 drivers/iio/temperature/mcp9600.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 6e9108d5cf75f..d53703c089031 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -23,8 +23,8 @@
 #include <linux/iio/iio.h>
 
 /* MCP9600 registers */
-#define MCP9600_HOT_JUNCTION 0x0
-#define MCP9600_COLD_JUNCTION 0x2
+#define MCP9600_HOT_JUNCTION		0x0
+#define MCP9600_COLD_JUNCTION		0x2
 #define MCP9600_STATUS			0x4
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
 #define MCP9600_ALERT_CFG1		0x8
@@ -38,10 +38,10 @@
 #define MCP9600_ALERT_LIMIT1		0x10
 #define MCP9600_ALERT_LIMIT(x)		(MCP9600_ALERT_LIMIT1 + (x - 1))
 #define MCP9600_ALERT_LIMIT_MASK	GENMASK(15, 2)
-#define MCP9600_DEVICE_ID 0x20
+#define MCP9600_DEVICE_ID		0x20
 
 /* MCP9600 device id value */
-#define MCP9600_DEVICE_ID_MCP9600 0x40
+#define MCP9600_DEVICE_ID_MCP9600	0x40
 
 #define MCP9600_ALERT_COUNT		4
 
-- 
2.50.1

Re: [PATCH v2 2/5] iio: mcp9600: White space cleanup for tab alignment

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 5:17 PM Ben Collins <bcollins@watter.com> wrote:
>
> Purely to align tabs for #defines.

...

> +#define MCP9600_HOT_JUNCTION           0x0
> +#define MCP9600_COLD_JUNCTION          0x2
>  #define MCP9600_STATUS                 0x4

>  #define MCP9600_ALERT_CFG1             0x8

I would also suggest making the register offsets to be fixed-width, e.g.
0x04 for STATUS. And since these are the most lines you are already
touching in this patch, repurpose it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/5] iio: mcp9600: Add compatibility for mcp9601

[Ben Collins]

MCP9601 is a super set of MCP9600. The drivers works without changes
on this chipset.

Signed-off-by: Ben Collins <bcollins@watter.com>
---
 drivers/iio/temperature/Kconfig   |  6 +++---
 drivers/iio/temperature/mcp9600.c | 31 +++++++++++++++++++++++--------
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d504..2a5d04b1d1c87 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -173,11 +173,11 @@ config MAX31865
 	  will be called max31865.
 
 config MCP9600
-	tristate "MCP9600 thermocouple EMF converter"
+	tristate "MCP9600 and similar thermocouple EMF converters"
 	depends on I2C
 	help
-	  If you say yes here you get support for MCP9600
-	  thermocouple EMF converter connected via I2C.
+	  If you say yes here you get support for MCP9600, MCP9601, and
+	  similar thermocouple EMF converters connected via I2C.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called mcp9600.
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index d53703c089031..104bc83de6da7 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -42,6 +42,7 @@
 
 /* MCP9600 device id value */
 #define MCP9600_DEVICE_ID_MCP9600	0x40
+#define MCP9600_DEVICE_ID_MCP9601	0x41
 
 #define MCP9600_ALERT_COUNT		4
 
@@ -418,14 +419,26 @@ static int mcp9600_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
 	struct mcp9600_data *data;
-	int ret, ch_sel;
+	int ret, ch_sel, dev_id;
+
+	dev_id = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
+	if (dev_id < 0)
+		return dev_err_probe(&client->dev, dev_id,
+				     "Failed to read device ID\n");
+
+	switch (dev_id) {
+	case MCP9600_DEVICE_ID_MCP9600:
+	case MCP9600_DEVICE_ID_MCP9601:
+		if (dev_id != id->driver_data)
+			dev_warn(&client->dev,
+				 "Expected id %x but detected %x. Ensure dt is correct\n",
+				 (u8)id->driver_data, (u8)dev_id);
+		break;
 
-	ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
-	if (ret < 0)
-		return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
-	if (ret != MCP9600_DEVICE_ID_MCP9600)
-		dev_warn(&client->dev, "Expected ID %x, got %x\n",
-				MCP9600_DEVICE_ID_MCP9600, ret);
+	default:
+		dev_warn(&client->dev, "Unknown id %x, using %x\n", (u8)dev_id,
+			(u8)id->driver_data);
+	}
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -448,13 +461,15 @@ static int mcp9600_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id mcp9600_id[] = {
-	{ "mcp9600" },
+	{ "mcp9600", MCP9600_DEVICE_ID_MCP9600 },
+	{ "mcp9601", MCP9600_DEVICE_ID_MCP9601 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, mcp9600_id);
 
 static const struct of_device_id mcp9600_of_match[] = {
 	{ .compatible = "microchip,mcp9600" },
+	{ .compatible = "microchip,mcp9601" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mcp9600_of_match);
-- 
2.50.1

Re: [PATCH v2 3/5] iio: mcp9600: Add compatibility for mcp9601

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 5:17 PM Ben Collins <bcollins@watter.com> wrote:
>
> MCP9601 is a super set of MCP9600. The drivers works without changes

superset

> on this chipset.

...

>  config MCP9600
> -       tristate "MCP9600 thermocouple EMF converter"
> +       tristate "MCP9600 and similar thermocouple EMF converters"
>         depends on I2C
>         help
> -         If you say yes here you get support for MCP9600
> -         thermocouple EMF converter connected via I2C.

> +         If you say yes here you get support for MCP9600, MCP9601, and
> +         similar thermocouple EMF converters connected via I2C.

To avoid a potential churn in the further changes to support a new HW,
I would suggest to convert this to the list of supported chips:

  ...get support for:
  - MCP9600
  - MCP9601
  and similar...

>           This driver can also be built as a module. If so, the module
>           will be called mcp9600.

...

> +       switch (dev_id) {
> +       case MCP9600_DEVICE_ID_MCP9600:
> +       case MCP9600_DEVICE_ID_MCP9601:
> +               if (dev_id != id->driver_data)

I prefer to see this to be converted to use chip_info before getting
to a new HW support.

> +                       dev_warn(&client->dev,
> +                                "Expected id %x but detected %x. Ensure dt is correct\n",

dt --> firmware description
(the world is not rotating around DT only)

> +                                (u8)id->driver_data, (u8)dev_id);

Use proper specifiers and drop castings.

> +               break;
>

> +       default:
> +               dev_warn(&client->dev, "Unknown id %x, using %x\n", (u8)dev_id,
> +                       (u8)id->driver_data);

Ditto.

> +       }

...

> +       { "mcp9600", MCP9600_DEVICE_ID_MCP9600 },
> +       { "mcp9601", MCP9600_DEVICE_ID_MCP9601 },

Nope, use chip_info from day 1, please.

...

>  static const struct of_device_id mcp9600_of_match[] = {
>         { .compatible = "microchip,mcp9600" },
> +       { .compatible = "microchip,mcp9601" },

Missed driver data.

>         { }
>  };

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/5] iio: mcp9600: Add support for dtbinding of thermocouple-type

[Ben Collins]

Adds dtbinding check for thermocouple-type and sets sensor config
to match. Add iio info attribute to show state as well.

Signed-off-by: Ben Collins <bcollins@watter.com>
---
 drivers/iio/temperature/mcp9600.c | 61 ++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 104bc83de6da7..5ead565f1bd8c 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -22,11 +22,15 @@
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 
+#include <dt-bindings/iio/temperature/thermocouple.h>
+
 /* MCP9600 registers */
 #define MCP9600_HOT_JUNCTION		0x0
 #define MCP9600_COLD_JUNCTION		0x2
 #define MCP9600_STATUS			0x4
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
+#define MCP9600_SENSOR_CFG		0x5
+#define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
 #define MCP9600_ALERT_CFG1		0x8
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
@@ -66,6 +70,23 @@ static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
 	[MCP9600_ALERT4] = "alert4",
 };
 
+/* Map dtbinding enums to mcp9600 sensor type */
+static const unsigned int mcp9600_type_map[] = {
+	[THERMOCOUPLE_TYPE_K] = 0,
+	[THERMOCOUPLE_TYPE_J] = 1,
+	[THERMOCOUPLE_TYPE_T] = 2,
+	[THERMOCOUPLE_TYPE_N] = 3,
+	[THERMOCOUPLE_TYPE_S] = 4,
+	[THERMOCOUPLE_TYPE_E] = 5,
+	[THERMOCOUPLE_TYPE_B] = 6,
+	[THERMOCOUPLE_TYPE_R] = 7,
+};
+
+/* Map mcp9600 sensor type to char */
+static const int mcp9600_tc_types[] = {
+	'B', 'E', 'J', 'K', 'N', 'R', 'S', 'T'
+};
+
 static const struct iio_event_spec mcp9600_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -89,7 +110,8 @@ static const struct iio_event_spec mcp9600_events[] = {
 			.type = IIO_TEMP,				       \
 			.address = MCP9600_HOT_JUNCTION,		       \
 			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
-					      BIT(IIO_CHAN_INFO_SCALE),	       \
+					      BIT(IIO_CHAN_INFO_SCALE) |       \
+					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE), \
 			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
 			.num_event_specs = hj_num_ev,			       \
 		},							       \
@@ -126,6 +148,7 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	u32 thermocouple_type;
 };
 
 static int mcp9600_read(struct mcp9600_data *data,
@@ -160,11 +183,32 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
+		*val = mcp9600_tc_types[data->thermocouple_type];
+		return IIO_VAL_CHAR;
+
 	default:
 		return -EINVAL;
 	}
 }
 
+static int mcp9600_config(struct mcp9600_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret, cfg;
+
+	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
+			  mcp9600_type_map[data->thermocouple_type]);
+
+	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to set sensor configuration\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
 {
 	if (channel2 == IIO_MOD_TEMP_AMBIENT) {
@@ -417,6 +461,7 @@ static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
 
 static int mcp9600_probe(struct i2c_client *client)
 {
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	struct iio_dev *indio_dev;
 	struct mcp9600_data *data;
 	int ret, ch_sel, dev_id;
@@ -447,6 +492,20 @@ static int mcp9600_probe(struct i2c_client *client)
 	data = iio_priv(indio_dev);
 	data->client = client;
 
+	/* Accept type from dt with default of Type-K. */
+	data->thermocouple_type = THERMOCOUPLE_TYPE_K;
+	ret = device_property_read_u32(&client->dev, "thermocouple-type",
+				       &data->thermocouple_type);
+	if (data->thermocouple_type >= ARRAY_SIZE(mcp9600_type_map))
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Invalid thermocouple-type property %d.\n",
+				     data->thermocouple_type);
+
+	/* Set initial config. */
+	ret = mcp9600_config(data);
+	if (ret < 0)
+		return ret;
+
 	ch_sel = mcp9600_probe_alerts(indio_dev);
 	if (ch_sel < 0)
 		return ch_sel;
-- 
2.50.1

Re: [PATCH v2 4/5] iio: mcp9600: Add support for dtbinding of thermocouple-type

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 5:17 PM Ben Collins <bcollins@watter.com> wrote:
>
> Adds dtbinding check for thermocouple-type and sets sensor config

Adds --> Add

Btw, I do not see dtbinding check here. There is some validation code
or so. Please, make sure your commit message is close enough to what's
going on.

> to match. Add iio info attribute to show state as well.

...

> +#define MCP9600_SENSOR_CFG             0x5

0x05

> +#define MCP9600_SENSOR_TYPE_MASK       GENMASK(6, 4)

...

> +/* Map dtbinding enums to mcp9600 sensor type */

I am not sure how this comment is useful, but since it does some
magic, this magic should be explained: why do you need to map these?

> +static const unsigned int mcp9600_type_map[] = {
> +       [THERMOCOUPLE_TYPE_K] = 0,
> +       [THERMOCOUPLE_TYPE_J] = 1,
> +       [THERMOCOUPLE_TYPE_T] = 2,
> +       [THERMOCOUPLE_TYPE_N] = 3,
> +       [THERMOCOUPLE_TYPE_S] = 4,
> +       [THERMOCOUPLE_TYPE_E] = 5,
> +       [THERMOCOUPLE_TYPE_B] = 6,
> +       [THERMOCOUPLE_TYPE_R] = 7,
> +};

Are they not 1:1?

...

> +/* Map mcp9600 sensor type to char */
> +static const int mcp9600_tc_types[] = {
> +       'B', 'E', 'J', 'K', 'N', 'R', 'S', 'T'

Please, leave the trailing comma, also why is this not indexed as the above?

> +};

...

>                         .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |         \
> -                                             BIT(IIO_CHAN_INFO_SCALE),        \
> +                                             BIT(IIO_CHAN_INFO_SCALE) |       \
> +                                             BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE), \

Make it only one line +, and *not* remove one / add two.

...

>   static int mcp9600_read_raw(struct iio_dev *indio_dev,

>                 *val = 62;
>                 *val2 = 500000;
>                 return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> +               *val = mcp9600_tc_types[data->thermocouple_type];
> +               return IIO_VAL_CHAR;

> +

Stray blank line (I don't see the above is using it) or the above is
wrong and you need one more blank line. Not enough context to me w/o
looking into the driver code.

>         default:
>                 return -EINVAL;
>         }
>  }

...

> +static int mcp9600_config(struct mcp9600_data *data)
> +{
> +       struct i2c_client *client = data->client;
> +       int ret, cfg;

Why is cfg signed?

> +       cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
> +                         mcp9600_type_map[data->thermocouple_type]);
> +
> +       ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "Failed to set sensor configuration\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}

...

> +       /* Accept type from dt with default of Type-K. */
> +       data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +       ret = device_property_read_u32(&client->dev, "thermocouple-type",
> +                                      &data->thermocouple_type);
> +       if (data->thermocouple_type >= ARRAY_SIZE(mcp9600_type_map))
> +               return dev_err_probe(&client->dev, -EINVAL,
> +                                    "Invalid thermocouple-type property %d.\n",

Please, make sure that all printf() specifiers are aligned with the
types of the respective parameters.

> +                                    data->thermocouple_type);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: mcp9600: Add support for dtbinding of thermocouple-type

[David Lechner]

On 8/13/25 10:15 AM, Ben Collins wrote:
> Adds dtbinding check for thermocouple-type and sets sensor config
> to match. Add iio info attribute to show state as well.
> 
> Signed-off-by: Ben Collins <bcollins@watter.com>
> ---

...

> @@ -447,6 +492,20 @@ static int mcp9600_probe(struct i2c_client *client)
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  
> +	/* Accept type from dt with default of Type-K. */
> +	data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +	ret = device_property_read_u32(&client->dev, "thermocouple-type",
> +				       &data->thermocouple_type);

ret is not checked. We should either check it or drop it and add
a comment explaining why it is OK to ignore the return value.

Typically, for optional properties, we would ignore only -EINVAL
meaning the property is not present and fail on other errors.

We also need another dt-bindings patch to add the default in the
bindings.

> +	if (data->thermocouple_type >= ARRAY_SIZE(mcp9600_type_map))
> +		return dev_err_probe(&client->dev, -EINVAL,
> +				     "Invalid thermocouple-type property %d.\n",
> +				     data->thermocouple_type);
> +
> +	/* Set initial config. */
> +	ret = mcp9600_config(data);
> +	if (ret < 0)
> +		return ret;
> +
>  	ch_sel = mcp9600_probe_alerts(indio_dev);
>  	if (ch_sel < 0)
>  		return ch_sel;

[PATCH v2 5/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.

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;
+		return IIO_VAL_INT;
 
 	default:
 		return -EINVAL;
@@ -199,6 +205,7 @@ static int mcp9600_config(struct mcp9600_data *data)
 
 	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
 			  mcp9600_type_map[data->thermocouple_type]);
+	cfg |= FIELD_PREP(MCP9600_FILTER_MASK, data->filter_level);
 
 	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
 	if (ret < 0) {
@@ -209,6 +216,37 @@ static int mcp9600_config(struct mcp9600_data *data)
 	return 0;
 }
 
+static int mcp9600_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return IIO_VAL_INT;
+	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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		if (val < 0 || val > 7)
+			return -EINVAL;
+
+		data->filter_level = val;
+		return mcp9600_config(data);
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
 {
 	if (channel2 == IIO_MOD_TEMP_AMBIENT) {
@@ -346,6 +384,8 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
 
 static const struct iio_info mcp9600_info = {
 	.read_raw = mcp9600_read_raw,
+	.write_raw = mcp9600_write_raw,
+	.write_raw_get_fmt = mcp9600_write_raw_get_fmt,
 	.read_event_config = mcp9600_read_event_config,
 	.write_event_config = mcp9600_write_event_config,
 	.read_event_value = mcp9600_read_thresh,
@@ -501,6 +541,9 @@ static int mcp9600_probe(struct i2c_client *client)
 				     "Invalid thermocouple-type property %d.\n",
 				     data->thermocouple_type);
 
+	/* Default filter level of the chip is 0 (off) */
+	data->filter_level = 0;
+
 	/* Set initial config. */
 	ret = mcp9600_config(data);
 	if (ret < 0)
-- 
2.50.1

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

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 5:17 PM Ben Collins <bcollins@watter.com> wrote:
>
> MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> to allow get/set of this value.
>

...

>         case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
>                 *val = mcp9600_tc_types[data->thermocouple_type];
>                 return IIO_VAL_CHAR;

Again, either here + blank line, or as already mentioned, remove a
stray one below.

> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +               *val = data->filter_level;
> +               return IIO_VAL_INT;
>
>         default:
>                 return -EINVAL;

...

>         cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
>                           mcp9600_type_map[data->thermocouple_type]);
> +       cfg |= FIELD_PREP(MCP9600_FILTER_MASK, data->filter_level);

FIELD_MODIFY() ?

...

> +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);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:

> +               if (val < 0 || val > 7)

in_range() ?
Where 8 will be an ARRAY_SIZE() ?

> +                       return -EINVAL;
> +
> +               data->filter_level = val;
> +               return mcp9600_config(data);

> +

So, make sure these blank lines in the switch-cases are all consistent
all over the code.

> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +       /* Default filter level of the chip is 0 (off) */
> +       data->filter_level = 0;

Why do we need an explicit assignment? If you want to have a comment
(which seems somehow valuable), find a better place for it without
likely unneeded assignment.

-- 
With Best Regards,
Andy Shevchenko

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

[David Lechner]

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?


```python
import numpy as np
import pandas as pd

# Sampling rate (can be replaced with actual sampling rate if known)
fs = 1.0  # Hz

# Define alpha as a function of n
def alpha(n):
    return 2 / (2**n + 1)

# Compute 3 dB frequency for each n from 0 to 7
results = []

for n in range(0, 8):
    a = alpha(n)

    numerator = -(a**2 + 2*a - 2)
    denominator = 2 * (1 - a)
    cos_val = numerator / denominator

    if -1 <= cos_val <= 1:
        omega_3dB = np.arccos(cos_val)  # radians/sample
        f_3dB = (omega_3dB * fs) / (2 * np.pi)  # Hz
    else:
        f_3dB = np.nan  # Outside arccos domain

    results.append({
        "n": n,
        "alpha": round(a, 5),
        "f_3dB (Hz)": None if np.isnan(f_3dB) else round(f_3dB, 5)
    })

# Display the results
df = pd.DataFrame(results)
print(df)

```


Here is the chatgpt transcript for checking the math:
To find the **3 dB point** (i.e., the cutoff frequency) of the given digital low-pass filter, we need to analyze its frequency response. The equation you've provided is a common **first-order IIR low-pass filter**:

$$
Y_k = (1 - n) \cdot Y_{k-1} + n \cdot X_k
$$

Where:

* $Y_k$: current filtered output
* $Y_{k-1}$: previous filtered output
* $X_k$: current input (unfiltered)
* $n$: filter coefficient (0 < $n$ < 1)

---

### Step 1: Transfer Function

We can find the transfer function $H(z)$ using Z-transforms.

Take the Z-transform of both sides:

$$
Y(z) = (1 - n) \cdot z^{-1} Y(z) + n \cdot X(z)
$$

Solve for $H(z) = \frac{Y(z)}{X(z)}$:

$$
Y(z) \left(1 - (1 - n) z^{-1} \right) = n X(z)
$$

$$
H(z) = \frac{Y(z)}{X(z)} = \frac{n}{1 - (1 - n) z^{-1}}
$$

---

### Step 2: Frequency Response

Substitute $z = e^{j\omega}$ to find the frequency response:

$$
H(e^{j\omega}) = \frac{n}{1 - (1 - n) e^{-j\omega}}
$$

We want the magnitude response $|H(e^{j\omega})|$, and the **3 dB point** is defined where the magnitude drops to:

$$
|H(e^{j\omega_{3dB}})| = \frac{1}{\sqrt{2}} \cdot |H(e^{j0})|
$$

At $\omega = 0$:

$$
|H(e^{j0})| = \frac{n}{1 - (1 - n)} = 1
$$

So the 3 dB point occurs when:

$$
|H(e^{j\omega_{3dB}})| = \frac{1}{\sqrt{2}} \approx 0.707
$$

---

### Step 3: Solving Numerically (Approximation)

The magnitude of the frequency response is:

$$
|H(e^{j\omega})| = \frac{n}{\sqrt{1 - 2(1 - n)\cos(\omega) + (1 - n)^2}}
$$

Set this equal to 0.707:

$$
\frac{n}{\sqrt{1 - 2(1 - n)\cos(\omega) + (1 - n)^2}} = 0.707
$$

This is a transcendental equation and cannot be solved algebraically in closed-form. However, you can **solve numerically** for $\omega_{3dB}$ for a given $n$.

---

### Step 4: Approximate Formula for 3 dB Frequency

For small $n$, an **approximate formula** for the normalized cutoff frequency (in radians/sample) is:

$$
\omega_{3dB} \approx n
$$

In terms of frequency in Hz (assuming a sample rate $f_s$):

$$
f_{3dB} \approx \frac{n \cdot f_s}{2\pi}
$$

---

### ✅ Summary

* **Filter type**: First-order IIR low-pass
* **Filter equation**:

  $$
  Y_k = (1 - n) Y_{k-1} + n X_k
  $$
* **3 dB point (approximate in Hz)**:

  $$
  f_{3dB} \approx \frac{n \cdot f_s}{2\pi}
  $$

  where $f_s$ is the sampling rate in Hz

Let me know your value of $n$ and the sampling rate if you want a specific 3 dB frequency calculated.

---

You're given a **discrete-time system** defined by the difference equation:

$$
y[n] = \alpha \cdot x[n] + (1 - \alpha) \cdot y[n - 1]
$$

This is a classic **first-order IIR low-pass filter**, just like in your previous question.

---

### 🔹 Step 1: Take the Z-transform

Using the properties of the Z-transform (assuming zero initial conditions):

* $\mathcal{Z}\{y[n]\} = Y(z)$
* $\mathcal{Z}\{y[n - 1]\} = z^{-1} Y(z)$
* $\mathcal{Z}\{x[n]\} = X(z)$

Substitute into the difference equation:

$$
Y(z) = \alpha X(z) + (1 - \alpha) z^{-1} Y(z)
$$

---

### 🔹 Step 2: Solve for the Transfer Function

Rearrange:

$$
Y(z) - (1 - \alpha) z^{-1} Y(z) = \alpha X(z)
$$

Factor $Y(z)$:

$$
Y(z) \left(1 - (1 - \alpha) z^{-1} \right) = \alpha X(z)
$$

$$
H(z) = \frac{Y(z)}{X(z)} = \frac{\alpha}{1 - (1 - \alpha) z^{-1}}
$$

---

### ✅ Final Answer:

$$
\boxed{H(z) = \frac{\alpha}{1 - (1 - \alpha) z^{-1}}}
$$

This is the **transfer function** of the filter in the Z-domain.

Let me know if you want the frequency response, pole/zero plot, or time-domain behavior from this!

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

[Ben Collins]

On Wed, Aug 13, 2025 at 05:52:04PM -0500, David Lechner wrote:
> On 8/13/25 10:15 AM, Ben Collins wrote:
> > MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
... 
> >  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.
...
> 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?

Thanks for notes. If I'm reading for datasheet formula right,

k = 2 / (2^n + 1)

So n=0 would be k=1. I did this formula for n=[0-7] and get:

n	k
0	1.00000
1	0.66667
2	0.40000
3	0.22222
4	0.11765
5	0.06061
6	0.03077
7	0.01550

I'm not versed in filter frequency, but would these be the correct
values to use for the coefficients?

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

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

[Ben Collins]

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

On Thu, Aug 14, 2025 at 09:06:39AM -0500, Ben Collins wrote:
> On Wed, Aug 13, 2025 at 05:52:04PM -0500, David Lechner wrote:
> > On 8/13/25 10:15 AM, Ben Collins wrote:
> > > MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> ... 
> > >  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.
> ...
> > 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?
> 
> Thanks for notes. If I'm reading for datasheet formula right,
> 
> k = 2 / (2^n + 1)
> 
> So n=0 would be k=1. I did this formula for n=[0-7] and get:
> 
> n	k
> 0	1.00000
> 1	0.66667
> 2	0.40000
> 3	0.22222
> 4	0.11765
> 5	0.06061
> 6	0.03077
> 7	0.01550
> 
> I'm not versed in filter frequency, but would these be the correct
> values to use for the coefficients?

This seems to be what I was looking for:

Got it. For a 1-pole IIR low-pass of the form
y_t = y_{t-1} + k(x_t - y_{t-1}), the –3 dB cutoff is

f_c = \frac{-\ln(1-k)}{2\pi}\,f_s

Using your k=\frac{2}{2^n+1} and a sample rate f_s = 3\ \text{Hz}, the equivalent –3 dB cutoff frequencies are:

n	k		f_c (Hz)	time constant τ (s) = 1/(2π f_c)
0	1.0000000000	∞		0.0000
1	0.6666666667	0.5245487288	0.3034
2	0.4000000000	0.2439012692	0.6525
3	0.2222222222	0.1199938006	1.3264
4	0.1176470588	0.0597609987	2.6632
5	0.0606060606	0.0298512716	5.3316
6	0.0307692308	0.0149219903	10.6658
7	0.0155038760	0.0074605397	21.3329

Notes:
	•	n=0 (k=1) is just “track the input” (no smoothing), so f_c\to\infty.
	•	If you’re using a different sampling rate, multiply the f_c values above by \frac{f_s}{3}.

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

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

[Jonathan Cameron]

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!)

Jonathan

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

[Ben Collins]

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

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.

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

[Jonathan Cameron]

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?
 

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

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

[Ben Collins]

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

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

[Ben Collins]

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

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

[David Lechner]

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.

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

[Jonathan Cameron]

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

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

[Jonathan Cameron]

On Sat, 16 Aug 2025 11:19:53 -0400
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.
> 
> >> 
> >> 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.
> 
New ABI is always annoying painful and it is useful to give userspace some hint
as to what the filter it is playing with actually is (even if super vague).

Jonathan