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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Julien,
Will update the commit message regarding why we need to export this for 
dtbo programming.
On 6/5/23 12:04 PM, Julien Grall wrote:
> 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@xxxxxxx>
> > Reviewed-by: Henry Wang <Henry.Wang@xxxxxxx>
> > ---
> >   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,