[U-Boot] [PATCH 03/23] fdt: Add resource parsing functions

[Simon Glass]

Hi Thierry,

On 19 August 2014 05:35, Thierry Reding <thierry.reding at gmail.com> wrote:
>
> On Mon, Aug 18, 2014 at 12:06:26PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On 18 August 2014 01:16, Thierry Reding <thierry.reding at gmail.com> wrote:
> > > From: Thierry Reding <treding at nvidia.com>
> > >
> > > Add the fdt_get_resource() and fdt_get_named_resource() functions which
> > > can be used to parse resources (memory regions) from an FDT. A helper to
> > > compute the size of a region is also provided.
> > >
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  include/fdtdec.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/fdtdec.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 91 insertions(+)
> > >
> > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > index 856e6cf766de..e9091eee6bae 100644
> > > --- a/include/fdtdec.h
> > > +++ b/include/fdtdec.h
> > > @@ -40,6 +40,23 @@ struct fdt_memory {
> > >         fdt_addr_t end;
> > >  };
> > >
> > > +/* information about a resource */
> >
> > Please add comments, e.g. that end is inclusive.
>
> I've added this comment:
>
> /*
>  * Information about a resource. start is the first address of the resource
>  * and end is the last address (inclusive). The length of the resource will
>  * be equal to: end - start + 1.
>  */
>
> Is that enough or do you want me to be more verbose?


LGTM

>
>
> > > +/**
> > > + * Obtain a named resource from a device property.
> > > + *
> > > + * Look up the index of the name in a list of strings and return the resource
> > > + * at that index.
> > > + *
> > > + * @param fdt          FDT blob
> > > + * @param node         node to examine
> > > + * @param property     name of the property to parse
> > > + * @param names                name of the property to obtain the match the name to
> > > + * @param name         the name of the entry to look up
> >
> > These two parameters are confusing. Perhaps rename names to something better?
>
> How about "prop_names" so that it indicates it is the name of a
> property? I've also adjusted the comment a bit:
>
>  * @param prop_names name of the property containing the list of names
>
> Hopefully that will make it more obvious.


OK

>
>
> > > +int fdt_get_resource(const void *fdt, int node, const char *property,
> > > +                    unsigned int index, struct fdt_resource *res)
> >
> > s/index/find_index/
>
> In my opinion the find_ prefix is redundant. After all the function name
> already indicates that we're looking for a resource inside a property.
> And index would be the offset in that property.


OK

>
>
> > > +       if (!ptr)
> > > +               return len;
> > > +
> > > +       end = ptr + len / 4;
> >
> > sizeof(*ptr) might be better than 4.
>
> Device tree explicitly specifies that cells are 32-bit, so this can't
> ever be anything other than 4. But I'll change it anyway if you prefer.


I see sizeof() used in the lifdt code, so this might not go upstream as is.

>
> > > +
> > > +       while (ptr + na + ns <= end) {
> > > +               if (i == index) {
> > > +                       res->start = fdt_addr_to_cpu(*ptr);
> >
> > This doesn't deal with 64-bit addresses. There is a half-hearted
> > attempt with fdt_addr_t, but I wonder if we need a helper function
> > like fdt_get_addr(ptr, num_cells)?
>
> The Linux kernel handles this by wrapping it in an of_read_number()
> helper and always returning u64, like this:
>
>         static inline u64 of_read_number(const __be32 *cell, int size)
>         {
>                 u64 r = 0;
>
>                 while (size--)
>                         r = (r << 32) | be32_to_cpu(*(cell++));
>
>                 return r;
>         }
>
> It obviously only works for size in { 0, 1, 2 }, but I can add a helper
> like that if you want.


That looks good. It works for the cases we need and it's obvious later
where the logic is if we want to extend it.

Regards,
Simon