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

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

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)