Re: [PATCH] dt-bindings: usb: Document the Microchip USB2514 hub

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/04/2024 18:41, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> Document the Microchip USB2514, USB2412, and USB2417 USB hubs.

There is 2514b already. Why it cannot be there? Is the existing file
only for I2C interface and here you add on-board-hub approach interface?

If so, mention it briefly in commit msg (one sentence is enough).


> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
>  .../bindings/usb/microchip,usb2514.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> new file mode 100644
> index 000000000000..8df7a5adfbe8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/microchip,usb2514.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip USB2514 Hub Controller
> +
> +maintainers:
> +  - Fabio Estevam <festevam@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - usb424,2412
> +      - usb424,2514
> +      - usb424,2417

Please keep the list ordered.

> +
> +  reg: true
> +
> +  reset-gpios:
> +    description: GPIO connected to the RESET_N pin.
> +
> +  vdd-supply:
> +    description: 3.3V power supply.
> +
> +  clocks:
> +    description: External 24MHz clock connected to the CLKIN pin.

maxItems.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: true

No, this must be false.

This does not make really sense. You miss $ref... and when you do not
have $ref you should use additionalProperties: false. Open existing
bindings for device of the same class.


> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6qdl-clock.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    usb {
> +        dr_mode = "host";

Drop property, it's kind of expected/obvious and we want to limit
chances schema will complain about something unrelated to your device.

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        hub@1 {
> +          compatible = "usb424,2514";

Inconsistent indentation. Use 4 spaces for example indentation.

> +          reg = <1>;
> +          clocks = <&clks IMX6QDL_CLK_CKO>;
> +          reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> +          vdd-supply = <&reg_3v3_hub>;
> +        };
> +    };

Best regards,
Krzysztof