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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/04/2025 15:26, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
> 
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. 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.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

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

Wrap according to coding style document.

> +
> +maintainers:
> +  - Victor Duicu <victor.duicu@microchip.com>
> +
> +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

alert1

> +      - const: ALERT2

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

temperature is in specific units, see property units in dtschema.

> +    
> +  microchip,extended-temp-range:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Set the chip to work in the extended temperature range -64 degrees C to 191.875 degrees C.

does not look wrapped.

> +      Omit this tag to set the default range 0 degrees C to 127.875 degrees C
> +    type: boolean

Why is this a property of the board? I imagine someone might want
different accuracy during runtime.

> +    
> +  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
> +    
> +  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.
> +    type: boolean
> +    
> +  microchip,recd12:
> +    description: |
> +      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.
> +      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

Then this should be restricted per compatible in allOf:if:then:.

> +      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]+$":

4 or 44? Drop +

> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      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.
> +          Omit this tag in order to set the default value.

default: X

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

additionalProperties or reference appropriate schema for subnode.

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

Missing spaces. Please see DTS coding style or any DTS style.



Best regards,
Krzysztof