[PATCH] mcp9600: Add support for mcp9601 and sensor config

[PATCH] mcp9600: Add support for mcp9601 and sensor config

[Ben Collins]

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

The mcp9600 dt binding doc claims to support thermocouple-type but
I don't see where this is implemented.

- Add support to detect mcp9601 device type
- Add support to use thermocouple-type dt prop
- Add thrermocouple iio info to get/set this from sysfs
- Add filter-level dt prop to set the filtering level of the chip
- Update dt binding docs

Signed-off-by: Ben Collins <bcollins@kernel.org>
Cc: Andrew Hepp <andrew.hepp@ahepp.dev>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: "Nuno Sá" <nuno.sa@analog.com>
---
 .../iio/temperature/microchip,mcp9600.yaml    |   9 +
 drivers/iio/temperature/mcp9600.c             | 185 ++++++++++++++++--
 2 files changed, 181 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..0ee0259471c6a 100644
--- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
@@ -42,6 +42,14 @@ properties:
       Use defines in dt-bindings/iio/temperature/thermocouple.h.
       Supported types are B, E, J, K, N, R, S, T.
 
+  filter-level:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Level of chip filtering to use. 0 means filtering is disabled.
+      See reference manual 5.2.2 - THERMOCOUPLE SENSOR CONFIGURATION
+      REGISTER, FIGURE 5-6 for Filter Step Response graph. Supported
+      values are in the range of 0-7.
+
   vdd-supply: true
 
 required:
@@ -65,6 +73,7 @@ examples:
             interrupts = <25 IRQ_TYPE_EDGE_RISING>;
             interrupt-names = "open-circuit";
             thermocouple-type = <THERMOCOUPLE_TYPE_K>;
+            filter-level = <1>;
             vdd-supply = <&vdd>;
         };
     };
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 6e9108d5cf75f..c1fe1e530786c 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -18,15 +18,23 @@
 #include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <dt-bindings/iio/temperature/thermocouple.h>
 
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 
 /* MCP9600 registers */
-#define MCP9600_HOT_JUNCTION 0x0
-#define MCP9600_COLD_JUNCTION 0x2
+#define MCP9600_HOT_JUNCTION		0x0
+#define MCP9600_COLD_JUNCTION		0x2
 #define MCP9600_STATUS			0x4
 #define MCP9600_STATUS_ALERT(x)		BIT(x)
+#define MCP9600_STATUS_OC_IR		BIT(4)
+#define MCP9601_STATUS_SC		BIT(5)
+#define MCP9600_SENSOR_CFG		0x5
+#define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
+#define MCP9600_SENSOR_TYPE(x)		((x << 4) & MCP9600_SENSOR_TYPE_MASK)
+#define MCP9600_FILTER_MASK		GENMASK(2, 0)
+#define MCP9600_FILTER(x)		((x << 0) & MCP9600_FILTER_MASK)
 #define MCP9600_ALERT_CFG1		0x8
 #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
 #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
@@ -38,10 +46,11 @@
 #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_DEVICE_ID_MCP9601	0x41
 
 #define MCP9600_ALERT_COUNT		4
 
@@ -58,6 +67,21 @@ enum mcp9600_alert {
 	MCP9600_ALERT4
 };
 
+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,
+};
+
+static const int mcp9600_tc_types[] = {
+	'B', 'E', 'J', 'K', 'N', 'R', 'S', 'T'
+};
+
 static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
 	[MCP9600_ALERT1] = "alert1",
 	[MCP9600_ALERT2] = "alert2",
@@ -87,8 +111,12 @@ static const struct iio_event_spec mcp9600_events[] = {
 		{							       \
 			.type = IIO_TEMP,				       \
 			.address = MCP9600_HOT_JUNCTION,		       \
-			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
-					      BIT(IIO_CHAN_INFO_SCALE),	       \
+			.info_mask_separate =                                  \
+					BIT(IIO_CHAN_INFO_RAW) |	       \
+					BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
+					BIT(IIO_CHAN_INFO_SCALE),              \
+			.info_mask_separate_available =                        \
+					BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE),  \
 			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
 			.num_event_specs = hj_num_ev,			       \
 		},							       \
@@ -125,6 +153,9 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	unsigned char dev_id;
+	u32 thermocouple_type;
+	u32 filter;
 };
 
 static int mcp9600_read(struct mcp9600_data *data,
@@ -155,13 +186,82 @@ 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;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+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_THERMOCOUPLE_TYPE:
+		return IIO_VAL_CHAR;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9600_config(struct mcp9600_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret, cfg;
+
+	cfg  = MCP9600_SENSOR_TYPE(mcp9600_type_map[data->thermocouple_type]) |
+		MCP9600_FILTER(data->filter);
+
+	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_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 tc_type = -1;
+	int i, ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
+		for (i = 0; i < ARRAY_SIZE(mcp9600_tc_types); i++) {
+			if (mcp9600_tc_types[i] == toupper(val)) {
+				tc_type = i;
+				break;
+			}
+		}
+		if (tc_type < 0)
+			return -EINVAL;
+
+		data->thermocouple_type = tc_type;
+		ret = mcp9600_config(data);
+		if (ret < 0)
+			return ret;
+
+		break;
+
 	default:
 		return -EINVAL;
 	}
+
+	return 0;
 }
 
 static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
@@ -299,12 +399,32 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
 	}
 }
 
+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)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
+		*vals = mcp9600_tc_types;
+		*type = IIO_VAL_CHAR;
+		*length = ARRAY_SIZE(mcp9600_tc_types);
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info mcp9600_info = {
 	.read_raw = mcp9600_read_raw,
+	.write_raw = mcp9600_write_raw,
+	.write_raw_get_fmt = mcp9600_write_raw_get_fmt,
 	.read_event_config = mcp9600_read_event_config,
 	.write_event_config = mcp9600_write_event_config,
 	.read_event_value = mcp9600_read_thresh,
 	.write_event_value = mcp9600_write_thresh,
+	.read_avail = mcp9600_read_avail,
 };
 
 static irqreturn_t mcp9600_alert_handler(void *private,
@@ -418,26 +538,65 @@ static int mcp9600_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
 	struct mcp9600_data *data;
-	int ret, ch_sel;
+	int ch_sel, dev_id, ret;
 
-	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);
+	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:
+		dev_info(&client->dev, "Identified as mcp9600");
+		break;
+	case MCP9600_DEVICE_ID_MCP9601:
+		dev_info(&client->dev, "Identified as mcp9601");
+		break;
+
+	default:
+		return dev_err_probe(&client->dev, -EINVAL, "Unknown device ID: %x\n",
+				     dev_id);
+	}
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
+
+	ret = device_property_read_u32(&client->dev, "thermocouple-type",
+				       &data->thermocouple_type);
+	if (ret) {
+		dev_warn(&client->dev,
+			 "Missing thermocouple-type property, using Type-K\n");
+		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
+	} else if (data->thermocouple_type < 0 || data->thermocouple_type >=
+		   ARRAY_SIZE(mcp9600_type_map)) {
+		dev_warn(&client->dev,
+			 "Invalid thermocouple-type property, using Type-K\n");
+		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
+	}
+
+	ret = device_property_read_u32(&client->dev, "filter-level",
+				       &data->filter);
+	if (ret) {
+		dev_warn(&client->dev,
+			 "Missing filter-level property, using 0\n");
+		data->filter = 0;
+	} else if (data->filter < 0 || data->filter > 7) {
+		dev_warn(&client->dev,
+			 "Invalid filter-level property, using 0\n");
+		data->filter = 0;
+	}
+
+	data->dev_id = dev_id;
 	data->client = client;
 
 	ch_sel = mcp9600_probe_alerts(indio_dev);
 	if (ch_sel < 0)
 		return ch_sel;
 
+	mcp9600_config(data);
+
 	indio_dev->info = &mcp9600_info;
 	indio_dev->name = "mcp9600";
 	indio_dev->modes = INDIO_DIRECT_MODE;
-- 
2.50.1

-- 
 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] mcp9600: Add support for mcp9601 and sensor config

[David Lechner]

On 8/12/25 8:08 AM, Ben Collins wrote:
> The mcp9600 dt binding doc claims to support thermocouple-type but
> I don't see where this is implemented.
> 
> - Add support to detect mcp9601 device type
> - Add support to use thermocouple-type dt prop
> - Add thrermocouple iio info to get/set this from sysfs
> - Add filter-level dt prop to set the filtering level of the chip
> - Update dt binding docs

This is too much for one patch. Split the thermocouple type and
filter level into separate patches.

> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> Cc: Andrew Hepp <andrew.hepp@ahepp.dev>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: "Nuno Sá" <nuno.sa@analog.com>
> ---

The dependency on your other patch should be mentioned here.
Or just include the other patch in the series too.

>  .../iio/temperature/microchip,mcp9600.yaml    |   9 +
>  drivers/iio/temperature/mcp9600.c             | 185 ++++++++++++++++--
>  2 files changed, 181 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..0ee0259471c6a 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> @@ -42,6 +42,14 @@ properties:
>        Use defines in dt-bindings/iio/temperature/thermocouple.h.
>        Supported types are B, E, J, K, N, R, S, T.
>  
> +  filter-level:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Level of chip filtering to use. 0 means filtering is disabled.
> +      See reference manual 5.2.2 - THERMOCOUPLE SENSOR CONFIGURATION
> +      REGISTER, FIGURE 5-6 for Filter Step Response graph. Supported
> +      values are in the range of 0-7.
> +

Devicetree binding changes need to be in a separate patch and CC the
relevant maintainers.

However, this looks like something that doesn't belong in a devicetree.
Unless you can justify why the filtering level depends on how the
system is wired up, this is more likely something that should be
done at runtime.

I only had a very quick look a the datasheet, but I think using
IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY to control the filter
range would make sense.

>    vdd-supply: true
>  
>  required:
> @@ -65,6 +73,7 @@ examples:
>              interrupts = <25 IRQ_TYPE_EDGE_RISING>;
>              interrupt-names = "open-circuit";
>              thermocouple-type = <THERMOCOUPLE_TYPE_K>;
> +            filter-level = <1>;
>              vdd-supply = <&vdd>;
>          };
>      };
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 6e9108d5cf75f..c1fe1e530786c 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -18,15 +18,23 @@
>  #include <linux/minmax.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <dt-bindings/iio/temperature/thermocouple.h>
>  
>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  /* MCP9600 registers */
> -#define MCP9600_HOT_JUNCTION 0x0
> -#define MCP9600_COLD_JUNCTION 0x2
> +#define MCP9600_HOT_JUNCTION		0x0
> +#define MCP9600_COLD_JUNCTION		0x2

Whitespace changes should also be in a separate patch to avoid
distraction from the actual changes.

>  #define MCP9600_STATUS			0x4
>  #define MCP9600_STATUS_ALERT(x)		BIT(x)
> +#define MCP9600_STATUS_OC_IR		BIT(4)
> +#define MCP9601_STATUS_SC		BIT(5)
> +#define MCP9600_SENSOR_CFG		0x5
> +#define MCP9600_SENSOR_TYPE_MASK	GENMASK(6, 4)
> +#define MCP9600_SENSOR_TYPE(x)		((x << 4) & MCP9600_SENSOR_TYPE_MASK)

We typically avoid making macros like this and just use
FIELD_PREP() at the call site.

> +#define MCP9600_FILTER_MASK		GENMASK(2, 0)
> +#define MCP9600_FILTER(x)		((x << 0) & MCP9600_FILTER_MASK)
>  #define MCP9600_ALERT_CFG1		0x8
>  #define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
>  #define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> @@ -38,10 +46,11 @@
>  #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_DEVICE_ID_MCP9601	0x41
>  
>  #define MCP9600_ALERT_COUNT		4
>  
> @@ -58,6 +67,21 @@ enum mcp9600_alert {
>  	MCP9600_ALERT4
>  };
>  
> +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,
> +};
> +
> +static const int mcp9600_tc_types[] = {
> +	'B', 'E', 'J', 'K', 'N', 'R', 'S', 'T'
> +};
> +
>  static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
>  	[MCP9600_ALERT1] = "alert1",
>  	[MCP9600_ALERT2] = "alert2",
> @@ -87,8 +111,12 @@ static const struct iio_event_spec mcp9600_events[] = {
>  		{							       \
>  			.type = IIO_TEMP,				       \
>  			.address = MCP9600_HOT_JUNCTION,		       \
> -			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	       \
> -					      BIT(IIO_CHAN_INFO_SCALE),	       \
> +			.info_mask_separate =                                  \
> +					BIT(IIO_CHAN_INFO_RAW) |	       \
> +					BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> +					BIT(IIO_CHAN_INFO_SCALE),              \
> +			.info_mask_separate_available =                        \
> +					BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE),  \

Since the type is set the the devicetree and can't be changed, there doesn't
seems much sense in listing the available types.

>  			.event_spec = &mcp9600_events[hj_ev_spec_off],	       \
>  			.num_event_specs = hj_num_ev,			       \
>  		},							       \
> @@ -125,6 +153,9 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>  
>  struct mcp9600_data {
>  	struct i2c_client *client;
> +	unsigned char dev_id;

This isn't used outside of the probe function, so doesn't need to be here.

> +	u32 thermocouple_type;
> +	u32 filter;

filter_level

>  };
>  
>  static int mcp9600_read(struct mcp9600_data *data,
> @@ -155,13 +186,82 @@ 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;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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_THERMOCOUPLE_TYPE:
> +		return IIO_VAL_CHAR;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp9600_config(struct mcp9600_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret, cfg;
> +
> +	cfg  = MCP9600_SENSOR_TYPE(mcp9600_type_map[data->thermocouple_type]) |
> +		MCP9600_FILTER(data->filter);
> +
> +	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_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 tc_type = -1;
> +	int i, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:

Why would we want to write over what was set in the devicetree?

> +		for (i = 0; i < ARRAY_SIZE(mcp9600_tc_types); i++) {
> +			if (mcp9600_tc_types[i] == toupper(val)) {
> +				tc_type = i;
> +				break;
> +			}
> +		}
> +		if (tc_type < 0)
> +			return -EINVAL;
> +
> +		data->thermocouple_type = tc_type;
> +		ret = mcp9600_config(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		break;

return 0; here

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

rather than here

>  }
>  
>  static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> @@ -299,12 +399,32 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +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)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> +		*vals = mcp9600_tc_types;
> +		*type = IIO_VAL_CHAR;
> +		*length = ARRAY_SIZE(mcp9600_tc_types);
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct iio_info mcp9600_info = {
>  	.read_raw = mcp9600_read_raw,
> +	.write_raw = mcp9600_write_raw,
> +	.write_raw_get_fmt = mcp9600_write_raw_get_fmt,
>  	.read_event_config = mcp9600_read_event_config,
>  	.write_event_config = mcp9600_write_event_config,
>  	.read_event_value = mcp9600_read_thresh,
>  	.write_event_value = mcp9600_write_thresh,
> +	.read_avail = mcp9600_read_avail,
>  };
>  
>  static irqreturn_t mcp9600_alert_handler(void *private,
> @@ -418,26 +538,65 @@ static int mcp9600_probe(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev;
>  	struct mcp9600_data *data;
> -	int ret, ch_sel;
> +	int ch_sel, dev_id, ret;
>  
> -	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);
> +	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:
> +		dev_info(&client->dev, "Identified as mcp9600");
> +		break;
> +	case MCP9600_DEVICE_ID_MCP9601:
> +		dev_info(&client->dev, "Identified as mcp9601");
> +		break;
> +

Experience has show that depending on ICs to report the correct device ID
is fragile. Instead, we should depend on the compatible string. Printing
this for debug purposes is acceptable though, but should be a separate
patch. Just don't fail if it is an unexpected value.

> +	default:
> +		return dev_err_probe(&client->dev, -EINVAL, "Unknown device ID: %x\n",
> +				     dev_id);
> +	}
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
> +
> +	ret = device_property_read_u32(&client->dev, "thermocouple-type",
> +				       &data->thermocouple_type);
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Missing thermocouple-type property, using Type-K\n");

The devicetree bindings should have a default listed. So this should not be
a warning, but rather the expected default.

> +		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +	} else if (data->thermocouple_type < 0 || data->thermocouple_type >=
> +		   ARRAY_SIZE(mcp9600_type_map)) {
> +		dev_warn(&client->dev,
> +			 "Invalid thermocouple-type property, using Type-K\n");

Should fail with error if the devicetree gives an invalid value.

> +		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +	}
> +
> +	ret = device_property_read_u32(&client->dev, "filter-level",
> +				       &data->filter);
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Missing filter-level property, using 0\n");
> +		data->filter = 0;
> +	} else if (data->filter < 0 || data->filter > 7) {
> +		dev_warn(&client->dev,
> +			 "Invalid filter-level property, using 0\n");
> +		data->filter = 0;
> +	}
> +
> +	data->dev_id = dev_id;
>  	data->client = client;
>  
>  	ch_sel = mcp9600_probe_alerts(indio_dev);
>  	if (ch_sel < 0)
>  		return ch_sel;
>  
> +	mcp9600_config(data);
> +
>  	indio_dev->info = &mcp9600_info;
>  	indio_dev->name = "mcp9600";
>  	indio_dev->modes = INDIO_DIRECT_MODE;

Re: [PATCH] mcp9600: Add support for mcp9601 and sensor config

[Jonathan Cameron]

On Tue, 12 Aug 2025 09:08:30 -0400
Ben Collins <bcollins@kernel.org> wrote:

> The mcp9600 dt binding doc claims to support thermocouple-type but
> I don't see where this is implemented.
> 
> - Add support to detect mcp9601 device type
> - Add support to use thermocouple-type dt prop
> - Add thrermocouple iio info to get/set this from sysfs
> - Add filter-level dt prop to set the filtering level of the chip
> - Update dt binding docs
> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> Cc: Andrew Hepp <andrew.hepp@ahepp.dev>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: "Nuno Sá" <nuno.sa@analog.com>

Hi

I tried not to overlap too much with David's review.
A few additional things inline.

Jonathan

> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 6e9108d5cf75f..c1fe1e530786c 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c

> +
> +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_THERMOCOUPLE_TYPE:
> +		return IIO_VAL_CHAR;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp9600_config(struct mcp9600_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret, cfg;
> +
> +	cfg  = MCP9600_SENSOR_TYPE(mcp9600_type_map[data->thermocouple_type]) |
> +		MCP9600_FILTER(data->filter);
> +
> +	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
> +
No blank line here. Keep error check associated with the call.
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to set sensor configuration\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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 tc_type = -1;
> +	int i, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
If this is something we specify in DT (which I think is valid) then
we probably should not allow override from userspace.

We might want a read only parameter though as type of thermocouple is
probably something that is useful to be able to read back.

> +		for (i = 0; i < ARRAY_SIZE(mcp9600_tc_types); i++) {
> +			if (mcp9600_tc_types[i] == toupper(val)) {
> +				tc_type = i;
> +				break;
> +			}
> +		}
> +		if (tc_type < 0)
> +			return -EINVAL;
> +
> +		data->thermocouple_type = tc_type;
> +		ret = mcp9600_config(data);
> +		if (ret < 0)
> +			return ret;
		return mcp9600_config(data);
Assuming that never returns > 0.
> +
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> +
> +	return 0;
Might as well return rather than break above.
>  }


>  static irqreturn_t mcp9600_alert_handler(void *private,
> @@ -418,26 +538,65 @@ static int mcp9600_probe(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev;
>  	struct mcp9600_data *data;
> -	int ret, ch_sel;
> +	int ch_sel, dev_id, ret;
>  
> -	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);
> +	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:
> +		dev_info(&client->dev, "Identified as mcp9600");

Too noisy.  It is fine to over ride the DT compatible if we match a known ID, but if
not paper over the missmatch so that in future a new chip can be supported via
a fall back compatible entry without a kernel upgrade.  dev_info on an unknown
device is fine though.

> +		break;
> +	case MCP9600_DEVICE_ID_MCP9601:
> +		dev_info(&client->dev, "Identified as mcp9601");
> +		break;
> +
> +	default:
> +		return dev_err_probe(&client->dev, -EINVAL, "Unknown device ID: %x\n",
> +				     dev_id);
> +	}
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
> +
> +	ret = device_property_read_u32(&client->dev, "thermocouple-type",
> +				       &data->thermocouple_type);
> +	if (ret) {

Likewise, must have a default value if it wasn't there from start so with
that in mind can use a similar pattern to the below suggestion.


> +		dev_warn(&client->dev,
> +			 "Missing thermocouple-type property, using Type-K\n");
> +		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +	} else if (data->thermocouple_type < 0 || data->thermocouple_type >=
> +		   ARRAY_SIZE(mcp9600_type_map)) {
> +		dev_warn(&client->dev,
> +			 "Invalid thermocouple-type property, using Type-K\n");
> +		data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> +	}
> +
> +	ret = device_property_read_u32(&client->dev, "filter-level",
> +				       &data->filter);
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Missing filter-level property, using 0\n");
> +		data->filter = 0;
If we were keeping this a common pattern for this sort of property (which can't
be required as wasn't there from the start) is
make data->filter a u32 if it isn't already.

	data->filter = 0;
	device_property_read_u32(&client->dev, "filter-level", &data->filter);
	if (data->filter > 7) {
		dev_warn(&...
		data->filter = 0;
	}

That is rely on no side effects in the property read if it returns an error,
unsigned property not less than 0  and that 0 is an allowed value.

However as David said unlikely to be something that belongs in DT.


> +	} else if (data->filter < 0 || data->filter > 7) {
> +		dev_warn(&client->dev,
> +			 "Invalid filter-level property, using 0\n");
> +		data->filter = 0;
> +	}
> +
> +	data->dev_id = dev_id;
>  	data->client = client;
>  
>  	ch_sel = mcp9600_probe_alerts(indio_dev);
>  	if (ch_sel < 0)
>  		return ch_sel;
>  
> +	mcp9600_config(data);
> +
>  	indio_dev->info = &mcp9600_info;
>  	indio_dev->name = "mcp9600";
>  	indio_dev->modes = INDIO_DIRECT_MODE;

Re: [PATCH] mcp9600: Add support for mcp9601 and sensor config

[Krzysztof Kozlowski]

On 12/08/2025 15:08, Ben Collins wrote:
> The mcp9600 dt binding doc claims to support thermocouple-type but
> I don't see where this is implemented.
> 
> - Add support to detect mcp9601 device type
> - Add support to use thermocouple-type dt prop
> - Add thrermocouple iio info to get/set this from sysfs
> - Add filter-level dt prop to set the filtering level of the chip
> - Update dt binding docs
> 
> Signed-off-by: Ben Collins <bcollins@kernel.org>
> Cc: Andrew Hepp <andrew.hepp@ahepp.dev>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: "Nuno Sá" <nuno.sa@analog.com>

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument, so you will
not CC people just because they made one commit years ago). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.
</form letter>


Best regards,
Krzysztof