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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

On Tue, Jun 06, 2023 at 12:09:35PM -0700, Vikram Garhwal wrote:
> 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.
Update the commit message in v8.
> > 
> > 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?
> > 
I haven't seen any overlay example for FPGA use cases where alias are added.
I have added a TODO in patch 16/19 where we are calling unflatten_device_tree().
> > >   }
> > > 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?
Done!
> > 
> > > + */
> > > +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?
We will also need the error from the function. So, that's why i kept it as it 
is.
Please review v8. I will send it in few hours.
> > 
> > > +
> > >   /**
> > >    * IRQ translation callback
> > >    * TODO: For the moment we assume that we only have ONE
> > 
> > Cheers,
> > 
>