[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

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

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

[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

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