On Tue, Feb 27, 2024 at 09:54:30AM +0530, Vignesh Raghavendra wrote:

On 26/02/24 20:05, Théo Lebrun wrote:

quoted

On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote:

quoted

On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote:

quoted

Hello Conor,

On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:

quoted

On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote:

quoted

Compatible can be A or B, not A or B or A+B. Remove last option.
A=ti,j721e-usb and B=ti,am64-usb.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..949f45eb45c2 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml

@@ -11,12 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    oneOf:
-      - const: ti,j721e-usb
-      - const: ti,am64-usb
-      - items:
-          - const: ti,j721e-usb
-          - const: ti,am64-usb

Correct, this makes no sense. The devices seem to be compatible though,
so I would expect this to actually be:
oneOf:
  - const: ti,j721e-usb
  - items:
      - const: ti,am64-usb
      - const: ti,j721e-usb

I need your help to grasp what that change is supposed to express? Would
you mind turning it into english sentences?
A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
that a device can either be compat with A or B. But B is compatible
with A so you express it as a list of items. If B is compat with A then
A is compat with B. Does the order of items matter?

The two devices are compatible with each other, based on an inspection of
the driver and the existing "A+B" setup. If this was a newly submitted
binding, "B" would not get approved because "A+B" allows support without
software changes and all that jazz.

Your patch says that allowing "A", "B" and "A+B" makes no sense and you
suggest removing "A+B". I am agreeing that it makes no sense to allow
all 3 of these situations.

What I also noticed is other problems with the binding. What should have
been "A+B" is actually documented as "B+A", but that doesn't make sense
when the originally supported device is "A".

This A and B stuff confused me, I should just have used the actual
compatibles. I meant
| What should have been "B+A" is actually documented as "A+B", but that
| doesn't make sense when the originally supported device is "A"

quoted

quoted

Therefore my suggestion was to only allow "A" and "A+B", which is what
we would (hopefully) tell you to do were you submitting the am64 support
as a new patch today.

Thank you for the in-depth explanation! It makes much more sense now,
especially the handling of historic stuff that ideally wouldn't have
been done this way but that won't be changed from now on.

IIRC, idea behind adding new compatible for AM64 even though register
map is very much compatible is just being future proof as AM64 and J721e
belong to different product groups and thus have differences wrt SoC
level integration etc which may need SoC specific handling later on.

That is fine, I don't think anyone here is disputing a soc-specific
compatible existing for this device.

Also, note that AM64 SoC support was added long after J721e. So ideally
should be B+A if at all we need a fallback compatible.

Correct, I accidentally wrote "A+B", but you can see that that
conflicted with the actual example I had given above.

I don't see any DT (now or in the past) using

compatible = B,A or compatible = A,B

So do we really need A+B to be supported by binding?

Given the mistake, I am going to take this as meaning should the
fallback be supported. My take is that if we are going to remove
something, it should be "ti,am64-usb" isolation that should go.
The devicetrees can be update without concerns about compatibility.

Cheers,
Conor.