Re: [RFC PATCH 1/3] dt-bindings: arm64: bcmbca: Merge BCM4908 into BCMBCA

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4541 bytes --]



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.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel