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

[Krzysztof Kozlowski]

On Fri, May 16, 2025 at 05:53:37PM GMT, zhangsenchuan@xxxxxxxxxxxxxxxxxx wrote:
> From: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx>
> 
> Add Device Tree binding documentation for the ESWIN EIC7700
> usb controller module.
> 
> Co-developed-by: Wei Yang <yangwei1@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Wei Yang <yangwei1@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx>
> ---
>  .../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@xxxxxxxxxxxxxxxxxx>
> +  - Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx>
> +
> +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@50480000 {

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