Re: [PATCH 2/2] dt-bindings: hwmon: Add TI TPS389008

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/03/2025 18:02, Flaviu Nistor wrote:
> Add device tree bindings and an example for the
> TI TPS389008 voltage monitor.
> 
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
> ---

This wasn't ever tested, so limited review.

BTW, bindings are before the user.

> +properties:
> +  compatible:
> +    oneOf:
> +      - const: ti,tps38908
> +      - items:
> +          - enum:
> +              - ti,tps38908
> +              - ti,tps38906
> +              - ti,tps38904
> +          - const: ti,tps38908
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    description: Required only if a child node is present.

Drop description

> +    const: 1
> +
> +  "#size-cells":
> +    description: Required only if a child node is present.

Drop description


> +    const: 0
> +
> +patternProperties:
> +  "^vmon@[1-8]$":

s/vmon/channel/

> +    description: |
> +      The node contains optional child nodes for up to x channels.
> +      There are 8 channels for TPS389008, 6 channels for TPS389006,
> +      and 4 channels for TPS389004. Each child node describes the information

Then you need allOf:if:then: disallowing these for specific variants.

> +      of the input source. Input channels default to disabled in the chip.
> +      Unless channels are explicitly enabled in device-tree,
> +      input channels will be disabled.
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      reg:
> +        description: |
> +          Must be 1 to 8,

Don't repeat constraints in free form text.

> corresponding to the VMONx
> +          ports of the TPS389008, TPS389006, or TPS389004, respectively.

Drop entire description

> +        enum: [ 1, 2, 3, 4, 5, 6, 7, 8 ]
> +
> +      label:
> +        description: Name of the input source.
> +
> +      ti,vrange-mult-4x:
> +        description: |
> +          Must have values 5 or 20 in order for the analog
> +          conversion to be done correctly.
> +          Value 5 indicates multiply by 1 (x1), and value 20 indicates
> +          multiply by 4 (x4).
> +        type: boolean
> +
> +      ti,vmon-enable:

Drop. Status tells it, no?

> +        description: |
> +          Sets each channel status. By default, all input channels are disabled.
> +            - 0 = channel disabled
> +            - 1 = channel active
> +        type: boolean
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        vmon@37 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "ti,tps389008", "ti,tps389006", "ti,tps389004";

Test your code before sending, not after.



Best regards,
Krzysztof