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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <afb7551e-404e-440b-92c5-6927c61417ab@kernel.org>
Date: Fri, 14 Jun 2024 10:49:10 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Abdulrasaq Lawani <abdulrasaqolawani@...il.com>, rafael@...nel.org,
 daniel.lezcano@...aro.org, rui.zhang@...el.com, lukasz.luba@....com,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
 javier.carrasco.cruz@...il.com
Subject: Re: [PATCH] dt-bindings: thermal: convert hisilicon-thermal.txt to
 dt-schema

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@...il.com>
> ---
> 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@...il.com>
> +

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@...30000 {

Drop entire node. One example is enough.


Best regards,
Krzysztof