Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devicetree bindings path misinterpreted #19536

Closed
pabigot opened this issue Oct 2, 2019 · 3 comments · Fixed by #19806
Closed

devicetree bindings path misinterpreted #19536

pabigot opened this issue Oct 2, 2019 · 3 comments · Fixed by #19806
Assignees
Labels
area: Devicetree bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@pabigot
Copy link
Collaborator

pabigot commented Oct 2, 2019

The Cmake/environment variable DTS_ROOT_BINDINGS provides a list of places that are searched for bindings files, which defaults to include dts/bindings found in APPLICATION_SOURCE_DIR,
BOARD_DIR, and ZEPHYR_BASE in that order. The expectation is that a binding for a specific compatible will be selected from the first location where it is found.

It appears that the current tooling picks the binding in the last directory searched. This is contrary to expected behavior.

@pabigot pabigot added bug The issue is a bug, or the PR is fixing a bug area: Devicetree labels Oct 2, 2019
@ulfalizer
Copy link
Collaborator

ulfalizer commented Oct 5, 2019

I thought having multiple bindings provide the same compatible was an error, until I remembered that that check is only done for filenames in include:.

Is this something you'd use, or just something you noticed?

@pabigot
Copy link
Collaborator Author

pabigot commented Oct 5, 2019

It is something that I have thought I might use: for example to change defaults, or add new properties for a custom driver that needs them.

It becomes more clearly useful if we start following Linux and specifying multiple compatibles in the binding because a driver supports multiple compatibles. There I may want to substitute a driver for one, but not all, of the listed compatibles, and provide additional functionality in that driver that requires more information about the device.

So I think multiple bindings with the same compatible might be useful., but I admit the cases above are theoretical at this time.

There are two acceptable solutions in my view:

  • Do the "expected" and associate the compatible with the first binding file found, ignoring associations in any subsequently found file. (Because compatibles are not associated with file names, reproducibility requires that files in a directory be processed in a well-defined order.)
  • Do the "helpful" and error out if a second binding is later found to have the same compatible

What's really not acceptable is discarding the locally provided binding in favor of a generic one and not saying anything about it. So "something must be done".

It should probably be the same for include (which might be used to inject a change to multiple bindings, in which case include_next would be nice to have alongside it). Based on existing behavior "make it an error" is probably the simplest path at this time.

@dleach02 dleach02 added the priority: low Low impact/importance bug label Oct 8, 2019
@ulfalizer
Copy link
Collaborator

ulfalizer commented Oct 14, 2019

PR to turn it into an error: #19806

ulfalizer added a commit to ulfalizer/zephyr that referenced this issue Oct 14, 2019
Previously, if two bindings had the same 'compatible:'/'parent-bus:'
values, the binding that happened to be loaded last would get used.

Turn it into an error instead. This avoids tricking people into thinking
that bindings get loaded in a defined order.

Maybe overriding bindings could be allowed later, if we need it.

Fixes: zephyrproject-rtos#19536

Signed-off-by: Ulf Magnusson <[email protected]>
carlescufi pushed a commit that referenced this issue Oct 15, 2019
Previously, if two bindings had the same 'compatible:'/'parent-bus:'
values, the binding that happened to be loaded last would get used.

Turn it into an error instead. This avoids tricking people into thinking
that bindings get loaded in a defined order.

Maybe overriding bindings could be allowed later, if we need it.

Fixes: #19536

Signed-off-by: Ulf Magnusson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
3 participants