linux-kernel - Re: [PATCH v1 1/2] dt-bindings: usb: Add Eswin EIC7700 Usb controller

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: <20250519-transparent-eggplant-beagle-3abb7c@kuoka>
Date: Mon, 19 May 2025 08:24:56 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: zhangsenchuan@...incomputing.com
Cc: gregkh@...uxfoundation.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, Thinh.Nguyen@...opsys.com, p.zabel@...gutronix.de, 
	ningyu@...incomputing.com, linmin@...incomputing.com, yangwei1@...incomputing.com
Subject: Re: [PATCH v1 1/2] dt-bindings: usb: Add Eswin EIC7700 Usb controller

On Fri, May 16, 2025 at 05:53:37PM GMT, zhangsenchuan@...incomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> 
> Add Device Tree binding documentation for the ESWIN EIC7700
> usb controller module.
> 
> Co-developed-by: Wei Yang <yangwei1@...incomputing.com>
> Signed-off-by: Wei Yang <yangwei1@...incomputing.com>
> Signed-off-by: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> ---
>  .../bindings/usb/eswin,eic7700-usb.yaml       | 120 ++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/eswin,eic7700-usb.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/eswin,eic7700-usb.yaml b/Documentation/devicetree/bindings/usb/eswin,eic7700-usb.yaml
> new file mode 100644
> index 000000000000..eb8260069b99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/eswin,eic7700-usb.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/eswin,eic7700-usb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Eswin EIC7700 SoC Usb Controller
> +
> +maintainers:
> +  - Wei Yang <yangwei1@...incomputing.com>
> +  - Senchuan Zhang <zhangsenchuan@...incomputing.com>
> +
> +description: |
> +  The Usb controller on EIC7700 SoC.
> +
> +properties:
> +  compatible:
> +    const: eswin,eic7700-usb
> +
> +  reg:
> +    maxItems: 3
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: peripheral
> +
> +  clocks:
> +    maxItems: 3
> +    description: handles to clock for the usb controller.

Useless description, drop. Do not explain what DT syntax is.

> +
> +  clock-names:
> +    items:
> +      - const: suspend
> +      - const: aclk
> +      - const: cfg_clk
> +    description: the name of each clock.

Drop description.

> +
> +  resets:
> +    description: resets to be used by the controller.

Missing constraints.

> +
> +  reset-names:
> +    items:
> +      - const: vaux
> +    description: names of the resets listed in resets property in the same order.

Drop description.

> +
> +  eswin,hsp_sp_csr:

DTS coding style

> +    description: |

Drop |

> +      High-speed equipment control register.

For what purpose?

> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Missing constraints.

> +
> +  ranges: true
> +
> +  dma-noncoherent: true
> +
> +  numa-node-id:
> +    maximum: 0

Huh? What is the point of this if this is fixed to 0?

There is no way this passes any tests. Please look at writing-schema
docs.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - interrupt-parent
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - eswin,hsp_sp_csr
> +  - dma-noncoherent
> +  - ranges
> +  - numa-node-id
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    usb {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        usbdrd3_0: usb0{

usb, missing space, drop unused label

> +          compatible = "eswin,eic7700-dwc3";
> +          #address-cells = <2>;
> +          #size-cells = <2>;

Order properties according to DTS coding style

> +          clocks = <&clock 270>,
> +                   <&clock 545>,
> +                   <&clock 546>;
> +          clock-names = "suspend","aclk", "cfg_clk";
> +          eswin,hsp_sp_csr = <&hsp_sp_csr 0x800 0x808 0x83c 0x840>;
> +          resets = <&reset 0x07 (1 << 15)>;
> +          reset-names = "vaux";
> +          ranges;
> +          numa-node-id = <0>;
> +          status = "disabled";

No. You cannot make it disable and it makes no sense.

> +          usbdrd_dwc3_0: dwc3@...80000 {

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

Fold the child node into the parent.

Best regards,
Krzysztof