From a253116f644667e9a9734cf6902ae92573563369 Mon Sep 17 00:00:00 2001 From: Christophe Dufaza Date: Mon, 9 Sep 2024 01:20:38 +0200 Subject: [PATCH] edtlib: preserve paths of properties from included child-bindings All child bindings are initialized at once after included binding files have been merged, with the top-level binding file set as their origin. As a consequence, properties of child bindings appear to all come from ("last modified" semantic) the top-level file. For example, properties of a channel child of an ADC controller with compatible "nordic,nrf-saadc" will all appear last modified by nordic,nrf-saadc.yaml instead of adc-controller.yaml. We can use the same approach we've already come to for property specifications: - https://github.com/zephyrproject-rtos/zephyr/issues/65135 - https://github.com/zephyrproject-rtos/zephyr/pull/65221 Signed-off-by: Christophe Dufaza --- .../src/devicetree/edtlib.py | 169 ++++++++++++++++-- .../tests/test-bindings-include/digest.yaml | 71 ++++++++ .../test-bindings-include/digest_base.yaml | 30 ++++ .../tests/test-bindings-include/left.yaml | 17 ++ .../test-bindings-include/left_right.yaml | 35 ++++ .../tests/test-bindings-include/right.yaml | 17 ++ .../python-devicetree/tests/test_edtlib.py | 159 ++++++++++++++++ 7 files changed, 483 insertions(+), 15 deletions(-) create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/digest.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/digest_base.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/left.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/left_right.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/right.yaml diff --git a/scripts/dts/python-devicetree/src/devicetree/edtlib.py b/scripts/dts/python-devicetree/src/devicetree/edtlib.py index e53925f96182d3c..4004a2709019795 100644 --- a/scripts/dts/python-devicetree/src/devicetree/edtlib.py +++ b/scripts/dts/python-devicetree/src/devicetree/edtlib.py @@ -223,25 +223,32 @@ def __init__(self, path: Optional[str], fname2path: Dict[str, str], # this binding itself defines or modifies self.prop2specs: Dict[str, 'PropertySpec'] = {} + # This binding's child-binding, + # using the same approach as above for property specifications: + # - we initially have no child-binding + # - we may inherit child-binding definitions from other bindings + # when merging included files + # - eventually, we may update inherited child-binding definitions + # with the content of the "child-binding" element of this binding's file + self.child_binding: Optional[Binding] = None + # Save child-binding element before merging. + if "child-binding" in raw: + raw_child_binding = deepcopy(raw["child-binding"]) + else: + raw_child_binding = None + # Merge any included files into self.raw. This also pulls in # inherited child binding definitions, so it has to be done - # before initializing those. + # before initializing this binding's child-binding. self.raw: dict = self._merge_includes(raw, self.path) - # Recursively initialize any child bindings. These don't - # require a 'compatible' or 'description' to be well defined, - # but they must be dicts. - if "child-binding" in raw: - if not isinstance(raw["child-binding"], dict): - _err(f"malformed 'child-binding:' in {self.path}, " - "expected a binding (dictionary with keys/values)") - self.child_binding: Optional['Binding'] = Binding( - path, fname2path, - raw=raw["child-binding"], - require_compatible=False, - require_description=False) - else: - self.child_binding = None + # When this binding's file contains a child-binding element, + # its content will: + # - update the child-binding definitions inherited from included files, + # if any + # - or initialize this binding's child-binding + if raw_child_binding: + self._update_child_binding(raw_child_binding) # Make sure this is a well defined object. self._check(require_compatible, require_description) @@ -414,6 +421,18 @@ def _load_raw(self, fname: str, # Register included property specs. self._add_included_prop2specs(fname, contents, allowlist, blocklist) + # Pull in child-binding definitions we may inherit + # from the included file. + if "child-binding" in contents: + self._add_included_child_binding( + path, + contents["child-binding"], + # Properties in child-binding have already been filtered, + # child_filter is only used to determine if the including + # binding takes the "Last modified here" semantic. + child_filter, + ) + return self._merge_includes(contents, path) def _add_included_prop2specs(self, fname: str, contents: dict, @@ -455,6 +474,126 @@ def _add_included_prop2specs(self, fname: str, contents: dict, if prop not in self.prop2specs: self.prop2specs[prop] = spec + # Add child-binding definitions inherited from an included binding. + # - path: YAML file containing the "child-binding" element + # - raw: The content of the "child-binding" element + # - child_filter: filters applied by the including binding + def _add_included_child_binding( + self, + path: Optional[str], + raw: Dict[str, Any], + child_filter: Optional[dict], + ) -> None: + inc_child_binding = self._mk_child_binding(path, raw) + + if self.child_binding or child_filter: + # Take "Last modified here" semantic when: + # - we've already included a child-binding + # - the including binding applies some filter + # to the included child-binding's properties + child_binding = self._mk_child_binding(self.path, {}) + if self.child_binding: + self._merge_child_binding(child_binding, self.child_binding) + self.child_binding = child_binding + + if self.child_binding: + self._merge_child_binding(self.child_binding, inc_child_binding) + else: + self.child_binding = inc_child_binding + + # Either: + # - update child-binding definitions we may inherited from included bindings + # - initialize this binding's child binding with its child-binding element + # + # raw_child_binding: content of the child-binding element + # in this binding file. + def _update_child_binding(self, raw_child_binding: Dict[str, Any]) -> None: + if self.child_binding: + raw_properties = raw_child_binding.get("properties", {}) + if raw_properties: + # This binding's child-binding element may redefine properties + # we've already inherited from included bindings. + # This will typically happen when the including file + # (this binding) set "required: true" on the inherited property. + # We can't create a child binding based solely on this latest + # definition: we before have to update the redefined properties + # with the attributes (type, description) we inherited. + for prop, raw in raw_properties.items(): + if prop in self.child_binding.prop2specs: + # A bit convoluted, but avoid accessing private + # member PropertySpec._raw. + inc_raw_prop = self.child_binding.prop2specs[ + prop + ].binding.raw.get("properties", {})[prop] + raw.update(inc_raw_prop) + + # Last modified child-binding. + last_child_binding = self._mk_child_binding( + self.path, raw_child_binding + ) + + if self.child_binding: + # Merge this file's child-binding element with the child-binding + # definitions we inherited from included files. + if self.child_binding.path != self.path: + # Taking "Last modified here" semantic if not already taken + # when we merged the included bindings, e.g. if the + # including file filtered the included child-binding's properties. + child_binding = self._mk_child_binding(self.path, {}) + self._merge_child_binding(child_binding, self.child_binding) + self.child_binding = child_binding + + self._merge_child_binding(self.child_binding, last_child_binding) + else: + self.child_binding = last_child_binding + + # Merge child bindings: + # - update child_binding's properties with properties + # from other_child_binding + # - update child_binding's cell specifiers with specifiers + # from other_child_binding + # - recursively merge child bindings of child_binding + # and other_child_binding + def _merge_child_binding( + self, + child_binding: "Binding", + other_child_binding: "Binding", + ) -> None: + child_binding.prop2specs.update(other_child_binding.prop2specs) + child_binding.specifier2cells.update( + other_child_binding.specifier2cells + ) + + other_grandchild_binding = other_child_binding.child_binding + if other_grandchild_binding: + if child_binding.child_binding: + self._merge_child_binding( + child_binding.child_binding, other_grandchild_binding + ) + else: + child_binding.child_binding = other_grandchild_binding + + def _mk_child_binding( + self, path: Optional[str], raw: Dict[str, Any] + ) -> "Binding": + # These don't require a 'compatible' or 'description' to be well defined, + # but they must be dicts. + if not isinstance(raw, dict): + _err( + f"malformed 'child-binding:' in {path}, " + "expected a binding (dictionary with keys/values)" + ) + return Binding( + path, + self._fname2path, + raw, + require_compatible=False, + require_description=False, + # Filters already applied. + inc_allowlist=None, + inc_blocklist=None, + ) + def _check(self, require_compatible: bool, require_description: bool): # Does sanity checking on the binding. diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/digest.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/digest.yaml new file mode 100644 index 000000000000000..1e27582ac95dd0a --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/digest.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: Apache-2.0 + +description: | + Extends allow-and-blocklist-multilevel.yaml test case, adds: + - check of bindings and property specs paths + - check of grand-grandchild-binding level + - check grandchild-binding filters + - check setting "required: true" won't truncate included + child-binding's property definitions + + All child-binding definitions come from a single file, + included multiple times, at different levels, + with different filters. + +compatible: vnd,digest + +# child-binding: +# + child-prop-1 +# +# grandchild-binding: +# + gandchild-prop-1 +# + gandchild-prop-2 +include: + - name: digest_base.yaml + property-allowlist: [x] + + child-binding: + property-allowlist: [child-prop-1] + + child-binding: + property-blocklist: [grandchild-prop-blocked] + +properties: + top-prop: + type: int + +child-binding: + include: + # child-binding: + # ~ child-prop-1 + # + x + # + z + - name: digest_base.yaml + property-blocklist: [y] + + # grandchild-binding: + # ~ gandchild-prop-1 + # ~ gandchild-prop-2 + # + child-prop-1 + # + child-prop-1 + child-binding: + property-blocklist: [child-prop-blocked] + + # grandgrandchild-binding: + # + gandchild-prop-1 + # + gandchild-prop-2 + child-binding: + property-blocklist: [grandchild-prop-blocked] + + properties: + child-prop-1: + required: true + z: + required: true + cb-prop: + type: int + + child-binding: + properties: + gcbb-prop: + type: int diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/digest_base.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/digest_base.yaml new file mode 100644 index 000000000000000..900c15bf6efa843 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/digest_base.yaml @@ -0,0 +1,30 @@ +# SPDX-License-Identifier: Apache-2.0 + +description: | + Base include file for the digest.yaml test case. + +properties: + x: + type: int + y: + type: int + z: + type: int + +child-binding: + properties: + child-prop-1: + type: int + child-prop-2: + type: int + child-prop-blocked: + type: int + + child-binding: + properties: + grandchild-prop-1: + type: int + grandchild-prop-2: + type: int + grandchild-prop-blocked: + type: int diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/left.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/left.yaml new file mode 100644 index 000000000000000..a9e33485b39d6da --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/left.yaml @@ -0,0 +1,17 @@ +# SPDX-License-Identifier: Apache-2.0 + +description: | + Left include file for left_rigt.yaml. + +properties: + left-prop-1: + type: int + left-prop-2: + type: int + +child-binding: + properties: + left-child-prop-1: + type: int + left-child-prop-2: + type: int diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/left_right.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/left_right.yaml new file mode 100644 index 000000000000000..e9fd3337a5ab669 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/left_right.yaml @@ -0,0 +1,35 @@ +# SPDX-License-Identifier: Apache-2.0 + +description: | + Complements digest.yaml test case, + here child-binding definitions come from two files. + +compatible: vnd,left_right + +include: + - name: left.yaml + property-allowlist: [left-prop-1] + child-binding: + property-allowlist: [left-child-prop-1] + + - name: right.yaml + property-allowlist: [right-prop-2] + child-binding: + property-allowlist: [right-child-prop-2] + +properties: + left-prop-1: + required: true + left-right-prop: + type: int + +child-binding: + properties: + left-child-prop-1: + required: true + child-prop: + type: int + child-binding: + properties: + grandchild-prop: + type: int diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/right.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/right.yaml new file mode 100644 index 000000000000000..f7b56d8b522c292 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/right.yaml @@ -0,0 +1,17 @@ +# SPDX-License-Identifier: Apache-2.0 + +description: | + Right include file for left_rigt.yaml. + +properties: + right-prop-1: + type: int + right-prop-2: + type: int + +child-binding: + properties: + right-child-prop-1: + type: int + right-child-prop-2: + type: int diff --git a/scripts/dts/python-devicetree/tests/test_edtlib.py b/scripts/dts/python-devicetree/tests/test_edtlib.py index a81aa929370a076..1fba0ae774f0135 100644 --- a/scripts/dts/python-devicetree/tests/test_edtlib.py +++ b/scripts/dts/python-devicetree/tests/test_edtlib.py @@ -393,7 +393,163 @@ def test_include_filters_included_bindings(): assert not top_blocks.prop2specs.get("x") assert top_blocks.prop2specs.get("y") +def test_include_child_binding_digest() -> None: + '''Extends allow-and-blocklist-multilevel.yaml test case. + Adds: + - check of bindings and property specs paths + - check of grand-grandchild-binding level + - check grandchild-binding filters + - check setting "required: true" won't truncate included + child-binding's property definitions + + All child-binding definitions come from a single file, + included multiple times, at different levels, + with different filters. + ''' + fname2path = { + "digest_base.yaml": "test-bindings-include/digest_base.yaml", + } + + binding = None + with from_here(): + binding = edtlib.Binding("test-bindings-include/digest.yaml", + fname2path) + assert "digest.yaml" == _basename(binding.path) + + assert {"x", "top-prop"} == set(binding.prop2specs.keys()) + assert "digest_base.yaml" == _basename(binding.prop2specs["x"].path) + assert "digest.yaml" == _basename( + binding.prop2specs["top-prop"].path + ) + + assert "int" == binding.prop2specs["x"].type + assert not binding.prop2specs["x"].required + + + child_binding = binding.child_binding + assert child_binding + # Last modified semantic. + assert "digest.yaml" == _basename(child_binding.path) + + assert {"x", "z", "child-prop-1", "cb-prop"} == set( + child_binding.prop2specs.keys() + ) + + assert "digest_base.yaml" == _basename(child_binding.prop2specs["x"].path) + # Taken "Last modfied here" semantic. + assert "digest.yaml" == _basename( + child_binding.prop2specs["child-prop-1"].path + ) + + assert "int" == child_binding.prop2specs["x"].type + assert not child_binding.prop2specs["x"].required + + assert "int" == child_binding.prop2specs["z"].type + assert child_binding.prop2specs["z"].required + + grandchild_binding = child_binding.child_binding + assert grandchild_binding + assert "digest_base.yaml" == _basename(grandchild_binding.path) + + assert { + "child-prop-1", + "child-prop-2", + "grandchild-prop-1", + "grandchild-prop-2", + "gcbb-prop", + } == set(grandchild_binding.prop2specs.keys()) + + assert "digest_base.yaml" == _basename( + grandchild_binding.prop2specs["child-prop-1"].path + ) + assert "digest_base.yaml" == _basename( + grandchild_binding.prop2specs["grandchild-prop-1"].path + ) + assert "digest.yaml" == _basename( + grandchild_binding.prop2specs["gcbb-prop"].path + ) + + ggchild_binding = grandchild_binding.child_binding + assert ggchild_binding + assert "digest_base.yaml" == _basename(ggchild_binding.path) + + assert { + "grandchild-prop-1", + "grandchild-prop-2", + } == set(ggchild_binding.prop2specs.keys()) + + assert "digest_base.yaml" == _basename( + ggchild_binding.prop2specs["grandchild-prop-1"].path + ) + assert "digest_base.yaml" == _basename( + ggchild_binding.prop2specs["grandchild-prop-2"].path + ) + +def test_include_multiple_child_bindings_digest() -> None: + '''Complement test case above, child-binding definitions come from two files.''' + fname2path = { + "left.yaml": "test-bindings-include/left.yaml", + "right.yaml": "test-bindings-include/right.yaml", + } + + binding = None + with from_here(): + binding = edtlib.Binding("test-bindings-include/left_right.yaml", + fname2path) + assert "left_right.yaml" == _basename(binding.path) + assert {"left-right-prop", "left-prop-1", "right-prop-2"} == set( + binding.prop2specs.keys() + ) + assert "left_right.yaml" == _basename( + binding.prop2specs["left-right-prop"].path + ) + # Taken "Last modified here" semantic. + assert "left_right.yaml" == _basename(binding.prop2specs["left-prop-1"].path) + assert "right.yaml" == _basename(binding.prop2specs["right-prop-2"].path) + + assert "int" == binding.prop2specs["left-prop-1"].type + assert binding.prop2specs["left-prop-1"].required + assert "int" == binding.prop2specs["right-prop-2"].type + assert not binding.prop2specs["right-prop-2"].required + + # Child-binding. + child_binding = binding.child_binding + assert child_binding + # Last modified semantic. + assert "left_right.yaml" == _basename(child_binding.path) + + assert {"left-child-prop-1", "right-child-prop-2", "child-prop"} == set( + child_binding.prop2specs.keys() + ) + + assert "left_right.yaml" == _basename( + child_binding.prop2specs["left-child-prop-1"].path + ) + assert "right.yaml" == _basename( + child_binding.prop2specs["right-child-prop-2"].path + ) + assert "left_right.yaml" == _basename( + child_binding.prop2specs["child-prop"].path + ) + + assert "int" == child_binding.prop2specs["left-child-prop-1"].type + assert child_binding.prop2specs["left-child-prop-1"].required + assert "int" == child_binding.prop2specs["right-child-prop-2"].type + assert not child_binding.prop2specs["right-child-prop-2"].required + + # GrandChild-binding. + grandchild_binding = child_binding.child_binding + assert grandchild_binding + assert "left_right.yaml" == _basename(grandchild_binding.path) + + assert { + "grandchild-prop", + } == set(grandchild_binding.prop2specs.keys()) + + assert "left_right.yaml" == _basename( + grandchild_binding.prop2specs["grandchild-prop"].path + ) def test_bus(): '''Test 'bus:' and 'on-bus:' in bindings''' @@ -882,3 +1038,6 @@ def verify_phandle_array_prop(node, name, values): assert actual.data == data else: assert actual is None + +def _basename(path): + return os.path.basename(path or "?")