[PATCH 0/6] OF device code merges and improvements

[PATCH 0/6] OF device code merges and improvements

[Grant Likely]

These patches merge more common of_device code between microblaze and
powerpc and some other miscellaneous improvement in preparation for
the merge of of_platform_bus_type with platform_bus_type.

These patches are based on top of the IRQ and address code merge which
I posted earlier.  If you want to test them, I've pushed the commits
out to the here:

git://git.secretlab.ca/git/linux-2.6 devicetree-next

Cheers,
g.

---

Grant Likely (6):
      of: Use full node name in resource structures
      of/device: merge of_device_uevent
      of: Modify of_device_get_modalias to be passed struct device
      of/device: Merge of_platform_bus_probe()
      of: Merge of_device_alloc
      of/device: populate platform_device (of_device) resource table on allocation


 arch/microblaze/include/asm/device.h      |    3 
 arch/microblaze/include/asm/of_device.h   |   21 ---
 arch/microblaze/include/asm/of_platform.h |   33 ----
 arch/microblaze/kernel/Makefile           |    2 
 arch/microblaze/kernel/of_device.c        |  112 ---------------
 arch/microblaze/kernel/of_platform.c      |  141 ++-----------------
 arch/powerpc/include/asm/device.h         |    3 
 arch/powerpc/include/asm/of_device.h      |   13 --
 arch/powerpc/include/asm/of_platform.h    |   11 -
 arch/powerpc/kernel/of_device.c           |  110 +-------------
 arch/powerpc/kernel/of_platform.c         |  131 -----------------
 drivers/macintosh/macio_sysfs.c           |    5 -
 drivers/of/address.c                      |    2 
 drivers/of/device.c                       |   62 +++++++-
 drivers/of/irq.c                          |    1 
 drivers/of/platform.c                     |  221 +++++++++++++++++++++++++++++
 include/linux/of_device.h                 |    6 +
 include/linux/of_platform.h               |   21 +++
 18 files changed, 337 insertions(+), 561 deletions(-)
 delete mode 100644 arch/microblaze/kernel/of_device.c

[PATCH 1/6] of: Use full node name in resource structures

[Grant Likely]

Resource names appear in human readable output, so when extracting IRQ
and address resources from a device tree node, use the full node name
to give proper context in places like /proc/iomem.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: Michal Simek <monstr@monstr.eu>
CC: Stephen Rothwell <sfr@canb.auug.org.au>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: microblaze-uclinux@itee.uq.edu.au
CC: linuxppc-dev@ozlabs.org
---
 drivers/of/address.c |    2 +-
 drivers/of/irq.c     |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5c220c3..fcadb72 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -549,7 +549,7 @@ static int __of_address_to_resource(struct device_node *dev, const u32 *addrp,
 		r->end = taddr + size - 1;
 	}
 	r->flags = flags;
-	r->name = dev->name;
+	r->name = dev->full_name;
 	return 0;
 }
 
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 5c097be..8e8cdce 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -330,6 +330,7 @@ unsigned int of_irq_to_resource(struct device_node *dev, int index,
 	if (r && irq != NO_IRQ) {
 		r->start = r->end = irq;
 		r->flags = IORESOURCE_IRQ;
+		r->name = dev->full_name;
 	}
 
 	return irq;

[PATCH 2/6] of/device: merge of_device_uevent

[Grant Likely]

Merge common code between powerpc and microblaze

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: Michal Simek <monstr@monstr.eu>
CC: Wolfram Sang <w.sang@pengutronix.de>
CC: Stephen Rothwell <sfr@canb.auug.org.au>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: microblaze-uclinux@itee.uq.edu.au
CC: linuxppc-dev@ozlabs.org
---
 arch/microblaze/include/asm/of_device.h |    3 --
 arch/microblaze/kernel/of_device.c      |   48 ------------------------------
 arch/powerpc/include/asm/of_device.h    |    3 --
 arch/powerpc/kernel/of_device.c         |   49 -------------------------------
 drivers/of/device.c                     |   48 ++++++++++++++++++++++++++++++
 include/linux/of_device.h               |    4 +++
 6 files changed, 52 insertions(+), 103 deletions(-)

diff --git a/arch/microblaze/include/asm/of_device.h b/arch/microblaze/include/asm/of_device.h
index 0a5f3f9..58e627d 100644
--- a/arch/microblaze/include/asm/of_device.h
+++ b/arch/microblaze/include/asm/of_device.h
@@ -22,9 +22,6 @@ extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
 
-extern int of_device_uevent(struct device *dev,
-			    struct kobj_uevent_env *env);
-
 extern void of_device_make_bus_id(struct of_device *dev);
 
 /* This is just here during the transition */
diff --git a/arch/microblaze/kernel/of_device.c b/arch/microblaze/kernel/of_device.c
index b372787..3a367d7 100644
--- a/arch/microblaze/kernel/of_device.c
+++ b/arch/microblaze/kernel/of_device.c
@@ -62,51 +62,3 @@ struct of_device *of_device_alloc(struct device_node *np,
 	return dev;
 }
 EXPORT_SYMBOL(of_device_alloc);
-
-int of_device_uevent(struct device *dev, struct kobj_uevent_env *env)
-{
-	struct of_device *ofdev;
-	const char *compat;
-	int seen = 0, cplen, sl;
-
-	if (!dev)
-		return -ENODEV;
-
-	ofdev = to_of_device(dev);
-
-	if (add_uevent_var(env, "OF_NAME=%s", ofdev->dev.of_node->name))
-		return -ENOMEM;
-
-	if (add_uevent_var(env, "OF_TYPE=%s", ofdev->dev.of_node->type))
-		return -ENOMEM;
-
-	/* Since the compatible field can contain pretty much anything
-	 * it's not really legal to split it out with commas. We split it
-	 * up using a number of environment variables instead. */
-
-	compat = of_get_property(ofdev->dev.of_node, "compatible", &cplen);
-	while (compat && *compat && cplen > 0) {
-		if (add_uevent_var(env, "OF_COMPATIBLE_%d=%s", seen, compat))
-			return -ENOMEM;
-
-		sl = strlen(compat) + 1;
-		compat += sl;
-		cplen -= sl;
-		seen++;
-	}
-
-	if (add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen))
-		return -ENOMEM;
-
-	/* modalias is trickier, we add it in 2 steps */
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-	sl = of_device_get_modalias(ofdev, &env->buf[env->buflen-1],
-				    sizeof(env->buf) - env->buflen);
-	if (sl >= (sizeof(env->buf) - env->buflen))
-		return -ENOMEM;
-	env->buflen += sl;
-
-	return 0;
-}
-EXPORT_SYMBOL(of_device_uevent);
diff --git a/arch/powerpc/include/asm/of_device.h b/arch/powerpc/include/asm/of_device.h
index cb36632..5d5103c 100644
--- a/arch/powerpc/include/asm/of_device.h
+++ b/arch/powerpc/include/asm/of_device.h
@@ -9,8 +9,5 @@ extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
 
-extern int of_device_uevent(struct device *dev,
-			    struct kobj_uevent_env *env);
-
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_OF_DEVICE_H */
diff --git a/arch/powerpc/kernel/of_device.c b/arch/powerpc/kernel/of_device.c
index df78e02..db91a9d 100644
--- a/arch/powerpc/kernel/of_device.c
+++ b/arch/powerpc/kernel/of_device.c
@@ -82,52 +82,3 @@ struct of_device *of_device_alloc(struct device_node *np,
 	return dev;
 }
 EXPORT_SYMBOL(of_device_alloc);
-
-int of_device_uevent(struct device *dev, struct kobj_uevent_env *env)
-{
-	struct of_device *ofdev;
-	const char *compat;
-	int seen = 0, cplen, sl;
-
-	if (!dev)
-		return -ENODEV;
-
-	ofdev = to_of_device(dev);
-
-	if (add_uevent_var(env, "OF_NAME=%s", ofdev->dev.of_node->name))
-		return -ENOMEM;
-
-	if (add_uevent_var(env, "OF_TYPE=%s", ofdev->dev.of_node->type))
-		return -ENOMEM;
-
-        /* Since the compatible field can contain pretty much anything
-         * it's not really legal to split it out with commas. We split it
-         * up using a number of environment variables instead. */
-
-	compat = of_get_property(ofdev->dev.of_node, "compatible", &cplen);
-	while (compat && *compat && cplen > 0) {
-		if (add_uevent_var(env, "OF_COMPATIBLE_%d=%s", seen, compat))
-			return -ENOMEM;
-
-		sl = strlen (compat) + 1;
-		compat += sl;
-		cplen -= sl;
-		seen++;
-	}
-
-	if (add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen))
-		return -ENOMEM;
-
-	/* modalias is trickier, we add it in 2 steps */
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-	sl = of_device_get_modalias(ofdev, &env->buf[env->buflen-1],
-				    sizeof(env->buf) - env->buflen);
-	if (sl >= (sizeof(env->buf) - env->buflen))
-		return -ENOMEM;
-	env->buflen += sl;
-
-	return 0;
-}
-EXPORT_SYMBOL(of_device_uevent);
-EXPORT_SYMBOL(of_device_get_modalias);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 7d18f8e..275cc9c 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -170,3 +170,51 @@ ssize_t of_device_get_modalias(struct of_device *ofdev,
 
 	return tsize;
 }
+
+/**
+ * of_device_uevent - Display OF related uevent information
+ */
+int of_device_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	const char *compat;
+	int seen = 0, cplen, sl;
+
+	if ((!dev) || (!dev->of_node))
+		return -ENODEV;
+
+	if (add_uevent_var(env, "OF_NAME=%s", dev->of_node->name))
+		return -ENOMEM;
+
+	if (add_uevent_var(env, "OF_TYPE=%s", dev->of_node->type))
+		return -ENOMEM;
+
+	/* Since the compatible field can contain pretty much anything
+	 * it's not really legal to split it out with commas. We split it
+	 * up using a number of environment variables instead. */
+
+	compat = of_get_property(dev->of_node, "compatible", &cplen);
+	while (compat && *compat && cplen > 0) {
+		if (add_uevent_var(env, "OF_COMPATIBLE_%d=%s", seen, compat))
+			return -ENOMEM;
+
+		sl = strlen(compat) + 1;
+		compat += sl;
+		cplen -= sl;
+		seen++;
+	}
+
+	if (add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen))
+		return -ENOMEM;
+
+	/* modalias is trickier, we add it in 2 steps */
+	if (add_uevent_var(env, "MODALIAS="))
+		return -ENOMEM;
+
+	sl = of_device_get_modalias(to_of_device(dev), &env->buf[env->buflen-1],
+				    sizeof(env->buf) - env->buflen);
+	if (sl >= (sizeof(env->buf) - env->buflen))
+		return -ENOMEM;
+	env->buflen += sl;
+
+	return 0;
+}
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index a3ae590..da83e73 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -44,6 +44,10 @@ static inline void of_device_free(struct of_device *dev)
 
 extern ssize_t of_device_get_modalias(struct of_device *ofdev,
 					char *str, ssize_t len);
+
+extern int of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
+
+
 #endif /* CONFIG_OF_DEVICE */
 
 #endif /* _LINUX_OF_DEVICE_H */

[PATCH 3/6] of: Modify of_device_get_modalias to be passed struct device

[Grant Likely]

Now that the of_node pointer is part of struct device,
of_device_get_modalias could be used on any struct device
that has the device node pointer set.  This patch changes
of_device_get_modalias to accept a struct device instead
of a struct of_device.

Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
CC: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
CC: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
CC: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Stephen Rothwell <sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org>
CC: microblaze-uclinux-rVRm/Wmeqae7NGdpmJTKYQ@public.gmane.org
CC: linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
---
 arch/microblaze/include/asm/of_device.h |    3 ---
 drivers/macintosh/macio_sysfs.c         |    5 +----
 drivers/of/device.c                     |   16 ++++++----------
 include/linux/of_device.h               |    2 +-
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/arch/microblaze/include/asm/of_device.h b/arch/microblaze/include/asm/of_device.h
index 58e627d..c9be534 100644
--- a/arch/microblaze/include/asm/of_device.h
+++ b/arch/microblaze/include/asm/of_device.h
@@ -15,9 +15,6 @@
 #include <linux/device.h>
 #include <linux/of.h>
 
-extern ssize_t of_device_get_modalias(struct of_device *ofdev,
-					char *str, ssize_t len);
-
 extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
diff --git a/drivers/macintosh/macio_sysfs.c b/drivers/macintosh/macio_sysfs.c
index 6999ce5..6024038 100644
--- a/drivers/macintosh/macio_sysfs.c
+++ b/drivers/macintosh/macio_sysfs.c
@@ -41,10 +41,7 @@ compatible_show (struct device *dev, struct device_attribute *attr, char *buf)
 static ssize_t modalias_show (struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
-	struct of_device *ofdev = to_of_device(dev);
-	int len;
-
-	len = of_device_get_modalias(ofdev, buf, PAGE_SIZE - 2);
+	int len = of_device_get_modalias(dev, buf, PAGE_SIZE - 2);
 
 	buf[len] = '\n';
 	buf[len+1] = 0;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 275cc9c..c2a98f5 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -68,10 +68,7 @@ static ssize_t name_show(struct device *dev,
 static ssize_t modalias_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct of_device *ofdev = to_of_device(dev);
-	ssize_t len = 0;
-
-	len = of_device_get_modalias(ofdev, buf, PAGE_SIZE - 2);
+	ssize_t len = of_device_get_modalias(dev, buf, PAGE_SIZE - 2);
 	buf[len] = '\n';
 	buf[len+1] = 0;
 	return len+1;
@@ -123,19 +120,18 @@ void of_device_unregister(struct of_device *ofdev)
 }
 EXPORT_SYMBOL(of_device_unregister);
 
-ssize_t of_device_get_modalias(struct of_device *ofdev,
-				char *str, ssize_t len)
+ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
 {
 	const char *compat;
 	int cplen, i;
 	ssize_t tsize, csize, repend;
 
 	/* Name & Type */
-	csize = snprintf(str, len, "of:N%sT%s", ofdev->dev.of_node->name,
-			 ofdev->dev.of_node->type);
+	csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
+			 dev->of_node->type);
 
 	/* Get compatible property if any */
-	compat = of_get_property(ofdev->dev.of_node, "compatible", &cplen);
+	compat = of_get_property(dev->of_node, "compatible", &cplen);
 	if (!compat)
 		return csize;
 
@@ -210,7 +206,7 @@ int of_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
 
-	sl = of_device_get_modalias(to_of_device(dev), &env->buf[env->buflen-1],
+	sl = of_device_get_modalias(dev, &env->buf[env->buflen-1],
 				    sizeof(env->buf) - env->buflen);
 	if (sl >= (sizeof(env->buf) - env->buflen))
 		return -ENOMEM;
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index da83e73..238e92e 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -42,7 +42,7 @@ static inline void of_device_free(struct of_device *dev)
 	of_release_dev(&dev->dev);
 }
 
-extern ssize_t of_device_get_modalias(struct of_device *ofdev,
+extern ssize_t of_device_get_modalias(struct device *dev,
 					char *str, ssize_t len);
 
 extern int of_device_uevent(struct device *dev, struct kobj_uevent_env *env);

[PATCH 4/6] of/device: Merge of_platform_bus_probe()

[Grant Likely]

Merge common code between PowerPC and microblaze.  This patch merges
the code that scans the tree and registers devices.  The functions
merged are of_platform_bus_probe(), of_platform_bus_create(), and
of_platform_device_create().

This patch also move the of_default_bus_ids[] table out of a Microblaze
header file and makes it non-static.  The device ids table isn't merged
because powerpc and microblaze use different default data.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: Michal Simek <monstr@monstr.eu>
CC: Grant Likely <grant.likely@secretlab.ca>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Stephen Rothwell <sfr@canb.auug.org.au>
CC: microblaze-uclinux@itee.uq.edu.au
CC: linuxppc-dev@ozlabs.org
---
 arch/microblaze/include/asm/of_platform.h |   33 -------
 arch/microblaze/kernel/of_platform.c      |  141 ++++-------------------------
 arch/powerpc/include/asm/of_platform.h    |   11 --
 arch/powerpc/kernel/of_platform.c         |  131 ---------------------------
 drivers/of/platform.c                     |  143 +++++++++++++++++++++++++++++
 include/linux/of_platform.h               |   17 +++
 6 files changed, 179 insertions(+), 297 deletions(-)

diff --git a/arch/microblaze/include/asm/of_platform.h b/arch/microblaze/include/asm/of_platform.h
index 3749127..625003f 100644
--- a/arch/microblaze/include/asm/of_platform.h
+++ b/arch/microblaze/include/asm/of_platform.h
@@ -14,39 +14,6 @@
 /* This is just here during the transition */
 #include <linux/of_platform.h>
 
-/*
- * The list of OF IDs below is used for matching bus types in the
- * system whose devices are to be exposed as of_platform_devices.
- *
- * This is the default list valid for most platforms. This file provides
- * functions who can take an explicit list if necessary though
- *
- * The search is always performed recursively looking for children of
- * the provided device_node and recursively if such a children matches
- * a bus type in the list
- */
-
-static const struct of_device_id of_default_bus_ids[] = {
-	{ .type = "soc", },
-	{ .compatible = "soc", },
-	{ .type = "plb5", },
-	{ .type = "plb4", },
-	{ .type = "opb", },
-	{ .type = "simple", },
-	{},
-};
-
-/* Platform devices and busses creation */
-extern struct of_device *of_platform_device_create(struct device_node *np,
-						const char *bus_id,
-						struct device *parent);
-/* pseudo "matches" value to not do deep probe */
-#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
-
-extern int of_platform_bus_probe(struct device_node *root,
-				const struct of_device_id *matches,
-				struct device *parent);
-
 extern struct of_device *of_find_device_by_phandle(phandle ph);
 
 extern void of_instantiate_rtc(void);
diff --git a/arch/microblaze/kernel/of_platform.c b/arch/microblaze/kernel/of_platform.c
index ccf6f42..a07abdd 100644
--- a/arch/microblaze/kernel/of_platform.c
+++ b/arch/microblaze/kernel/of_platform.c
@@ -37,132 +37,27 @@ static int __init of_bus_driver_init(void)
 }
 postcore_initcall(of_bus_driver_init);
 
-struct of_device *of_platform_device_create(struct device_node *np,
-					    const char *bus_id,
-					    struct device *parent)
-{
-	struct of_device *dev;
-
-	dev = of_device_alloc(np, bus_id, parent);
-	if (!dev)
-		return NULL;
-
-	dev->archdata.dma_mask = 0xffffffffUL;
-	dev->dev.bus = &of_platform_bus_type;
-
-	/* We do not fill the DMA ops for platform devices by default.
-	 * This is currently the responsibility of the platform code
-	 * to do such, possibly using a device notifier
-	 */
-
-	if (of_device_register(dev) != 0) {
-		of_device_free(dev);
-		return NULL;
-	}
-
-	return dev;
-}
-EXPORT_SYMBOL(of_platform_device_create);
-
-/**
- * of_platform_bus_create - Create an OF device for a bus node and all its
- * children. Optionally recursively instanciate matching busses.
- * @bus: device node of the bus to instanciate
- * @matches: match table, NULL to use the default, OF_NO_DEEP_PROBE to
- * disallow recursive creation of child busses
- */
-static int of_platform_bus_create(const struct device_node *bus,
-				  const struct of_device_id *matches,
-				  struct device *parent)
-{
-	struct device_node *child;
-	struct of_device *dev;
-	int rc = 0;
-
-	for_each_child_of_node(bus, child) {
-		pr_debug("   create child: %s\n", child->full_name);
-		dev = of_platform_device_create(child, NULL, parent);
-		if (dev == NULL)
-			rc = -ENOMEM;
-		else if (!of_match_node(matches, child))
-			continue;
-		if (rc == 0) {
-			pr_debug("   and sub busses\n");
-			rc = of_platform_bus_create(child, matches, &dev->dev);
-		}
-		if (rc) {
-			of_node_put(child);
-			break;
-		}
-	}
-	return rc;
-}
-
-
-/**
- * of_platform_bus_probe - Probe the device-tree for platform busses
- * @root: parent of the first level to probe or NULL for the root of the tree
- * @matches: match table, NULL to use the default
- * @parent: parent to hook devices from, NULL for toplevel
+/*
+ * The list of OF IDs below is used for matching bus types in the
+ * system whose devices are to be exposed as of_platform_devices.
  *
- * Note that children of the provided root are not instanciated as devices
- * unless the specified root itself matches the bus list and is not NULL.
+ * This is the default list valid for most platforms. This file provides
+ * functions who can take an explicit list if necessary though
+ *
+ * The search is always performed recursively looking for children of
+ * the provided device_node and recursively if such a children matches
+ * a bus type in the list
  */
 
-int of_platform_bus_probe(struct device_node *root,
-			  const struct of_device_id *matches,
-			  struct device *parent)
-{
-	struct device_node *child;
-	struct of_device *dev;
-	int rc = 0;
-
-	if (matches == NULL)
-		matches = of_default_bus_ids;
-	if (matches == OF_NO_DEEP_PROBE)
-		return -EINVAL;
-	if (root == NULL)
-		root = of_find_node_by_path("/");
-	else
-		of_node_get(root);
-
-	pr_debug("of_platform_bus_probe()\n");
-	pr_debug(" starting at: %s\n", root->full_name);
-
-	/* Do a self check of bus type, if there's a match, create
-	 * children
-	 */
-	if (of_match_node(matches, root)) {
-		pr_debug(" root match, create all sub devices\n");
-		dev = of_platform_device_create(root, NULL, parent);
-		if (dev == NULL) {
-			rc = -ENOMEM;
-			goto bail;
-		}
-		pr_debug(" create all sub busses\n");
-		rc = of_platform_bus_create(root, matches, &dev->dev);
-		goto bail;
-	}
-	for_each_child_of_node(root, child) {
-		if (!of_match_node(matches, child))
-			continue;
-
-		pr_debug("  match: %s\n", child->full_name);
-		dev = of_platform_device_create(child, NULL, parent);
-		if (dev == NULL)
-			rc = -ENOMEM;
-		else
-			rc = of_platform_bus_create(child, matches, &dev->dev);
-		if (rc) {
-			of_node_put(child);
-			break;
-		}
-	}
- bail:
-	of_node_put(root);
-	return rc;
-}
-EXPORT_SYMBOL(of_platform_bus_probe);
+const struct of_device_id of_default_bus_ids[] = {
+	{ .type = "soc", },
+	{ .compatible = "soc", },
+	{ .type = "plb5", },
+	{ .type = "plb4", },
+	{ .type = "opb", },
+	{ .type = "simple", },
+	{},
+};
 
 static int of_dev_node_match(struct device *dev, void *data)
 {
diff --git a/arch/powerpc/include/asm/of_platform.h b/arch/powerpc/include/asm/of_platform.h
index d4aaa34..b37d2dc 100644
--- a/arch/powerpc/include/asm/of_platform.h
+++ b/arch/powerpc/include/asm/of_platform.h
@@ -11,17 +11,6 @@
  *
  */
 
-/* Platform devices and busses creation */
-extern struct of_device *of_platform_device_create(struct device_node *np,
-						   const char *bus_id,
-						   struct device *parent);
-/* pseudo "matches" value to not do deep probe */
-#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
-
-extern int of_platform_bus_probe(struct device_node *root,
-				 const struct of_device_id *matches,
-				 struct device *parent);
-
 extern struct of_device *of_find_device_by_phandle(phandle ph);
 
 extern void of_instantiate_rtc(void);
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index 487a988..0b5cc6d 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -40,7 +40,7 @@
  * a bus type in the list
  */
 
-static const struct of_device_id of_default_bus_ids[] = {
+const struct of_device_id of_default_bus_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
 	{ .type = "spider", },
@@ -64,135 +64,6 @@ static int __init of_bus_driver_init(void)
 
 postcore_initcall(of_bus_driver_init);
 
-struct of_device* of_platform_device_create(struct device_node *np,
-					    const char *bus_id,
-					    struct device *parent)
-{
-	struct of_device *dev;
-
-	dev = of_device_alloc(np, bus_id, parent);
-	if (!dev)
-		return NULL;
-
-	dev->archdata.dma_mask = 0xffffffffUL;
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-
-	dev->dev.bus = &of_platform_bus_type;
-
-	/* We do not fill the DMA ops for platform devices by default.
-	 * This is currently the responsibility of the platform code
-	 * to do such, possibly using a device notifier
-	 */
-
-	if (of_device_register(dev) != 0) {
-		of_device_free(dev);
-		return NULL;
-	}
-
-	return dev;
-}
-EXPORT_SYMBOL(of_platform_device_create);
-
-
-
-/**
- * of_platform_bus_create - Create an OF device for a bus node and all its
- * children. Optionally recursively instanciate matching busses.
- * @bus: device node of the bus to instanciate
- * @matches: match table, NULL to use the default, OF_NO_DEEP_PROBE to
- * disallow recursive creation of child busses
- */
-static int of_platform_bus_create(const struct device_node *bus,
-				  const struct of_device_id *matches,
-				  struct device *parent)
-{
-	struct device_node *child;
-	struct of_device *dev;
-	int rc = 0;
-
-	for_each_child_of_node(bus, child) {
-		pr_debug("   create child: %s\n", child->full_name);
-		dev = of_platform_device_create(child, NULL, parent);
-		if (dev == NULL)
-			rc = -ENOMEM;
-		else if (!of_match_node(matches, child))
-			continue;
-		if (rc == 0) {
-			pr_debug("   and sub busses\n");
-			rc = of_platform_bus_create(child, matches, &dev->dev);
-		} if (rc) {
-			of_node_put(child);
-			break;
-		}
-	}
-	return rc;
-}
-
-/**
- * of_platform_bus_probe - Probe the device-tree for platform busses
- * @root: parent of the first level to probe or NULL for the root of the tree
- * @matches: match table, NULL to use the default
- * @parent: parent to hook devices from, NULL for toplevel
- *
- * Note that children of the provided root are not instanciated as devices
- * unless the specified root itself matches the bus list and is not NULL.
- */
-
-int of_platform_bus_probe(struct device_node *root,
-			  const struct of_device_id *matches,
-			  struct device *parent)
-{
-	struct device_node *child;
-	struct of_device *dev;
-	int rc = 0;
-
-	if (matches == NULL)
-		matches = of_default_bus_ids;
-	if (matches == OF_NO_DEEP_PROBE)
-		return -EINVAL;
-	if (root == NULL)
-		root = of_find_node_by_path("/");
-	else
-		of_node_get(root);
-
-	pr_debug("of_platform_bus_probe()\n");
-	pr_debug(" starting at: %s\n", root->full_name);
-
-	/* Do a self check of bus type, if there's a match, create
-	 * children
-	 */
-	if (of_match_node(matches, root)) {
-		pr_debug(" root match, create all sub devices\n");
-		dev = of_platform_device_create(root, NULL, parent);
-		if (dev == NULL) {
-			rc = -ENOMEM;
-			goto bail;
-		}
-		pr_debug(" create all sub busses\n");
-		rc = of_platform_bus_create(root, matches, &dev->dev);
-		goto bail;
-	}
-	for_each_child_of_node(root, child) {
-		if (!of_match_node(matches, child))
-			continue;
-
-		pr_debug("  match: %s\n", child->full_name);
-		dev = of_platform_device_create(child, NULL, parent);
-		if (dev == NULL)
-			rc = -ENOMEM;
-		else
-			rc = of_platform_bus_create(child, matches, &dev->dev);
-		if (rc) {
-			of_node_put(child);
-			break;
-		}
-	}
- bail:
-	of_node_put(root);
-	return rc;
-}
-EXPORT_SYMBOL(of_platform_bus_probe);
-
 static int of_dev_node_match(struct device *dev, void *data)
 {
 	return to_of_device(dev)->dev.of_node == data;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 7dacc1e..d9c81e9 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -14,6 +14,7 @@
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
 
@@ -396,3 +397,145 @@ void of_unregister_driver(struct of_platform_driver *drv)
 	driver_unregister(&drv->driver);
 }
 EXPORT_SYMBOL(of_unregister_driver);
+
+#if !defined(CONFIG_SPARC)
+/*
+ * The following routines scan a subtree and registers a device for
+ * each applicable node.
+ *
+ * Note: sparc doesn't use these routines because it has a different
+ * mechanism for creating devices from device tree nodes.
+ */
+
+/**
+ * of_platform_device_create - Alloc, initialize and register an of_device
+ * @np: pointer to node to create device for
+ * @bus_id: name to assign device
+ * @parent: Linux device model parent device.
+ */
+struct of_device *of_platform_device_create(struct device_node *np,
+					    const char *bus_id,
+					    struct device *parent)
+{
+	struct of_device *dev;
+
+	dev = of_device_alloc(np, bus_id, parent);
+	if (!dev)
+		return NULL;
+
+	dev->archdata.dma_mask = 0xffffffffUL;
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dev->dev.bus = &of_platform_bus_type;
+
+	/* We do not fill the DMA ops for platform devices by default.
+	 * This is currently the responsibility of the platform code
+	 * to do such, possibly using a device notifier
+	 */
+
+	if (of_device_register(dev) != 0) {
+		of_device_free(dev);
+		return NULL;
+	}
+
+	return dev;
+}
+EXPORT_SYMBOL(of_platform_device_create);
+
+/**
+ * of_platform_bus_create - Create an OF device for a bus node and all its
+ * children. Optionally recursively instantiate matching busses.
+ * @bus: device node of the bus to instantiate
+ * @matches: match table, NULL to use the default, OF_NO_DEEP_PROBE to
+ * disallow recursive creation of child busses
+ */
+static int of_platform_bus_create(const struct device_node *bus,
+				  const struct of_device_id *matches,
+				  struct device *parent)
+{
+	struct device_node *child;
+	struct of_device *dev;
+	int rc = 0;
+
+	for_each_child_of_node(bus, child) {
+		pr_debug("   create child: %s\n", child->full_name);
+		dev = of_platform_device_create(child, NULL, parent);
+		if (dev == NULL)
+			rc = -ENOMEM;
+		else if (!of_match_node(matches, child))
+			continue;
+		if (rc == 0) {
+			pr_debug("   and sub busses\n");
+			rc = of_platform_bus_create(child, matches, &dev->dev);
+		}
+		if (rc) {
+			of_node_put(child);
+			break;
+		}
+	}
+	return rc;
+}
+
+/**
+ * of_platform_bus_probe - Probe the device-tree for platform busses
+ * @root: parent of the first level to probe or NULL for the root of the tree
+ * @matches: match table, NULL to use the default
+ * @parent: parent to hook devices from, NULL for toplevel
+ *
+ * Note that children of the provided root are not instantiated as devices
+ * unless the specified root itself matches the bus list and is not NULL.
+ */
+int of_platform_bus_probe(struct device_node *root,
+			  const struct of_device_id *matches,
+			  struct device *parent)
+{
+	struct device_node *child;
+	struct of_device *dev;
+	int rc = 0;
+
+	if (matches == NULL)
+		matches = of_default_bus_ids;
+	if (matches == OF_NO_DEEP_PROBE)
+		return -EINVAL;
+	if (root == NULL)
+		root = of_find_node_by_path("/");
+	else
+		of_node_get(root);
+
+	pr_debug("of_platform_bus_probe()\n");
+	pr_debug(" starting at: %s\n", root->full_name);
+
+	/* Do a self check of bus type, if there's a match, create
+	 * children
+	 */
+	if (of_match_node(matches, root)) {
+		pr_debug(" root match, create all sub devices\n");
+		dev = of_platform_device_create(root, NULL, parent);
+		if (dev == NULL) {
+			rc = -ENOMEM;
+			goto bail;
+		}
+		pr_debug(" create all sub busses\n");
+		rc = of_platform_bus_create(root, matches, &dev->dev);
+		goto bail;
+	}
+	for_each_child_of_node(root, child) {
+		if (!of_match_node(matches, child))
+			continue;
+
+		pr_debug("  match: %s\n", child->full_name);
+		dev = of_platform_device_create(child, NULL, parent);
+		if (dev == NULL)
+			rc = -ENOMEM;
+		else
+			rc = of_platform_bus_create(child, matches, &dev->dev);
+		if (rc) {
+			of_node_put(child);
+			break;
+		}
+	}
+ bail:
+	of_node_put(root);
+	return rc;
+}
+EXPORT_SYMBOL(of_platform_bus_probe);
+#endif /* !CONFIG_SPARC */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 1643d37..4bbba41 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -25,6 +25,8 @@
  */
 extern struct bus_type of_platform_bus_type;
 
+extern const struct of_device_id of_default_bus_ids[];
+
 /*
  * An of_platform_driver driver is attached to a basic of_device on
  * the "platform bus" (of_platform_bus_type).
@@ -63,6 +65,21 @@ static inline void of_unregister_platform_driver(struct of_platform_driver *drv)
 extern struct of_device *of_find_device_by_node(struct device_node *np);
 
 extern int of_bus_type_init(struct bus_type *bus, const char *name);
+
+#if !defined(CONFIG_SPARC) /* SPARC has its own device registration method */
+/* Platform devices and busses creation */
+extern struct of_device *of_platform_device_create(struct device_node *np,
+						   const char *bus_id,
+						   struct device *parent);
+
+/* pseudo "matches" value to not do deep probe */
+#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
+
+extern int of_platform_bus_probe(struct device_node *root,
+				 const struct of_device_id *matches,
+				 struct device *parent);
+#endif /* !CONFIG_SPARC */
+
 #endif /* CONFIG_OF_DEVICE */
 
 #endif	/* _LINUX_OF_PLATFORM_H */

[PATCH 5/6] of: Merge of_device_alloc

[Grant Likely]

Merge common code between PowerPC and Microblaze

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: Michal Simek <monstr@monstr.eu>
CC: Grant Likely <grant.likely@secretlab.ca>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Stephen Rothwell <sfr@canb.auug.org.au>
CC: microblaze-uclinux@itee.uq.edu.au
CC: linuxppc-dev@ozlabs.org
CC: devicetree-discuss@lists.ozlabs.org
---
 arch/microblaze/include/asm/device.h    |    3 +
 arch/microblaze/include/asm/of_device.h |   15 -------
 arch/microblaze/kernel/Makefile         |    2 -
 arch/microblaze/kernel/of_device.c      |   64 -------------------------------
 arch/powerpc/include/asm/device.h       |    3 +
 arch/powerpc/include/asm/of_device.h    |   10 -----
 arch/powerpc/kernel/of_device.c         |   61 ++++--------------------------
 drivers/of/platform.c                   |   53 ++++++++++++++++++++++++++
 include/linux/of_platform.h             |    4 ++
 9 files changed, 72 insertions(+), 143 deletions(-)
 delete mode 100644 arch/microblaze/kernel/of_device.c

diff --git a/arch/microblaze/include/asm/device.h b/arch/microblaze/include/asm/device.h
index 123b2fe..5e63552 100644
--- a/arch/microblaze/include/asm/device.h
+++ b/arch/microblaze/include/asm/device.h
@@ -21,6 +21,9 @@ struct pdev_archdata {
 	u64 dma_mask;
 };
 
+/* Don't override the default bus id behaviour */
+#define of_device_make_bus_id __of_device_make_bus_id
+
 #endif /* _ASM_MICROBLAZE_DEVICE_H */
 
 
diff --git a/arch/microblaze/include/asm/of_device.h b/arch/microblaze/include/asm/of_device.h
index c9be534..47e8d42 100644
--- a/arch/microblaze/include/asm/of_device.h
+++ b/arch/microblaze/include/asm/of_device.h
@@ -10,19 +10,4 @@
 
 #ifndef _ASM_MICROBLAZE_OF_DEVICE_H
 #define _ASM_MICROBLAZE_OF_DEVICE_H
-#ifdef __KERNEL__
-
-#include <linux/device.h>
-#include <linux/of.h>
-
-extern struct of_device *of_device_alloc(struct device_node *np,
-					 const char *bus_id,
-					 struct device *parent);
-
-extern void of_device_make_bus_id(struct of_device *dev);
-
-/* This is just here during the transition */
-#include <linux/of_device.h>
-
-#endif /* __KERNEL__ */
 #endif /* _ASM_MICROBLAZE_OF_DEVICE_H */
diff --git a/arch/microblaze/kernel/Makefile b/arch/microblaze/kernel/Makefile
index e51bc15..727e2cb 100644
--- a/arch/microblaze/kernel/Makefile
+++ b/arch/microblaze/kernel/Makefile
@@ -15,7 +15,7 @@ endif
 extra-y := head.o vmlinux.lds
 
 obj-y += dma.o exceptions.o \
-	hw_exception_handler.o init_task.o intc.o irq.o of_device.o \
+	hw_exception_handler.o init_task.o intc.o irq.o \
 	of_platform.o process.o prom.o prom_parse.o ptrace.o \
 	setup.o signal.o sys_microblaze.o timer.o traps.o reset.o
 
diff --git a/arch/microblaze/kernel/of_device.c b/arch/microblaze/kernel/of_device.c
deleted file mode 100644
index 3a367d7..0000000
--- a/arch/microblaze/kernel/of_device.c
+++ /dev/null
@@ -1,64 +0,0 @@
-#include <linux/string.h>
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/slab.h>
-#include <linux/of_device.h>
-
-#include <linux/errno.h>
-
-void of_device_make_bus_id(struct of_device *dev)
-{
-	static atomic_t bus_no_reg_magic;
-	struct device_node *node = dev->dev.of_node;
-	const u32 *reg;
-	u64 addr;
-	int magic;
-
-	/*
-	 * For MMIO, get the physical address
-	 */
-	reg = of_get_property(node, "reg", NULL);
-	if (reg) {
-		addr = of_translate_address(node, reg);
-		if (addr != OF_BAD_ADDR) {
-			dev_set_name(&dev->dev, "%llx.%s",
-				     (unsigned long long)addr, node->name);
-			return;
-		}
-	}
-
-	/*
-	 * No BusID, use the node name and add a globally incremented
-	 * counter (and pray...)
-	 */
-	magic = atomic_add_return(1, &bus_no_reg_magic);
-	dev_set_name(&dev->dev, "%s.%d", node->name, magic - 1);
-}
-EXPORT_SYMBOL(of_device_make_bus_id);
-
-struct of_device *of_device_alloc(struct device_node *np,
-				  const char *bus_id,
-				  struct device *parent)
-{
-	struct of_device *dev;
-
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return NULL;
-
-	dev->dev.of_node = of_node_get(np);
-	dev->dev.dma_mask = &dev->archdata.dma_mask;
-	dev->dev.parent = parent;
-	dev->dev.release = of_release_dev;
-
-	if (bus_id)
-		dev_set_name(&dev->dev, bus_id);
-	else
-		of_device_make_bus_id(dev);
-
-	return dev;
-}
-EXPORT_SYMBOL(of_device_alloc);
diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
index a3954e4..3c897a8 100644
--- a/arch/powerpc/include/asm/device.h
+++ b/arch/powerpc/include/asm/device.h
@@ -31,4 +31,7 @@ struct pdev_archdata {
 	u64 dma_mask;
 };
 
+/* Override the default bus_id behaviour for OF stuff */
+extern void of_device_make_bus_id(struct device *dev);
+
 #endif /* _ASM_POWERPC_DEVICE_H */
diff --git a/arch/powerpc/include/asm/of_device.h b/arch/powerpc/include/asm/of_device.h
index 5d5103c..04f7671 100644
--- a/arch/powerpc/include/asm/of_device.h
+++ b/arch/powerpc/include/asm/of_device.h
@@ -1,13 +1,3 @@
 #ifndef _ASM_POWERPC_OF_DEVICE_H
 #define _ASM_POWERPC_OF_DEVICE_H
-#ifdef __KERNEL__
-
-#include <linux/device.h>
-#include <linux/of.h>
-
-extern struct of_device *of_device_alloc(struct device_node *np,
-					 const char *bus_id,
-					 struct device *parent);
-
-#endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_OF_DEVICE_H */
diff --git a/arch/powerpc/kernel/of_device.c b/arch/powerpc/kernel/of_device.c
index db91a9d..0b60783 100644
--- a/arch/powerpc/kernel/of_device.c
+++ b/arch/powerpc/kernel/of_device.c
@@ -5,32 +5,29 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
-#include <linux/of_device.h>
+#include <linux/of_platform.h>
 
 #include <asm/errno.h>
 #include <asm/dcr.h>
 
-static void of_device_make_bus_id(struct of_device *dev)
+void of_device_make_bus_id(struct device *dev)
 {
-	static atomic_t bus_no_reg_magic;
-	struct device_node *node = dev->dev.of_node;
+#ifdef CONFIG_PPC_DCR
+	struct device_node *node = dev->of_node;
 	const u32 *reg;
-	u64 addr;
-	int magic;
 
 	/*
 	 * If it's a DCR based device, use 'd' for native DCRs
 	 * and 'D' for MMIO DCRs.
 	 */
-#ifdef CONFIG_PPC_DCR
 	reg = of_get_property(node, "dcr-reg", NULL);
 	if (reg) {
 #ifdef CONFIG_PPC_DCR_NATIVE
-		dev_set_name(&dev->dev, "d%x.%s", *reg, node->name);
+		dev_set_name(dev, "d%x.%s", *reg, node->name);
 #else /* CONFIG_PPC_DCR_NATIVE */
-		addr = of_translate_dcr_address(node, *reg, NULL);
+		u64 addr = of_translate_dcr_address(node, *reg, NULL);
 		if (addr != OF_BAD_ADDR) {
-			dev_set_name(&dev->dev, "D%llx.%s",
+			dev_set_name(dev, "D%llx.%s",
 				     (unsigned long long)addr, node->name);
 			return;
 		}
@@ -38,47 +35,5 @@ static void of_device_make_bus_id(struct of_device *dev)
 	}
 #endif /* CONFIG_PPC_DCR */
 
-	/*
-	 * For MMIO, get the physical address
-	 */
-	reg = of_get_property(node, "reg", NULL);
-	if (reg) {
-		addr = of_translate_address(node, reg);
-		if (addr != OF_BAD_ADDR) {
-			dev_set_name(&dev->dev, "%llx.%s",
-				     (unsigned long long)addr, node->name);
-			return;
-		}
-	}
-
-	/*
-	 * No BusID, use the node name and add a globally incremented
-	 * counter (and pray...)
-	 */
-	magic = atomic_add_return(1, &bus_no_reg_magic);
-	dev_set_name(&dev->dev, "%s.%d", node->name, magic - 1);
-}
-
-struct of_device *of_device_alloc(struct device_node *np,
-				  const char *bus_id,
-				  struct device *parent)
-{
-	struct of_device *dev;
-
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return NULL;
-
-	dev->dev.of_node = of_node_get(np);
-	dev->dev.dma_mask = &dev->archdata.dma_mask;
-	dev->dev.parent = parent;
-	dev->dev.release = of_release_dev;
-
-	if (bus_id)
-		dev_set_name(&dev->dev, "%s", bus_id);
-	else
-		of_device_make_bus_id(dev);
-
-	return dev;
+	__of_device_make_bus_id(dev);
 }
-EXPORT_SYMBOL(of_device_alloc);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index d9c81e9..4bb898d 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -407,6 +407,59 @@ EXPORT_SYMBOL(of_unregister_driver);
  * mechanism for creating devices from device tree nodes.
  */
 
+void __of_device_make_bus_id(struct device *dev)
+{
+	static atomic_t bus_no_reg_magic;
+	struct device_node *node = dev->of_node;
+	const u32 *reg;
+	u64 addr;
+	int magic;
+
+	/*
+	 * For MMIO, get the physical address
+	 */
+	reg = of_get_property(node, "reg", NULL);
+	if (reg) {
+		addr = of_translate_address(node, reg);
+		if (addr != OF_BAD_ADDR) {
+			dev_set_name(dev, "%llx.%s",
+				     (unsigned long long)addr, node->name);
+			return;
+		}
+	}
+
+	/*
+	 * No BusID, use the node name and add a globally incremented
+	 * counter (and pray...)
+	 */
+	magic = atomic_add_return(1, &bus_no_reg_magic);
+	dev_set_name(dev, "%s.%d", node->name, magic - 1);
+}
+
+struct of_device *of_device_alloc(struct device_node *np,
+				  const char *bus_id,
+				  struct device *parent)
+{
+	struct of_device *dev;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return NULL;
+
+	dev->dev.of_node = of_node_get(np);
+	dev->dev.dma_mask = &dev->archdata.dma_mask;
+	dev->dev.parent = parent;
+	dev->dev.release = of_release_dev;
+
+	if (bus_id)
+		dev_set_name(&dev->dev, "%s", bus_id);
+	else
+		of_device_make_bus_id(&dev->dev);
+
+	return dev;
+}
+EXPORT_SYMBOL(of_device_alloc);
+
 /**
  * of_platform_device_create - Alloc, initialize and register an of_device
  * @np: pointer to node to create device for
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 4bbba41..8c82a2f 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -60,6 +60,10 @@ static inline void of_unregister_platform_driver(struct of_platform_driver *drv)
 	of_unregister_driver(drv);
 }
 
+extern void __of_device_make_bus_id(struct device *dev);
+extern struct of_device *of_device_alloc(struct device_node *np,
+					 const char *bus_id,
+					 struct device *parent);
 #include <asm/of_platform.h>
 
 extern struct of_device *of_find_device_by_node(struct device_node *np);

[PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

When allocating a platform_device to represent an OF node, also allocate
space for the resource table and populate it with IRQ and reg property
information.  This change is in preparation for merging the
of_platform_bus_type with the platform_bus_type so that existing
platform_driver code can retrieve base addresses and IRQs data.

Background: a previous commit removed struct of_device and made it a
#define alias for platform_device.

Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
CC: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
CC: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
CC: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
CC: Stephen Rothwell <sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org>
CC: microblaze-uclinux-rVRm/Wmeqae7NGdpmJTKYQ@public.gmane.org
CC: linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
CC: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
---
 drivers/of/platform.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 4bb898d..10ad0b6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -441,16 +441,41 @@ struct of_device *of_device_alloc(struct device_node *np,
 				  struct device *parent)
 {
 	struct of_device *dev;
-
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	int rc, i, num_reg = 0, num_irq = 0;
+	struct resource *res, temp_res;
+
+	/* First count how many resources are needed */
+	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
+		num_reg++;
+	while (of_irq_to_resource(np, num_irq, &temp_res) != NO_IRQ)
+		num_irq++;
+
+	/* Allocate the device including space for the resource table, and
+	 * initialize it. */
+	i = num_reg + num_irq;
+	dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
 	if (!dev)
 		return NULL;
-
 	dev->dev.of_node = of_node_get(np);
 	dev->dev.dma_mask = &dev->archdata.dma_mask;
 	dev->dev.parent = parent;
 	dev->dev.release = of_release_dev;
 
+	/* Populate the resource table */
+	if (num_irq || num_reg) {
+		dev->resource = (void*)&dev[1];
+		dev->num_resources = num_reg + num_irq;
+		res = dev->resource;
+		for (i = 0; i < num_reg; i++, res++) {
+			rc = of_address_to_resource(np, i, res);
+			WARN_ON(rc);
+		}
+		for (i = 0; i < num_irq; i++, res++) {
+			rc = of_irq_to_resource(np, i, res);
+			WARN_ON(rc == NO_IRQ);
+		}
+	}
+
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
[...]
> +	dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
>  	if (!dev)
>  		return NULL;
> -
>  	dev->dev.of_node = of_node_get(np);
>  	dev->dev.dma_mask = &dev->archdata.dma_mask;
>  	dev->dev.parent = parent;
>  	dev->dev.release = of_release_dev;
>  
> +	/* Populate the resource table */
> +	if (num_irq || num_reg) {
> +		dev->resource = (void*)&dev[1];

This is ugly. Why not allocate the memory specifically for
dev->resource? Is this because you plan to get rid of
of_release_dev(), and the generic release_dev() won't
know if it should free the dev->resource? There must
be a better way to handle this.

p.s.

[Two Minutes Hate for Grant.]

Just wonder what happened to of_gpio stuff? You blocked it
in 2.6.34 for no reason saying "I'll pick it into my OF
tree before the 2.6.35 merge window" and it's 2.6.36 merge
window quite soon.

It's still in origin/test-devicetree, which is obviously
not -next. What is your plan now?

Thanks,

> +		dev->num_resources = num_reg + num_irq;
> +		res = dev->resource;
> +		for (i = 0; i < num_reg; i++, res++) {
> +			rc = of_address_to_resource(np, i, res);
> +			WARN_ON(rc);
> +		}
> +		for (i = 0; i < num_irq; i++, res++) {
> +			rc = of_irq_to_resource(np, i, res);
> +			WARN_ON(rc == NO_IRQ);
> +		}
> +	}
> +
>  	if (bus_id)
>  		dev_set_name(&dev->dev, "%s", bus_id);
>  	else

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Tue, Jun 8, 2010 at 9:57 AM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
> [...]
>> +     dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
>>       if (!dev)
>>               return NULL;
>> -
>>       dev->dev.of_node = of_node_get(np);
>>       dev->dev.dma_mask = &dev->archdata.dma_mask;
>>       dev->dev.parent = parent;
>>       dev->dev.release = of_release_dev;
>>
>> +     /* Populate the resource table */
>> +     if (num_irq || num_reg) {
>> +             dev->resource = (void*)&dev[1];
>
> This is ugly. Why not allocate the memory specifically for
> dev->resource? Is this because you plan to get rid of
> of_release_dev(), and the generic release_dev() won't
> know if it should free the dev->resource? There must
> be a better way to handle this.

Allocating in one big block means less potential memory fragmentation,
and the kernel can free it all at once.  This is a common pattern.

> p.s.
>
> Just wonder what happened to of_gpio stuff? You blocked it
> in 2.6.34 for no reason saying "I'll pick it into my OF
> tree before the 2.6.35 merge window" and it's 2.6.36 merge
> window quite soon.

It's in my test-devicetree branch.  I'll be posting for 2nd review in
the next few days and then adding to my devicetree-next branch
probably before the end of the week.

g.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Tue, Jun 08, 2010 at 10:02:49AM -0600, Grant Likely wrote:
> On Tue, Jun 8, 2010 at 9:57 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
> > [...]
> >> +     dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
> >>       if (!dev)
> >>               return NULL;
> >> -
> >>       dev->dev.of_node = of_node_get(np);
> >>       dev->dev.dma_mask = &dev->archdata.dma_mask;
> >>       dev->dev.parent = parent;
> >>       dev->dev.release = of_release_dev;
> >>
> >> +     /* Populate the resource table */
> >> +     if (num_irq || num_reg) {
> >> +             dev->resource = (void*)&dev[1];
> >
> > This is ugly. Why not allocate the memory specifically for
> > dev->resource? Is this because you plan to get rid of
> > of_release_dev(), and the generic release_dev() won't
> > know if it should free the dev->resource? There must
> > be a better way to handle this.
> 
> Allocating in one big block means less potential memory fragmentation,
> and the kernel can free it all at once.

Are there any numbers of saved amount of memory so that we
could compare?

The "less memory fragmentation" is indeed potential, but
introduction of obscure code is going on now at this precise
moment.

> This is a common pattern.

This can't be true because it produces ugly casts and fragile
code all over the place -- which is exactly what everybody
tries to avoid in the kernel.

Such a pattern is common when a driver allocates e.g. tx and rx
buffers (of the same type) together, and then split the buffer
into two pointers.

But I heard of no such pattern for 'struct device + struct
resources' allocation without even some kind of _priv struct,
which is surely something new, and ugly.

If you really want to avoid (an unproven) memory fragmentation,
you could do:

struct of_device_with_resources {
	struct device dev;
	struct resource resourses[0];
};

This at least will get rid of the casts and incomprehensible
"dev->resource = (void*)&dev[1];" construction.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Tue, Jun 8, 2010 at 10:46 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Tue, Jun 08, 2010 at 10:02:49AM -0600, Grant Likely wrote:
>> On Tue, Jun 8, 2010 at 9:57 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
>> > On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
>> > [...]
>> >> +     dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
>> >>       if (!dev)
>> >>               return NULL;
>> >> -
>> >>       dev->dev.of_node = of_node_get(np);
>> >>       dev->dev.dma_mask = &dev->archdata.dma_mask;
>> >>       dev->dev.parent = parent;
>> >>       dev->dev.release = of_release_dev;
>> >>
>> >> +     /* Populate the resource table */
>> >> +     if (num_irq || num_reg) {
>> >> +             dev->resource = (void*)&dev[1];
>> >
>> > This is ugly. Why not allocate the memory specifically for
>> > dev->resource? Is this because you plan to get rid of
>> > of_release_dev(), and the generic release_dev() won't
>> > know if it should free the dev->resource? There must
>> > be a better way to handle this.
>>
>> Allocating in one big block means less potential memory fragmentation,
>> and the kernel can free it all at once.
>
> Are there any numbers of saved amount of memory so that we
> could compare?
>
> The "less memory fragmentation" is indeed potential, but
> introduction of obscure code is going on now at this precise
> moment.

It's not obscure.  It's smaller and simpler code with fewer error paths.

>> This is a common pattern.
>
> This can't be true because it produces ugly casts and fragile
> code all over the place -- which is exactly what everybody
> tries to avoid in the kernel.

Fragile?  How?  &var[1] *always* gives you a pointer to the first
address after a structure.  If the structure changes, then so does the
offset.  Heck, if the type of 'var' changes, then the offset changes
in kind too.  If anything, I should have also used sizeof(*res) in the
kzalloc call so that the allocated size is protected against a type
change to 'res' too.

If you prefer, I can move the dev->resource assignment to immediately
after the kzalloc to keep everything contained within 4 lines of each
other.

> Such a pattern is common when a driver allocates e.g. tx and rx
> buffers (of the same type) together, and then split the buffer
> into two pointers.

tx & rx buffers are different because they must be DMAable.  That
imposes alignment requirements with kzalloc() guarantees.

> But I heard of no such pattern for 'struct device + struct
> resources' allocation without even some kind of _priv struct,
> which is surely something new, and ugly.

git grep '\*).*&[a-z1-9_]*\[1\]'

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Tue, Jun 08, 2010 at 12:41:47PM -0600, Grant Likely wrote:
[...]
> >> This is a common pattern.
> >
> > This can't be true because it produces ugly casts and fragile
> > code all over the place -- which is exactly what everybody
> > tries to avoid in the kernel.
> 
> Fragile?  How?  &var[1] *always* gives you a pointer to the first
> address after a structure.  If the structure changes, then so does the
> offset.  Heck, if the type of 'var' changes, then the offset changes
> in kind too.  If anything, I should have also used sizeof(*res) in the
> kzalloc call so that the allocated size is protected against a type
> change to 'res' too.

You just introduced an unnamed structure of device + resources,
it isn't declared anywhere but in the code itself (either via
&foo[1] or buf + sizeof(*foo)).

You're not the only one who hacks (or at least have to
understand) the OF stuff, so let's try keep this stuff
readable?

I told you several ways of how to improve the code (based on
the ideas from drivers/base/, so the ideas aren't even mine,
fwiw).

> If you prefer, I can move the dev->resource assignment to immediately
> after the kzalloc to keep everything contained within 4 lines of each
> other.

Sure, that would be better, but I already said what would I
really prefer, i.e.

- A dedicated allocation;
- Or, at least, the same thing as drivers/base/platform.c does:
  platform_object { platform_device; name[1]; };

> > But I heard of no such pattern for 'struct device + struct
> > resources' allocation without even some kind of _priv struct,
> > which is surely something new, and ugly.
> 
> git grep '\*).*&[a-z1-9_]*\[1\]'

Ugh? This produces a lot of false positives. But OK, let's run it.

~/linux-2.6$ git grep '\*).*&[a-z1-9_]*\[1\]' | wc -l
164

^^^ Now let's presume that half of it (and not just a few hits)
    is the ugly hacks that we're talking about. Then compare:

~/linux-2.6$ git grep 'struct.*_priv' | wc -l
22448

^^^ this is a common pattern.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Benjamin Herrenschmidt]

> You just introduced an unnamed structure of device + resources,
> it isn't declared anywhere but in the code itself (either via
> &foo[1] or buf + sizeof(*foo)).
> 
> You're not the only one who hacks (or at least have to
> understand) the OF stuff, so let's try keep this stuff
> readable?
> 
> I told you several ways of how to improve the code (based on
> the ideas from drivers/base/, so the ideas aren't even mine,
> fwiw).

I tend to agree with Anton here.

BTW. Why not make of_device a wrapper (or even alias of)
platform_device ? :-) That way you get the resource array etc.. for free
and it will make the whole of_device vs. platform_device issue moot.

Cheers,
Ben.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Thu, Jun 10, 2010 at 12:17 AM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
>
>> You just introduced an unnamed structure of device + resources,
>> it isn't declared anywhere but in the code itself (either via
>> &foo[1] or buf + sizeof(*foo)).
>>
>> You're not the only one who hacks (or at least have to
>> understand) the OF stuff, so let's try keep this stuff
>> readable?
>>
>> I told you several ways of how to improve the code (based on
>> the ideas from drivers/base/, so the ideas aren't even mine,
>> fwiw).
>
> I tend to agree with Anton here.

The reason I'm confident doing it that way is that it is *not* a
structure.  There is no structure relationship between the resource
table and the platform_device other than they are allocated with the
same kzalloc() call.  All the code that cares about that is contained
within 4 lines of code.  I'm resistant to using a structure because it
is adds an additional 5-6 lines of code to add a structure that won't
be used anywhere else, and is only 4 lines to begin with.

> BTW. Why not make of_device a wrapper (or even alias of)
> platform_device ? :-) That way you get the resource array etc.. for free
> and it will make the whole of_device vs. platform_device issue moot.

of_device is an alias of platform_device now.  The resource array in
platform devices is not statically defined.  It is allocated
separately.  I can't currently use the platform_device_alloc code
which does separate deallocation because the OF code needs its own
release hook to put the node.  OTOH, I can probably change the guts of
of_release_dev() to be called by platform_device_release().

okay, I'll try changing this an see how it looks.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[M. Warner Losh]

In message: <AANLkTilpUi1cazljWSFbzliY78RKyHUlvBshUD3NPHPv-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
            Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> writes:
: On Thu, Jun 10, 2010 at 12:17 AM, Benjamin Herrenschmidt
: <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
: >
: >> You just introduced an unnamed structure of device + resources,
: >> it isn't declared anywhere but in the code itself (either via
: >> &foo[1] or buf + sizeof(*foo)).
: >>
: >> You're not the only one who hacks (or at least have to
: >> understand) the OF stuff, so let's try keep this stuff
: >> readable?
: >>
: >> I told you several ways of how to improve the code (based on
: >> the ideas from drivers/base/, so the ideas aren't even mine,
: >> fwiw).
: >
: > I tend to agree with Anton here.
: 
: The reason I'm confident doing it that way is that it is *not* a
: structure.  There is no structure relationship between the resource
: table and the platform_device other than they are allocated with the
: same kzalloc() call.  All the code that cares about that is contained
: within 4 lines of code.  I'm resistant to using a structure because it
: is adds an additional 5-6 lines of code to add a structure that won't
: be used anywhere else, and is only 4 lines to begin with.

I tend to agree with Grant here.  The idiom he's using is very wide
spread in the industry and works extremely well.  It keeps the
ugliness confined to a couple of lines and is less ugly than the
alternatives for this design pattern.  It is a little surprising when
you see the code the first time, granted, but I think its expressive
power trumps that small surprise.

Warner

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
[...]
> : >> I told you several ways of how to improve the code (based on
> : >> the ideas from drivers/base/, so the ideas aren't even mine,
> : >> fwiw).
> : >
> : > I tend to agree with Anton here.
> : 
> : The reason I'm confident doing it that way is that it is *not* a
> : structure.  There is no structure relationship between the resource
> : table and the platform_device other than they are allocated with the
> : same kzalloc() call.  All the code that cares about that is contained
> : within 4 lines of code.  I'm resistant to using a structure because it
> : is adds an additional 5-6 lines of code to add a structure that won't
> : be used anywhere else, and is only 4 lines to begin with.
> 
> I tend to agree with Grant here.  The idiom he's using is very wide
> spread in the industry and works extremely well.  It keeps the
> ugliness confined to a couple of lines and is less ugly than the
> alternatives for this design pattern.  It is a little surprising when
> you see the code the first time, granted, but I think its expressive
> power trumps that small surprise.

Oh, come on. Both constructions are binary equivalent.

So how can people seriously be with *that* code:

	dev->resource = (void *)&dev[1];

which, semantically, is a nonsense and asks for a fix.

While
	dev_obj->dev.resource = dev_obj->resource;

simply makes sense.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
[...]
> : >> I told you several ways of how to improve the code (based on
> : >> the ideas from drivers/base/, so the ideas aren't even mine,
> : >> fwiw).
> : >
> : > I tend to agree with Anton here.
> : 
> : The reason I'm confident doing it that way is that it is *not* a
> : structure.  There is no structure relationship between the resource
> : table and the platform_device other than they are allocated with the
> : same kzalloc() call.  All the code that cares about that is contained
> : within 4 lines of code.  I'm resistant to using a structure because it
> : is adds an additional 5-6 lines of code to add a structure that won't
> : be used anywhere else, and is only 4 lines to begin with.
> 
> I tend to agree with Grant here.  The idiom he's using is very wide
> spread in the industry and works extremely well.  It keeps the
> ugliness confined to a couple of lines and is less ugly than the
> alternatives for this design pattern.  It is a little surprising when
> you see the code the first time, granted, but I think its expressive
> power trumps that small surprise.

Oh, come on. Both constructions are binary equivalent.

So how can people seriously be with *that* code:

	dev->resource = (void *)&dev[1];

which, semantically, is a nonsense and asks for a fix.

While
	dev_obj->dev.resource = dev_obj->resource;

simply makes sense.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
[...]
> : >> I told you several ways of how to improve the code (based on
> : >> the ideas from drivers/base/, so the ideas aren't even mine,
> : >> fwiw).
> : >
> : > I tend to agree with Anton here.
> : 
> : The reason I'm confident doing it that way is that it is *not* a
> : structure.  There is no structure relationship between the resource
> : table and the platform_device other than they are allocated with the
> : same kzalloc() call.  All the code that cares about that is contained
> : within 4 lines of code.  I'm resistant to using a structure because it
> : is adds an additional 5-6 lines of code to add a structure that won't
> : be used anywhere else, and is only 4 lines to begin with.
> 
> I tend to agree with Grant here.  The idiom he's using is very wide
> spread in the industry and works extremely well.  It keeps the
> ugliness confined to a couple of lines and is less ugly than the
> alternatives for this design pattern.  It is a little surprising when
> you see the code the first time, granted, but I think its expressive
> power trumps that small surprise.

Oh, come on. Both constructions are binary equivalent.

So how can people seriously be with *that* code:

	dev->resource = (void *)&dev[1];

which, semantically, is a nonsense and asks for a fix.

While
	dev_obj->dev.resource = dev_obj->resource;

simply makes sense.

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[M. Warner Losh]

In message: <20100610154741.GA7484-wnGakbxT3iijyJ0x5qLZdcN33GVbZNy3@public.gmane.org>
            Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
: On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
: [...]
: > : >> I told you several ways of how to improve the code (based on
: > : >> the ideas from drivers/base/, so the ideas aren't even mine,
: > : >> fwiw).
: > : >
: > : > I tend to agree with Anton here.
: > : 
: > : The reason I'm confident doing it that way is that it is *not* a
: > : structure.  There is no structure relationship between the resource
: > : table and the platform_device other than they are allocated with the
: > : same kzalloc() call.  All the code that cares about that is contained
: > : within 4 lines of code.  I'm resistant to using a structure because it
: > : is adds an additional 5-6 lines of code to add a structure that won't
: > : be used anywhere else, and is only 4 lines to begin with.
: > 
: > I tend to agree with Grant here.  The idiom he's using is very wide
: > spread in the industry and works extremely well.  It keeps the
: > ugliness confined to a couple of lines and is less ugly than the
: > alternatives for this design pattern.  It is a little surprising when
: > you see the code the first time, granted, but I think its expressive
: > power trumps that small surprise.
: 
: Oh, come on. Both constructions are binary equivalent.
: 
: So how can people seriously be with *that* code:
: 
: 	dev->resource = (void *)&dev[1];
: 
: which, semantically, is a nonsense and asks for a fix.

It isn't nonsense.  That's just your opinion of it, nothing more.

: While
: 	dev_obj->dev.resource = dev_obj->resource;
: 
: simply makes sense.

But this requires extra, bogus fields in the structure and creates a
bogus sizeof issue.

There are problems both ways.  Yelling about it isn't going to make
you any more right, or convince me that I'm wrong.  It is an argument
that is at least two decades old...

Warner

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Thu, Jun 10, 2010 at 10:01:40AM -0600, M. Warner Losh wrote:
[...]
> But this requires extra, bogus fields in the structure

False. The [0] field isn't bogus, it has a defined meaning.
It says: here is some amount of resouces may be allocated.

> and creates a bogus sizeof issue.

Creates? False. The same sizeof problem exists in Grant's
approach. sizeof(*dev) != what_we_have_allocated. Which is
isn't great, I agree. And that's exactly why I proposed a
dedicated allocation in the first place.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Mitch Bradley]

Wow, there is some serious bikeshedding going on with this argument 
about structures and arrays .

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Thu, Jun 10, 2010 at 11:09 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> Wow, there is some serious bikeshedding going on with this argument about
> structures and arrays .

I know! Isn't it fun?

g.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[M. Warner Losh]

In message: <20100610165243.GA18043-wnGakbxT3iijyJ0x5qLZdcN33GVbZNy3@public.gmane.org>
            Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
: On Thu, Jun 10, 2010 at 10:01:40AM -0600, M. Warner Losh wrote:
: [...]
: > But this requires extra, bogus fields in the structure
: 
: False. The [0] field isn't bogus, it has a defined meaning.
: It says: here is some amount of resouces may be allocated.

Not false: It is extra fields and extra lines of code.  The fact that
it is well defined doesn't make it any less bogus.  But again, that's
just opinion, much like your objection to the equally well defined
structure it replaces is bogus.

: > and creates a bogus sizeof issue.
: 
: Creates? False. The same sizeof problem exists in Grant's
: approach. sizeof(*dev) != what_we_have_allocated. Which is
: isn't great, I agree. And that's exactly why I proposed a
: dedicated allocation in the first place.

Your solution doesn't solve the sizeof issue.  Don't pretend it does.
Both ways allocate a random amount of memory larger than sizeof the
structure.

Warner

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Benjamin Herrenschmidt]

On Thu, 2010-06-10 at 10:01 -0600, M. Warner Losh wrote:
> : Oh, come on. Both constructions are binary equivalent.
> : 
> : So how can people seriously be with *that* code:
> : 
> :       dev->resource = (void *)&dev[1];
> : 
> : which, semantically, is a nonsense and asks for a fix.
> 
> It isn't nonsense.  That's just your opinion of it, nothing more.

No, it's dangerously fragile abuse of the C language which loses type
safety etc... It might be correct, but if somebody comes back in 2 year
to change something in that code, the chances of breaking it are higher
than having the type safe:

> :       dev_obj->dev.resource = dev_obj->resource;
> : 

Variant.

It's also less ugly.

> : simply makes sense.
> 
> But this requires extra, bogus fields in the structure and creates a
> bogus sizeof issue.
> 
> There are problems both ways.  Yelling about it isn't going to make
> you any more right, or convince me that I'm wrong.  It is an argument
> that is at least two decades old... 

Cheers,
Ben.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Thu, Jun 10, 2010 at 9:47 AM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
> [...]
>> : >> I told you several ways of how to improve the code (based on
>> : >> the ideas from drivers/base/, so the ideas aren't even mine,
>> : >> fwiw).
>> : >
>> : > I tend to agree with Anton here.
>> :
>> : The reason I'm confident doing it that way is that it is *not* a
>> : structure.  There is no structure relationship between the resource
>> : table and the platform_device other than they are allocated with the
>> : same kzalloc() call.  All the code that cares about that is contained
>> : within 4 lines of code.  I'm resistant to using a structure because it
>> : is adds an additional 5-6 lines of code to add a structure that won't
>> : be used anywhere else, and is only 4 lines to begin with.
>>
>> I tend to agree with Grant here.  The idiom he's using is very wide
>> spread in the industry and works extremely well.  It keeps the
>> ugliness confined to a couple of lines and is less ugly than the
>> alternatives for this design pattern.  It is a little surprising when
>> you see the code the first time, granted, but I think its expressive
>> power trumps that small surprise.
>
> Oh, come on. Both constructions are binary equivalent.
>
> So how can people seriously be with *that* code:
>
>        dev->resource = (void *)&dev[1];
>
> which, semantically, is a nonsense and asks for a fix.
>
> While
>        dev_obj->dev.resource = dev_obj->resource;
>
> simply makes sense.

Well, my choices are (without whitespace so as not to bias line count):

A)
struct of_device *alloc_function(int num_res)
{
	struct device *ofdev;
	struct resource *res;
	ofdev = kzalloc(sizeof(*ofdev) + (sizeof(*res) * num_res), GFP_KERNEL);
	if (!ofdev)
		return NULL;
	res = (struct resource *)&ofdev[1];
	...
	return ofdev;
}
10 lines of code

B)
struct of_device_object {
	struct of_device ofdev;
	struct resource resource[0];
};
struct of_device *alloc_function(int num_res)
{
	struct of_device_object *ofobj;
	struct of_device *ofdev;
	struct resource *res;
	ofobj = kzalloc(sizeof(*ofobj) + (sizeof(*res) * num_res), GFP_KERNEL);
	if (!ofobj)
		return NULL;
	res = ofobj->resource;
	...
	return &ofobj->ofdev;
}
15 lines of code

C)
struct of_device *alloc_function(int num_res)
{
	struct device *ofdev;
	struct resource *res;
	ofdev = kzalloc(sizeof(*ofdev), GFP_KERNEL)
	if (!ofdev)
		return NULL;
	res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
	if (!res) {
		kfree(ofdev);  /* or goto an error unwind label */
		return NULL;
	}
	res = (struct resource *)&ofdev[1];
	...
	return ofdev;
}
15 lines of code, plus an extra few lines at kfree(ofdev) time.

When I look at the three, option A is more concise and clearer in it's
intent to me.

That being said, I'm looking at refactoring to use
platform_device_alloc() instead, which is effectively option C. (which
I'd normally avoid, but it removes otherwise duplicate code from
drivers/of).

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov]

On Thu, Jun 10, 2010 at 10:30:26AM -0600, Grant Likely wrote:
[...]
> C)
> struct of_device *alloc_function(int num_res)
> {
> 	struct device *ofdev;
> 	struct resource *res;
> 	ofdev = kzalloc(sizeof(*ofdev), GFP_KERNEL)
> 	if (!ofdev)
> 		return NULL;
> 	res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
> 	if (!res) {
> 		kfree(ofdev);  /* or goto an error unwind label */
> 		return NULL;
> 	}
> 	res = (struct resource *)&ofdev[1];

You mean ofdev->resource = res; ?

> That being said, I'm looking at refactoring to use
> platform_device_alloc() instead, which is effectively option C. (which
> I'd normally avoid, but it removes otherwise duplicate code from
> drivers/of).

Sounds great!

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Grant Likely]

On Thu, Jun 10, 2010 at 11:10 AM, Anton Vorontsov
<cbouatmailru@gmail.com> wrote:
> On Thu, Jun 10, 2010 at 10:30:26AM -0600, Grant Likely wrote:
> [...]
>>       res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
>>       if (!res) {
>>               kfree(ofdev);  /* or goto an error unwind label */
>>               return NULL;
>>       }
>>       res = (struct resource *)&ofdev[1];
>
> You mean ofdev->resource = res; ?

Yeah, cut-and-paste error.

g.