iio: mcp9600: Features and improvements

From: Ben Collins <[email protected]>

The mcp9600 driver supports the mcp9601 chip, but complains about not recognizing the device id on probe. A separate patch...

iio: mcp9600: Recognize chip id for mcp9601

...addresses this. This patch updates the dt-bindings for this chip to reflect the change to allow explicitly setting microchip,mcp9601 as the expected chip type.

The mcp9601 also supports features not found on the mcp9600, so this will also allow the driver to differentiate the support of these features.

In addition, the thermocouple-type needs a default of 3 (k-type). The driver doesn't support this, yet. A later patch in this series adds it:

iio: mcp9600: Add support for thermocouple-type

Lastly, the open/short circuit functionality is dependent on mcp9601 chipsset. Add constraints for this and a new property, microchip,vsense, enables this feature since it depends on the chip being wired properly.

Passed dt_binding_check.

Signed-off-by Ben Collins <[email protected]>

---
 .../iio/temperature/microchip,mcp9600.yaml    | 69 +++++++++++++++----
 1 file changed, 56 insertions(+), 13 deletions(-)

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +4 -2

index d2cafa38a5442..1caeb6526fd20 100644
@@ -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 <[email protected]>

@@ -14,29 +14,30 @@ description:
 
 properties:
   compatible:
-    const: microchip,mcp9600
+    oneOf:
+      - const: microchip,mcp9600
+      - items:

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +0 -2

   interrupt-names:
     minItems: 1
-    maxItems: 6
     items:
-      enum:

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +12 -0

   thermocouple-type:
     $ref: /schemas/types.yaml#/definitions/uint32
+    default: 3
     description:
       Type of thermocouple (THERMOCOUPLE_TYPE_K if omitted).
       Use defines in dt-bindings/iio/temperature/thermocouple.h.

@@ -44,6 +45,33 @@ properties:
 
   vdd-supply: true
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: microchip,mcp9601
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          maxItems: 6

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +6 -2

+          type: boolean
+
 required:
   - compatible
   - reg

@@ -62,9 +90,24 @@ examples:
             compatible = "microchip,mcp9600";
             reg = <0x60>;
             interrupt-parent = <&gpio>;
-            interrupts = <25 IRQ_TYPE_EDGE_RISING>;
-            interrupt-names = "open-circuit";
+            interrupts = <25 IRQ_TYPE_EDGE_RISIN>;
+            interrupt-names = "alert1";
             thermocouple-type = <THERMOCOUPLE_TYPE_K>;
             vdd-supply = <&vdd>;
         };
     };
+  - |
+    #include <dt-bindings/iio/temperature/thermocouple.h>

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml description: · line 28

Quoted 4 lines · Ben Collins

interrupts:
minItems: 1

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +1 -1

-    maxItems: 6
+    maxItems: 4

Why?
I did not find explanation of this in commit msg.

Quoted 7 lines

interrupt-names:
minItems: 1

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +0 -2

-    maxItems: 6
     items:
-      enum:

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml description: · line 36

Quoted 10 lines

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +4 -6

-        - open-circuit
-        - short-circuit
-        - alert1
-        - alert2
-        - alert3
-        - alert4
+      - const: alert1
+      - const: alert2
+      - const: alert3
+      - const: alert4

Neither this and it is ABI break. ABI breaking needs clear reasoning why and some evaluation of impact on users.

Quoted 6 lines

thermocouple-type:
$ref: /schemas/types.yaml#/definitions/uint32
+ default: 3

Quoted 3 lines

description:
Type of thermocouple (THERMOCOUPLE_TYPE_K if omitted). Use defines in dt-bindings/iio/temperature/thermocouple.h.

From: Ben Collins <[email protected]>

The mcp9600 driver supports the mcp9601 chip, but complains about not recognizing the device id on probe. A separate patch...

iio: mcp9600: Recognize chip id for mcp9601

...addresses this. This patch updates the dt-bindings for this chip to reflect the change to allow explicitly setting microchip,mcp9601 as the expected chip type.

The mcp9601 also supports features not found on the mcp9600, so this will also allow the driver to differentiate the support of these features.

In addition, the thermocouple-type needs a default of 3 (k-type). The driver doesn't support this, yet. A later patch in this series adds it:

iio: mcp9600: Add support for thermocouple-type

Lastly, the open/short circuit functionality is dependent on mcp9601 chipsset. Add constraints for this and a new property, microchip,vsense, enables this feature since it depends on the chip being wired properly.

Passed dt_binding_check.

Signed-off-by Ben Collins <[email protected]>

---
 .../iio/temperature/microchip,mcp9600.yaml    | 69 +++++++++++++++----
 1 file changed, 56 insertions(+), 13 deletions(-)

Quoted 2 lines · Ben Collins

From: Ben Collins <[email protected]>

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.example.dts:34.34-35 syntax error FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1527: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

From: Ben Collins <[email protected]>

The mcp9600 driver supports the mcp9601 chip, but complains about not recognizing the device id on probe. A separate patch...

iio: mcp9600: Recognize chip id for mcp9601

...addresses this. This patch updates the dt-bindings for this chip to reflect the change to allow explicitly setting microchip,mcp9601 as the expected chip type.

The mcp9601 also supports features not found on the mcp9600, so this will also allow the driver to differentiate the support of these features.

In addition, the thermocouple-type needs a default of 3 (k-type). The driver doesn't support this, yet. A later patch in this series adds it:

iio: mcp9600: Add support for thermocouple-type

Lastly, the open/short circuit functionality is dependent on mcp9601 chipsset. Add constraints for this and a new property, microchip,vsense, enables this feature since it depends on the chip being wired properly.

Passed dt_binding_check.

Quoted 18 lines · Ben Collins

From: Ben Collins <[email protected]>

The mcp9600 driver supports the mcp9601 chip, but complains about not recognizing the device id on probe. A separate patch...

iio: mcp9600: Recognize chip id for mcp9601

...addresses this. This patch updates the dt-bindings for this chip to reflect the change to allow explicitly setting microchip,mcp9601 as the expected chip type.

The mcp9601 also supports features not found on the mcp9600, so this will also allow the driver to differentiate the support of these features.

In addition, the thermocouple-type needs a default of 3 (k-type). The driver doesn't support this, yet. A later patch in this series adds it:

Quoted 8 lines

iio: mcp9600: Add support for thermocouple-type

Lastly, the open/short circuit functionality is dependent on mcp9601 chipsset. Add constraints for this and a new property, microchip,vsense, enables this feature since it depends on the chip being wired properly.

Passed dt_binding_check.

Yeah...

...

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml examples: · line 93

Quoted 3 lines

Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml +1 -2

-            interrupts = <25 IRQ_TYPE_EDGE_RISING>;
-            interrupt-names = "open-circuit";
+            interrupts = <25 IRQ_TYPE_EDGE_RISIN>;

Except that it wasn't it. You need to test your final code, after you commit. Mentioning that you tested it and then actually do not test and send something which does not build, heh...

From: Ben Collins <[email protected]>

The current driver works with mcp9601, but emits a warning because it does not recognize the chip id.

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

However, the 9601 chip supports open/closed-circuit detection if wired properly, so we'll need to be able to differentiate between them.

Signed-off-by Ben Collins <[email protected]>

drivers/iio/temperature/mcp9600.c +0 -0

index 40906bb200ec9..54de38a39292e 100644
@@ -42,6 +42,7 @@
 

@@ -416,16 +422,33 @@ static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
 
 static int mcp9600_probe(struct i2c_client *client)

drivers/iio/temperature/mcp9600.c +18 -6

+
+	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 != chip_info->chip_id)
+			dev_warn(&client->dev,
+				 "Expected id %02x, but device responded with %02x\n",
+				 chip_info->chip_id, 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", dev_id,
+			 chip_info->chip_id);
+	}

From: Ben Collins <[email protected]>

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

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

Signed-off-by Ben Collins <[email protected]>

---
 drivers/iio/temperature/mcp9600.c | 157 ++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

drivers/iio/temperature/mcp9600.c +22 -0

index fa382a988a991..54bc657460e5d 100644
@@ -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,

drivers/iio/temperature/mcp9600.c +3 -0

 					      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,			       \
 		},							       \

drivers/iio/temperature/mcp9600.c +7 -0

+
+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 int mcp9600_read(struct mcp9600_data *data,

drivers/iio/temperature/mcp9600.c +12 -0

+			return -EINVAL;
+
+		*val = mcp_iir_coefficients_avail[data->filter_level - 1][0];
+		*val2 = mcp_iir_coefficients_avail[data->filter_level - 1][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9600_read_avail(struct iio_dev *indio_dev,

drivers/iio/temperature/mcp9600.c +6 -0

+
+		*vals = (int *)mcp_iir_coefficients_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = 2 * ARRAY_SIZE(mcp_iir_coefficients_avail);
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}

@@ -211,6 +291,7 @@ static int mcp9600_config(struct mcp9600_data *data)
 
 	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
 			  mcp9600_type_map[data->thermocouple_type]);
+	FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level);
 
 	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
 	if (ret < 0) {

drivers/iio/temperature/mcp9600.c +27 -0

+	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:
+		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;
+
+		data->filter_level = i + 1;
+		return mcp9600_config(data);
+
+	default:

drivers/iio/temperature/mcp9600.c +10 -0

+		return MCP9600_FILTER_TYPE_NONE;
+
+	return MCP9600_FILTER_TYPE_EMA;
+}
+
+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);

drivers/iio/temperature/mcp9600.c +12 -0

+			return mcp9600_config(data);
+		}
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
 {
 	if (channel2 == IIO_MOD_TEMP_AMBIENT) {

@@ -358,6 +512,9 @@ 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,
+	.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,

Quoted 2 lines · On Sun, 17 Aug 2025 23:59:53 -0400 Ben Collins <[email protected]> wrote:

From: Ben Collins <[email protected]>

Quoted 4 lines

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

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

Hi Ben,

A few comments inline. You also need to send an additional patch to update the filter_type docs in Documentation/ABI/testing/sysfs-bus-iio

Quoted 7 lines

Signed-off-by Ben Collins <[email protected]>

---

(quoted diff) +0 -0

@@ -134,6 +160,26 @@ static const struct iio_event_spec mcp9600_events[] = {
 		},							       \
 	}
 

drivers/iio/temperature/mcp9600.c static const struct iio_event_spec mcp9600_events[] = { · line 167

Quoted 5 lines

drivers/iio/temperature/mcp9600.c +5 -0

+static int mcp9600_get_filter(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan);
+static int mcp9600_set_filter(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      unsigned int mode);

shuffle the code around so you don't need a forward definition.. There is no particular reason I can see to have get and set later.

Quoted 7 lines

drivers/iio/temperature/mcp9600.c +7 -0

+
+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,
+};

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 242

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 static int mcp9600_read(struct mcp9600_data *data,

@@ -191,13 +239,45 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 		return IIO_VAL_INT;
+

Unrelated changes. White space changes should not be mixed in a patch doing anything significant. If you want to do this, extra patch needed.

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 5 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+

If you want the extra space put it in previous patch.

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 12 lines

drivers/iio/temperature/mcp9600.c +12 -0

+			return -EINVAL;
+
+		*val = mcp_iir_coefficients_avail[data->filter_level - 1][0];
+		*val2 = mcp_iir_coefficients_avail[data->filter_level - 1][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9600_read_avail(struct iio_dev *indio_dev,

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 275

Quoted 10 lines

drivers/iio/temperature/mcp9600.c +10 -0

+			      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_level == 0)
+			return -EINVAL;

Don't block this. It makes for a confusing interface.

Quoted 20 lines

drivers/iio/temperature/mcp9600.c +6 -0

+
+		*vals = (int *)mcp_iir_coefficients_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = 2 * ARRAY_SIZE(mcp_iir_coefficients_avail);
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}

@@ -211,6 +291,7 @@ static int mcp9600_config(struct mcp9600_data *data)
 
 	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
 			  mcp9600_type_map[data->thermocouple_type]);
+	FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level);
 
 	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
 	if (ret < 0) {

@@ -221,6 +302,79 @@ static int mcp9600_config(struct mcp9600_data *data)
 	return 0;
 }
 

drivers/iio/temperature/mcp9600.c static int mcp9600_config(struct mcp9600_data *data) · line 311

Quoted 7 lines

drivers/iio/temperature/mcp9600.c +7 -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_PLUS_MICRO;

That's the default so you shouldn't need a write_raw_get_fmt for this.

Quoted 27 lines

drivers/iio/temperature/mcp9600.c +27 -0

+	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:
+		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;
+
+		data->filter_level = i + 1;
+		return mcp9600_config(data);
+
+	default:

drivers/iio/temperature/mcp9600.c static int mcp9600_config(struct mcp9600_data *data) · line 348

Quoted 10 lines

drivers/iio/temperature/mcp9600.c +10 -0

+		return -EINVAL;
+	}
+}
+
+static int mcp9600_get_filter(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+
+	if (data->filter_level == 0)

I'd use a separate enable flag for this.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c +10 -0

+		return MCP9600_FILTER_TYPE_NONE;
+
+	return MCP9600_FILTER_TYPE_EMA;
+}
+
+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);

drivers/iio/temperature/mcp9600.c static int mcp9600_config(struct mcp9600_data *data) · line 368

Quoted 10 lines

drivers/iio/temperature/mcp9600.c +10 -0

+
+	switch (mode) {
+	case MCP9600_FILTER_TYPE_NONE:
+		data->filter_level = 0;
+		return mcp9600_config(data);
+
+	case MCP9600_FILTER_TYPE_EMA:
+		if (data->filter_level == 0) {
+			/* Minimum filter by default */
+			data->filter_level = 1;

As above, I'd just let the user write it whenever they like (default to 1 on boot) and then they have to see if it is set to none to see whether it has an effect.

Quoted 22 lines

drivers/iio/temperature/mcp9600.c +12 -0

+			return mcp9600_config(data);
+		}
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
 {
 	if (channel2 == IIO_MOD_TEMP_AMBIENT) {

@@ -358,6 +512,9 @@ 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,
+	.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,

Quoted 11 lines · Jonathan Cameron

Quoted 6 lines · On Sun, 17 Aug 2025 23:59:53 -0400 Ben Collins <[email protected]> wrote:

From: Ben Collins <[email protected]>

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

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

Hi Ben,

Quoted 2 lines

A few comments inline. You also need to send an additional patch to update the filter_type docs in Documentation/ABI/testing/sysfs-bus-iio

Hi Jonathan,

I just sent a v6 because I was getting too many comments on the dt-bindings patch.

I'll send a v7 with these changes and anything else that comes up.

Quoted 15 lines · Ben Collins

Quoted 13 lines · Jonathan Cameron

Quoted 6 lines · On Sun, 17 Aug 2025 23:59:53 -0400 Ben Collins <[email protected]> wrote:

From: Ben Collins <[email protected]>

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

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

Hi Ben,

A few comments inline. You also need to send an additional patch to update the filter_type docs in Documentation/ABI/testing/sysfs-bus-iio

Quoted 4 lines

Hi Jonathan,

I just sent a v6 because I was getting too many comments on the dt-bindings patch.

Actually, folks will be happier if you slow down a bit. General advice is only submit one revision per week since some people only have time to review once per week.

Quoted 8 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+  

If you want the extra space put it in previous patch.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)  

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

Quoted 19 lines

Quoted 8 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+  

If you want the extra space put it in previous patch.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)  

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 4 lines

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI).

Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type.

This is a good argument against the whole 'none' value for filter type
- that's not much used so we could deprecate it for new drivers.

I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use.

So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in.

ABI design is a pain sometimes.

Quoted 38 lines · Jonathan Cameron

Quoted 19 lines

Quoted 8 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+    

If you want the extra space put it in previous patch.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)    

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 4 lines

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI).

Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type.

This is a good argument against the whole 'none' value for filter type
- that's not much used so we could deprecate it for new drivers.

Quoted 3 lines

I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use.

I'm somewhat wondering if the filter frequency and frequency_available attributes should not even show in sysfs unless the filter_type was something other than "none".

Quoted 2 lines

So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in.

Just to make sure I understand, you'd like to see a filter_enable attribute and filter_type would not contain "none", then frequency and frequency_available would always show something for whatever was in filter_type?

Quoted 2 lines

ABI design is a pain sometimes.

The epitome of being able to paint yourself into a corner.

Quoted 39 lines · On Mon, 18 Aug 2025 16:00:20 -0400 Ben Collins <[email protected]> wrote:

Quoted 38 lines · Jonathan Cameron

Quoted 19 lines

Quoted 8 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+      

If you want the extra space put it in previous patch.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)      

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 4 lines

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI).

Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type.

This is a good argument against the whole 'none' value for filter type
- that's not much used so we could deprecate it for new drivers.

Quoted 8 lines

Quoted 3 lines

I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use.

I'm somewhat wondering if the filter frequency and frequency_available attributes should not even show in sysfs unless the filter_type was something other than "none".

I'm not keen on that and trying to bolt is_visible into the mess of how we generate attributes would be hard and actual add and remove of attributes is horrible for races with userspace.

Quoted 3 lines

Quoted 2 lines

So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in.

Quoted 4 lines

Just to make sure I understand, you'd like to see a filter_enable attribute and filter_type would not contain "none", then frequency and frequency_available would always show something for whatever was in filter_type?

Yes. I think that is best way forwards. If we want to retrofit the one user of none to support the new ABI as well it should be easy to do.

Quoted 5 lines

Quoted 2 lines

ABI design is a pain sometimes.

The epitome of being able to paint yourself into a corner.

Yup.

J

Quoted 62 lines · On Tue, 19 Aug 2025 19:28:51 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 39 lines · On Mon, 18 Aug 2025 16:00:20 -0400 Ben Collins <[email protected]> wrote:

Quoted 38 lines · Jonathan Cameron

Quoted 19 lines

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+      

If you want the extra space put it in previous patch.

Quoted 10 lines

Review

Quoted 6 lines

(quoted diff) +3 -0

 	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_level == 0)      

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 4 lines

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI).

Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type.

This is a good argument against the whole 'none' value for filter type
- that's not much used so we could deprecate it for new drivers.

Quoted 8 lines

Quoted 3 lines

I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use.

I'm somewhat wondering if the filter frequency and frequency_available attributes should not even show in sysfs unless the filter_type was something other than "none".

I'm not keen on that and trying to bolt is_visible into the mess of how we generate attributes would be hard and actual add and remove of attributes is horrible for races with userspace.

Quoted 7 lines

Quoted 2 lines

So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in.

Just to make sure I understand, you'd like to see a filter_enable attribute and filter_type would not contain "none", then frequency and frequency_available would always show something for whatever was in filter_type?

Quoted 3 lines

Yes. I think that is best way forwards. If we want to retrofit the one user of none to support the new ABI as well it should be easy to do.

Ignore that. David convinced me otherwise. Lets take this a bit slow and discuss it fully in other branch of this thread.

Quoted 39 lines · On Mon, 18 Aug 2025 16:00:20 -0400 Ben Collins <[email protected]> wrote:

Quoted 38 lines · Jonathan Cameron

Quoted 19 lines

Quoted 8 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 247

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +1 -0

 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
 		*val2 = 500000;
 		return IIO_VAL_INT_PLUS_MICRO;
+    

If you want the extra space put it in previous patch.

Quoted 10 lines

drivers/iio/temperature/mcp9600.c static int mcp9600_read_raw(struct iio_dev *indio_dev, · line 253

Quoted 6 lines

drivers/iio/temperature/mcp9600.c +3 -0

 	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_level == 0)    

Return the current requested value. An error is just going to confuse someone who tried to write this before enabling the filter and then checked to see if the write was successful.

Quoted 4 lines

I could not get a concensus on this. On the one hand, if a user sets a value here, would they not assume that the filter was enabled? What about cases where a filter_type can be more than one valid type with different available coefficients for each? What should it show then?

So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI).

Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type.

This is a good argument against the whole 'none' value for filter type
- that's not much used so we could deprecate it for new drivers.

Quoted 8 lines

Quoted 3 lines

I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use.

I'm somewhat wondering if the filter frequency and frequency_available attributes should not even show in sysfs unless the filter_type was something other than "none".

I'm not keen on that and trying to bolt is_visible into the mess of how we generate attributes would be hard and actual add and remove of attributes is horrible for races with userspace.

Quoted 3 lines

Quoted 2 lines

So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in.

Quoted 4 lines

Just to make sure I understand, you'd like to see a filter_enable attribute and filter_type would not contain "none", then frequency and frequency_available would always show something for whatever was in filter_type?

Yes. I think that is best way forwards. If we want to retrofit the one user of none to support the new ABI as well it should be easy to do.