Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751865Ab0HIFMp (ORCPT ); Mon, 9 Aug 2010 01:12:45 -0400 Received: from mail-gy0-f174.google.com ([209.85.160.174]:65165 "EHLO mail-gy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751232Ab0HIFMn convert rfc822-to-8bit (ORCPT ); Mon, 9 Aug 2010 01:12:43 -0400 MIME-Version: 1.0 In-Reply-To: <20100808231116.21c7d6f3@dev.queued.net> References: <20100808231116.21c7d6f3@dev.queued.net> From: Grant Likely Date: Sun, 8 Aug 2010 23:12:21 -0600 X-Google-Sender-Auth: o1x7UH8fCc2cTSl-e6oSYTW5bCQ Message-ID: Subject: Re: [PATCH 2/3] sparc: make driver/of/pdt no longer sparc-specific To: Andres Salomon Cc: devicetree-discuss@lists.ozlabs.org, sparclinux@vger.kernel.org, davem@davemloft.net, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 17122 Lines: 516 Hi Andres, thanks for the patch. Comments below. g. On Sun, Aug 8, 2010 at 9:11 PM, Andres Salomon wrote: > > Clean up pdt.c: > ?- make build dependent upon config OF_PROMTREE > ?- #ifdef out the sparc-specific stuff > ?- create pdt-specific header > ?- create a pdt_ops struct that pdt uses to call arch-specific prom routines > > Signed-off-by: Andres Salomon > --- > ?arch/sparc/Kconfig ? ? ? ? ? ? ?| ? ?1 + > ?arch/sparc/include/asm/prom.h ? | ? ?5 +- > ?arch/sparc/kernel/prom.h ? ? ? ?| ? ?6 -- > ?arch/sparc/kernel/prom_common.c | ? 57 ++++++++++++++++++++++- > ?drivers/of/Kconfig ? ? ? ? ? ? ?| ? ?4 ++ > ?drivers/of/Makefile ? ? ? ? ? ? | ? ?1 + > ?drivers/of/pdt.c ? ? ? ? ? ? ? ?| ? 98 +++++++++++++++++++++++++------------- > ?include/linux/of_pdt.h ? ? ? ? ?| ? 42 +++++++++++++++++ > ?8 files changed, 171 insertions(+), 43 deletions(-) > ?create mode 100644 include/linux/of_pdt.h > > diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig > index 13a9f2f..ed3f009 100644 > --- a/arch/sparc/Kconfig > +++ b/arch/sparc/Kconfig > @@ -24,6 +24,7 @@ config SPARC > ? ? ? ?select HAVE_ARCH_KGDB if !SMP || SPARC64 > ? ? ? ?select HAVE_ARCH_TRACEHOOK > ? ? ? ?select ARCH_WANT_OPTIONAL_GPIOLIB > + ? ? ? select OF_PROMTREE Group this with the select OF from earlier in the config SPARC option. > ? ? ? ?select RTC_CLASS > ? ? ? ?select RTC_DRV_M48T59 > ? ? ? ?select HAVE_PERF_EVENTS > diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h > index f845828..329a976 100644 > --- a/arch/sparc/include/asm/prom.h > +++ b/arch/sparc/include/asm/prom.h > @@ -18,6 +18,7 @@ > ?* 2 of the License, or (at your option) any later version. > ?*/ > ?#include > +#include > ?#include > ?#include > ?#include > @@ -65,8 +66,8 @@ extern struct device_node *of_console_device; > ?extern char *of_console_path; > ?extern char *of_console_options; > > -extern void (*prom_build_more)(struct device_node *dp, struct device_node ***nextp); > -extern char *build_full_name(struct device_node *dp); > +extern void irq_trans_init(struct device_node *dp); > +extern char *build_path_component(struct device_node *dp); > > ?#endif /* __KERNEL__ */ > ?#endif /* _SPARC_PROM_H */ > diff --git a/arch/sparc/kernel/prom.h b/arch/sparc/kernel/prom.h > index eeb04a7..cf5fe1c 100644 > --- a/arch/sparc/kernel/prom.h > +++ b/arch/sparc/kernel/prom.h > @@ -4,12 +4,6 @@ > ?#include > ?#include > > -extern void * prom_early_alloc(unsigned long size); > -extern void irq_trans_init(struct device_node *dp); > - > -extern unsigned int prom_unique_id; > - > -extern char *build_path_component(struct device_node *dp); > ?extern void of_console_init(void); > > ?extern unsigned int prom_early_allocated; > diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c > index 7b454f6..4c5f67f 100644 > --- a/arch/sparc/kernel/prom_common.c > +++ b/arch/sparc/kernel/prom_common.c > @@ -20,6 +20,7 @@ > ?#include > ?#include > ?#include > +#include > ?#include > ?#include > ?#include > @@ -117,6 +118,60 @@ int of_find_in_proplist(const char *list, const char *match, int len) > ?} > ?EXPORT_SYMBOL(of_find_in_proplist); > > +/* > + * SPARC32 and SPARC64's prom_firstprop/prom_nextprop do things differently > + * here, despite sharing the same interface. ?SPARC32 doesn't fill in 'buf', > + * returning NULL on an error. ?SPARC64 fills in 'buf', but sets it to an > + * empty string upon error. > + */ > +static int __init handle_prop_quirks(char *buf, const char *name) > +{ > + ? ? ? if (!name || strlen(name) == 0) > + ? ? ? ? ? ? ? return -1; > + > +#ifdef CONFIG_SPARC32 > + ? ? ? strcpy(buf, name); > +#endif > + ? ? ? return 0; > +} > + > +static int __init prom_common_firstprop(phandle node, char *buf) > +{ > + ? ? ? const char *name; > + > + ? ? ? buf[0] = '\0'; > + ? ? ? name = prom_firstprop(node, buf); > + ? ? ? return handle_prop_quirks(buf, name); > +} > + > +static int __init prom_common_nextprop(phandle node, const char *prev, > + ? ? ? ? ? ? ? char *buf) > +{ > + ? ? ? const char *name; > + > + ? ? ? buf[0] = '\0'; > + ? ? ? name = prom_nextprop(node, prev, buf); > + ? ? ? return handle_prop_quirks(buf, name); > +} Rather than having both prom_common_{firstprop,nextprop}(), there only needs to be one hook; prom_common_nextprop(). Make it use the firstprop behaviour when it is passed a NULL in the prev pointer. This will simplify the users of this code further down. > + > ?unsigned int prom_early_allocated __initdata; > > -#include "../../../drivers/of/pdt.c" > +static struct of_pdt_ops prom_sparc_ops __initdata = { > + ? ? ? .firstprop = prom_common_firstprop, > + ? ? ? .nextprop = prom_common_nextprop, > + ? ? ? .getproplen = (int (*)(phandle, const char *))prom_getproplen, > + ? ? ? .getproperty = (int (*)(phandle, const char *, char *, int))prom_getproperty, > + ? ? ? .getchild = (phandle (*)(phandle))prom_getchild, > + ? ? ? .getsibling = (phandle (*)(phandle))prom_getsibling, If you have to explicitly cast these function pointers, then you're doing it wrong. :-) Listen to and fix the compiler complaint here. > +}; > + > +void __init prom_build_devicetree(void) > +{ > + ? ? ? of_pdt_set_ops(&prom_sparc_ops); > + ? ? ? of_pdt_build_devicetree(prom_root_node); Maybe I'm being nitpicky here, but I would pass the ops structure into of_pdt_build_devicetree() directly. I don't like the implied state of setting the ops pointer separate from parsing the tree. > + > + ? ? ? of_console_init(); > + > + ? ? ? printk(KERN_INFO "PROM: Built device tree with %u bytes of memory.\n", > + ? ? ? ? ? ? ?prom_early_allocated); pr_info() > +} > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index 1678dbc..c8a4b7c 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -5,6 +5,10 @@ config OF_FLATTREE > ? ? ? ?bool > ? ? ? ?depends on OF > > +config OF_PROMTREE > + ? ? ? bool > + ? ? ? depends on OF > + I can tell from the context here you're working from an older tree. Please rebase onto Linus' current top-of-tree. :-) A bunch of OF related patches have been merged for 2.6.36 that will conflict with this patch. > ?config OF_DYNAMIC > ? ? ? ?def_bool y > ? ? ? ?depends on OF && PPC_OF > diff --git a/drivers/of/Makefile b/drivers/of/Makefile > index f232cc9..54e8517 100644 > --- a/drivers/of/Makefile > +++ b/drivers/of/Makefile > @@ -1,5 +1,6 @@ > ?obj-y = base.o > ?obj-$(CONFIG_OF_FLATTREE) += fdt.o > +obj-$(CONFIG_OF_PROMTREE) += pdt.o > ?obj-$(CONFIG_OF_DEVICE) += device.o platform.o > ?obj-$(CONFIG_OF_GPIO) ? += gpio.o > ?obj-$(CONFIG_OF_I2C) ? += of_i2c.o > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c > index 61d9477..22f46fb 100644 > --- a/drivers/of/pdt.c > +++ b/drivers/of/pdt.c > @@ -1,5 +1,4 @@ > -/* prom_common.c: OF device tree support common code. > - * > +/* You should still retain a one-line description of what this file is for. > ?* Paul Mackerras ? ? ?August 1996. > ?* Copyright (C) 1996-2005 Paul Mackerras. > ?* > @@ -7,6 +6,7 @@ > ?* ? ?{engebret|bergner}@us.ibm.com > ?* > ?* ?Adapted for sparc by David S. Miller davem@davemloft.net > + * ?Adapted for multiple architectures by Andres Salomon > ?* > ?* ? ? ?This program is free software; you can redistribute it and/or > ?* ? ? ?modify it under the terms of the GNU General Public License > @@ -20,13 +20,44 @@ > ?#include > ?#include > ?#include > +#include > ?#include > -#include > -#include > > -void (*prom_build_more)(struct device_node *dp, struct device_node ***nextp); > +void __initdata (*prom_build_more)(struct device_node *dp, > + ? ? ? ? ? ? ? struct device_node ***nextp); > + > +static struct of_pdt_ops prom_ops __initdata; Why a full copy of the structure instead of a pointer? > + > +#if defined(CONFIG_SPARC) > +static unsigned int prom_unique_id __initdata; > + > +#define inc_unique_id(p) do { \ > + ? ? ? (p)->unique_id = prom_unique_id++; \ > +} while (0) Use a static inline. C code is preferred over preprocessor code. Also preserver the namespace and use the of_pdt_ prefix (that goes for all the new functions here in this file). > + > +static inline const char *fetch_node_name(struct device_node *dp) > +{ > + ? ? ? return dp->path_component_name; > +} > + > +#else > + > +static inline void inc_unique_id(void *p) > +{ > + ? ? ? /* unused on non-SPARC architectures */ > +} > + > +static inline const char *fetch_node_name(struct device_node *dp) > +{ > + ? ? ? return dp->name; > +} It would be nice to rationalize the differences between how sparc and powerpc use the ->name/->path_component_name fields; but I haven't investigated what the differences are. > + > +static inline void irq_trans_init(struct device_node *dp) > +{ > + ? ? ? /* unused on non-SPARC architectures */ > +} For empty statics like this; I'm fine with this more concise form: +static inline void inc_unique_id(void *p) { } +static inline void irq_trans_init(struct device_node *dp) { } (again, add the of_pdt_ prefix) > > -unsigned int prom_unique_id; > +#endif /* !CONFIG_SPARC */ > > ?static struct property * __init build_one_prop(phandle node, char *prev, > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? char *special_name, > @@ -35,7 +66,6 @@ static struct property * __init build_one_prop(phandle node, char *prev, > ?{ > ? ? ? ?static struct property *tmp = NULL; > ? ? ? ?struct property *p; > - ? ? ? const char *name; > > ? ? ? ?if (tmp) { > ? ? ? ? ? ? ? ?p = tmp; > @@ -43,7 +73,7 @@ static struct property * __init build_one_prop(phandle node, char *prev, > ? ? ? ? ? ? ? ?tmp = NULL; > ? ? ? ?} else { > ? ? ? ? ? ? ? ?p = prom_early_alloc(sizeof(struct property) + 32); > - ? ? ? ? ? ? ? p->unique_id = prom_unique_id++; > + ? ? ? ? ? ? ? inc_unique_id(p); > ? ? ? ?} > > ? ? ? ?p->name = (char *) (p + 1); > @@ -53,27 +83,24 @@ static struct property * __init build_one_prop(phandle node, char *prev, > ? ? ? ? ? ? ? ?p->value = prom_early_alloc(special_len); > ? ? ? ? ? ? ? ?memcpy(p->value, special_val, special_len); > ? ? ? ?} else { > - ? ? ? ? ? ? ? if (prev == NULL) { > - ? ? ? ? ? ? ? ? ? ? ? name = prom_firstprop(node, p->name); > - ? ? ? ? ? ? ? } else { > - ? ? ? ? ? ? ? ? ? ? ? name = prom_nextprop(node, prev, p->name); > - ? ? ? ? ? ? ? } > + ? ? ? ? ? ? ? int err; > > - ? ? ? ? ? ? ? if (!name || strlen(name) == 0) { > + ? ? ? ? ? ? ? if (prev == NULL) > + ? ? ? ? ? ? ? ? ? ? ? err = prom_ops.firstprop(node, p->name); > + ? ? ? ? ? ? ? else > + ? ? ? ? ? ? ? ? ? ? ? err = prom_ops.nextprop(node, prev, p->name); As mentioned earlier, this is better with a single .nextprop() hook that behaves differently when a NULL prev pointer is passed. > + ? ? ? ? ? ? ? if (err) { > ? ? ? ? ? ? ? ? ? ? ? ?tmp = p; > ? ? ? ? ? ? ? ? ? ? ? ?return NULL; > ? ? ? ? ? ? ? ?} > -#ifdef CONFIG_SPARC32 > - ? ? ? ? ? ? ? strcpy(p->name, name); > -#endif > - ? ? ? ? ? ? ? p->length = prom_getproplen(node, p->name); > + ? ? ? ? ? ? ? p->length = prom_ops.getproplen(node, p->name); > ? ? ? ? ? ? ? ?if (p->length <= 0) { > ? ? ? ? ? ? ? ? ? ? ? ?p->length = 0; > ? ? ? ? ? ? ? ?} else { > ? ? ? ? ? ? ? ? ? ? ? ?int len; > > ? ? ? ? ? ? ? ? ? ? ? ?p->value = prom_early_alloc(p->length + 1); > - ? ? ? ? ? ? ? ? ? ? ? len = prom_getproperty(node, p->name, p->value, > + ? ? ? ? ? ? ? ? ? ? ? len = prom_ops.getproperty(node, p->name, p->value, > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p->length); > ? ? ? ? ? ? ? ? ? ? ? ?if (len <= 0) > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?p->length = 0; > @@ -106,10 +133,10 @@ static char * __init get_one_property(phandle node, const char *name) > ? ? ? ?char *buf = ""; > ? ? ? ?int len; > > - ? ? ? len = prom_getproplen(node, name); > + ? ? ? len = prom_ops.getproplen(node, name); > ? ? ? ?if (len > 0) { > ? ? ? ? ? ? ? ?buf = prom_early_alloc(len); > - ? ? ? ? ? ? ? len = prom_getproperty(node, name, buf, len); > + ? ? ? ? ? ? ? len = prom_ops.getproperty(node, name, buf, len); > ? ? ? ?} > > ? ? ? ?return buf; > @@ -124,7 +151,7 @@ static struct device_node * __init prom_create_node(phandle node, > ? ? ? ? ? ? ? ?return NULL; > > ? ? ? ?dp = prom_early_alloc(sizeof(*dp)); > - ? ? ? dp->unique_id = prom_unique_id++; > + ? ? ? inc_unique_id(dp); > ? ? ? ?dp->parent = parent; > > ? ? ? ?kref_init(&dp->kref); > @@ -140,13 +167,13 @@ static struct device_node * __init prom_create_node(phandle node, > ? ? ? ?return dp; > ?} > > -char * __init build_full_name(struct device_node *dp) > +static char * __init build_full_name(struct device_node *dp) > ?{ > ? ? ? ?int len, ourlen, plen; > ? ? ? ?char *n; > > ? ? ? ?plen = strlen(dp->parent->full_name); > - ? ? ? ourlen = strlen(dp->path_component_name); > + ? ? ? ourlen = strlen(fetch_node_name(dp)); > ? ? ? ?len = ourlen + plen + 2; > > ? ? ? ?n = prom_early_alloc(len); > @@ -155,7 +182,7 @@ char * __init build_full_name(struct device_node *dp) > ? ? ? ? ? ? ? ?strcpy(n + plen, "/"); > ? ? ? ? ? ? ? ?plen++; > ? ? ? ?} > - ? ? ? strcpy(n + plen, dp->path_component_name); > + ? ? ? strcpy(n + plen, fetch_node_name(dp)); > > ? ? ? ?return n; > ?} > @@ -182,36 +209,39 @@ static struct device_node * __init prom_build_tree(struct device_node *parent, > ? ? ? ? ? ? ? ?*(*nextp) = dp; > ? ? ? ? ? ? ? ?*nextp = &dp->allnext; > > +#if defined(CONFIG_SPARC) > ? ? ? ? ? ? ? ?dp->path_component_name = build_path_component(dp); > +#endif > ? ? ? ? ? ? ? ?dp->full_name = build_full_name(dp); > > - ? ? ? ? ? ? ? dp->child = prom_build_tree(dp, prom_getchild(node), nextp); > + ? ? ? ? ? ? ? dp->child = prom_build_tree(dp, prom_ops.getchild(node), nextp); > > ? ? ? ? ? ? ? ?if (prom_build_more) > ? ? ? ? ? ? ? ? ? ? ? ?prom_build_more(dp, nextp); > > - ? ? ? ? ? ? ? node = prom_getsibling(node); > + ? ? ? ? ? ? ? node = prom_ops.getsibling(node); > ? ? ? ?} > > ? ? ? ?return ret; > ?} > > -void __init prom_build_devicetree(void) > +void __init of_pdt_build_devicetree(int root_node) > ?{ > ? ? ? ?struct device_node **nextp; > > - ? ? ? allnodes = prom_create_node(prom_root_node, NULL); > + ? ? ? allnodes = prom_create_node(root_node, NULL); > ? ? ? ?allnodes->path_component_name = ""; > ? ? ? ?allnodes->full_name = "/"; > > ? ? ? ?nextp = &allnodes->allnext; > ? ? ? ?allnodes->child = prom_build_tree(allnodes, > - ? ? ? ? ? ? ? ? ? ? ? prom_getchild(allnodes->phandle), > + ? ? ? ? ? ? ? ? ? ? ? prom_ops.getchild(allnodes->phandle), > ? ? ? ? ? ? ? ? ? ? ? ?&nextp); > +} > > - ? ? ? of_console_init(); > +void __init of_pdt_set_ops(struct of_pdt_ops *ops) > +{ > + ? ? ? BUG_ON(!ops); > > - ? ? ? printk("PROM: Built device tree with %u bytes of memory.\n", > - ? ? ? ? ? ? ?prom_early_allocated); > + ? ? ? prom_ops = *ops; As mentioned above, why is the structure copied instead of just storing the pointer. > ?} > - > diff --git a/include/linux/of_pdt.h b/include/linux/of_pdt.h > new file mode 100644 > index 0000000..1324ba5 > --- /dev/null > +++ b/include/linux/of_pdt.h > @@ -0,0 +1,42 @@ > +/* > + * Definitions for building a device tree by calling into the > + * Open Firmware PROM. > + * > + * Copyright (C) 2010 ?Andres Salomon > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + */ > + > +#ifndef _LINUX_OF_PDT_H > +#define _LINUX_OF_PDT_H > + > +extern void *prom_early_alloc(unsigned long size); > + > +/* overridable operations for calling into the PROM */ > +struct of_pdt_ops { > + ? ? ? /* buffers passed should be 32 bytes; return 0 on success */ > + ? ? ? int (*firstprop)(phandle node, char *buf); > + ? ? ? int (*nextprop)(phandle node, const char *prev, char *buf); > + > + ? ? ? /* for both functions, return proplen on success; -1 on error */ > + ? ? ? int (*getproplen)(phandle node, const char *prop); > + ? ? ? int (*getproperty)(phandle node, const char *prop, char *buf, > + ? ? ? ? ? ? ? ? ? ? ? int bufsize); > + > + ? ? ? /* phandles are 0 if no child or sibling exists */ > + ? ? ? phandle (*getchild)(phandle parent); > + ? ? ? phandle (*getsibling)(phandle node); > +}; > + > +extern void of_pdt_set_ops(struct of_pdt_ops *ops); > + > +/* for building the device tree */ > +extern void of_pdt_build_devicetree(int root_node); > + > +extern void (*prom_build_more)(struct device_node *dp, > + ? ? ? ? ? ? ? struct device_node ***nextp); > + > +#endif /* _LINUX_OF_PDT_H */ > -- > 1.5.6.5 > > -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/