From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Date: Tue, 19 Aug 2014 13:35:50 +0200 Subject: [U-Boot] [PATCH 03/23] fdt: Add resource parsing functions In-Reply-To: References: <1408346196-30419-1-git-send-email-thierry.reding@gmail.com> <1408346196-30419-4-git-send-email-thierry.reding@gmail.com> Message-ID: <20140819113549.GC19515@ulmo> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, Aug 18, 2014 at 12:06:26PM -0600, Simon Glass wrote: > Hi Thierry, > > On 18 August 2014 01:16, Thierry Reding wrote: > > From: Thierry Reding > > > > 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 > > --- > > 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: