Re: [PATCH 17/24] binman: fit: Refactor to reduce function size

On 08/02/2022 21:50, Simon Glass wrote:

Split subnode and property processing into separate functions to make the _AddNode() function a little smaller. Tweak a few comments.

This does not change any functionality.

Signed-off-by: Simon Glass sjg@chromium.org

I know this just moves code around a bit, but I think the code here could be cleaned up much further with a bit of redesign. I'm not sure of the details, but was thinking of at least:

- self._add_fit_image() to handle image/* subnodes - self._add_fit_config() to handle configuration/* subnodes - self._gen_fdt_nodes() to handle template nodes by calling the above - Switching away from recursion to iterating subnodes of fixed nodes

tools/binman/etype/fit.py | 116 ++++++++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 6ad4a686df..b159844960 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -141,12 +141,82 @@ class Entry_fit(Entry): super().ReadNode()

 def ReadEntries(self):

I think the following functions could be moved out of this one into the class scope, even including _AddNode.

•    def _process_prop(pname, prop):
  

•        """Process special properties
  

•        Handles properties with generated values. At present the only
  

•        supported property is 'default', i.e. the default device tree in
  

•        the configurations node.
  

•        Args:
  

•            pname (str): Name of property
  

•            prop (Prop): Property to process
  

•        """
  

•        if pname == 'default':
  

•            val = prop.value
  

•            # Handle the 'default' property
  

•            if val.startswith('@'):
  

•                if not self._fdts:
  

•                    return
  

•                if not self._fit_default_dt:
  

•                    self.Raise("Generated 'default' node requires default-dt entry argument")
  

•                if self._fit_default_dt not in self._fdts:
  

•                    self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
  

•                               (self._fit_default_dt,
  

•                                ', '.join(self._fdts)))
  

•                seq = self._fdts.index(self._fit_default_dt)
  

•                val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
  

•                fsw.property_string(pname, val)
  

•                return
  

•        fsw.property(pname, prop.bytes)
  

•    def _generate_node(subnode, depth, in_images):
  

•        """Generate nodes from a template
  

•        This creates one node for each member of self._fdts using the
  

•        provided template. If a property value contains 'NAME' it is
  

•        replaced with the filename of the FDT. If a property value contains
  

•        SEQ it is replaced with the node sequence number, where 1 is the
  

•        first.
  

•        Args:
  

•            subnode (None): Generator node to process
  

•            depth: Current node depth (0 is the base 'fit' node)
  

•            in_images: True if this is inside the 'images' node, so that
  

•                'data' properties should be generated
  

•        """
  

•        if self._fdts:
  

•            # Generate nodes for each FDT
  

•            for seq, fdt_fname in enumerate(self._fdts):
  

•                node_name = subnode.name[1:].replace('SEQ',
  

•                                                     str(seq + 1))
  

•                fname = tools.GetInputFilename(fdt_fname + '.dtb')
  

•                with fsw.add_node(node_name):
  

•                    for pname, prop in subnode.props.items():
  

•                        val = prop.bytes.replace(
  

•                            b'NAME', tools.ToBytes(fdt_fname))
  

•                        val = val.replace(
  

•                            b'SEQ', tools.ToBytes(str(seq + 1)))
  

•                        fsw.property(pname, val)
  

•                    # Add data for 'images' nodes (but not 'config')
  

•                    if depth == 1 and in_images:
  

•                        fsw.property('data',
  

•                                     tools.ReadFile(fname))
  

•        else:
  

•            if self._fdts is None:
  

•                if self._fit_list_prop:
  

•                    self.Raise("Generator node requires '%s' entry argument" %
  

•                               self._fit_list_prop.value)
  

•                else:
  

•                    self.Raise("Generator node requires 'fit,fdt-list' property")
  

•    def _AddNode(base_node, depth, node):
         """Add a node to the FIT
  
         Args:
             base_node: Base Node of the FIT (with 'description' property)
  

•            depth: Current node depth (0 is the base node)
  

•            depth: Current node depth (0 is the base 'fit' node)
             node: Current node to process
  
         There are two cases to deal with:
  

@@ -156,23 +226,7 @@ class Entry_fit(Entry): """ for pname, prop in node.props.items(): if not pname.startswith('fit,'):

•                if pname == 'default':
  

•                    val = prop.value
  

•                    # Handle the 'default' property
  

•                    if val.startswith('@'):
  

•                        if not self._fdts:
  

•                            continue
  

•                        if not self._fit_default_dt:
  

•                            self.Raise("Generated 'default' node requires default-dt entry argument")
  

•                        if self._fit_default_dt not in self._fdts:
  

•                            self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
  

•                                       (self._fit_default_dt,
  

•                                        ', '.join(self._fdts)))
  

•                        seq = self._fdts.index(self._fit_default_dt)
  

•                        val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
  

•                        fsw.property_string(pname, val)
  

•                        continue
  

•                fsw.property(pname, prop.bytes)
  

•                _process_prop(pname, prop)
  
         rel_path = node.path[len(base_node.path):]
         in_images = rel_path.startswith('/images')
  

@@ -195,31 +249,7 @@ class Entry_fit(Entry): # fsw.add_node() or _AddNode() for it. pass elif self.GetImage().generate and subnode.name.startswith('@'):

•                if self._fdts:
  

•                    # Generate notes for each FDT
  

•                    for seq, fdt_fname in enumerate(self._fdts):
  

•                        node_name = subnode.name[1:].replace('SEQ',
  

•                                                             str(seq + 1))
  

•                        fname = tools.GetInputFilename(fdt_fname + '.dtb')
  

•                        with fsw.add_node(node_name):
  

•                            for pname, prop in subnode.props.items():
  

•                                val = prop.bytes.replace(
  

•                                    b'NAME', tools.ToBytes(fdt_fname))
  

•                                val = val.replace(
  

•                                    b'SEQ', tools.ToBytes(str(seq + 1)))
  

•                                fsw.property(pname, val)
  

•                            # Add data for 'fdt' nodes (but not 'config')
  

•                            if depth == 1 and in_images:
  

•                                fsw.property('data',
  

•                                             tools.ReadFile(fname))
  

•                else:
  

•                    if self._fdts is None:
  

•                        if self._fit_list_prop:
  

•                            self.Raise("Generator node requires '%s' entry argument" %
  

•                                       self._fit_list_prop.value)
  

•                        else:
  

•                            self.Raise("Generator node requires 'fit,fdt-list' property")
  

•                _generate_node(subnode, depth, in_images)
             else:
                 with fsw.add_node(subnode.name):
                     _AddNode(base_node, depth + 1, subnode)