Re: [XEN][PATCH v7 04/19] common/device_tree: change __unflatten_device_tree() type

[Julien Grall <julien@xen.org>]

Hi,

Title:

'type' is a bit confusing here. How about "Export __unflatten_device_tre()"?

On 02/06/2023 01:48, Vikram Garhwal wrote:
> Following changes are done to __unflatten_device_tree():
>      1. __unflatten_device_tree() is renamed to unflatten_device_tree().
>      2. Remove __init and static function type.

As there is no external caller yet, please explain why you want to 
export the function.

Cheers,

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
> ---
>   xen/common/device_tree.c      | 9 ++++-----
>   xen/include/xen/device_tree.h | 5 +++++
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index bbdab07596..16b4b4e946 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const void *fdt,
>   }
>   
>   /**
> - * __unflatten_device_tree - create tree of device_nodes from flat blob
> + * unflatten_device_tree - create tree of device_nodes from flat blob
>    *
>    * unflattens a device-tree, creating the
>    * tree of struct device_node. It also fills the "name" and "type"
> @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const void *fdt,
>    * @fdt: The fdt to expand
>    * @mynodes: The device_node tree created by the call
>    */
> -static int __init __unflatten_device_tree(const void *fdt,
> -                                          struct dt_device_node **mynodes)
> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
>   {
>       unsigned long start, mem, size;
>       struct dt_device_node **allnextp = mynodes;
> @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
>   
>   void __init dt_unflatten_host_device_tree(void)
>   {
> -    int error = __unflatten_device_tree(device_tree_flattened, &dt_host);
> +    int error = unflatten_device_tree(device_tree_flattened, &dt_host);
>   
>       if ( error )
> -        panic("__unflatten_device_tree failed with error %d\n", error);
> +        panic("unflatten_device_tree failed with error %d\n", error);
>   
>       dt_alias_scan();

This function doesn't seem to be called in the case of the overlay 
device-tree. Does this mean that it will never contain any alias?

>   }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index c2eada7489..2c35c0d391 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, int node,
>    */
>   void dt_unflatten_host_device_tree(void);
>   
> +/**
> + * unflatten any device tree.

Most of the exported function in device_tre.h have documentation. Can 
you do the same here?

> + */
> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);

NIT: From an external interface perspective, do we actually need to pass 
an extra pointer? Can't we instead, return the pointer?

> +
>   /**
>    * IRQ translation callback
>    * TODO: For the moment we assume that we only have ONE

Cheers,

-- 
Julien Grall