Messages in this thread | | | Date | Wed, 13 Jul 2022 13:29:38 -0700 | Subject | Re: [RFC PATCH 1/3] dt-bindings: arm64: bcmbca: Merge BCM4908 into BCMBCA | From | William Zhang <> |
| |
On 7/13/22 13:23, Rafał Miłecki wrote: > On 2022-07-13 20:37, William Zhang wrote: >> Hi Rafal, >> >> On 7/13/22 03:58, Rafał Miłecki wrote: >>> On 2022-07-13 12:50, Rafał Miłecki wrote: >>>> On 2022-07-13 02:57, William Zhang wrote: >>>>> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>>>>> On 12/07/2022 19:37, William Zhang wrote: >>>>>>>>> + - description: BCM4908 Family based boards >>>>>>>>> + items: >>>>>>>>> + - enum: >>>>>>>>> + # BCM4908 SoC based boards >>>>>>>>> + - brcm,bcm94908 >>>>>>>>> + - asus,gt-ac5300 >>>>>>>>> + - netgear,raxe500 >>>>>>>>> + # BCM4906 SoC based boards >>>>>>>>> + - brcm,bcm94906 >>>>>>>>> + - netgear,r8000p >>>>>>>>> + - tplink,archer-c2300-v1 >>>>>>>>> + - enum: >>>>>>>>> + - brcm,bcm4908 >>>>>>>>> + - brcm,bcm4906 >>>>>>>>> + - brcm,bcm49408 >>>>>>>> >>>>>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not >>>>>>>> look >>>>>>>> like valid list of compatibles. >>>>>>>> >>>>>>> For 4908 board variant, it will need to be followed by 4908 chip. >>>>>>> Sorry >>>>>>> for the basic question but is there any requirement to enforce >>>>>>> this kind >>>>>>> of rule? I would assume dts writer know what he/she is doing and >>>>>>> select >>>>>>> the right combination. >>>>>> >>>>>> The entire point of DT schema is to validate DTS. Combination like >>>>>> above >>>>>> prevents that goal. >>>>>> >>>>>> Best regards, >>>>>> Krzysztof >>>>> Understand the DT schema purpose. But items property allows multiple >>>>> enums in the list which gives a lot of flexibility but make it hard to >>>>> validate. I am not familiar with DT schema, is there any directive to >>>>> specify one enum value depending on another so dts validation tool can >>>>> report error if combination is wrong? >>>>> >>>>> This is our preferred format of all bcmbca compatible string >>>>> especially when we could have more than 10 chip variants for the same >>>>> chip family and we really want to work on the chip family id. We will >>>>> make sure they are in the right combination in our own patch and patch >>>>> from other contributors. Would this work? If not, I will probably have >>>>> to revert the change of 4908(maybe append brcm,bcmbca as this chip >>>>> belongs to the same bca group) and use "enum board variant", "const >>>>> main chip id", "brcm,bca" for all other chips as our secondary choice. >>>> >>>> I'm not sure why I didn't even receive 1/3 and half of discussion >>>> e-mails. >>>> >>>> You can't just put all strings into a single bag and allow mixing them >>>> in any combos. Please check how it's properly handled in the current >>>> existing binding: >>>> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml >>>> >>>> Above binding enforces that non-matching compatible strings are not >>>> used >>>> together. >>> >>> I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so >>> you must be aware of that file. >>> >>> So you see a cleanly working binding in the brcm,bcm4908.yaml but >>> instead copying it you decided to wrote your own one from scratch. >>> Incorrectly. >>> >>> This smells of NIH (not invented here). Please just use that binding I >>> wrote and move if it needed. >> >> Not mean to discredit any of your work and I did copy over your >> binding and combine them into one SoC entry to the new bcmbca.yaml and >> add you as one of the maintainer to this file. As this change would >> certainly concern you, that's why I sent RFC first. As I explained in >> the cover letter, the purpose of the change is to reduce the number of >> compatible strings and keep one entry for one chip family due to >> possible large number of chip variants. But since there is no way to >> validate the combination, I will copy the existing 4908 bindings as >> they are now > > Right. I believe we need that. > > >> but I would propose to append "brcm, bcmbca" as it is >> part of bcmbca chip. And for the other chips, we would just use enum >> "board variant", const "main chip id", const "brcm,bca". Does that >> sound good to you? > > Nitpicking: you meant "brcm,bcmbca" (typo) but sounds absolutely fine! Yup its a typo. Will append "brcm,bcmbca" and send out new patch. [unhandled content-type:application/pkcs7-signature]
| |