Re: [PATCH] dt-bindings: usb: dwc3-st: convert to dt schema

[Krzysztof Kozlowski]

On Fri, Dec 13, 2024 at 10:45:55AM +0530, Kanak Shilledar wrote:
> removed st,syscon from the required property, added st,syscfg
> property.

Please say in the commit msg why.

> 
> Signed-off-by: Kanak Shilledar <kanakshilledar@xxxxxxxxx>
> ---
>  .../devicetree/bindings/usb/dwc3-st.txt       |  66 ----------
>  .../bindings/usb/st,stih407-dwc3.yaml         | 123 ++++++++++++++++++
>  2 files changed, 123 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/usb/dwc3-st.txt
>  create mode 100644 Documentation/devicetree/bindings/usb/st,stih407-dwc3.yaml

...

> diff --git a/Documentation/devicetree/bindings/usb/st,stih407-dwc3.yaml b/Documentation/devicetree/bindings/usb/st,stih407-dwc3.yaml
> new file mode 100644
> index 000000000000..709cdb17f28d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/st,stih407-dwc3.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/st,stih407-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ST DWC3 glue logic
> +
> +description:
> +  This binding describes the parameters for the dwc3-st driver,
> +  which controls the glue logic used to configure the DWC3 core on
> +  STiH407-based platforms.
> +
> +maintainers:
> +  - Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    const: st,stih407-dwc3
> +
> +  reg:
> +    # minItems: 2

Drop line

> +    items:
> +      - description: Glue logic base address.
> +      - description: USB syscfg ctrl register offset.
> +
> +  reg-names:
> +    # minItems: 2

Drop line

> +    items:
> +      - const: reg-glue
> +      - const: syscfg-reg
> +
> +  st,syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to system configuration node which.

which?

> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: powerdown
> +      - const: softreset
> +
> +  st,syscfg:

What's this? Missing type, description.

> +    maxItems: 1
> +
> +  '#address-cells':

Keep consistent quotes, either ' or "

> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
> +  pinctrl-names:
> +    items:
> +      - const: default

Drop property

> +
> +  pinctrl-0:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      Pin control group
> +      See: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +

Drop property

> +  ranges:
> +    description: Valid 1:1 translation between child's and parent's address space.

Drop description, just :true


> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - resets
> +  - reset-names
> +  - pinctrl-names
> +  - pinctrl-0


address/size cells should be required. This device makes no sense
without actual DWC3 core.


> +  - ranges
> +
> +additionalProperties: false
> +
> +patternProperties:

patternProperties follow immediately "properties:" block.

> +  "^usb@[0-9a-f]+$":
> +    type: object
> +    description: DWC3 core sub-node
> +    $ref: snps,dwc3.yaml#
> +    unevaluatedProperties: false
> +    properties:
> +      dr_mode:
> +        enum:
> +          - host
> +          - device

That's odd, device is not a valid value. I guess you took it from the
old binding, but it was not correct. I don't think you tested this part
with different values.

> +        description: |
> +          Specifies the operating mode. The default value "otg" is not
> +          supported by this SoC. Valid values are "host" or "device".
> +          See: Documentation/devicetree/bindings/usb/usb-drd.yaml

Drop description, redundant.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/phy/phy.h>
> +    #include <dt-bindings/reset/stih407-resets.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    st_dwc3: dwc3@8f94000 {
> +        compatible	= "st,stih407-dwc3";

Only one space before '='

> +        reg		= <0x08f94000 0x1000>, <0x110 0x4>;
> +        reg-names	= "reg-glue", "syscfg-reg";
> +        st,syscfg	= <&syscfg_core>;
> +        resets		= <&powerdown STIH407_USB3_POWERDOWN>,
> +              <&softreset STIH407_MIPHY2_SOFTRESET>;

Some total misalignment.

Best regards,
Krzysztof