Re: [XEN][PATCH v9 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Julien,
On Tue, Aug 22, 2023 at 08:21:17PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Add device_tree_find_node_by_path() to find a matching node with path for a
> > dt_device_node.
> > 
> > Reason behind this function:
> >      Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
> 
> Typo: missing space before (.
> 
> >      device_tree_flattened) is created and updated with overlay nodes. This
> >      updated fdt is further unflattened to a dt_host_new. Next, we need to find
> >      the overlay nodes in dt_host_new, find the overlay node's parent in dt_host
> >      and add the nodes as child under their parent in the dt_host. Thus we need
> >      this function to search for node in different unflattened device trees.
> > 
> > Also, make dt_find_node_by_path() static inline.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> > ---
> > Changes from v7:
> >      Rename device_tree_find_node_by_path() to dt_find_node_by_path_from().
> >      Fix indentation.
> > ---
> > ---
> >   xen/common/device_tree.c      |  5 +++--
> >   xen/include/xen/device_tree.h | 18 ++++++++++++++++--
> >   2 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 67e9b6de3e..0f10037745 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -358,11 +358,12 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> >       return np;
> >   }
> > -struct dt_device_node *dt_find_node_by_path(const char *path)
> > +struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node *from,
> > +                                                 const char *path)
> 
> Any change this both 'from' and the return can be const?
Doing this will also need changes in dt_find_node_by_path() and
dt_for_each_device_node(). This function calls both of these.
> 
> >   {
> >       struct dt_device_node *np;
> > -    dt_for_each_device_node(dt_host, np)
> > +    dt_for_each_device_node(from, np)
> >           if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
> >               break;
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 5941599eff..e507658b23 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -568,13 +568,27 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> >   struct dt_device_node *dt_find_node_by_alias(const char *alias);
> >   /**
> > - * dt_find_node_by_path - Find a node matching a full DT path
> > + * dt_find_node_by_path_from - Generic function to find a node matching the
> > + * full DT path for any given unflatten device tree
> > + * @from: The device tree node to start searching from
> >    * @path: The full path to match
> >    *
> >    * Returns a node pointer.
> >    */
> > -struct dt_device_node *dt_find_node_by_path(const char *path);
> > +struct dt_device_node *
> > +                    dt_find_node_by_path_from(struct dt_device_node *from,
> > +                                                  const char *path);
> 
> Coding style: The indentation look rather odd. I am not sure it will render
> properly here. But one style that is closer to the rest of the file and Xen
> is:
> 
> struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node
> *from,
>                                                  const char *path);
> 
> Where the return type, function name and first parameter is one line and the
> second parameter is on the second line with the type aligned with the type
> of the first parameter.
Will do!
> 
> > +/**
> > + * dt_find_node_by_path - Find a node matching a full DT path in dt_host
> > + * @path: The full path to match
> > + *
> > + * Returns a node pointer.
> > + */
> > +static inline struct dt_device_node *dt_find_node_by_path(const char *path)
> > +{
> > +    return dt_find_node_by_path_from(dt_host, path);
> > +}
> >   /**
> >    * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
> 
> Cheers,
> 
> -- 
> Julien Grall