Re: [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Vikram,

On 07/12/2022 07:15, Vikram Garhwal wrote:
> 
> 
> Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
Here and in commit title you say _dt_find_by_path but you introduce 
device_tree_find_node_by_path.
Also, it would be beneficial to state why such change is needed.

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> ---
>  xen/common/device_tree.c      |  5 +++--
>  xen/include/xen/device_tree.h | 16 ++++++++++++++--
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6518eff9b0..acf26a411d 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 *device_tree_find_node_by_path(struct dt_device_node 
> *dt,
> +                                                     const char *path)
>  {
>      struct dt_device_node *np;
> 
> -    dt_for_each_device_node(dt_host, np)
> +    dt_for_each_device_node(dt, 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 bde46d7120..51e251b0b4 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -537,13 +537,25 @@ 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 - Find a node matching a full DT path
This description and ...
> + * @dt_node: The device tree to search
>   * @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 
> *dt,
> +                                                     const char *path);
> 
> +/**
> + * dt_find_node_by_path - Find a node matching a full DT path
... this are identical. I think you should describe the difference.
The function names are also very similar and can be confused but I won't oppose 
it.
I will leave the decision to maintainers.


> + * @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
> --
> 2.17.1
> 
> 
~Michal