Re: [PATCH v2 2/2] fdt: make fdt handling reusable across arch

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/3/23 14:05, Luca Fancellu wrote:
> Hi Daniel,
>
> [...]
> > diff --git a/xen/common/fdt.c b/xen/common/fdt.c
> > new file mode 100644
> > index 0000000000..8d7acaaa43
> > --- /dev/null
> > +++ b/xen/common/fdt.c
> > @@ -0,0 +1,153 @@
> > +/*
> > + * Flattened Device Tree
> > + *
> > + * Copyright (C) 2012-2014 Citrix Systems, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> Can you add an empty line here, I think it improves readability, I know that 
> some other
> headers don’t add it unfortunately
Yah, that is no problem.

> > +#include <xen/fdt.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/types.h>
> > +
> > +bool __init device_tree_node_matches(
> > +    const void *fdt, int node, const char *match)
> > +{
> [...]
>
> Empty line
Ack.

> > +#ifndef __XEN_FDT_H__
> > +#define __XEN_FDT_H__
> > +
> > +#include <xen/init.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/types.h>
> For the new files, apart from Michal’s comments, if I remember correctly in the 
> past I was asked
> to add these lines to the end of the file:
>
> /*
> * Local variables:
> * mode: C
> * c-file-style: "BSD"
> * c-basic-offset: 4
> * indent-tabs-mode: nil
> * End:
> */
You are correct, I will get them added.

> Regarding the coding style, I think it’s better to keep the style you’ve found 
> in the original file,
> and change only some bits when the code is not following it.
>
> I know there is nothing enforcing parameters on the same line of the function 
> definition at the
> moment, but it is how it’s done from the original file so I would stick with it.
>
> Regarding the u32/u64 types, maybe since you are moving the code it can be the 
> occasion to
> convert them, but check with the maintainer before.
I can leave the main code as is, but I do think header decl's should be 
styled correctly as there is no need to have them churn in the future 
over purely style changes.
> I’ve also tested this serie and it works fine! I’m not leaving any tag because 
> this patch is going to
> change anyway.
No worries, thank you for taking the time to review the series.

v/r,
dps