edtlib: link child nodes to parent for nodes with child-bindings #62417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabiobaltieri
force-pushed
the
edt-childs
branch
3 times, most recently
from
2ba5252
to
6a4a37d
Compare
fabiobaltieri
requested review from
mbolivar-ampere,
galak,
ifyall and
npal-cy
as code owners
zephyrbot
added
area: Devicetree
platform: Infineon
zephyrbot
requested review from
brockus-zephyr,
Dino-Li,
GTLin08,
jackrosenthal,
jeremybettis,
keith-zephyr,
nashif,
nordicjm,
sjg20 and
tejlmand
|
@gmarull note the first two patches moves around few pinctrl nodes for ite and psoc6, this is necessary to avoid circular dependencies, could not really find any better solution to avoid that and keep those nodes under the same parent. It's not really a problem since the pin definition have no actual driver but the script does not know about that. |
keith-zephyr
reviewed
Sep 8, 2023
fabiobaltieri
force-pushed
the
edt-childs
branch
from
6a4a37d
to
e1eeadc
Compare
gmarull
reviewed
Sep 12, 2023
fabiobaltieri
force-pushed
the
edt-childs
branch
from
e1eeadc
to
f336570
Compare
gmarull
previously approved these changes
Sep 13, 2023
mbolivar-ampere
requested changes
Sep 13, 2023
Comment on lines
2058
to
2059
| Link node properties to parent_node. Iterates over child nodes | ||
| recursively if necessary. |
There was a problem hiding this comment.
The purpose of the function is not clear from its name and docstring; can you please expand this? The arguments are parent_node and node but the caller is passing node, node. This feels confusingly written and the scope is a bit muddy.
There was a problem hiding this comment.
Fair enough, I've renamed functions and variables, added few commments, reworded the function comment and added a wrapper for the first call. Let me know if that's clear enough.
fabiobaltieri
force-pushed
the
edt-childs
branch
2 times, most recently
from
07efa9c
to
6c04f8f
Compare
gmarull
previously approved these changes
Sep 14, 2023
fabiobaltieri
force-pushed
the
edt-childs
branch
from
6c04f8f
to
58c015a
Compare
|
@gmarull sorry I just re-reworded the comments. :-) |
fabiobaltieri
force-pushed
the
edt-childs
branch
from
58c015a
to
df4dc0a
Compare
gmarull
previously approved these changes
Sep 14, 2023
fabiobaltieri
force-pushed
the
edt-childs
branch
from
df4dc0a
to
0c898b6
Compare
|
rebased (merge conflict in |
gmarull
previously approved these changes
Sep 21, 2023
Refactor the pinctrl nodes slightly so that the port devices are not child of the main pinctrl node. This is because the pinctrl node is being used as parent for pinctrl setting nodes itself, and having the port nodes as child end up creating a circular depdency with the edt child enumeration patch. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Refactor the pinctrl nodes slightly so that the port devices are not child of the main pinctrl node. This is because the pinctrl node is being used as parent for pinctrl setting nodes itself, and having the port nodes as child end up creating a circular depdency with the edt child enumeration patch. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
The current EDT graph logic only use properties directly under a specific node to add dependencies. For nodes properties in child-bindings, this means that the child phandles are only linked by the child node itself, which does have an ordinal but no corresponding "sturct device" in the code, causing those dependencies to be silently ignored by gen_handles.py. Fix that by adding the recursive logic to visit child bindings when present, which causes all child node property handles to be linked to the parent node. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Now that child nodes are handled by edtlib there's no need to parse child nodes in check_init_priorities anymore. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
fabiobaltieri
force-pushed
the
edt-childs
branch
from
0c898b6
to
cd62221
Compare
|
one more conflict, rebase again, @mbolivar-ampere ping |
|
@mbolivar-ampere ping |
mbolivar-ampere
approved these changes
Oct 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, this is pretty much a reiteration of #55657. At the time I did not realize that it was actually fixing an issue and dropped it, but now that we have ordinals in the device init sequence it's pretty clear that not doing this is an issue.
Example of the problem:
Before:
After:
In the first case, the missing dependency is causing the ordinal of
pwmledsdevices to be lower than thepwmone that they depend on, that means that if they were set with the same level and priority, they would initialize in the incorrect order.Also tested on https://github.com/zephyrproject-rtos/zephyr-testing/tree/vfabio-edt-child-branch (two unrelated breakages there that don't show up on the main repo for some reason)