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

[Thierry Reding]

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?

> > +/**
> > + * 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.

> > +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.

> > +       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.

> > +
> > +       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.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140819/92553822/attachment.pgp>