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

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

[Ben Collins]

ChangeLog:
v6 -> v7:
  - Separate out the mcp9600 IIR series into its own series as there is
    a lot of conversation around implementation (removed related
    comments from this changelog).

v5 -> v6:
  - Fix accidental typo added in dt-bindings: IRQ_TYPE_EDGE_RISIN
  - Correct some constraints in dt-bindings
  - Reverse if/then for mcp9601 vs mcp9600 constraints in dt-bindings
  - Updates to changelog for patch 2/6 (dt-bindings mcp9600)
  - Cleanup tabs that were converted to spaces
  - Split thermocouple-type default to separate patch

v4 -> v5:
  - None

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

v1 -> v2:
  - Break into individual patches

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

Signed-off-by: Ben Collins <bcollins@watter.com>
---
Ben Collins (5):
      dt-bindings: iio: mcp9600: Set default 3 for thermocouple-type
      dt-bindings: iio: mcp9600: Add microchip,mcp9601 and add constraints
      iio: mcp9600: White space and fixed width cleanup
      iio: mcp9600: Recognize chip id for mcp9601
      iio: mcp9600: Add support for thermocouple-type

 .../iio/temperature/microchip,mcp9600.yaml         |  59 ++++++++-
 drivers/iio/temperature/Kconfig                    |   8 +-
 drivers/iio/temperature/mcp9600.c                  | 146 ++++++++++++++++++---
 3 files changed, 186 insertions(+), 27 deletions(-)
---
base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
change-id: 20250819-upstream-changes-c89af86743fa

Best regards,
-- 
Ben Collins <bcollins@watter.com>

[PATCH v7 1/5] dt-bindings: iio: mcp9600: Set default 3 for thermocouple-type

[Ben Collins]

As is already documented in this file, Type-K is the default, so make
that explicit in the dt-bindings.

Signed-off-by: Ben Collins <bcollins@watter.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
index d2cafa38a544..57b387a1accc 100644
--- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
@@ -37,6 +37,7 @@ properties:
 
   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.

-- 
2.39.5

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

[Ben Collins]

Add microchip,mcp9601 compatible in addition to the original
microchip,mcp9600 to designate support between these two chips.

The current dt-binding has open-circuit and short-circuit as interrupt
names, but these are only supported in mcp9601.

The OC and SC detection requires that mcp9601 VSENSE be wired up, which
not only enables the OC SC interrupts, but also the OC and SC status
register bits.

Add a microchip,vsense boolean to show the chip is wired for this
support.

Add constraints so this feature only applies if the mcp9601 compatible
is selected.

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

diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
index 57b387a1accc..6506ae429d16 100644
--- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Microchip MCP9600 thermocouple EMF converter
+title: Microchip MCP9600 and similar thermocouple EMF converters
 
 maintainers:
   - Andrew Hepp <andrew.hepp@ahepp.dev>
@@ -14,7 +14,11 @@ description:
 
 properties:
   compatible:
-    const: microchip,mcp9600
+    oneOf:
+      - const: microchip,mcp9600
+      - items:
+          - const: microchip,mcp9601
+          - const: microchip,mcp9600
 
   reg:
     maxItems: 1
@@ -43,8 +47,37 @@ properties:
       Use defines in dt-bindings/iio/temperature/thermocouple.h.
       Supported types are B, E, J, K, N, R, S, T.
 
+  microchip,vsense:
+    type: boolean
+    description:
+      This flag indicates that the chip has been wired with VSENSE to
+      enable open and short circuit detect.
+
   vdd-supply: true
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              const: microchip,mcp9601
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          maxItems: 4
+        interrupt-names:
+          minItems: 1
+          maxItems: 4
+          items:
+            enum:
+              - alert1
+              - alert2
+              - alert3
+              - alert4
+        microchip,vsense: false
+
 required:
   - compatible
   - reg
@@ -64,8 +97,27 @@ examples:
             reg = <0x60>;
             interrupt-parent = <&gpio>;
             interrupts = <25 IRQ_TYPE_EDGE_RISING>;
-            interrupt-names = "open-circuit";
+            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@62 {
+            compatible = "microchip,mcp9601",
+                         "microchip,mcp9600";
+            microchip,vsense;
+            reg = <0x62>;
+            interrupt-parent = <&gpio>;
+            interrupts = <22 IRQ_TYPE_EDGE_RISING
+                          23 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "open-circuit", "short-circuit";
+            vdd-supply = <&vdd>;
+        };
+    };

-- 
2.39.5

[PATCH v7 3/5] iio: mcp9600: White space and fixed width cleanup

[Ben Collins]

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

Signed-off-by: Ben Collins <bcollins@watter.com>
Reviewed-by: David Lechner <dlechner@baylibre.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 6e9108d5cf75..40906bb200ec 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 v7 4/5] iio: mcp9600: Recognize chip id for mcp9601

[Ben Collins]

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.

Moved "struct mcp9600_data" up in the file since a later patch will
need it and chip_info before the declerations.

Signed-off-by: Ben Collins <bcollins@watter.com>
Reviewed-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/temperature/Kconfig   |  8 +++--
 drivers/iio/temperature/mcp9600.c | 63 ++++++++++++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d50..9328b2250ace 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 40906bb200ec..4654b3aaaf2a 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
 
@@ -82,6 +83,15 @@ static const struct iio_event_spec mcp9600_events[] = {
 	},
 };
 
+struct mcp_chip_info {
+	u8 chip_id;
+	const char *chip_name;
+};
+
+struct mcp9600_data {
+	struct i2c_client *client;
+};
+
 #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
 	{								       \
 		{							       \
@@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
 	MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
 };
 
-struct mcp9600_data {
-	struct i2c_client *client;
-};
-
 static int mcp9600_read(struct mcp9600_data *data,
 			struct iio_chan_spec const *chan, int *val)
 {
@@ -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)
+		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 v7 5/5] iio: mcp9600: Add support for thermocouple-type

[Ben Collins]

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>
Reviewed-by: David Lechner <dlechner@baylibre.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 4654b3aaaf2a..cd46f48e47d6 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 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,
@@ -90,14 +118,34 @@ struct mcp_chip_info {
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	u32 thermocouple_type;
 };
 
+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;
+}
+
 #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
 	{								       \
 		{							       \
 			.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,			       \
@@ -165,6 +213,9 @@ 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;
 	}
@@ -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

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

[Krzysztof Kozlowski]

On Tue, Aug 19, 2025 at 07:44:43PM -0400, Ben Collins wrote:
> +  - |
> +    #include <dt-bindings/iio/temperature/thermocouple.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temperature-sensor@62 {
> +            compatible = "microchip,mcp9601",
> +                         "microchip,mcp9600";

One line.

> +            microchip,vsense;
> +            reg = <0x62>;

reg is always the second property. microchip,vsense goes to the end as
vendor property, see DTS coding style.

> +            interrupt-parent = <&gpio>;
> +            interrupts = <22 IRQ_TYPE_EDGE_RISING
> +                          23 IRQ_TYPE_EDGE_RISING>;

Two tuples <>, not one. It also looks like it fits in 80-char limit/

With above changes:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v7 3/5] iio: mcp9600: White space and fixed width cleanup

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>
> Make tabs consistent for register definitions and also fix width
> to byte size.

Reviewed-by: Andy Shevchenko <abdy@kernel.org>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/5] iio: mcp9600: Recognize chip id for mcp9601

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>
> 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.
>
> Moved "struct mcp9600_data" up in the file since a later patch will
> need it and chip_info before the declerations.

declarations

...

> +struct mcp9600_data {
> +       struct i2c_client *client;
> +};
> +
>  #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
>         {                                                                      \
>                 {                                                              \
> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>         MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
>  };
>
> -struct mcp9600_data {
> -       struct i2c_client *client;
> -};
> -

It's not obvious why this piece of change is needed. AFAICS it's a stray change.

...

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

It's hard to maintain and prone to subtle errors if we split
assignment and check, so please move assignment here.

       chip_info = i2c_get_match_data(client);

> +       if (!chip_info)
> +               return dev_err_probe(&client->dev, -EINVAL,

In such cases we usually use ENODEV.

> +                                    "No chip-info found for device\n");

...

> +               return dev_err_probe(&client->dev, dev_id,
> +                                    "Failed to read device ID\n");

With

  struct device *dev = &client->dev;

at the top of the function this and other statements become neater and
easier to follow. In particular, I believe this one may become a one
liner after the change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/5] iio: mcp9600: Add support for thermocouple-type

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>
> 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.

...

> +       /* 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)

' < 0' part is redundant.

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

Maybe here as well, but I haven't checked the actual code of the callee.

> +               return ret;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/5] iio: mcp9600: Recognize chip id for mcp9601

[Ben Collins]

> On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>> 
>> 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.
>> 
>> Moved "struct mcp9600_data" up in the file since a later patch will
>> need it and chip_info before the declerations.
> 
> declarations
> 
> ...
> 
>> +struct mcp9600_data {
>> +       struct i2c_client *client;
>> +};
>> +
>> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
>>        {                                                                      \
>>                {                                                              \
>> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>>        MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
>> };
>> 
>> -struct mcp9600_data {
>> -       struct i2c_client *client;
>> -};
>> -
> 
> It's not obvious why this piece of change is needed. AFAICS it's a stray change.

The explanation is in the changelog above. A follow up patch needs both struct
declarations to be where I added one and moved mcp9600_data to. It’s just ordering
so I don’t later have to forward declare new functions for filter_type, which make
use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
declaration.

I guess I could move mcp9600_data in that series, but I had this in here before
I split that series out, and it seemed simple enough to leave in.

Re: [PATCH v7 4/5] iio: mcp9600: Recognize chip id for mcp9601

[Ben Collins]

> On Aug 20, 2025, at 7:11 AM, Ben Collins <bcollins@watter.com> wrote:
> 
>> 
>> On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> 
>> On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>>> 
>>> 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.
>>> 
>>> Moved "struct mcp9600_data" up in the file since a later patch will
>>> need it and chip_info before the declerations.
>> 
>> declarations
>> 
>> ...
>> 
>>> +struct mcp9600_data {
>>> +       struct i2c_client *client;
>>> +};
>>> +
>>> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
>>>       {                                                                      \
>>>               {                                                              \
>>> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>>>       MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
>>> };
>>> 
>>> -struct mcp9600_data {
>>> -       struct i2c_client *client;
>>> -};
>>> -
>> 
>> It's not obvious why this piece of change is needed. AFAICS it's a stray change.
> 
> The explanation is in the changelog above. A follow up patch needs both struct
> declarations to be where I added one and moved mcp9600_data to. It’s just ordering
> so I don’t later have to forward declare new functions for filter_type, which make
> use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
> declaration.
> 
> I guess I could move mcp9600_data in that series, but I had this in here before
> I split that series out, and it seemed simple enough to leave in.

Correction, patch 5/5 needs it. Either I move it in 4/5 (cleaner since I was
already adding a new related struct), or in 5/5, or a longer reorg to include
moving that struct and mcp9600_config() in the mcp9600-iir series.

Re: [PATCH v7 4/5] iio: mcp9600: Recognize chip id for mcp9601

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 07:11:08AM -0400, Ben Collins wrote:
> > On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:

...

> >> +struct mcp9600_data {
> >> +       struct i2c_client *client;
> >> +};
> >> +
> >> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
> >>        {                                                                      \
> >>                {                                                              \
> >> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
> >>        MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
> >> };
> >> 
> >> -struct mcp9600_data {
> >> -       struct i2c_client *client;
> >> -};
> >> -
> > 
> > It's not obvious why this piece of change is needed. AFAICS it's a stray change.
> 
> The explanation is in the changelog above. A follow up patch needs both struct
> declarations to be where I added one and moved mcp9600_data to. It’s just ordering
> so I don’t later have to forward declare new functions for filter_type, which make
> use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
> declaration.
> 
> I guess I could move mcp9600_data in that series, but I had this in here before
> I split that series out, and it seemed simple enough to leave in.

The usual thing is to avoid changes that are not used. In this series this move
is not used, put it to the patch which actually needs it.

-- 
With Best Regards,
Andy Shevchenko