On 03/10/2022 19:06, Michał Grzelak wrote: > On 02/10/2022 10:23, Marcin Wojtas wrote: >> niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski >> <krzysztof.kozlowski@xxxxxxxxxx> napisał(a): >>> >>> On 01/10/2022 17:53, Michał Grzelak wrote: >>>> Hi Krzysztof, >>>> >>>> Thanks for your comments and time spent on reviewing my patch. >>>> All of those improvements will be included in next version. >>>> Also, I would like to know your opinion about one. >>>> >>>>>> + >>>>>> + marvell,system-controller: >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>>> + description: a phandle to the system controller. >>>>>> + >>>>>> +patternProperties: >>>>>> + '^eth[0-9a-f]*(@.*)?$': >>>>> >>>>> The name should be "(ethernet-)?port", unless anything depends on >>>>> particular naming? >>>> >>>> What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"? >>>> It resembles pattern found in net/ethernet-phy.yaml like >>>> properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while >>>> still passing `dt_binding_check' and `dtbs_check'. It should also >>>> comply with your comment. >>> >>> Node names like ethernet-eth do not make much sense because they contain >>> redundant ethernet or eth. AFAIK, all other bindings like that call >>> these ethernet-ports (or sometimes shorter - ports). Unless this device >>> is different than all others? >>> >> >> IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine >> (as long as we don't have to modify the existing .dtsi files) - there >> is no dependency in the driver code on that. > > Indeed, driver's code isn't dependent; however, there is a dependency > on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.: > > https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190 > https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72 > > Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files > including the above would have to be modified to pass through `dtbs_check'. I didn't get it. What is the "dependency"? Usage of some names is not a dependency... Old bindings were not precising any specific name of subnodes, therefore I commented to change it. If the DTS already use some other name, you can change them if none of upstream implementations (BSD, bootloaders, firmware, Linux kernel) depend on it. Best regards, Krzysztof
