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

[Julien Grall <julien@xen.org>]

Hi,

On 02/06/2023 01:48, Vikram Garhwal wrote:
> Add device_tree_find_node_by_path() to find a matching node with path for a

AFAICT, the only difference in name between the new function and the 
existing one is "device_tree" vs "dt". The latter is just a shorthand of 
"device tree", so it feels to me the name are a bit too similar.

 From my understanding, the main difference between the two functions 
are that the current one is starting from root whereas the current one 
is starting from a given node. So how about "dt_find_node_by_path_from()"?

> dt_device_node.
> 
> Reason behind this function:
>      Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
>      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 v6:
>      Rename "dt_node" to "from"
> ---
>   xen/common/device_tree.c      |  6 ++++--
>   xen/include/xen/device_tree.h | 18 ++++++++++++++++--
>   2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 16b4b4e946..c5250a1644 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -358,11 +358,13 @@ 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 *
> +                    device_tree_find_node_by_path(struct dt_device_node *from,
> +                                                  const char *path)
>   {
>       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 2c35c0d391..e239f7de26 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -561,13 +561,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
> + * device_tree_find_node_by_path - 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 *
> +                    device_tree_find_node_by_path(struct dt_device_node *from,
> +                                                  const char *path);

The indentation looks slightly odd. In general, if the return type is on 
its own line, then the function name is not indented on the new line.

>   
> +/**
> + * 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 device_tree_find_node_by_path(dt_host, path);
> +}
>   
>   /**
>    * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the

Cheers,

-- 
Julien Grall