Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Jonathan Cameron]

On Tue, 15 Apr 2025 16:26:22 +0300
<victor.duicu@xxxxxxxxxxxxx> wrote:

> From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
> 
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
Hi Victor,

Please state briefly here in what way the parts are incompatible
as a justification for no fallback compatibles.  Quite a bit
of that will become apparent when you enforce validity of parameters
as suggested below.

Various comments inline.
> 
> Signed-off-by: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
> ---
>  .../iio/temperature/microchip,mcp9982.yaml    | 182 ++++++++++++++++++
>  1 file changed, 182 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> new file mode 100644
> index 000000000000..8cbf897d1278
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
> +
> +maintainers:
> +  - Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
> +
> +description: |
> +  The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
> +  automotive temperature monitor.
> +  The datasheet can be found here:
> +    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp9933
> +      - microchip,mcp9933D
> +      - microchip,mcp9982
> +      - microchip,mcp9982D
> +      - microchip,mcp9983
> +      - microchip,mcp9983D
> +      - microchip,mcp9984
> +      - microchip,mcp9984D
> +      - microchip,mcp9985
> +      - microchip,mcp9985D
> +
> +  reg:
> +    maxItems: 1
> +    
> +  interrupts:
> +    maxItems: 2
> +    
> +  interrupt-names:
> +    description: |
> +      ALERT1 indicates a HIGH or LOW limit was exceeded.
> +      ALERT2 indicates a THERM limit was exceeded.
> +    items:
> +      - const: ALERT1
> +      - const: ALERT2
> +    
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  microchip,temp-hysteresis:
> +    description: |
> +      Value of temperature limit hysteresis.
> +      Omit this tag to set the default value.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Can we just make this a userspace thing using appropriate _hysteresis ABI element?


> +    
> +  microchip,extended-temp-range:
> +    description: |
> +      Set the chip to work in the extended temperature range -64 degrees C to 191.875 degrees C.
> +      Omit this tag to set the default range 0 degrees C to 127.875 degrees C
> +    type: boolean

I'm curious.  Why does this belong in the DT binding?

> +    
> +  microchip,beta-channel1:
> +    description: |
> +      The beta compensation factor for external channel 1 can be set
> +      by the user, or can be set automatically by the chip.
> +      If one wants to enable beta autodetection, omit this tag.
> +      Please consult the documentation if one wants to set a specific beta.
> +      If anti-parallel diode operation is enabled, the default value is set
> +      and can't be changed.
> +    type: boolean

Why is this a hardware thing that belongs in dt?  Enforce the constraint
in the schema rather than text.

> +    
> +  microchip,beta-channel2:
> +    description: |
> +      The beta compensation factor for external channel 2 can be set
> +      by the user, or can be set automatically by the chip.
> +      If one wants to enable beta autodetection, omit this tag.
> +      Please consult the documentation if one wants to set a specific beta.
> +      If anti-parallel diode operation is enabled, the default value is set
> +      and can't be changed.
> +    type: boolean
> +    
> +  microchip,apdd-state:
> +    description: |
> +      Enable anti-parallel diode mode operation.
> +      Omit this tag to disable anti-parallel diode mode by default.

This one is unusual.  Maybe a little more description (I looked it up
and am fine with why this is in DT)

> +    type: boolean
> +    
> +  microchip,recd12:
> +    description: |

No need for | on paragraphs where formatting doesn't need to be maintained.

> +      Enable resistance error correction for external channels 1 and 2.
> +      Not all chips support resistance error correction on external
> +      channels 1 and 2, please consult the documentation.

Enforce it in the schema, no need to say that chips don't support it
in text. Look at the various allOf statements with compatible matches
in other bindings for how to do that.

> +      Omit this tag to disable REC for channels 1 and 2 by default.
> +    type: boolean
> +    
> +  microchip,recd34:
> +    description: |
> +      Enable resistance error correction for external channels 3 and 4.
> +      Not all chips support resistance error correction on external
> +      channels 3 and 4, please consult the documentation.
> +      Omit this tag to disable REC for channels 3 and 4 by default.
> +    type: boolean
> +    
> +  label:
> +    description: Unique name to identify which device this is.
> +    
> +  vdd-supply: true
> + 
> +patternProperties:
> +  "^channel@[1-4]+$":
> +    description: |
> +      Represents the external temperature channels to which a remote diode is
> +      connected.
> +    type: object
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 1
> +          maximum: 4
> +      
> +      microchip,ideality-factor:
> +        description: |
> +          Each channel has an ideality factor.
> +          Beta compensation and resistance error correction automatically correct
> +          for most ideality error. So ideality factor does not need to be adjusted in general.

wrap at 80 chars. Also try to avoid explicit formatting where it isn't needed.

> +          Omit this tag in order to set the default value.
> +          Please consult the documentation if one wants to set a specific ideality value.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +      
> +      label:
> +        description: Unique name to identify which channel this is.
> +    
> +    required:
> +      - reg
> +    
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        
> +        temperature-sensor@4c {
> +            compatible = "microchip,mcp9985";
> +            reg = <0x4c>;
> +            
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            label = "temperature-sensor";
> +            
> +            microchip,temp-hysteresis = <10>;
> +            microchip,extended-temp-range;
> +            microchip,apdd-state;
> +            microchip,recd12;
> +            microchip,recd34;
> +            vdd-supply = <&vdd>;
> +            
> +            channel@1{
> +                reg = <0x1>;
> +                label = "CPU Temperature";
> +            };
> +            
> +            channel@2{
> +                reg = <0x2>;
> +                label = "GPU Temperature";
> +            };
> +        };
> +    };
> +
> +...