Re: [PATCH v2] dt-bindings: usb: ti,keystone-dwc3.yaml: Improve schema

[Roger Quadros]

Hi,

On 15/06/2020 20:41, Rob Herring wrote:
> On Fri, Jun 05, 2020 at 10:52:15AM +0300, Roger Quadros wrote:
> > There were some review comments after the patch was integrated.
> > Address those.
> >
> > Fixes: 1883a934e156 ("dt-bindings: usb: convert keystone-usb.txt to YAML")
> > Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> > ---
> >
> > Changelog:
> > v2
> > - don't use quotes for enum/const string
> > - use phandle instead of phandle-array for phys
> > - add maxItems for phy-names
> >
> >   .../bindings/usb/ti,keystone-dwc3.yaml        | 50 ++++++++++++++-----
> >   1 file changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml b/Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml
> > index f127535feb0b..394e47d2f5d7 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml
> > @@ -11,64 +11,88 @@ maintainers:
> >   properties:
> >     compatible:
> > -    oneOf:
> > -      - const: "ti,keystone-dwc3"
> > -      - const: "ti,am654-dwc3"
> > +    items:
> > +      - enum:
> > +        - ti,keystone-dwc3
> > +        - ti,am654-dwc3
> >     reg:
> >       maxItems: 1
> > -    description: Address and length of the register set for the USB subsystem on
> > -      the SOC.
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 1
> > +
> > +  ranges: true
>
> blank line

There is a blank line before interrupts.

> >     interrupts:
> >       maxItems: 1
> > -    description: The irq number of this device that is used to interrupt the MPU.
> > -
> >     clocks:
> > +    $ref: /schemas/types.yaml#definitions/phandle-array
>
> Common property. It already has a type and doesn't need to be
> redefined here.
>
> Just "maxItems: 1" if it is a single clock is enough, or an 'items' list
> for each entry if more than 1.

Some nodes need 1 and some need 2. So "minItems: 1" and "maxItems: 2"?
The driver doesn't really care, and relies of device/clock framework
to just set the right clock parent.

In this case do I need to add items list?

cheers,
-roger

> >       description: Clock ID for USB functional clock.
>
> Drop.
>
> > +  assigned-clocks:
> > +    $ref: /schemas/types.yaml#definitions/phandle-array
>
>
> > +
> > +  assigned-clock-parents:
> > +    $ref: /schemas/types.yaml#definitions/phandle-array
> > +
> >     power-domains:
> > +    $ref: /schemas/types.yaml#definitions/phandle-array
>
> Same as 'clocks'.
>
> >       description: Should contain a phandle to a PM domain provider node
> >         and an args specifier containing the USB device id
> >         value. This property is as per the binding,
> >         Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
> >     phys:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
>
> Same as 'clocks'.
>
> >       description:
> >         PHY specifier for the USB3.0 PHY. Some SoCs need the USB3.0 PHY
> >         to be turned on before the controller.
> >         Documentation/devicetree/bindings/phy/phy-bindings.txt
> >     phy-names:
> > +    maxItems: 1
> >       items:
> > -      - const: "usb3-phy"
> > +      - const: usb3-phy
>
> Don't need maxItems as that's implied by the length of 'items'.
>
> > +
> > +  dma-coherent: true
> > -  dwc3:
> > +  dma-ranges: true
> > +
> > +patternProperties:
> > +  "usb@[a-f0-9]+$":
> > +    type: object
> >       description: This is the node representing the DWC3 controller instance
> >         Documentation/devicetree/bindings/usb/dwc3.txt
> >   required:
> >     - compatible
> >     - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> >     - interrupts
> > -  - clocks
> > +
> > +additionalProperties: false
> >   examples:
> >     - |
> >       #include <dt-bindings/interrupt-controller/arm-gic.h>
> > -    usb: usb@2680000 {
> > +    dwc3@2680000 {
> >         compatible = "ti,keystone-dwc3";
> >         #address-cells = <1>;
> >         #size-cells = <1>;
> >         reg = <0x2680000 0x10000>;
> >         clocks = <&clkusb>;
> > -      clock-names = "usb";
> >         interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> >         ranges;
> > -      dwc3@2690000 {
> > +      usb@2690000 {
> >           compatible = "synopsys,dwc3";
> >           reg = <0x2690000 0x70000>;
> >           interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> > --
> > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki