Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 20/05/2022 11:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Add binding description for temperature channels. Currently, support for
> label and offset is implemented.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> index 066c02541fcf..9a5aa78d4db1 100644
> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> @@ -62,6 +62,37 @@ required:
>  
>  additionalProperties: false
>  
> +patternProperties:

Which models use this?

> +  "^channel@([0-2])$":
> +    type: object
> +    description: |

No need for |

> +      Represents channels of the device and their specific configuration.
> +
> +    properties:
> +      reg:
> +        description: |

The same.

> +          The channel number. 0 is local channel, 1-2 are remote channels.
> +        items:
> +          minimum: 0
> +          maximum: 2
> +
> +      label:
> +        description: |

The same.

> +          A descriptive name for this channel, like "ambient" or "psu".
> +
> +      offset:
> +        description: |

This does not look like standard property, so you need vendor and unit
suffix.

> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> +          (if supported by device).

You described programming model which should not be put in the bindings.
Please describe the hardware.

> +          For remote channels only.
> +        $ref: /schemas/types.yaml#/definitions/int32
> +        default: 0
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
>  examples:
>    - |
>      #include <dt-bindings/interrupt-controller/irq.h>
> @@ -76,5 +107,13 @@ examples:
>              vcc-supply = <&palmas_ldo6_reg>;
>              interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
>              #thermal-sensor-cells = <1>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
I assume you tested the bindings with dt_bindings_check?

I have some doubts, as this should fail.

> +
> +            channel@0 {
> +                reg = <0x0>;
> +                label = "internal";
> +                offset = <1000>;
> +            };
>          };
>      };


Best regards,
Krzysztof