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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  .../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@xxxxxxxxx>
> @@ -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