scripts: edtlib: Make order irrelevant when including multiple files

When foo.yaml set some property 'required: true' and bar.yaml set the
same property 'required: false', the check for changing
'required: false' to 'required: true' would raise an error for

    include: [bar.yaml, foo.yaml]

(with that particular order due to implementation details).

The order files are included in shouldn't matter. To fix it, change the
logic so that 'required' values are ORed together between included files
(so that 'required: true' is always respected), and remove the
'required' true-to-false check when merging included files.

Keep the true-to-false check when merging the (merged) included files
into the main binding (the binding with the 'include:' in it). This
might give a good organization, and the old scripts do it too.

Piggyback two fixes/cleanups:

 - 'compatible' should be allowed to appear in included files

 - No need to allow an 'inherits' key in _check_binding(), because
   it has been removed before then, when merging bindings

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>

dts/binding-template.yaml

@@ -31,10 +31,19 @@ compatible: "manufacturer,device"
 # 'include', which are merged into the binding (with a recursive dictionary
 # merge).
 #
-# If a field appears both in the binding and in a file it includes, then the
-# value in the binding takes precedence. This can be used e.g. to change a
+# If a key appears both in the binding and in a file it includes, then the
+# value in the binding takes precedence. This can be used to change a
 # 'required: false' from an inherited file to a 'required: true' (see the
 # 'properties' description below).
+#
+# An error is raised if this binding has 'required: false' for some property
+# for which the included file(s) have 'required: true'. Bindings can only
+# "strengthen" requirements from files they include. This is meant to keep the
+# organization clean.
+#
+# When including multiple files, any overlapping 'required' keys on properties
+# in the included files are ORed together. This makes sure that a
+# 'required: true' is always respected.
 include: other.yaml # or [other1.yaml, other2.yaml]
 
 # If the node describes a bus, then the bus type should be given, like below

scripts/dts/edtlib.py

@@ -206,7 +206,7 @@ class EDT:
                 (binding, binding_path)
 
     def _merge_included_bindings(self, binding, binding_path):
-        # Merges any bindings listed in the 'include:' section of the binding
+        # Merges any bindings listed in the 'include:' section of 'binding'
         # into the top level of 'binding'. Also supports the legacy
         # 'inherits: !include ...' syntax for including bindings.
         #
@@ -241,31 +241,51 @@ class EDT:
                 _err("malformed 'inherits:' in " + binding_path)
             fnames += inherits
 
-        for fname in fnames:
-            included = self._file_yaml(fname)
-            _merge_props(
-                binding, self._merge_included_bindings(included, binding_path),
-                None, binding_path)
+        if not fnames:
+            return binding
+
+        # Got a list of included files in 'fnames'. Now we need to merge them
+        # together and then merge them into 'binding'.
+
+        # First, merge the included files together. If more than one included
+        # file has a 'required:' for a particular property, OR the values
+        # together, so that 'required: true' wins.
+
+        merged_included = self._load_binding(fnames[0])
+        for fname in fnames[1:]:
+            included = self._load_binding(fname)
+            _merge_props(merged_included, included, None, binding_path,
+                         check_required=False)
+
+        # Next, merge the merged included files into 'binding'. Error out if
+        # 'binding' has 'required: false' while the merged included files have
+        # 'required: true'.
+
+        _merge_props(binding, merged_included, None, binding_path,
+                     check_required=True)
 
         return binding
 
-    def _file_yaml(self, filename):
-        # _merge_included_bindings() helper for loading an included file.
-        # 'include:' lists just the basenames of the files, so we check that
-        # there aren't multiple candidates.
+    def _load_binding(self, fname):
+        # Returns the contents of the binding given by 'fname' after merging
+        # any bindings it lists in 'include:' into it. 'fname' is just the
+        # basename of the file, so we check that there aren't multiple
+        # candidates.
 
         paths = [path for path in self._binding_paths
-                 if os.path.basename(path) == filename]
+                 if os.path.basename(path) == fname]
 
         if not paths:
-            _err("'{}' not found".format(filename))
+            _err("'{}' not found".format(fname))
 
         if len(paths) > 1:
             _err("multiple candidates for included file '{}': {}"
-                 .format(filename, ", ".join(paths)))
+                 .format(fname, ", ".join(paths)))
 
         with open(paths[0], encoding="utf-8") as f:
-            return yaml.load(f, Loader=yaml.Loader)
+            return self._merge_included_bindings(
+                yaml.load(f, Loader=yaml.Loader),
+                paths[0])
 
     def _init_devices(self):
         # Creates a list of devices (Device objects) from the DT nodes, in
@@ -1302,10 +1322,20 @@ def _binding_inc_error(msg):
     raise yaml.constructor.ConstructorError(None, None, "error: " + msg)
 
 
-def _merge_props(to_dict, from_dict, parent, binding_path):
+def _merge_props(to_dict, from_dict, parent, binding_path, check_required):
     # Recursively merges 'from_dict' into 'to_dict', to implement 'include:'.
-    # If a key exists in both 'from_dict' and 'to_dict', then the value in
-    # 'to_dict' takes precedence.
+    #
+    # If 'from_dict' and 'to_dict' contain a 'required:' key for the same
+    # property, then the values are ORed together.
+    #
+    # If 'check_required' is True, then an error is raised if 'from_dict' has
+    # 'required: true' while 'to_dict' has 'required: false'. This prevents
+    # bindings from "downgrading" requirements from bindings they include,
+    # which might help keep bindings well-organized.
+    #
+    # It's an error for most other keys to appear in both 'from_dict' and
+    # 'to_dict'. When it's not an error, the value in 'to_dict' takes
+    # precedence.
     #
     # 'parent' is the name of the parent key containing 'to_dict' and
     # 'from_dict', and 'binding_path' is the path to the top-level binding.
@@ -1314,34 +1344,53 @@ def _merge_props(to_dict, from_dict, parent, binding_path):
     for prop in from_dict:
         if isinstance(to_dict.get(prop), dict) and \
            isinstance(from_dict[prop], dict):
-            _merge_props(to_dict[prop], from_dict[prop], prop, binding_path)
+            _merge_props(to_dict[prop], from_dict[prop], prop, binding_path,
+                         check_required)
         elif prop not in to_dict:
             to_dict[prop] = from_dict[prop]
-        elif _bad_overwrite(to_dict, from_dict, prop):
+        elif _bad_overwrite(to_dict, from_dict, prop, check_required):
             _err("{} (in '{}'): '{}' from included file overwritten "
                  "('{}' replaced with '{}')".format(
                      binding_path, parent, prop, from_dict[prop],
                      to_dict[prop]))
+        elif prop == "required":
+            # Need a separate check here, because this code runs before
+            # _check_binding()
+            if not (isinstance(from_dict["required"], bool) and
+                    isinstance(to_dict["required"], bool)):
+                _err("malformed 'required:' setting for '{}' in 'properties' "
+                     "in {}, expected true/false".format(parent, binding_path))
+
+            # 'required: true' takes precedence
+            to_dict["required"] = to_dict["required"] or from_dict["required"]
+        elif prop == "category":
+            # Legacy property key. 'category: required' takes precedence.
+            if "required" in (to_dict["category"], from_dict["category"]):
+                to_dict["category"] = "required"
 
 
-def _bad_overwrite(to_dict, from_dict, prop):
+def _bad_overwrite(to_dict, from_dict, prop, check_required):
     # _merge_props() helper. Returns True in cases where it's bad that
     # to_dict[prop] takes precedence over from_dict[prop].
 
-    # These are overridden deliberately
-    if prop in {"title", "description"}:
-        return False
-
     if to_dict[prop] == from_dict[prop]:
         return False
 
-    # Allow a property to be made required when it previously was optional
-    # without a warning
-    if (prop == "required" and to_dict[prop] and not from_dict[prop]) or \
-       (prop == "category" and to_dict[prop] == "required" and
-        from_dict[prop] == "optional"):
+    # These are overridden deliberately
+    if prop in {"title", "description", "compatible"}:
         return False
 
+    if prop == "required":
+        if not check_required:
+            return False
+        return from_dict[prop] and not to_dict[prop]
+
+    # Legacy property key
+    if prop == "category":
+        if not check_required:
+            return False
+        return from_dict[prop] == "required" and to_dict[prop] == "optional"
+
     return True
 
 
@@ -1371,8 +1420,8 @@ def _check_binding(binding, binding_path):
             _err("missing, malformed, or empty '{}' in {}"
                  .format(prop, binding_path))
 
-    ok_top = {"title", "description", "compatible", "inherits", "properties",
-              "#cells", "parent", "child", "sub-node"}
+    ok_top = {"title", "description", "compatible", "properties", "#cells",
+              "parent", "child", "sub-node"}
 
     for prop in binding:
         if prop not in ok_top:

scripts/dts/test-bindings/foo-optional.yaml

@@ -0,0 +1,4 @@
+properties:
+    foo:
+        type: int
+        required: false

scripts/dts/test-bindings/foo-required.yaml

@@ -0,0 +1,4 @@
+properties:
+    foo:
+        type: int
+        required: true

scripts/dts/test-bindings/order-1.yaml

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+title: Include ordering test
+description: Include ordering test
+
+compatible: "order-1"
+
+include: ["foo-required.yaml", "foo-optional.yaml"]

scripts/dts/test-bindings/order-2.yaml

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+title: Include ordering test
+description: Include ordering test
+
+compatible: "order-2"
+
+include: ["foo-optional.yaml", "foo-required.yaml"]

scripts/dts/test.dts

@@ -330,6 +330,23 @@
 		};
 	};
 
+	//
+	// For testing that neither 'include: [foo.yaml, bar.yaml]' nor
+	// 'include: [bar.yaml, foo.yaml]' causes errors when one of the files
+	// has 'required: true' and the other 'required: false'
+	//
+
+	include-order {
+		node-1 {
+			compatible = "order-1";
+			foo = <1>;
+		};
+		node-2 {
+			compatible = "order-2";
+			foo = <2>;
+		};
+	};
+
 	//
 	// For testing deprecated features
 	//