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

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>
---

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

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)

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(-)
>
Applied to u-boot-dm, thanks!

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> 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

This code is significantly different now, so see what you think.

The last point is interesting...at present you can use property
substitution in any node. I tend to agree the recursion doesn't really
help, but let me know what you think of the latest version.

>
> >
> >  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.

OK, will hold off on this one until after the series.

[..]

Regards,
Simon