[PATCH v5 0/5] iio: mcp9600: Features and improvements

[PATCH v5 0/5] iio: mcp9600: Features and improvements

[Ben Collins]

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

v3 -> v4:
  - Based on feedback from David Lechner <dlechner@baylibre.com>
    * Allow fallback compatible in dt-bindings for mcp9601.
  - Based on feedback from Jonathan Cameron <jic23@kernel.org>
    * Be explicit in patch description for fixed width changes.
    * Check chip_info for NULL to quiet warnings from kernel-test-robot
    * Remove "and similar" for long description of MCP9600.
  - Based on lots of feedback, use frequency values for IIR, and use
    filter_type[none, ema] to enable or disable.
  - Set default 3 for thermocouple in dt-binding
  - Rework open/short circuit in dt-bindings

v2 -> v3:
  - Improve changelogs in each patch
  - Based on feedback from Andy Shevchenko <andy.shevchenko@gmail.com>
    * Set register offsets to fixed width
    * Fix typos
    * Future-proof Kconfig changes
    * Convert to using chip_info paradigm
    * Verbiage: dt -> firmware description
    * Use proper specifiers and drop castings
    * Fix register offset to be fixed-width
    * u8 for cfg var
    * Fix % type for u32 to be %u
    * Make blank lines consistent between case statements
    * FIELD_PREP -> FIELD_MODIFY
    * Remove explicit setting of 0 value in filter_level
  - Based on feedback from David Lechner <dlechner@baylibre.com>
    * Rework IIR values exposed to sysfs. Using the ratios, there was no
      way to represent "disabled" (i.e. infinity). Based on the bmp280
      driver I went with using the power coefficients (e.g. 1, 2, 4, 8,
      ...) where 1 is disabled (n=0).

v1 -> v2:
  - Break into individual patches

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

Ben Collins (5):
  dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601
  iio: mcp9600: White space and fixed width cleanup
  iio: mcp9600: Recognize chip id for mcp9601
  iio: mcp9600: Add support for thermocouple-type
  iio: mcp9600: Add support for IIR filter

 .../iio/temperature/microchip,mcp9600.yaml    |  25 +-
 drivers/iio/temperature/Kconfig               |   8 +-
 drivers/iio/temperature/mcp9600.c             | 220 ++++++++++++++++--
 3 files changed, 231 insertions(+), 22 deletions(-)

-- 
2.39.5

[PATCH v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Ben Collins]

From: Ben Collins <bcollins@watter.com>

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 <bcollins@watter.com>
---
 .../iio/temperature/microchip,mcp9600.yaml    | 69 +++++++++++++++----
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
index d2cafa38a5442..1caeb6526fd20 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,29 +14,30 @@ description:
 
 properties:
   compatible:
-    const: microchip,mcp9600
+    oneOf:
+      - const: microchip,mcp9600
+      - items:
+          - const: microchip,mcp9600
+          - const: microchip,mcp9601
 
   reg:
     maxItems: 1
 
   interrupts:
     minItems: 1
-    maxItems: 6
+    maxItems: 4
 
   interrupt-names:
     minItems: 1
-    maxItems: 6
     items:
-      enum:
-        - open-circuit
-        - short-circuit
-        - alert1
-        - alert2
-        - alert3
-        - alert4
+      - const: alert1
+      - const: alert2
+      - const: alert3
+      - const: alert4
 
   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
+        interrupt-names:
+          items:
+            - const: alert1
+            - const: alert2
+            - const: alert3
+            - const: alert4
+            - const: open-circuit
+            - const: short-circuit
+        microchip,vsense:
+          default: false
+          description:
+            This flag indicates that the chip has been wired with VSENSE to
+            enable open and short circuit detect. By default, this is false,
+            since there's no way to detect that the chip is wired correctly.
+          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>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temperature-sensor@60 {
+            compatible = "microchip,mcp9601", "microchip,mcp9600";
+            microchip,vsense;
+            reg = <0x62>;
+            interrupt-parent = <&gpio>;
+            vdd-supply = <&vdd>;
+        };
+    };
-- 
2.39.5

[PATCH v5 2/5] iio: mcp9600: White space and fixed width cleanup

[Ben Collins]

From: Ben Collins <bcollins@watter.com>

Make tabs consistent for register definitions and also fix width
to byte size.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 6e9108d5cf75f..40906bb200ec9 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -23,25 +23,25 @@
 #include <linux/iio/iio.h>
 
 /* MCP9600 registers */
-#define MCP9600_HOT_JUNCTION 0x0
-#define MCP9600_COLD_JUNCTION 0x2
-#define MCP9600_STATUS			0x4
+#define MCP9600_HOT_JUNCTION		0x00
+#define MCP9600_COLD_JUNCTION		0x02
+#define MCP9600_STATUS			0x04
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
-#define MCP9600_ALERT_CFG1		0x8
+#define MCP9600_ALERT_CFG1		0x08
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
 #define MCP9600_ALERT_CFG_ACTIVE_HIGH	BIT(2)
 #define MCP9600_ALERT_CFG_FALLING	BIT(3)
 #define MCP9600_ALERT_CFG_COLD_JUNCTION	BIT(4)
-#define MCP9600_ALERT_HYSTERESIS1	0xc
+#define MCP9600_ALERT_HYSTERESIS1	0x0c
 #define MCP9600_ALERT_HYSTERESIS(x)	(MCP9600_ALERT_HYSTERESIS1 + (x - 1))
 #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.39.5

[PATCH v5 3/5] iio: mcp9600: Recognize chip id for mcp9601

[Ben Collins]

From: Ben Collins <bcollins@watter.com>

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 <bcollins@watter.com>
---
 drivers/iio/temperature/Kconfig   |  8 +++--
 drivers/iio/temperature/mcp9600.c | 55 +++++++++++++++++++++++++------
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d504..9328b2250aced 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -173,11 +173,13 @@ 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
+	  ...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 40906bb200ec9..54de38a39292e 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
 
@@ -123,6 +124,11 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
 	MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
 };
 
+struct mcp_chip_info {
+	u8 chip_id;
+	const char *chip_name;
+};
+
 struct mcp9600_data {
 	struct i2c_client *client;
 };
@@ -416,16 +422,33 @@ static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
 
 static int mcp9600_probe(struct i2c_client *client)
 {
+	const struct mcp_chip_info *chip_info = i2c_get_match_data(client);
 	struct iio_dev *indio_dev;
 	struct mcp9600_data *data;
-	int ret, ch_sel;
+	int ch_sel, dev_id, ret;
+
+	if (chip_info == NULL)
+		return dev_err_probe(&client->dev, -EINVAL,
+                                     "No chip-info found for device\n");
+
+	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);
+	}
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -439,7 +462,7 @@ static int mcp9600_probe(struct i2c_client *client)
 		return ch_sel;
 
 	indio_dev->info = &mcp9600_info;
-	indio_dev->name = "mcp9600";
+	indio_dev->name = chip_info->chip_name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = mcp9600_channels[ch_sel];
 	indio_dev->num_channels = ARRAY_SIZE(mcp9600_channels[ch_sel]);
@@ -447,14 +470,26 @@ static int mcp9600_probe(struct i2c_client *client)
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
+static const struct mcp_chip_info mcp9600_chip_info = {
+	.chip_id   = MCP9600_DEVICE_ID_MCP9600,
+	.chip_name = "mcp9600",
+};
+
+static const struct mcp_chip_info mcp9601_chip_info = {
+	.chip_id   = MCP9600_DEVICE_ID_MCP9601,
+	.chip_name = "mcp9601",
+};
+
 static const struct i2c_device_id mcp9600_id[] = {
-	{ "mcp9600" },
+	{ "mcp9600", .driver_data = (kernel_ulong_t)&mcp9600_chip_info },
+	{ "mcp9601", .driver_data = (kernel_ulong_t)&mcp9601_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, mcp9600_id);
 
 static const struct of_device_id mcp9600_of_match[] = {
-	{ .compatible = "microchip,mcp9600" },
+	{ .compatible = "microchip,mcp9600", .data = &mcp9600_chip_info },
+	{ .compatible = "microchip,mcp9601", .data = &mcp9601_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mcp9600_of_match);
-- 
2.39.5

[PATCH v5 4/5] iio: mcp9600: Add support for thermocouple-type

[Ben Collins]

From: Ben Collins <bcollins@watter.com>

dt-bindings documentation for this driver claims to support
thermocouple-type, but the driver does not actually make use of
the property.

Implement usage of the property to configure the chip for the
selected thermocouple-type.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 54de38a39292e..fa382a988a991 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		0x00
 #define MCP9600_COLD_JUNCTION		0x02
 #define MCP9600_STATUS			0x04
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
+#define MCP9600_SENSOR_CFG		0x05
+#define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
 #define MCP9600_ALERT_CFG1		0x08
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
@@ -66,6 +70,30 @@ static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
 	[MCP9600_ALERT4] = "alert4",
 };
 
+/* Map between dt-bindings enum and the chip's type value */
+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 thermocouple type to a char for for iio info in sysfs */
+static const int mcp9600_tc_types[] = {
+	[THERMOCOUPLE_TYPE_K] = 'K',
+	[THERMOCOUPLE_TYPE_J] = 'J',
+	[THERMOCOUPLE_TYPE_T] = 'T',
+	[THERMOCOUPLE_TYPE_N] = 'N',
+	[THERMOCOUPLE_TYPE_S] = 'S',
+	[THERMOCOUPLE_TYPE_E] = 'E',
+	[THERMOCOUPLE_TYPE_B] = 'B',
+	[THERMOCOUPLE_TYPE_R] = 'R',
+};
+
 static const struct iio_event_spec mcp9600_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -89,6 +117,7 @@ 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_THERMOCOUPLE_TYPE) | \
 					      BIT(IIO_CHAN_INFO_SCALE),	       \
 			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
 			.num_event_specs = hj_num_ev,			       \
@@ -131,6 +160,7 @@ struct mcp_chip_info {
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	u32 thermocouple_type;
 };
 
 static int mcp9600_read(struct mcp9600_data *data,
@@ -165,11 +195,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;
+	u8 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) {
@@ -457,6 +508,24 @@ 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 (ret < 0 && ret != -EINVAL)
+		return dev_err_probe(&client->dev, ret,
+				     "Error reading thermocouple-type property\n");
+
+	if (data->thermocouple_type >= ARRAY_SIZE(mcp9600_type_map))
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Invalid thermocouple-type property %u.\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.39.5

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

[Ben Collins]

From: Ben Collins <bcollins@watter.com>

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 <bcollins@watter.com>
---
 drivers/iio/temperature/mcp9600.c | 157 ++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index fa382a988a991..54bc657460e5d 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -31,6 +31,7 @@
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
 #define MCP9600_SENSOR_CFG		0x05
 #define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
+#define MCP9600_FILTER_MASK		GENMASK(2, 0)
 #define MCP9600_ALERT_CFG1		0x08
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
@@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
 	[THERMOCOUPLE_TYPE_R] = 'R',
 };
 
+enum mcp9600_filter {
+	MCP9600_FILTER_TYPE_NONE,
+	MCP9600_FILTER_TYPE_EMA,
+};
+
+static const char * const mcp9600_filter_type[] = {
+	[MCP9600_FILTER_TYPE_NONE] = "none",
+	[MCP9600_FILTER_TYPE_EMA] = "ema",
+};
+
+static const int mcp_iir_coefficients_avail[7][2] = {
+	/* Level 0 is no filter */
+	{ 0, 524549 },
+	{ 0, 243901 },
+	{ 0, 119994 },
+	{ 0,  59761 },
+	{ 0,  29851 },
+	{ 0,  14922 },
+	{ 0,   7461 },
+};
+
 static const struct iio_event_spec mcp9600_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -118,7 +140,11 @@ static const struct iio_event_spec mcp9600_events[] = {
 			.address = MCP9600_HOT_JUNCTION,		       \
 			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
 					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
+					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |  \
 					      BIT(IIO_CHAN_INFO_SCALE),	       \
+			.info_mask_separate_available =                        \
+					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
+			.ext_info = mcp9600_ext_filter,			       \
 			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
 			.num_event_specs = hj_num_ev,			       \
 		},							       \
@@ -134,6 +160,26 @@ static const struct iio_event_spec mcp9600_events[] = {
 		},							       \
 	}
 
+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);
+
+static const struct iio_enum mcp9600_filter_enum = {
+	.items = mcp9600_filter_type,
+	.num_items = ARRAY_SIZE(mcp9600_filter_type),
+	.get = mcp9600_get_filter,
+	.set = mcp9600_set_filter,
+};
+
+static const struct iio_chan_spec_ext_info mcp9600_ext_filter[] = {
+	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &mcp9600_filter_enum),
+	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
+			   &mcp9600_filter_enum),
+	{ }
+};
+
 static const struct iio_chan_spec mcp9600_channels[][2] = {
 	MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
 	MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
@@ -161,6 +207,8 @@ struct mcp_chip_info {
 struct mcp9600_data {
 	struct i2c_client *client;
 	u32 thermocouple_type;
+	/* Filter enabled is 1-7, with 0 being off (filter_type none) */
+	u8 filter_level;
 };
 
 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;
+
 	case IIO_CHAN_INFO_SCALE:
 		*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;
+
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		if (data->filter_level == 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,
+			      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;
+
+		*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;
 }
 
+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;
+	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:
+		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)
+		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);
+
+	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;
+			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,
-- 
2.39.5

Re: [PATCH v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Krzysztof Kozlowski]

On 18/08/2025 05:59, Ben Collins wrote:
> From: Ben Collins <bcollins@watter.com>
> 
> 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:
> 

None of this driver argument here and earlier is relevant. Please
describe the hardware and reasons behind this patch.

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

Drop, not relevant. You do not have to say that you built your code. You
must build your code.

> 
> Signed-off-by: Ben Collins <bcollins@watter.com>
> ---
>  .../iio/temperature/microchip,mcp9600.yaml    | 69 +++++++++++++++----
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> index d2cafa38a5442..1caeb6526fd20 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,29 +14,30 @@ description:
>  
>  properties:
>    compatible:
> -    const: microchip,mcp9600
> +    oneOf:
> +      - const: microchip,mcp9600
> +      - items:
> +          - const: microchip,mcp9600
> +          - const: microchip,mcp9601
>  
>    reg:
>      maxItems: 1
>  
>    interrupts:
>      minItems: 1
> -    maxItems: 6
> +    maxItems: 4

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

>  
>    interrupt-names:
>      minItems: 1
> -    maxItems: 6
>      items:
> -      enum:
> -        - 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.


>  
>    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
> +        interrupt-names:
> +          items:
> +            - const: alert1
> +            - const: alert2
> +            - const: alert3
> +            - const: alert4
> +            - const: open-circuit
> +            - const: short-circuit
> +        microchip,vsense:
> +          default: false

There is no default for bool.

> +          description:
> +            This flag indicates that the chip has been wired with VSENSE to
> +            enable open and short circuit detect. By default, this is false,
> +            since there's no way to detect that the chip is wired correctly.

Properties should be defined in top level. Here you only disallow them
(see example schema).

> +          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>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temperature-sensor@60 {
> +            compatible = "microchip,mcp9601", "microchip,mcp9600";
> +            microchip,vsense;
> +            reg = <0x62>;
> +            interrupt-parent = <&gpio>;

Incomplete interrupts.

> +            vdd-supply = <&vdd>;
> +        };
> +    };


Best regards,
Krzysztof

Re: [PATCH v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Rob Herring (Arm)]

On Sun, 17 Aug 2025 23:59:49 -0400, Ben Collins wrote:
> From: Ben Collins <bcollins@watter.com>
> 
> 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 <bcollins@watter.com>
> ---
>  .../iio/temperature/microchip,mcp9600.yaml    | 69 +++++++++++++++----
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

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

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250818035953.35216-2-bcollins@kernel.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Krzysztof Kozlowski]

On 18/08/2025 05:59, Ben Collins wrote:
> From: Ben Collins <bcollins@watter.com>
> 
> 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.

Yeah...

...


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


Best regards,
Krzysztof

Re: [PATCH v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Ben Collins]

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

On Mon, Aug 18, 2025 at 01:33:03AM -0500, Rob Herring (Arm) wrote:
> 
> On Sun, 17 Aug 2025 23:59:49 -0400, Ben Collins wrote:
> > From: Ben Collins <bcollins@watter.com>
> > 
> 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

Thanks. Found this already and fix will be in v6.

-- 
 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 v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Ben Collins]

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

On Mon, Aug 18, 2025 at 08:40:26AM -0500, Krzysztof Kozlowski wrote:
> On 18/08/2025 05:59, Ben Collins wrote:
> > From: Ben Collins <bcollins@watter.com>
> > 
> > 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.
> 
> Yeah...
> 
> ...
> 
> 
> > -            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...

I actually did, and fixed it, but it didn't make it into the commit when
I emailed.

-- 
 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 v5 1/5] dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints

[Conor Dooley]

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

On Mon, Aug 18, 2025 at 08:28:30AM +0200, Krzysztof Kozlowski wrote:
> On 18/08/2025 05:59, Ben Collins wrote:
> >    interrupts:
> >      minItems: 1
> > -    maxItems: 6
> > +    maxItems: 4
> 
> Why?
> I did not find explanation of this in commit msg.

It's also not correct, since the outermost constraint remains 6 after
the patch, so the if/else should reduce the constraints, rather than
increase it as is done here.

> 
> >  
> >    interrupt-names:
> >      minItems: 1
> > -    maxItems: 6
> >      items:
> > -      enum:
> > -        - 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.

I think it should be a standalone patch too, since it is a fix for the
existing mcp9600 device rather than something for the mcp9601 device
that is being added by this patch...
> 
> 
> >  
> >    thermocouple-type:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 3

As is this, which is codifying the existing restriction rather than
being something new as 0x03 is what THERMOCOUPLE_TYPE_K is defined to
be.

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

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

Re: [PATCH v5 3/5] iio: mcp9600: Recognize chip id for mcp9601

[Jonathan Cameron]

On Sun, 17 Aug 2025 23:59:51 -0400
Ben Collins <bcollins@kernel.org> wrote:

> From: Ben Collins <bcollins@watter.com>
> 
> 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 <bcollins@watter.com>

Hi Ben

One minor thing inline that need fixing up.

> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 40906bb200ec9..54de38a39292e 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -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)
>  {
> +	const struct mcp_chip_info *chip_info = i2c_get_match_data(client);
>  	struct iio_dev *indio_dev;
>  	struct mcp9600_data *data;
> -	int ret, ch_sel;
> +	int ch_sel, dev_id, ret;
> +
> +	if (chip_info == NULL)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +                                     "No chip-info found for device\n");

This line seems to be indented with spaces, not tabs then spaces which
is the coding style. I only noticed because of the shift seen in this reply.



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

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

[Jonathan Cameron]

On Sun, 17 Aug 2025 23:59:53 -0400
Ben Collins <bcollins@kernel.org> wrote:

> From: Ben Collins <bcollins@watter.com>
> 
> 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

Thanks,

Jonathan

> 
> Signed-off-by: Ben Collins <bcollins@watter.com>
> ---
>  drivers/iio/temperature/mcp9600.c | 157 ++++++++++++++++++++++++++++++
>  1 file changed, 157 insertions(+)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index fa382a988a991..54bc657460e5d 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -31,6 +31,7 @@
>  #define MCP9600_STATUS_ALERT(x)		BIT(x)
>  #define MCP9600_SENSOR_CFG		0x05
>  #define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
> +#define MCP9600_FILTER_MASK		GENMASK(2, 0)
>  #define MCP9600_ALERT_CFG1		0x08
>  #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
>  #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> @@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
>  	[THERMOCOUPLE_TYPE_R] = 'R',
>  };
>  
> +enum mcp9600_filter {
> +	MCP9600_FILTER_TYPE_NONE,
> +	MCP9600_FILTER_TYPE_EMA,
> +};
> +
> +static const char * const mcp9600_filter_type[] = {
> +	[MCP9600_FILTER_TYPE_NONE] = "none",
> +	[MCP9600_FILTER_TYPE_EMA] = "ema",
> +};
> +
> +static const int mcp_iir_coefficients_avail[7][2] = {
> +	/* Level 0 is no filter */
> +	{ 0, 524549 },
> +	{ 0, 243901 },
> +	{ 0, 119994 },
> +	{ 0,  59761 },
> +	{ 0,  29851 },
> +	{ 0,  14922 },
> +	{ 0,   7461 },
> +};
> +
>  static const struct iio_event_spec mcp9600_events[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -118,7 +140,11 @@ static const struct iio_event_spec mcp9600_events[] = {
>  			.address = MCP9600_HOT_JUNCTION,		       \
>  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
>  					      BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> +					      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |  \

Probably just one space before \ is the most consistent choice.

>  					      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,			       \
>  		},							       \
> @@ -134,6 +160,26 @@ static const struct iio_event_spec mcp9600_events[] = {
>  		},							       \
>  	}
>  
> +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.

> +
> +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,
> @@ -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.

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

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

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

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

> +		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);
> +
> +	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.

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

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

[Ben Collins]

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

On Mon, Aug 18, 2025 at 07:15:39PM -0500, Jonathan Cameron wrote:
> On Sun, 17 Aug 2025 23:59:53 -0400
> Ben Collins <bcollins@kernel.org> wrote:
> 
> > From: Ben Collins <bcollins@watter.com>
> > 
> > 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

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.

More comments below.

> > 
> > Signed-off-by: Ben Collins <bcollins@watter.com>
> > ---
> > @@ -134,6 +160,26 @@ static const struct iio_event_spec mcp9600_events[] = {
> >  		},							       \
> >  	}
> >  
> > +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.

The set function, for one, calls mcp9600_config, which comes after the
use of the get/set in the ext_info. I'll see if I can shuffle that
around in the prior patch to avoid this.

> > +
> > +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,
> > @@ -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.

Noted.

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

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

Ok.

> > +	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:
> > +		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.
> 
> > +		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);
> > +
> > +	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.
> 
> > +			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,
> 

-- 
 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 v5 5/5] iio: mcp9600: Add support for IIR filter

[David Lechner]

On 8/18/25 1:47 PM, Ben Collins wrote:
> On Mon, Aug 18, 2025 at 07:15:39PM -0500, Jonathan Cameron wrote:
>> On Sun, 17 Aug 2025 23:59:53 -0400
>> Ben Collins <bcollins@kernel.org> wrote:
>>
>>> From: Ben Collins <bcollins@watter.com>
>>>
>>> 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
> 
> 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.

If you are really in a hurry, there should still be no more than
one revision per day. Otherwise, it is really hard for reviewers
to keep up.

As it is, the subject of what I presume is v6 still says v5 in the
cover letter and doesn't have a version in the rest of the patches.
And there are still some of the same problems with the devicetree
patch that didn't get addressed. If you slow down a bit and take
a little more care before firing off the next one, it will likely
be better received.

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

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

[Jonathan Cameron]

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

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.

Thanks,

Jonathan

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

[Ben Collins]

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

On Mon, Aug 18, 2025 at 01:59:34PM -0500, David Lechner wrote:
> On 8/18/25 1:47 PM, Ben Collins wrote:
> > On Mon, Aug 18, 2025 at 07:15:39PM -0500, Jonathan Cameron wrote:
> >> On Sun, 17 Aug 2025 23:59:53 -0400
> >> Ben Collins <bcollins@kernel.org> wrote:
> >>
> >>> From: Ben Collins <bcollins@watter.com>
> >>>
> >>> 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
> > 
> > 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.

I appreciate the info. Admittedly, when I was more prolific with kernel
development, the pipline and protocol was much leaner, so I'm not used
to the more recent "norm" quite yet.

Not complaining, just noting. I'll get there.

-- 
 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 v5 5/5] iio: mcp9600: Add support for IIR filter

[Ben Collins]

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

On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote:
> 
> > > >  	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.
> > >   
> > > >  	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?
> 
> 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.

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

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

> ABI design is a pain sometimes.

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

-- 
 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 v5 5/5] iio: mcp9600: Add support for IIR filter

[Jonathan Cameron]

On Mon, 18 Aug 2025 16:00:20 -0400
Ben Collins <bcollins@kernel.org> wrote:

> On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote:
> >   
> > > > >  	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.
> > > >     
> > > > >  	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?  
> > 
> > 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.  
> 
> 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.

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

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.

> 
> > ABI design is a pain sometimes.  
> 
> The epitome of being able to paint yourself into a corner.
> 
Yup.

J

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

[Jonathan Cameron]

On Tue, 19 Aug 2025 19:28:51 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 18 Aug 2025 16:00:20 -0400
> Ben Collins <bcollins@kernel.org> wrote:
> 
> > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote:  
> > >     
> > > > > >  	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.
> > > > >       
> > > > > >  	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?    
> > > 
> > > 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.    
> > 
> > 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.
> 
> > > 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?  
> 
> 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.

J

> >   
> > > ABI design is a pain sometimes.    
> > 
> > The epitome of being able to paint yourself into a corner.
> >   
> Yup.
> 
> J
> 

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

[Ben Collins]

On Tue, Aug 19, 2025 at 07:28:51PM -0500, Jonathan Cameron wrote:
> On Mon, 18 Aug 2025 16:00:20 -0400
> Ben Collins <bcollins@kernel.org> wrote:
> 
> > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote:
> > >   
> > > > > >  	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.
> > > > >     
> > > > > >  	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?  
> > > 
> > > 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.  
> > 
> > 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.
> 
> > > 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?
> 
> 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.

I'm good with that. I'll get a patch to update the ABI, add filter_enable
usage in mcp9600 IIR patch, and provide another patch for ad4080 to convert
to this.

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

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

[Ben Collins]

On Tue, Aug 19, 2025 at 07:38:37PM -0500, Jonathan Cameron wrote:
> On Tue, 19 Aug 2025 19:28:51 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Mon, 18 Aug 2025 16:00:20 -0400
> > Ben Collins <bcollins@kernel.org> wrote:
> > 
> > > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote:  
> > > >     
> > > > > > >  	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.
> > > > > >       
> > > > > > >  	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?    
> > > > 
> > > > 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.    
> > > 
> > > 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.
> > 
> > > > 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?  
> > 
> > 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.

Maybe a working solution would help the conversation to see how it would
look.

I think I can rework ad4080 so that it keeps "none" filter_type, but
also can make use of filter_enable. That way it can continue to work
for users while still providing a better interface for anything else
moving forward.

I can send this as a separate RFC series from my current mcp9600 changes
to break this out.

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