Re: [XEN][PATCH v7 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,

On 6/5/23 12:12 PM, Julien Grall wrote:
> 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()"?
Thank you for the suggestion. This name was added in v3 as Luca Fancellu 
suggested to rename this function to "device_tree_find_node_by_path". I 
am okay with renaming it to dt_find_node_by_path_from().

Luca, Michal and Henry: Does the dt_find_node_by_path_from() name works 
for you?

Regards,
Vikram
>
>> 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.
Will fix the indention.
>
>>   +/**
>> + * 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,
>