On 24/06/2024 13:59, Adam Ford wrote: >>> diff --git a/Documentation/devicetree/bindings/net/davinci_emac.yaml b/Documentation/devicetree/bindings/net/davinci_emac.yaml >>> new file mode 100644 >>> index 000000000000..4c2640aef8a1 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/net/davinci_emac.yaml >> >> Filename matching compatible format. Missing vendor prefix. Underscores >> are not used in names or compatibles. > > Thank you for the review. > > Would a proper name be ti,davinci-emac.yaml? Yes, it's fine. > >> >> >>> @@ -0,0 +1,111 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/net/davinci_emac.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Texas Instruments Davici EMAC >>> + >>> +maintainers: >>> + - Adam Ford <aford@xxxxxxxxx> >>> + >>> +description: >>> + Ethernet based on the Programmable Real-Time Unit and Industrial >>> + Communication Subsystem. >>> + >>> +allOf: >>> + - $ref: ethernet-controller.yaml# >>> + >>> +properties: >>> + compatible: >>> + items: >> >> >> That's just enum, no need for items here. >> >>> + - enum: >>> + - ti,davinci-dm6467-emac # da850 >>> + - ti,dm816-emac >>> + - ti,am3517-emac >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + minItems: 4 >> >> You need to list and describe the items. >> >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + clock-names: >>> + items: >>> + - const: ick >>> + >>> + power-domains: >>> + maxItems: 1 >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + local-mac-address: true >> >> Drop >> >>> + mac-address: true >> >> Drop >> >> You miss top-level $ref to appropriate schema. >> >>> + >>> + syscon: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: a phandle to the global system controller on >>> + to enable/disable interrupts >> >> Drop entire property. There was no such property in old binding and >> nothing explains why it was added. > > The am3517.dtsi emac node has a syscon, so I didn't want to break it. > I'll take a look to see what the syscon node on the am3517 does. I > struggle with if statements in yaml, but if it's necessary for the > am3517, can we keep it if I elaborate on it in the commit message? Explain in commit msg changes to the binding done during conversion. With a rationale why these are needed (e.g. existing DTS and Linux drivers use them). Best regards, Krzysztof
