Re: [PATCH] dt-bindings: thermal: convert hisilicon-thermal.txt to dt-schema

[Krzysztof Kozlowski]

On 14/06/2024 00:42, Abdulrasaq Lawani wrote:
> Convert the hisilicon SoCs tsensor txt bindings to dt-schema
> 
> Signed-off-by: Abdulrasaq Lawani <abdulrasaqolawani@xxxxxxxxx>
> ---
> Validated with dtschema and tested against `hi3660-hikey960.dts`
> 


> diff --git a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.yaml b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.yaml
> new file mode 100644
> index 000000000000..56ded6ebe1b2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.yaml

Filename: hisilicon,tsensor.yaml

> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/hisilicon-thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Temperature Sensor on hisilicon SoCs
> +
> +maintainers:
> +  - Abdulrasaq Lawani <abdulrasaqolawani@xxxxxxxxx>
> +

Missing $ref to thermal-sensor.yaml#

> +properties:
> +  compatible:
> +    items:

No need for items

> +      - enum:
> +          - hisilicon,tsensor
> +          - hisilicon,hi3660-tsensor
> +
> +  reg:
> +    description: physical base address of thermal sensor and length of memory mapped region.

Drop description, pointless.

> +    minItems: 1
> +    maxItems: 2

Instead you need to list items and describe them. But don't repeat
redundant parts like "physical base address". Just say which block is this.

Or... it's just wrong. Why two items?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: thermal_clk
> +
> +  interrupts:
> +    description:
> +      The interrupt number to the cpu. Defines the interrupt used
> +      by /SOCTHERM/tsensor.

No need for description, it's redudant.

> +    maxItems: 1
> +
> +  # See Documentation/devicetree/bindings/thermal/thermal-sensor.yaml for details

Drop

> +  '#thermal-sensor-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - '#thermal-sensor-cells'
> +
> +additionalProperties: false

unevaluatedProperties instead (after adding ref)
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/hi6220-clock.h>
> +
> +     // for Hi6220:
> +     tsensor: tsensor@0,f7030700 {

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 = "hisilicon,tsensor";
> +     reg = <0x0 0xf7030700 0x0 0x1000>;

Oh man...  get this past your mentors first.

1. Messed indentation.
2. 0, unit address looks unnecessary.


> +     interrupts = <0 7 0x4>;

Use proper defines for both common constants. There is a reason you
included arm-gic header, right?

> +     clocks = <&sys_ctrl HI6220_TSENSOR_CLK>;
> +     clock-names = "thermal_clk";
> +     #thermal-sensor-cells = <1>;
> +     };
> +
> +     // for Hi3660:
> +     tsensor1: tsensor@fff30000 {

Drop entire node. One example is enough.


Best regards,
Krzysztof