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

On 2022-07-13 20:37, William Zhang wrote:

Hi Rafal,

On 7/13/22 03:58, Rafał Miłecki wrote:

quoted

On 2022-07-13 12:50, Rafał Miłecki wrote:

quoted

On 2022-07-13 02:57, William Zhang wrote:

quoted

On 7/12/22 11:18, Krzysztof Kozlowski wrote:

quoted

On 12/07/2022 19:37, William Zhang wrote:

quoted

quoted

quoted

+      - 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!

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