[RFC 0/3] of: Refactor device tree linked lists

[RFC 0/3] of: Refactor device tree linked lists

[Grant Likely]

I threw these patches together as part of getting the device_node
lifecycle issues sorted out so that the overlay patches can be merged.
It makes it possible to add and remove nodes without jumping through
crazy hoops.

I haven't yet decided if it is a good idea. It does increase the size of
deviec_node by 2 pointers which increases the footprint, but it
simplifies a lot of the code.  Although, if I was really concerned about
footprint I would be trying to get rid of the ->full_name string and
just use the path component instead. The primary user of full_name is
various printk format strings around the kernel and we could get the
same behaviour using a %p variant.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Grant Likely]

There are a bunch of users open coding the for_each_node_by_name() by
calling of_find_node_by_name() directly instead of using the macro. This
is getting in the way of some cleanups, and the possibility of removing
of_find_node_by_name() entirely. Clean it up so that all the users are
consistent.

Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
 arch/powerpc/platforms/powermac/pci.c      |  2 +-
 arch/powerpc/platforms/powermac/smp.c      |  2 +-
 arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
 arch/powerpc/platforms/pseries/setup.c     |  3 +--
 drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
 drivers/edac/cell_edac.c                   |  3 +--
 drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
 drivers/tty/serial/pmac_zilog.c            |  9 +++------
 sound/ppc/pmac.c                           |  6 +++---
 10 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
index 63d82bbc05e9..39e1d163c427 100644
--- a/arch/powerpc/platforms/powermac/feature.c
+++ b/arch/powerpc/platforms/powermac/feature.c
@@ -2805,25 +2805,20 @@ set_initial_features(void)
 		/* Enable GMAC for now for PCI probing. It will be disabled
 		 * later on after PCI probe
 		 */
-		np = of_find_node_by_name(NULL, "ethernet");
-		while(np) {
+		for_each_node_by_name(np, "ethernet")
 			if (of_device_is_compatible(np, "K2-GMAC"))
 				g5_gmac_enable(np, 0, 1);
-			np = of_find_node_by_name(np, "ethernet");
-		}
 
 		/* Enable FW before PCI probe. Will be disabled later on
 		 * Note: We should have a batter way to check that we are
 		 * dealing with uninorth internal cell and not a PCI cell
 		 * on the external PCI. The code below works though.
 		 */
-		np = of_find_node_by_name(NULL, "firewire");
-		while(np) {
+		for_each_node_by_name(np, "firewire") {
 			if (of_device_is_compatible(np, "pci106b,5811")) {
 				macio_chips[0].flags |= MACIO_FLAG_FW_SUPPORTED;
 				g5_fw_enable(np, 0, 1);
 			}
-			np = of_find_node_by_name(np, "firewire");
 		}
 	}
 #else /* CONFIG_POWER4 */
@@ -2834,13 +2829,11 @@ set_initial_features(void)
 		/* Enable GMAC for now for PCI probing. It will be disabled
 		 * later on after PCI probe
 		 */
-		np = of_find_node_by_name(NULL, "ethernet");
-		while(np) {
+		for_each_node_by_name(np, "ethernet") {
 			if (np->parent
 			    && of_device_is_compatible(np->parent, "uni-north")
 			    && of_device_is_compatible(np, "gmac"))
 				core99_gmac_enable(np, 0, 1);
-			np = of_find_node_by_name(np, "ethernet");
 		}
 
 		/* Enable FW before PCI probe. Will be disabled later on
@@ -2848,8 +2841,7 @@ set_initial_features(void)
 		 * dealing with uninorth internal cell and not a PCI cell
 		 * on the external PCI. The code below works though.
 		 */
-		np = of_find_node_by_name(NULL, "firewire");
-		while(np) {
+		for_each_node_by_name(np, "firewire") {
 			if (np->parent
 			    && of_device_is_compatible(np->parent, "uni-north")
 			    && (of_device_is_compatible(np, "pci106b,18") ||
@@ -2858,18 +2850,16 @@ set_initial_features(void)
 				macio_chips[0].flags |= MACIO_FLAG_FW_SUPPORTED;
 				core99_firewire_enable(np, 0, 1);
 			}
-			np = of_find_node_by_name(np, "firewire");
 		}
 
 		/* Enable ATA-100 before PCI probe. */
 		np = of_find_node_by_name(NULL, "ata-6");
-		while(np) {
+		for_each_node_by_name(np, "ata-6") {
 			if (np->parent
 			    && of_device_is_compatible(np->parent, "uni-north")
 			    && of_device_is_compatible(np, "kauai-ata")) {
 				core99_ata100_enable(np, 1);
 			}
-			np = of_find_node_by_name(np, "ata-6");
 		}
 
 		/* Switch airport off */
diff --git a/arch/powerpc/platforms/powermac/pci.c b/arch/powerpc/platforms/powermac/pci.c
index cf7009b8c7b6..7e868ccf3b0d 100644
--- a/arch/powerpc/platforms/powermac/pci.c
+++ b/arch/powerpc/platforms/powermac/pci.c
@@ -698,7 +698,7 @@ static void __init fixup_nec_usb2(void)
 {
 	struct device_node *nec;
 
-	for (nec = NULL; (nec = of_find_node_by_name(nec, "usb")) != NULL;) {
+	for_each_node_by_name(nec, "usb") {
 		struct pci_controller *hose;
 		u32 data;
 		const u32 *prop;
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 5cbd4d67d5c4..af094ae03dbb 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -577,7 +577,7 @@ static void __init smp_core99_setup_i2c_hwsync(int ncpus)
 	int ok;
 
 	/* Look for the clock chip */
-	while ((cc = of_find_node_by_name(cc, "i2c-hwclock")) != NULL) {
+	for_each_node_by_name(cc, "i2c-hwclock") {
 		p = of_get_parent(cc);
 		ok = p && of_device_is_compatible(p, "uni-n-i2c");
 		of_node_put(p);
diff --git a/arch/powerpc/platforms/powermac/udbg_adb.c b/arch/powerpc/platforms/powermac/udbg_adb.c
index 44e0b55a2a02..366bd221edec 100644
--- a/arch/powerpc/platforms/powermac/udbg_adb.c
+++ b/arch/powerpc/platforms/powermac/udbg_adb.c
@@ -191,7 +191,7 @@ int __init udbg_adb_init(int force_btext)
 	 * of type "adb". If not, we return a failure, but we keep the
 	 * bext output set for now
 	 */
-	for (np = NULL; (np = of_find_node_by_name(np, "keyboard")) != NULL;) {
+	for_each_node_by_name(np, "keyboard") {
 		struct device_node *parent = of_get_parent(np);
 		int found = (parent && strcmp(parent->type, "adb") == 0);
 		of_node_put(parent);
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 099d2df976a2..4131f6644fd6 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -232,8 +232,7 @@ static void __init pseries_discover_pic(void)
 	struct device_node *np;
 	const char *typep;
 
-	for (np = NULL; (np = of_find_node_by_name(np,
-						   "interrupt-controller"));) {
+	for_each_node_by_name(np, "interrupt-controller") {
 		typep = of_get_property(np, "compatible", NULL);
 		if (strstr(typep, "open-pic")) {
 			pSeries_mpic_node = of_node_get(np);
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 8bc422977b5b..4ff86878727f 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -499,8 +499,7 @@ static int __init g5_pm72_cpufreq_init(struct device_node *cpunode)
 	}
 
 	/* Lookup the i2c hwclock */
-	for (hwclock = NULL;
-	     (hwclock = of_find_node_by_name(hwclock, "i2c-hwclock")) != NULL;){
+	for_each_node_by_name(hwclock, "i2c-hwclock") {
 		const char *loc = of_get_property(hwclock,
 				"hwctrl-location", NULL);
 		if (loc == NULL)
diff --git a/drivers/edac/cell_edac.c b/drivers/edac/cell_edac.c
index 374b57fc596d..a12c8552f6a6 100644
--- a/drivers/edac/cell_edac.c
+++ b/drivers/edac/cell_edac.c
@@ -134,8 +134,7 @@ static void cell_edac_init_csrows(struct mem_ctl_info *mci)
 	int				j;
 	u32				nr_pages;
 
-	for (np = NULL;
-	     (np = of_find_node_by_name(np, "memory")) != NULL;) {
+	for_each_node_by_name(np, "memory") {
 		struct resource r;
 
 		/* We "know" that the Cell firmware only creates one entry
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 4796c15fba94..f276ddcb27cc 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -369,11 +369,11 @@ static void __exit cleanup_slots(void)
 
 static int __init rpaphp_init(void)
 {
-	struct device_node *dn = NULL;
+	struct device_node *dn;
 
 	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
 
-	while ((dn = of_find_node_by_name(dn, "pci")))
+	for_each_node_by_name(dn, "pci")
 		rpaphp_add_slot(dn);
 
 	return 0;
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index e9d420ff3931..1d6d16b8f8b1 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1650,8 +1650,7 @@ static int __init pmz_probe(void)
 	/*
 	 * Find all escc chips in the system
 	 */
-	node_p = of_find_node_by_name(NULL, "escc");
-	while (node_p) {
+	for_each_node_by_name(node_p, "escc") {
 		/*
 		 * First get channel A/B node pointers
 		 * 
@@ -1669,7 +1668,7 @@ static int __init pmz_probe(void)
 			of_node_put(node_b);
 			printk(KERN_ERR "pmac_zilog: missing node %c for escc %s\n",
 				(!node_a) ? 'a' : 'b', node_p->full_name);
-			goto next;
+			continue;
 		}
 
 		/*
@@ -1696,11 +1695,9 @@ static int __init pmz_probe(void)
 			of_node_put(node_b);
 			memset(&pmz_ports[count], 0, sizeof(struct uart_pmac_port));
 			memset(&pmz_ports[count+1], 0, sizeof(struct uart_pmac_port));
-			goto next;
+			continue;
 		}
 		count += 2;
-next:
-		node_p = of_find_node_by_name(node_p, "escc");
 	}
 	pmz_ports_count = count;
 
diff --git a/sound/ppc/pmac.c b/sound/ppc/pmac.c
index 7a43c0c38316..8a431bcb056c 100644
--- a/sound/ppc/pmac.c
+++ b/sound/ppc/pmac.c
@@ -992,9 +992,9 @@ static int snd_pmac_detect(struct snd_pmac *chip)
 		return -ENODEV;
 
 	if (!sound) {
-		sound = of_find_node_by_name(NULL, "sound");
-		while (sound && sound->parent != chip->node)
-			sound = of_find_node_by_name(sound, "sound");
+		for_each_node_by_name(sound, "sound")
+			if (sound->parent == chip->node)
+				break;
 	}
 	if (! sound) {
 		of_node_put(chip->node);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/3] of: Replace custom linked list with list_head

[Grant Likely]

The device tree data structure uses a custom linked list implementation
which is baroque and prone to bugs. Replace the allnodes list with a
list_head and the common list_head accessor functions.

At the same time, move the private for_each_of_allnodes() macro into
drivers/of/base.c. It must not be used outside of the core code because
the macro doesn't take into account device tree locking.

Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/mach-vexpress/v2m.c        |   2 +-
 drivers/of/base.c                   | 126 +++++++++++++++++++-----------------
 drivers/of/fdt.c                    |  27 ++++----
 drivers/of/pdt.c                    |  26 +++-----
 drivers/pci/hotplug/rpadlpar_core.c |   2 +-
 include/linux/of.h                  |   9 ++-
 include/linux/of_fdt.h              |   3 +-
 include/linux/of_pdt.h              |   3 +-
 8 files changed, 96 insertions(+), 102 deletions(-)

diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 4f8b8cb17ff5..57151ba5edee 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -412,7 +412,7 @@ void __init v2m_dt_init_early(void)
 	vexpress_sysreg_of_early_init();
 
 	/* Confirm board type against DT property, if available */
-	if (of_property_read_u32(of_allnodes, "arm,hbi", &dt_hbi) == 0) {
+	if (of_property_read_u32(of_root, "arm,hbi", &dt_hbi) == 0) {
 		u32 hbi = vexpress_get_hbi(VEXPRESS_SITE_MASTER);
 
 		if (WARN_ON(dt_hbi != hbi))
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 567e6e1b7921..6c0bd86b802f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -31,14 +31,22 @@
 
 LIST_HEAD(aliases_lookup);
 
-struct device_node *of_allnodes;
+LIST_HEAD(of_allnodes);
 EXPORT_SYMBOL(of_allnodes);
-struct device_node *of_chosen;
-struct device_node *of_aliases;
+struct device_node *of_root = NULL;
+struct device_node *of_chosen = NULL;
+struct device_node *of_aliases = NULL;
 static struct device_node *of_stdout;
 
 static struct kset *of_kset;
 
+#define of_allnodes_prepare(dn) \
+	list_prepare_entry(dn, &of_allnodes, allnext)
+#define for_each_of_allnodes(dn) \
+	list_for_each_entry(dn, &of_allnodes, allnext)
+#define for_each_of_allnodes_continue(dn) \
+	list_for_each_entry_continue(dn, &of_allnodes, allnext)
+
 /*
  * Used to protect the of_aliases; but also overloaded to hold off addition of
  * nodes to sysfs
@@ -299,7 +307,7 @@ static int __init of_init(void)
 	mutex_unlock(&of_aliases_mutex);
 
 	/* Symlink in /proc as required by userspace ABI */
-	if (of_allnodes)
+	if (of_root)
 		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
 
 	return 0;
@@ -348,18 +356,18 @@ EXPORT_SYMBOL(of_find_property);
  * Returns a node pointer with refcount incremented, use
  * of_node_put() on it when done.
  */
-struct device_node *of_find_all_nodes(struct device_node *prev)
+struct device_node *of_find_all_nodes(struct device_node *np)
 {
-	struct device_node *np;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = prev ? prev->allnext : of_allnodes;
-	for (; np != NULL; np = np->allnext)
-		if (of_node_get(np))
-			break;
-	of_node_put(prev);
+	of_node_put(np);
+	np = list_next_entry(of_allnodes_prepare(np), allnext);
+	if (&np->allnext == &of_allnodes)
+		np = NULL;
+	of_node_get(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
 	return np;
 }
 EXPORT_SYMBOL(of_find_all_nodes);
@@ -831,7 +839,7 @@ struct device_node *of_find_node_by_path(const char *path)
 	unsigned long flags;
 
 	if (strcmp(path, "/") == 0)
-		return of_node_get(of_allnodes);
+		return of_node_get(of_root);
 
 	/* The path could begin with an alias */
 	if (*path != '/') {
@@ -856,7 +864,7 @@ struct device_node *of_find_node_by_path(const char *path)
 	/* Step down the tree matching path components */
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	if (!np)
-		np = of_node_get(of_allnodes);
+		np = of_node_get(of_root);
 	while (np && *path == '/') {
 		path++; /* Increment past '/' delimiter */
 		np = __of_find_node_by_path(np, path);
@@ -881,16 +889,18 @@ EXPORT_SYMBOL(of_find_node_by_path);
 struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = from ? from->allnext : of_allnodes;
-	for (; np; np = np->allnext)
-		if (np->name && (of_node_cmp(np->name, name) == 0)
-		    && of_node_get(np))
-			break;
 	of_node_put(from);
+	from = of_allnodes_prepare(from);
+	for_each_of_allnodes_continue(from) {
+		if (from->name && (of_node_cmp(from->name, name) == 0)) {
+			np = of_node_get(from);
+			break;
+		}
+	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
@@ -911,16 +921,18 @@ EXPORT_SYMBOL(of_find_node_by_name);
 struct device_node *of_find_node_by_type(struct device_node *from,
 	const char *type)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = from ? from->allnext : of_allnodes;
-	for (; np; np = np->allnext)
-		if (np->type && (of_node_cmp(np->type, type) == 0)
-		    && of_node_get(np))
-			break;
 	of_node_put(from);
+	from = of_allnodes_prepare(from);
+	for_each_of_allnodes_continue(from) {
+		if (from->type && (of_node_cmp(from->type, type) == 0)) {
+			np = of_node_get(from);
+			break;
+		}
+	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
@@ -943,18 +955,20 @@ EXPORT_SYMBOL(of_find_node_by_type);
 struct device_node *of_find_compatible_node(struct device_node *from,
 	const char *type, const char *compatible)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = from ? from->allnext : of_allnodes;
-	for (; np; np = np->allnext) {
-		if (__of_device_is_compatible(np, compatible, type, NULL) &&
-		    of_node_get(np))
+	of_node_put(from);
+	from = of_allnodes_prepare(from);
+	for_each_of_allnodes_continue(from) {
+		if (__of_device_is_compatible(from, compatible, type, NULL)) {
+			np = of_node_get(from);
 			break;
+		}
 	}
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
 	return np;
 }
 EXPORT_SYMBOL(of_find_compatible_node);
@@ -974,22 +988,22 @@ EXPORT_SYMBOL(of_find_compatible_node);
 struct device_node *of_find_node_with_property(struct device_node *from,
 	const char *prop_name)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = from ? from->allnext : of_allnodes;
-	for (; np; np = np->allnext) {
-		for (pp = np->properties; pp; pp = pp->next) {
+	of_node_put(from);
+	from = of_allnodes_prepare(from);
+	for_each_of_allnodes_continue(from) {
+		for (pp = from->properties; pp; pp = pp->next) {
 			if (of_prop_cmp(pp->name, prop_name) == 0) {
-				of_node_get(np);
+				np = of_node_get(from);
 				goto out;
 			}
 		}
 	}
-out:
-	of_node_put(from);
+ out:
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
@@ -1054,7 +1068,7 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
 					const struct of_device_id *matches,
 					const struct of_device_id **match)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	const struct of_device_id *m;
 	unsigned long flags;
 
@@ -1062,16 +1076,16 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
 		*match = NULL;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	np = from ? from->allnext : of_allnodes;
-	for (; np; np = np->allnext) {
-		m = __of_match_node(matches, np);
-		if (m && of_node_get(np)) {
+	of_node_put(from);
+	from = of_allnodes_prepare(from);
+	for_each_of_allnodes_continue(from) {
+		if ((m = __of_match_node(matches, from))) {
+			np = of_node_get(from);
 			if (match)
 				*match = m;
 			break;
 		}
 	}
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
@@ -1113,14 +1127,16 @@ EXPORT_SYMBOL_GPL(of_modalias_node);
  */
 struct device_node *of_find_node_by_phandle(phandle handle)
 {
-	struct device_node *np;
+	struct device_node *tmp, *np = NULL;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	for (np = of_allnodes; np; np = np->allnext)
-		if (np->phandle == handle)
+	for_each_of_allnodes(tmp) {
+		if (tmp->phandle == handle) {
+			np = of_node_get(tmp);
 			break;
-	of_node_get(np);
+		}
+	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
@@ -1959,10 +1975,9 @@ int of_attach_node(struct device_node *np)
 		return rc;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
+	list_add_tail(&np->allnext, &of_allnodes);
 	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
 	np->parent->child = np;
-	of_allnodes = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
@@ -2000,16 +2015,7 @@ int of_detach_node(struct device_node *np)
 		return rc;
 	}
 
-	if (of_allnodes == np)
-		of_allnodes = np->allnext;
-	else {
-		struct device_node *prev;
-		for (prev = of_allnodes;
-		     prev->allnext != np;
-		     prev = prev->allnext)
-			;
-		prev->allnext = np->allnext;
-	}
+	list_del(&np->allnext);
 
 	if (parent->child == np)
 		parent->child = np->sibling;
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 17be90f5445f..6876ac3d4b30 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -96,14 +96,14 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
  * @mem: Memory chunk to use for allocating device nodes and properties
  * @p: pointer to node in flat tree
  * @dad: Parent struct device_node
- * @allnextpp: pointer to ->allnext from last allocated device_node
+ * @allnext: list of ->allnext from this tree
  * @fpsize: Size of the node path up at the current depth.
  */
 static void * unflatten_dt_node(void *blob,
 				void *mem,
 				int *poffset,
 				struct device_node *dad,
-				struct device_node ***allnextpp,
+				struct list_head *allnext,
 				unsigned long fpsize)
 {
 	const __be32 *p;
@@ -151,7 +151,7 @@ static void * unflatten_dt_node(void *blob,
 
 	np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl,
 				__alignof__(struct device_node));
-	if (allnextpp) {
+	if (allnext) {
 		char *fn;
 		of_node_init(np);
 		np->full_name = fn = ((char *)np) + sizeof(*np);
@@ -173,8 +173,7 @@ static void * unflatten_dt_node(void *blob,
 		memcpy(fn, pathp, l);
 
 		prev_pp = &np->properties;
-		**allnextpp = np;
-		*allnextpp = &np->allnext;
+		list_add_tail(&np->allnext, allnext);
 		if (dad != NULL) {
 			np->parent = dad;
 			/* we temporarily use the next field as `last_child'*/
@@ -205,7 +204,7 @@ static void * unflatten_dt_node(void *blob,
 			has_name = 1;
 		pp = unflatten_dt_alloc(&mem, sizeof(struct property),
 					__alignof__(struct property));
-		if (allnextpp) {
+		if (allnext) {
 			/* We accept flattened tree phandles either in
 			 * ePAPR-style "phandle" properties, or the
 			 * legacy "linux,phandle" properties.  If both
@@ -247,7 +246,7 @@ static void * unflatten_dt_node(void *blob,
 		sz = (pa - ps) + 1;
 		pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
 					__alignof__(struct property));
-		if (allnextpp) {
+		if (allnext) {
 			pp->name = "name";
 			pp->length = sz;
 			pp->value = pp + 1;
@@ -259,7 +258,7 @@ static void * unflatten_dt_node(void *blob,
 				(char *)pp->value);
 		}
 	}
-	if (allnextpp) {
+	if (allnext) {
 		*prev_pp = NULL;
 		np->name = of_get_property(np, "name", NULL);
 		np->type = of_get_property(np, "device_type", NULL);
@@ -275,7 +274,7 @@ static void * unflatten_dt_node(void *blob,
 	if (depth < 0)
 		depth = 0;
 	while (*poffset > 0 && depth > old_depth)
-		mem = unflatten_dt_node(blob, mem, poffset, np, allnextpp,
+		mem = unflatten_dt_node(blob, mem, poffset, np, allnext,
 					fpsize);
 
 	if (*poffset < 0 && *poffset != -FDT_ERR_NOTFOUND)
@@ -297,13 +296,12 @@ static void * unflatten_dt_node(void *blob,
  * for the resulting tree
  */
 static void __unflatten_device_tree(void *blob,
-			     struct device_node **mynodes,
+			     struct list_head *allnext,
 			     void * (*dt_alloc)(u64 size, u64 align))
 {
 	unsigned long size;
 	int start;
 	void *mem;
-	struct device_node **allnextp = mynodes;
 
 	pr_debug(" -> unflatten_device_tree()\n");
 
@@ -339,11 +337,10 @@ static void __unflatten_device_tree(void *blob,
 
 	/* Second pass, do actual unflattening */
 	start = 0;
-	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
+	unflatten_dt_node(blob, mem, &start, NULL, allnext, 0);
 	if (be32_to_cpup(mem + size) != 0xdeadbeef)
 		pr_warning("End of tree marker overwritten: %08x\n",
 			   be32_to_cpup(mem + size));
-	*allnextp = NULL;
 
 	pr_debug(" <- unflatten_device_tree()\n");
 }
@@ -361,8 +358,7 @@ static void *kernel_tree_alloc(u64 size, u64 align)
  * pointers of the nodes so the normal device-tree walking functions
  * can be used.
  */
-void of_fdt_unflatten_tree(unsigned long *blob,
-			struct device_node **mynodes)
+void of_fdt_unflatten_tree(unsigned long *blob, struct list_head *mynodes)
 {
 	__unflatten_device_tree(blob, mynodes, &kernel_tree_alloc);
 }
@@ -904,6 +900,7 @@ void __init unflatten_device_tree(void)
 {
 	__unflatten_device_tree(initial_boot_params, &of_allnodes,
 				early_init_dt_alloc_memory_arch);
+	of_root = list_first_entry_or_null(&of_allnodes, struct device_node, allnext);
 
 	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
 	of_alias_scan(early_init_dt_alloc_memory_arch);
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 36b4035881b0..38dd814870b4 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -25,8 +25,7 @@
 
 static struct of_pdt_ops *of_pdt_prom_ops __initdata;
 
-void __initdata (*of_pdt_build_more)(struct device_node *dp,
-		struct device_node ***nextp);
+void __initdata (*of_pdt_build_more)(struct device_node *dp);
 
 #if defined(CONFIG_SPARC)
 unsigned int of_pdt_unique_id __initdata;
@@ -192,8 +191,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 }
 
 static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
-						   phandle node,
-						   struct device_node ***nextp)
+						   phandle node)
 {
 	struct device_node *ret = NULL, *prev_sibling = NULL;
 	struct device_node *dp;
@@ -210,16 +208,15 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
 			ret = dp;
 		prev_sibling = dp;
 
-		*(*nextp) = dp;
-		*nextp = &dp->allnext;
+		list_add_tail(&of_allnodes, &np->allnext);
 
 		dp->full_name = of_pdt_build_full_name(dp);
 
 		dp->child = of_pdt_build_tree(dp,
-				of_pdt_prom_ops->getchild(node), nextp);
+				of_pdt_prom_ops->getchild(node));
 
 		if (of_pdt_build_more)
-			of_pdt_build_more(dp, nextp);
+			of_pdt_build_more(dp);
 
 		node = of_pdt_prom_ops->getsibling(node);
 	}
@@ -234,20 +231,17 @@ static void * __init kernel_tree_alloc(u64 size, u64 align)
 
 void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
 {
-	struct device_node **nextp;
-
 	BUG_ON(!ops);
 	of_pdt_prom_ops = ops;
 
-	of_allnodes = of_pdt_create_node(root_node, NULL);
+	of_root = of_pdt_create_node(root_node, NULL);
 #if defined(CONFIG_SPARC)
-	of_allnodes->path_component_name = "";
+	of_root->path_component_name = "";
 #endif
-	of_allnodes->full_name = "/";
+	of_root->full_name = "/";
 
-	nextp = &of_allnodes->allnext;
-	of_allnodes->child = of_pdt_build_tree(of_allnodes,
-			of_pdt_prom_ops->getchild(of_allnodes->phandle), &nextp);
+	list_add_tail(&of_allnodes, &of_root->allnext);
+	of_root->child = of_pdt_build_tree(of_root, of_pdt_prom_ops->getchild(of_root->phandle));
 
 	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
 	of_alias_scan(kernel_tree_alloc);
diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
index 4fcdeedda31b..e25e4d1ec241 100644
--- a/drivers/pci/hotplug/rpadlpar_core.c
+++ b/drivers/pci/hotplug/rpadlpar_core.c
@@ -68,7 +68,7 @@ static struct device_node *find_php_slot_pci_node(char *drc_name,
 	char *type;
 	int rc;
 
-	while ((np = of_find_node_by_name(np, "pci"))) {
+	for_each_node_by_name(np, "pci") {
 		rc = rpaphp_get_drc_props(np, NULL, &name, &type, NULL);
 		if (rc == 0)
 			if (!strcmp(drc_name, name) && !strcmp(drc_type, type))
diff --git a/include/linux/of.h b/include/linux/of.h
index 3bad8d106e0e..e082db3c284f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -56,7 +56,7 @@ struct device_node {
 	struct	device_node *child;
 	struct	device_node *sibling;
 	struct	device_node *next;	/* next device of same type */
-	struct	device_node *allnext;	/* next in list of all nodes */
+	struct	list_head allnext;
 	struct	kobject kobj;
 	unsigned long _flags;
 	void	*data;
@@ -110,14 +110,15 @@ static inline void of_node_put(struct device_node *node) { }
 #ifdef CONFIG_OF
 
 /* Pointer for first entry in chain of all nodes. */
-extern struct device_node *of_allnodes;
+extern struct list_head of_allnodes;
+extern struct device_node *of_root;
 extern struct device_node *of_chosen;
 extern struct device_node *of_aliases;
 extern raw_spinlock_t devtree_lock;
 
 static inline bool of_have_populated_dt(void)
 {
-	return of_allnodes != NULL;
+	return !list_empty(&of_allnodes);
 }
 
 static inline bool of_node_is_root(const struct device_node *node)
@@ -208,8 +209,6 @@ static inline const char *of_node_full_name(const struct device_node *np)
 	return np ? np->full_name : "<no-node>";
 }
 
-#define for_each_of_allnodes(dn) \
-	for (dn = of_allnodes; dn; dn = dn->allnext)
 extern struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name);
 extern struct device_node *of_find_node_by_type(struct device_node *from,
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 5c0ab057eecf..efa7615a1ce9 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -35,8 +35,7 @@ extern int of_fdt_is_compatible(const void *blob,
 				const char *compat);
 extern int of_fdt_match(const void *blob, unsigned long node,
 			const char *const *compat);
-extern void of_fdt_unflatten_tree(unsigned long *blob,
-			       struct device_node **mynodes);
+extern void of_fdt_unflatten_tree(unsigned long *blob, struct list_head *mynodes);
 
 /* TBD: Temporary export of fdt globals - remove when code fully merged */
 extern int __initdata dt_root_addr_cells;
diff --git a/include/linux/of_pdt.h b/include/linux/of_pdt.h
index c65a18a0cfdf..7e09244bb679 100644
--- a/include/linux/of_pdt.h
+++ b/include/linux/of_pdt.h
@@ -39,7 +39,6 @@ extern void *prom_early_alloc(unsigned long size);
 /* for building the device tree */
 extern void of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops);
 
-extern void (*of_pdt_build_more)(struct device_node *dp,
-		struct device_node ***nextp);
+extern void (*of_pdt_build_more)(struct device_node *dp);
 
 #endif /* _LINUX_OF_PDT_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 3/3] of: Refactor device_node child list to use list_head

[Grant Likely]

The device tree data structure uses a custom linked list implemenation
which is baroque and prone to bugs. Replace the child node lists with a
list_head and the common list_head accessor functions.

Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/base.c  | 43 +++++++++++++++++++------------------------
 drivers/of/fdt.c   | 12 +++---------
 include/linux/of.h |  8 +++++---
 3 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6c0bd86b802f..69f5648babd8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -47,6 +47,12 @@ static struct kset *of_kset;
 #define for_each_of_allnodes_continue(dn) \
 	list_for_each_entry_continue(dn, &of_allnodes, allnext)
 
+#define of_children_prepare(parent, child) \
+	list_prepare_entry(child, &parent->children, childnext)
+#define for_each_of_children(parent, child) \
+	list_for_each_entry(child, &parent->children, childnext)
+#define for_each_of_children_continue(parent, child) \
+	list_for_each_entry_continue(child, &parent->children, childnext)
 /*
  * Used to protect the of_aliases; but also overloaded to hold off addition of
  * nodes to sysfs
@@ -711,11 +717,11 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
 	if (!node)
 		return NULL;
 
-	next = prev ? prev->sibling : node->child;
-	for (; next; next = next->sibling)
-		if (of_node_get(next))
-			break;
 	of_node_put(prev);
+	next = list_next_entry(of_children_prepare(node, prev), childnext);
+	if (&next->childnext == &node->children)
+		next = NULL;
+	of_node_get(next);
 	return next;
 }
 #define __for_each_child_of_node(parent, child) \
@@ -754,21 +760,21 @@ EXPORT_SYMBOL(of_get_next_child);
 struct device_node *of_get_next_available_child(const struct device_node *node,
 	struct device_node *prev)
 {
-	struct device_node *next;
+	struct device_node *next = NULL;
 	unsigned long flags;
 
 	if (!node)
 		return NULL;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	next = prev ? prev->sibling : node->child;
-	for (; next; next = next->sibling) {
-		if (!__of_device_is_available(next))
-			continue;
-		if (of_node_get(next))
+	of_node_put(prev);
+	prev = of_children_prepare(node, prev);
+	for_each_of_children_continue(node, prev) {
+		if (__of_device_is_available(prev)) {
+			next = of_node_get(prev);
 			break;
+		}
 	}
-	of_node_put(prev);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return next;
 }
@@ -1976,8 +1982,7 @@ int of_attach_node(struct device_node *np)
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	list_add_tail(&np->allnext, &of_allnodes);
-	np->sibling = np->parent->child;
-	np->parent->child = np;
+	list_add_tail(&np->childnext, &np->parent->children);
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
@@ -2016,17 +2021,7 @@ int of_detach_node(struct device_node *np)
 	}
 
 	list_del(&np->allnext);
-
-	if (parent->child == np)
-		parent->child = np->sibling;
-	else {
-		struct device_node *prevsib;
-		for (prevsib = np->parent->child;
-		     prevsib->sibling != np;
-		     prevsib = prevsib->sibling)
-			;
-		prevsib->sibling = np->sibling;
-	}
+	list_del(&np->childnext);
 
 	of_node_set_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6876ac3d4b30..2ff96cac1496 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -155,6 +155,7 @@ static void * unflatten_dt_node(void *blob,
 		char *fn;
 		of_node_init(np);
 		np->full_name = fn = ((char *)np) + sizeof(*np);
+		np->parent = dad;
 		if (new_format) {
 			/* rebuild full path for new format */
 			if (dad && dad->parent) {
@@ -174,15 +175,8 @@ static void * unflatten_dt_node(void *blob,
 
 		prev_pp = &np->properties;
 		list_add_tail(&np->allnext, allnext);
-		if (dad != NULL) {
-			np->parent = dad;
-			/* we temporarily use the next field as `last_child'*/
-			if (dad->next == NULL)
-				dad->child = np;
-			else
-				dad->next->sibling = np;
-			dad->next = np;
-		}
+		if (np->parent)
+			list_add_tail(&np->childnext, &np->parent->children);
 	}
 	/* process properties */
 	for (offset = fdt_first_property_offset(blob, *poffset);
diff --git a/include/linux/of.h b/include/linux/of.h
index e082db3c284f..11bef100bc43 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -52,10 +52,11 @@ struct device_node {
 
 	struct	property *properties;
 	struct	property *deadprops;	/* removed properties */
+
+	struct	list_head children;	/* next device of same type */
 	struct	device_node *parent;
-	struct	device_node *child;
-	struct	device_node *sibling;
-	struct	device_node *next;	/* next device of same type */
+
+	struct	list_head childnext;
 	struct	list_head allnext;
 	struct	kobject kobj;
 	unsigned long _flags;
@@ -80,6 +81,7 @@ extern int of_node_add(struct device_node *node);
 extern struct kobj_type of_node_ktype;
 static inline void of_node_init(struct device_node *node)
 {
+	INIT_LIST_HEAD(&node->children);
 	kobject_init(&node->kobj, &of_node_ktype);
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Rob Herring]

On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> There are a bunch of users open coding the for_each_node_by_name() by
> calling of_find_node_by_name() directly instead of using the macro. This
> is getting in the way of some cleanups, and the possibility of removing
> of_find_node_by_name() entirely. Clean it up so that all the users are
> consistent.
>
> Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
>  arch/powerpc/platforms/powermac/pci.c      |  2 +-
>  arch/powerpc/platforms/powermac/smp.c      |  2 +-
>  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
>  arch/powerpc/platforms/pseries/setup.c     |  3 +--
>  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
>  drivers/edac/cell_edac.c                   |  3 +--
>  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
>  drivers/tty/serial/pmac_zilog.c            |  9 +++------
>  sound/ppc/pmac.c                           |  6 +++---
>  10 files changed, 19 insertions(+), 35 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
> index 63d82bbc05e9..39e1d163c427 100644
> --- a/arch/powerpc/platforms/powermac/feature.c
> +++ b/arch/powerpc/platforms/powermac/feature.c
> @@ -2805,25 +2805,20 @@ set_initial_features(void)
>                 /* Enable GMAC for now for PCI probing. It will be disabled
>                  * later on after PCI probe
>                  */
> -               np = of_find_node_by_name(NULL, "ethernet");
> -               while(np) {
> +               for_each_node_by_name(np, "ethernet")
>                         if (of_device_is_compatible(np, "K2-GMAC"))

Can't for_each_compatible_node be used here instead?

>                                 g5_gmac_enable(np, 0, 1);
> -                       np = of_find_node_by_name(np, "ethernet");
> -               }
>
>                 /* Enable FW before PCI probe. Will be disabled later on
>                  * Note: We should have a batter way to check that we are
>                  * dealing with uninorth internal cell and not a PCI cell
>                  * on the external PCI. The code below works though.
>                  */
> -               np = of_find_node_by_name(NULL, "firewire");
> -               while(np) {
> +               for_each_node_by_name(np, "firewire") {
>                         if (of_device_is_compatible(np, "pci106b,5811")) {

and here...

>                                 macio_chips[0].flags |= MACIO_FLAG_FW_SUPPORTED;
>                                 g5_fw_enable(np, 0, 1);
>                         }
> -                       np = of_find_node_by_name(np, "firewire");
>                 }
>         }
>  #else /* CONFIG_POWER4 */
> @@ -2834,13 +2829,11 @@ set_initial_features(void)
>                 /* Enable GMAC for now for PCI probing. It will be disabled
>                  * later on after PCI probe
>                  */
> -               np = of_find_node_by_name(NULL, "ethernet");
> -               while(np) {
> +               for_each_node_by_name(np, "ethernet") {
>                         if (np->parent
>                             && of_device_is_compatible(np->parent, "uni-north")
>                             && of_device_is_compatible(np, "gmac"))

This one is a bit more complicated, but should still work with
for_each_compatible_node.

There's a few more that could be converted. I guess it depends if you
think dropping looking at node names is okay.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Grant Likely]

On Thu, 5 Jun 2014 12:48:54 -0500, Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > There are a bunch of users open coding the for_each_node_by_name() by
> > calling of_find_node_by_name() directly instead of using the macro. This
> > is getting in the way of some cleanups, and the possibility of removing
> > of_find_node_by_name() entirely. Clean it up so that all the users are
> > consistent.
> >
> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
> >  arch/powerpc/platforms/powermac/pci.c      |  2 +-
> >  arch/powerpc/platforms/powermac/smp.c      |  2 +-
> >  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
> >  arch/powerpc/platforms/pseries/setup.c     |  3 +--
> >  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
> >  drivers/edac/cell_edac.c                   |  3 +--
> >  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
> >  drivers/tty/serial/pmac_zilog.c            |  9 +++------
> >  sound/ppc/pmac.c                           |  6 +++---
> >  10 files changed, 19 insertions(+), 35 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
> > index 63d82bbc05e9..39e1d163c427 100644
> > --- a/arch/powerpc/platforms/powermac/feature.c
> > +++ b/arch/powerpc/platforms/powermac/feature.c
> > @@ -2805,25 +2805,20 @@ set_initial_features(void)
> >                 /* Enable GMAC for now for PCI probing. It will be disabled
> >                  * later on after PCI probe
> >                  */
> > -               np = of_find_node_by_name(NULL, "ethernet");
> > -               while(np) {
> > +               for_each_node_by_name(np, "ethernet")
> >                         if (of_device_is_compatible(np, "K2-GMAC"))
> 
> Can't for_each_compatible_node be used here instead?

Not easily without changing the behaviour. It would need to then check
the name inside the block.

> 
> >                                 g5_gmac_enable(np, 0, 1);
> > -                       np = of_find_node_by_name(np, "ethernet");
> > -               }
> >
> >                 /* Enable FW before PCI probe. Will be disabled later on
> >                  * Note: We should have a batter way to check that we are
> >                  * dealing with uninorth internal cell and not a PCI cell
> >                  * on the external PCI. The code below works though.
> >                  */
> > -               np = of_find_node_by_name(NULL, "firewire");
> > -               while(np) {
> > +               for_each_node_by_name(np, "firewire") {
> >                         if (of_device_is_compatible(np, "pci106b,5811")) {
> 
> and here...

same.

> 
> >                                 macio_chips[0].flags |= MACIO_FLAG_FW_SUPPORTED;
> >                                 g5_fw_enable(np, 0, 1);
> >                         }
> > -                       np = of_find_node_by_name(np, "firewire");
> >                 }
> >         }
> >  #else /* CONFIG_POWER4 */
> > @@ -2834,13 +2829,11 @@ set_initial_features(void)
> >                 /* Enable GMAC for now for PCI probing. It will be disabled
> >                  * later on after PCI probe
> >                  */
> > -               np = of_find_node_by_name(NULL, "ethernet");
> > -               while(np) {
> > +               for_each_node_by_name(np, "ethernet") {
> >                         if (np->parent
> >                             && of_device_is_compatible(np->parent, "uni-north")
> >                             && of_device_is_compatible(np, "gmac"))
> 
> This one is a bit more complicated, but should still work with
> for_each_compatible_node.
> 
> There's a few more that could be converted. I guess it depends if you
> think dropping looking at node names is okay.

I'll take another look through and see if anything can be simplified.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Rob Herring]

On Thu, Jun 26, 2014 at 11:03 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 5 Jun 2014 12:48:54 -0500, Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > There are a bunch of users open coding the for_each_node_by_name() by
>> > calling of_find_node_by_name() directly instead of using the macro. This
>> > is getting in the way of some cleanups, and the possibility of removing
>> > of_find_node_by_name() entirely. Clean it up so that all the users are
>> > consistent.
>> >
>> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > ---
>> >  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
>> >  arch/powerpc/platforms/powermac/pci.c      |  2 +-
>> >  arch/powerpc/platforms/powermac/smp.c      |  2 +-
>> >  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
>> >  arch/powerpc/platforms/pseries/setup.c     |  3 +--
>> >  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
>> >  drivers/edac/cell_edac.c                   |  3 +--
>> >  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
>> >  drivers/tty/serial/pmac_zilog.c            |  9 +++------
>> >  sound/ppc/pmac.c                           |  6 +++---
>> >  10 files changed, 19 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
>> > index 63d82bbc05e9..39e1d163c427 100644
>> > --- a/arch/powerpc/platforms/powermac/feature.c
>> > +++ b/arch/powerpc/platforms/powermac/feature.c
>> > @@ -2805,25 +2805,20 @@ set_initial_features(void)
>> >                 /* Enable GMAC for now for PCI probing. It will be disabled
>> >                  * later on after PCI probe
>> >                  */
>> > -               np = of_find_node_by_name(NULL, "ethernet");
>> > -               while(np) {
>> > +               for_each_node_by_name(np, "ethernet")
>> >                         if (of_device_is_compatible(np, "K2-GMAC"))
>>
>> Can't for_each_compatible_node be used here instead?
>
> Not easily without changing the behaviour. It would need to then check
> the name inside the block.

Why would it change behavior? If the compatible string matches, do you
really have cases where the node name is not "ethernet"? I don't
believe it's the kernel's job to validate DT bindings.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Grant Likely]

On Thu, Jun 26, 2014 at 7:22 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jun 26, 2014 at 11:03 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, 5 Jun 2014 12:48:54 -0500, Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> > There are a bunch of users open coding the for_each_node_by_name() by
>>> > calling of_find_node_by_name() directly instead of using the macro. This
>>> > is getting in the way of some cleanups, and the possibility of removing
>>> > of_find_node_by_name() entirely. Clean it up so that all the users are
>>> > consistent.
>>> >
>>> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> > ---
>>> >  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
>>> >  arch/powerpc/platforms/powermac/pci.c      |  2 +-
>>> >  arch/powerpc/platforms/powermac/smp.c      |  2 +-
>>> >  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
>>> >  arch/powerpc/platforms/pseries/setup.c     |  3 +--
>>> >  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
>>> >  drivers/edac/cell_edac.c                   |  3 +--
>>> >  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
>>> >  drivers/tty/serial/pmac_zilog.c            |  9 +++------
>>> >  sound/ppc/pmac.c                           |  6 +++---
>>> >  10 files changed, 19 insertions(+), 35 deletions(-)
>>> >
>>> > diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
>>> > index 63d82bbc05e9..39e1d163c427 100644
>>> > --- a/arch/powerpc/platforms/powermac/feature.c
>>> > +++ b/arch/powerpc/platforms/powermac/feature.c
>>> > @@ -2805,25 +2805,20 @@ set_initial_features(void)
>>> >                 /* Enable GMAC for now for PCI probing. It will be disabled
>>> >                  * later on after PCI probe
>>> >                  */
>>> > -               np = of_find_node_by_name(NULL, "ethernet");
>>> > -               while(np) {
>>> > +               for_each_node_by_name(np, "ethernet")
>>> >                         if (of_device_is_compatible(np, "K2-GMAC"))
>>>
>>> Can't for_each_compatible_node be used here instead?
>>
>> Not easily without changing the behaviour. It would need to then check
>> the name inside the block.
>
> Why would it change behavior? If the compatible string matches, do you
> really have cases where the node name is not "ethernet"? I don't
> believe it's the kernel's job to validate DT bindings.

Yes, there are actually some bindings that have the same compatible
property but behaviour changes based on node name! I don't want to do
the legwork to figure out if these are in that group. Someone else can
do that job.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Rob Herring]

On Thu, Jun 26, 2014 at 1:56 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jun 26, 2014 at 7:22 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Jun 26, 2014 at 11:03 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On Thu, 5 Jun 2014 12:48:54 -0500, Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> > There are a bunch of users open coding the for_each_node_by_name() by
>>>> > calling of_find_node_by_name() directly instead of using the macro. This
>>>> > is getting in the way of some cleanups, and the possibility of removing
>>>> > of_find_node_by_name() entirely. Clean it up so that all the users are
>>>> > consistent.
>>>> >
>>>> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> > ---
>>>> >  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
>>>> >  arch/powerpc/platforms/powermac/pci.c      |  2 +-
>>>> >  arch/powerpc/platforms/powermac/smp.c      |  2 +-
>>>> >  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
>>>> >  arch/powerpc/platforms/pseries/setup.c     |  3 +--
>>>> >  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
>>>> >  drivers/edac/cell_edac.c                   |  3 +--
>>>> >  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
>>>> >  drivers/tty/serial/pmac_zilog.c            |  9 +++------
>>>> >  sound/ppc/pmac.c                           |  6 +++---
>>>> >  10 files changed, 19 insertions(+), 35 deletions(-)
>>>> >
>>>> > diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
>>>> > index 63d82bbc05e9..39e1d163c427 100644
>>>> > --- a/arch/powerpc/platforms/powermac/feature.c
>>>> > +++ b/arch/powerpc/platforms/powermac/feature.c
>>>> > @@ -2805,25 +2805,20 @@ set_initial_features(void)
>>>> >                 /* Enable GMAC for now for PCI probing. It will be disabled
>>>> >                  * later on after PCI probe
>>>> >                  */
>>>> > -               np = of_find_node_by_name(NULL, "ethernet");
>>>> > -               while(np) {
>>>> > +               for_each_node_by_name(np, "ethernet")
>>>> >                         if (of_device_is_compatible(np, "K2-GMAC"))
>>>>
>>>> Can't for_each_compatible_node be used here instead?
>>>
>>> Not easily without changing the behaviour. It would need to then check
>>> the name inside the block.
>>
>> Why would it change behavior? If the compatible string matches, do you
>> really have cases where the node name is not "ethernet"? I don't
>> believe it's the kernel's job to validate DT bindings.
>
> Yes, there are actually some bindings that have the same compatible
> property but behaviour changes based on node name! I don't want to do
> the legwork to figure out if these are in that group. Someone else can
> do that job.

Well, that's just wrong.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] of: Migrate of_find_node_by_name() users to for_each_node_by_name()

[Grant Likely]

On Thu, Jun 26, 2014 at 8:00 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jun 26, 2014 at 1:56 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Jun 26, 2014 at 7:22 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Thu, Jun 26, 2014 at 11:03 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> On Thu, 5 Jun 2014 12:48:54 -0500, Rob Herring <rob.herring-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> On Thu, Jun 5, 2014 at 10:43 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> > There are a bunch of users open coding the for_each_node_by_name() by
>>>>> > calling of_find_node_by_name() directly instead of using the macro. This
>>>>> > is getting in the way of some cleanups, and the possibility of removing
>>>>> > of_find_node_by_name() entirely. Clean it up so that all the users are
>>>>> > consistent.
>>>>> >
>>>>> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> > ---
>>>>> >  arch/powerpc/platforms/powermac/feature.c  | 20 +++++---------------
>>>>> >  arch/powerpc/platforms/powermac/pci.c      |  2 +-
>>>>> >  arch/powerpc/platforms/powermac/smp.c      |  2 +-
>>>>> >  arch/powerpc/platforms/powermac/udbg_adb.c |  2 +-
>>>>> >  arch/powerpc/platforms/pseries/setup.c     |  3 +--
>>>>> >  drivers/cpufreq/pmac64-cpufreq.c           |  3 +--
>>>>> >  drivers/edac/cell_edac.c                   |  3 +--
>>>>> >  drivers/pci/hotplug/rpaphp_core.c          |  4 ++--
>>>>> >  drivers/tty/serial/pmac_zilog.c            |  9 +++------
>>>>> >  sound/ppc/pmac.c                           |  6 +++---
>>>>> >  10 files changed, 19 insertions(+), 35 deletions(-)
>>>>> >
>>>>> > diff --git a/arch/powerpc/platforms/powermac/feature.c b/arch/powerpc/platforms/powermac/feature.c
>>>>> > index 63d82bbc05e9..39e1d163c427 100644
>>>>> > --- a/arch/powerpc/platforms/powermac/feature.c
>>>>> > +++ b/arch/powerpc/platforms/powermac/feature.c
>>>>> > @@ -2805,25 +2805,20 @@ set_initial_features(void)
>>>>> >                 /* Enable GMAC for now for PCI probing. It will be disabled
>>>>> >                  * later on after PCI probe
>>>>> >                  */
>>>>> > -               np = of_find_node_by_name(NULL, "ethernet");
>>>>> > -               while(np) {
>>>>> > +               for_each_node_by_name(np, "ethernet")
>>>>> >                         if (of_device_is_compatible(np, "K2-GMAC"))
>>>>>
>>>>> Can't for_each_compatible_node be used here instead?
>>>>
>>>> Not easily without changing the behaviour. It would need to then check
>>>> the name inside the block.
>>>
>>> Why would it change behavior? If the compatible string matches, do you
>>> really have cases where the node name is not "ethernet"? I don't
>>> believe it's the kernel's job to validate DT bindings.
>>
>> Yes, there are actually some bindings that have the same compatible
>> property but behaviour changes based on node name! I don't want to do
>> the legwork to figure out if these are in that group. Someone else can
>> do that job.
>
> Well, that's just wrong.

true.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html