[RFC PATCH v3 0/2] amba bus device tree probing

[RFC PATCH v3 0/2] amba bus device tree probing

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This is updated amba bus DT probe support. It has changed significantly since
v2 based on the discussion and the conclusions from that discussion:

- The Linux amba_bus is really for ARM Primecell peripherals, not a type of bus.
- The amba_bus is not going to be replaced by platform_bus.

The bus scanning is done entirely by of_platform_bus_populate/probe and only
the device creation is handled by the amba bus code. This patch set can support
Grant's options 2 or 3 depending on the match table used.

Applies to devicetree/next on git://git.secretlab.ca/git/linux-2.6

Rob

Rob Herring (2):
  drivers/amba: create devices from device tree
  dt: add custom device creation to platform bus scan

 drivers/amba/bus.c          |   51 +++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c       |   27 ++++++++++++++++------
 include/linux/amba/bus.h    |    5 ++++
 include/linux/of_platform.h |   11 ++++++--
 4 files changed, 83 insertions(+), 11 deletions(-)

-- 
1.7.4.1

[RFC PATCH v3 1/2] drivers/amba: create devices from device tree

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add a function to create amba bus devices (i.e. primecell peripherals) from
device tree nodes. The device tree scanning is done by
of_platform_probe/populate functions which can call of_amba_device_create
based on a match table entry.

Nodes with a "arm,amba-deviceid" property can override the h/w peripheral id
value.

Based on the original work by Jeremy Kerr.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: linux at arm.linux.org.uk
Cc: arnd at arndb.de
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/amba/bus.c       |   51 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |    5 ++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 7025593..6438ffd 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -13,6 +13,11 @@
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/amba/bus.h>
@@ -780,3 +785,49 @@ EXPORT_SYMBOL(amba_device_unregister);
 EXPORT_SYMBOL(amba_find_device);
 EXPORT_SYMBOL(amba_request_regions);
 EXPORT_SYMBOL(amba_release_regions);
+
+#ifdef CONFIG_OF
+int of_amba_device_create(struct device_node *node, struct device *parent)
+{
+	struct amba_device *dev;
+	const void *prop;
+	int i, ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	/* setup generic device info */
+	dev->dev.coherent_dma_mask = ~0;
+	dev->dev.of_node = node;
+	dev->dev.parent = parent;
+	of_device_make_bus_id(&dev->dev);
+
+	/* setup amba-specific device info */
+	dev->dma_mask = ~0;
+
+	/* Allow the arm,amba-deviceid value to override the h/w value */
+	prop = of_get_property(node, "arm,amba-deviceid", NULL);
+	if (prop)
+		dev->periphid = of_read_ulong(prop, 1);
+
+	/* Decode the IRQs and address ranges */
+	for (i = 0; i < AMBA_NR_IRQS; i++)
+		dev->irq[i] = irq_of_parse_and_map(node, i);
+
+	ret = of_address_to_resource(node, 0, &dev->res);
+	if (ret)
+		goto err_free;
+
+	ret = amba_device_register(dev, &iomem_resource);
+	if (ret)
+		goto err_free;
+
+	return 0;
+
+err_free:
+	kfree(dev);
+	return ret;
+}
+
+#endif /* CONFIG_OF */
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index fcbbe71..08349c3 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -94,4 +94,9 @@ void amba_release_regions(struct amba_device *);
 #define amba_manf(d)	AMBA_MANF_BITS((d)->periphid)
 #define amba_part(d)	AMBA_PART_BITS((d)->periphid)
 
+#ifdef CONFIG_OF
+struct device_node;
+int of_amba_device_create(struct device_node *node, struct device *parent);
+#endif
+
 #endif
-- 
1.7.4.1

[RFC PATCH v3 1/2] drivers/amba: create devices from device tree

[Linus Walleij]

2011/5/25 Rob Herring <robherring2@gmail.com>:

Trivia:

> + ? ? ? /* Allow the arm,amba-deviceid value to override the h/w value */
> + ? ? ? prop = of_get_property(node, "arm,amba-deviceid", NULL);
> + ? ? ? if (prop)
> + ? ? ? ? ? ? ? dev->periphid = of_read_ulong(prop, 1);

That currently does not override the H/W value at all. But when/if patch
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6829/1
gets merged, it eventually will.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add support to the platform bus scanning to call custom device creation
functions. This enables creation of non-platform devices like amba_bus.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: linux at arm.linux.org.uk
Cc: arnd at arndb.de
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/of/platform.c       |   27 +++++++++++++++++++--------
 include/linux/of_platform.h |   11 ++++++++---
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b785be..8c0f43400 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -176,9 +176,9 @@ EXPORT_SYMBOL(of_device_alloc);
  * Returns pointer to created platform device, or NULL if a device was not
  * registered.  Unavailable devices will not get registered.
  */
-struct platform_device *of_platform_device_create(struct device_node *np,
-					    const char *bus_id,
-					    struct device *parent)
+struct device *of_platform_device_create(struct device_node *np,
+					 const char *bus_id,
+					 struct device *parent)
 {
 	struct platform_device *dev;
 
@@ -205,7 +205,7 @@ struct platform_device *of_platform_device_create(struct device_node *np,
 		return NULL;
 	}
 
-	return dev;
+	return &dev->dev;
 }
 EXPORT_SYMBOL(of_platform_device_create);
 
@@ -224,7 +224,9 @@ static int of_platform_bus_create(struct device_node *bus,
 				  struct device *parent, bool strict)
 {
 	struct device_node *child;
-	struct platform_device *dev;
+	struct device *dev;
+	struct of_device_id *id;
+	struct of_platform_match_data *mdata;
 	int rc = 0;
 
 	/* Make sure it has a compatible property */
@@ -234,13 +236,22 @@ static int of_platform_bus_create(struct device_node *bus,
 		return 0;
 	}
 
-	dev = of_platform_device_create(bus, NULL, parent);
-	if (!dev || !of_match_node(matches, bus))
+	id = of_match_node(matches, bus);
+	if (!id)
+		return 0;
+
+	/* Found a matching node, so create the device */
+	mdata = id->data;
+	if (mdata && mdata->dev_create)
+		dev = mdata->dev_create(bus, parent);
+	else
+		dev = of_platform_device_create(bus, NULL, parent);
+	if (!dev)
 		return 0;
 
 	for_each_child_of_node(bus, child) {
 		pr_debug("   create child: %s\n", child->full_name);
-		rc = of_platform_bus_create(child, matches, &dev->dev, strict);
+		rc = of_platform_bus_create(child, matches, dev, strict);
 		if (rc) {
 			of_node_put(child);
 			break;
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 43c723d..f59d25e 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -40,6 +40,11 @@ struct of_platform_driver
 #define	to_of_platform_driver(drv) \
 	container_of(drv,struct of_platform_driver, driver)
 
+struct of_platform_match_data {
+	struct device *	(*dev_create)(struct device_node *node,
+				      struct device *parent);
+};
+
 /* Platform drivers register/unregister */
 extern struct platform_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
@@ -48,9 +53,9 @@ extern struct platform_device *of_find_device_by_node(struct device_node *np);
 
 #if !defined(CONFIG_SPARC) /* SPARC has its own device registration method */
 /* Platform devices and busses creation */
-extern struct platform_device *of_platform_device_create(struct device_node *np,
-						   const char *bus_id,
-						   struct device *parent);
+extern struct device *of_platform_device_create(struct device_node *np,
+						const char *bus_id,
+						struct device *parent);
 
 extern int of_platform_bus_probe(struct device_node *root,
 				 const struct of_device_id *matches,
-- 
1.7.4.1

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Arnd Bergmann]

On Wednesday 25 May 2011, Rob Herring wrote:
> 
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Add support to the platform bus scanning to call custom device creation
> functions. This enables creation of non-platform devices like amba_bus.
> 
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: linux at arm.linux.org.uk
> Cc: arnd at arndb.de
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>

This creates a confusing mix of match table entries: Normally,
all entries in the match table are meant to identify child buses,
but if I read your patch correctly, you now also need to match
on the amba devices themselves, including the creation of
platform devices for each child device node under an amba
device.

I don't think that was the intention. Maybe we need to pass
two match tables into of_platform_bus_probe() instead:
one to identify the buses, and another one that is used
to create the actual devices.

	Arnd

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Rob Herring]

Arnd,

On 05/26/2011 08:11 AM, Arnd Bergmann wrote:
> On Wednesday 25 May 2011, Rob Herring wrote:
>>
>> From: Rob Herring<rob.herring@calxeda.com>
>>
>> Add support to the platform bus scanning to call custom device creation
>> functions. This enables creation of non-platform devices like amba_bus.
>>
>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: linux at arm.linux.org.uk
>> Cc: arnd at arndb.de
>> Cc: Linus Walleij<linus.walleij@linaro.org>
>> Signed-off-by: Rob Herring<rob.herring@calxeda.com>
>
> This creates a confusing mix of match table entries: Normally,
> all entries in the match table are meant to identify child buses,
> but if I read your patch correctly, you now also need to match
> on the amba devices themselves, including the creation of
> platform devices for each child device node under an amba
> device.
>
We should only create devices for each matching bus and the immediate 
children of each bus. A child device of an amba device would be 
something like an i2c bus which we don't want to create devices for. Or 
am I missing something?

> I don't think that was the intention. Maybe we need to pass
> two match tables into of_platform_bus_probe() instead:
> one to identify the buses, and another one that is used
> to create the actual devices.
>
That was my original thinking too, but some reason I had concluded 1 
could get by with just 1 table. After more thought, I think you are 
right. In fact, I broke platform device creation with this patch. I need 
to be able to tell if no match means create a platform device (child of 
bus) or not (child of a device).

Here's an updated version with just the interesting hunk. I've tested it 
with a made up bus structure that looks something like this:

soc bus
  -plat dev
  -amba dev
  -sub-bus
    -plat dev
    -amba dev
  -plat dev

As of_platform_bus_probe is not recommended to be used by Grant, I only 
plan to add 2 match tables to of_platform_bus_populate.

@@ -234,18 +237,32 @@ static int of_platform_bus_create(struct 
device_node *bus,
  		return 0;
  	}

-	dev = of_platform_device_create(bus, NULL, parent);
-	if (!dev || !of_match_node(matches, bus))
-		return 0;
-
-	for_each_child_of_node(bus, child) {
-		pr_debug("   create child: %s\n", child->full_name);
-		rc = of_platform_bus_create(child, matches, &dev->dev, strict);
-		if (rc) {
-			of_node_put(child);
-			break;
+	id = of_match_node(bus_matches, bus);
+	if (id) {
+		dev = of_platform_device_create(bus, NULL, parent);
+		if (!dev)
+			return 0;
+		for_each_child_of_node(bus, child) {
+			pr_debug("   create child: %s\n", child->full_name);
+			rc = of_platform_bus_create(child, bus_matches,
+						    dev_matches, dev, strict);
+			if (rc) {
+				of_node_put(child);
+				break;
+			}
  		}
+		return rc;
  	}
+
+	id = of_match_node(dev_matches, bus);
+	mdata = id ? id->data : NULL;
+	if (id && mdata && mdata->dev_create)
+		dev = mdata->dev_create(bus, parent);
+	else
+		dev = of_platform_device_create(bus, NULL, parent);
+	if (!dev)
+		return 0;
+
  	return rc;
  }


Rob

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Arnd Bergmann]

On Thursday 26 May 2011, Rob Herring wrote:
> On 05/26/2011 08:11 AM, Arnd Bergmann wrote:
> > On Wednesday 25 May 2011, Rob Herring wrote:
> > This creates a confusing mix of match table entries: Normally,
> > all entries in the match table are meant to identify child buses,
> > but if I read your patch correctly, you now also need to match
> > on the amba devices themselves, including the creation of
> > platform devices for each child device node under an amba
> > device.
> >
> We should only create devices for each matching bus and the immediate 
> children of each bus. A child device of an amba device would be 
> something like an i2c bus which we don't want to create devices for. Or 
> am I missing something?

Exactly, that was my point.

> > I don't think that was the intention. Maybe we need to pass
> > two match tables into of_platform_bus_probe() instead:
> > one to identify the buses, and another one that is used
> > to create the actual devices.
> >
> That was my original thinking too, but some reason I had concluded 1 
> could get by with just 1 table. After more thought, I think you are 
> right. In fact, I broke platform device creation with this patch. I need 
> to be able to tell if no match means create a platform device (child of 
> bus) or not (child of a device).

Ok.

> @@ -234,18 +237,32 @@ static int of_platform_bus_create(struct 
> device_node *bus,
>   		return 0;
>   	}
> 
> -	dev = of_platform_device_create(bus, NULL, parent);
> -	if (!dev || !of_match_node(matches, bus))
> -		return 0;
> -
> -	for_each_child_of_node(bus, child) {
> -		pr_debug("   create child: %s\n", child->full_name);
> -		rc = of_platform_bus_create(child, matches, &dev->dev, strict);
> -		if (rc) {
> -			of_node_put(child);
> -			break;
> +	id = of_match_node(bus_matches, bus);
> +	if (id) {
> +		dev = of_platform_device_create(bus, NULL, parent);
> +		if (!dev)
> +			return 0;
> +		for_each_child_of_node(bus, child) {
> +			pr_debug("   create child: %s\n", child->full_name);
> +			rc = of_platform_bus_create(child, bus_matches,
> +						    dev_matches, dev, strict);
> +			if (rc) {
> +				of_node_put(child);
> +				break;
> +			}
>   		}
> +		return rc;
>   	}
> +
> +	id = of_match_node(dev_matches, bus);
> +	mdata = id ? id->data : NULL;
> +	if (id && mdata && mdata->dev_create)
> +		dev = mdata->dev_create(bus, parent);
> +	else
> +		dev = of_platform_device_create(bus, NULL, parent);
> +	if (!dev)
> +		return 0;
> +

Yes, that looks like it should work.

It still feels a bit strange, because it's not exactly how we normally
probe devices: In all other cases, we bind a device to a driver when we
find it, and that driver in turn scans it, and potentially creates
child devices that it finds.

What we do here is to let the platform decide how to interpret the
data that is coming in. To make the probing more well-behaved, another
approach would be:

* Bind a platform_driver to compatible="arm,amba" (or whatever we
  had in the binding).

* In that driver, do nothing except register an amba_device as a child.

This would create a somewhat deeper device hierarchy, but be still
completely logical: you have a device that cannot be probed (identified
simply by its register space), which can be probed internally because
the registers actually have a meaning.

	Arnd

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Rob Herring]

On 05/27/2011 07:06 AM, Arnd Bergmann wrote:
> On Thursday 26 May 2011, Rob Herring wrote:
>> On 05/26/2011 08:11 AM, Arnd Bergmann wrote:
>>> On Wednesday 25 May 2011, Rob Herring wrote:
>>> This creates a confusing mix of match table entries: Normally,
>>> all entries in the match table are meant to identify child buses,
>>> but if I read your patch correctly, you now also need to match
>>> on the amba devices themselves, including the creation of
>>> platform devices for each child device node under an amba
>>> device.
>>>
>> We should only create devices for each matching bus and the immediate
>> children of each bus. A child device of an amba device would be
>> something like an i2c bus which we don't want to create devices for. Or
>> am I missing something?
>
> Exactly, that was my point.
>
>>> I don't think that was the intention. Maybe we need to pass
>>> two match tables into of_platform_bus_probe() instead:
>>> one to identify the buses, and another one that is used
>>> to create the actual devices.
>>>
>> That was my original thinking too, but some reason I had concluded 1
>> could get by with just 1 table. After more thought, I think you are
>> right. In fact, I broke platform device creation with this patch. I need
>> to be able to tell if no match means create a platform device (child of
>> bus) or not (child of a device).
>
> Ok.
>
>> @@ -234,18 +237,32 @@ static int of_platform_bus_create(struct
>> device_node *bus,
>>    		return 0;
>>    	}
>>
>> -	dev = of_platform_device_create(bus, NULL, parent);
>> -	if (!dev || !of_match_node(matches, bus))
>> -		return 0;
>> -
>> -	for_each_child_of_node(bus, child) {
>> -		pr_debug("   create child: %s\n", child->full_name);
>> -		rc = of_platform_bus_create(child, matches,&dev->dev, strict);
>> -		if (rc) {
>> -			of_node_put(child);
>> -			break;
>> +	id = of_match_node(bus_matches, bus);
>> +	if (id) {
>> +		dev = of_platform_device_create(bus, NULL, parent);
>> +		if (!dev)
>> +			return 0;
>> +		for_each_child_of_node(bus, child) {
>> +			pr_debug("   create child: %s\n", child->full_name);
>> +			rc = of_platform_bus_create(child, bus_matches,
>> +						    dev_matches, dev, strict);
>> +			if (rc) {
>> +				of_node_put(child);
>> +				break;
>> +			}
>>    		}
>> +		return rc;
>>    	}
>> +
>> +	id = of_match_node(dev_matches, bus);
>> +	mdata = id ? id->data : NULL;
>> +	if (id&&  mdata&&  mdata->dev_create)
>> +		dev = mdata->dev_create(bus, parent);
>> +	else
>> +		dev = of_platform_device_create(bus, NULL, parent);
>> +	if (!dev)
>> +		return 0;
>> +
>
> Yes, that looks like it should work.
>
> It still feels a bit strange, because it's not exactly how we normally
> probe devices: In all other cases, we bind a device to a driver when we
> find it, and that driver in turn scans it, and potentially creates
> child devices that it finds.
>
> What we do here is to let the platform decide how to interpret the
> data that is coming in. To make the probing more well-behaved, another
> approach would be:
>
> * Bind a platform_driver to compatible="arm,amba" (or whatever we
>    had in the binding).
>
> * In that driver, do nothing except register an amba_device as a child.
>
> This would create a somewhat deeper device hierarchy, but be still
> completely logical: you have a device that cannot be probed (identified
> simply by its register space), which can be probed internally because
> the registers actually have a meaning.

Shouldn't the hierarchy in linux reflect the h/w? It seems a bit 
pointless to me to create a device just to create another device. 
amba_bus is already a bit strange in that it is not really a bus type, 
but a certain class of peripherals.

I'd like to hear Grant's thoughts on this.

Rob

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Grant Likely]

On Wed, Jun 1, 2011 at 10:52 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 05/27/2011 07:06 AM, Arnd Bergmann wrote:
>>
>> On Thursday 26 May 2011, Rob Herring wrote:
>>>
>>> On 05/26/2011 08:11 AM, Arnd Bergmann wrote:
>>>>
>>>> On Wednesday 25 May 2011, Rob Herring wrote:
>>>> This creates a confusing mix of match table entries: Normally,
>>>> all entries in the match table are meant to identify child buses,
>>>> but if I read your patch correctly, you now also need to match
>>>> on the amba devices themselves, including the creation of
>>>> platform devices for each child device node under an amba
>>>> device.
>>>>
>>> We should only create devices for each matching bus and the immediate
>>> children of each bus. A child device of an amba device would be
>>> something like an i2c bus which we don't want to create devices for. Or
>>> am I missing something?
>>
>> Exactly, that was my point.
>>
>>>> I don't think that was the intention. Maybe we need to pass
>>>> two match tables into of_platform_bus_probe() instead:
>>>> one to identify the buses, and another one that is used
>>>> to create the actual devices.
>>>>
>>> That was my original thinking too, but some reason I had concluded 1
>>> could get by with just 1 table. After more thought, I think you are
>>> right. In fact, I broke platform device creation with this patch. I need
>>> to be able to tell if no match means create a platform device (child of
>>> bus) or not (child of a device).
>>
>> Ok.
>>
>>> @@ -234,18 +237,32 @@ static int of_platform_bus_create(struct
>>> device_node *bus,
>>> ? ? ? ? ? ? ? ?return 0;
>>> ? ? ? ?}
>>>
>>> - ? ? ? dev = of_platform_device_create(bus, NULL, parent);
>>> - ? ? ? if (!dev || !of_match_node(matches, bus))
>>> - ? ? ? ? ? ? ? return 0;
>>> -
>>> - ? ? ? for_each_child_of_node(bus, child) {
>>> - ? ? ? ? ? ? ? pr_debug(" ? create child: %s\n", child->full_name);
>>> - ? ? ? ? ? ? ? rc = of_platform_bus_create(child, matches,&dev->dev,
>>> strict);
>>> - ? ? ? ? ? ? ? if (rc) {
>>> - ? ? ? ? ? ? ? ? ? ? ? of_node_put(child);
>>> - ? ? ? ? ? ? ? ? ? ? ? break;
>>> + ? ? ? id = of_match_node(bus_matches, bus);
>>> + ? ? ? if (id) {
>>> + ? ? ? ? ? ? ? dev = of_platform_device_create(bus, NULL, parent);
>>> + ? ? ? ? ? ? ? if (!dev)
>>> + ? ? ? ? ? ? ? ? ? ? ? return 0;
>>> + ? ? ? ? ? ? ? for_each_child_of_node(bus, child) {
>>> + ? ? ? ? ? ? ? ? ? ? ? pr_debug(" ? create child: %s\n",
>>> child->full_name);
>>> + ? ? ? ? ? ? ? ? ? ? ? rc = of_platform_bus_create(child, bus_matches,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_matches, dev,
>>> strict);
>>> + ? ? ? ? ? ? ? ? ? ? ? if (rc) {
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? of_node_put(child);
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>>> + ? ? ? ? ? ? ? ? ? ? ? }
>>> ? ? ? ? ? ? ? ?}
>>> + ? ? ? ? ? ? ? return rc;
>>> ? ? ? ?}
>>> +
>>> + ? ? ? id = of_match_node(dev_matches, bus);
>>> + ? ? ? mdata = id ? id->data : NULL;
>>> + ? ? ? if (id&& ?mdata&& ?mdata->dev_create)
>>> + ? ? ? ? ? ? ? dev = mdata->dev_create(bus, parent);
>>> + ? ? ? else
>>> + ? ? ? ? ? ? ? dev = of_platform_device_create(bus, NULL, parent);
>>> + ? ? ? if (!dev)
>>> + ? ? ? ? ? ? ? return 0;
>>> +
>>
>> Yes, that looks like it should work.
>>
>> It still feels a bit strange, because it's not exactly how we normally
>> probe devices: In all other cases, we bind a device to a driver when we
>> find it, and that driver in turn scans it, and potentially creates
>> child devices that it finds.
>>
>> What we do here is to let the platform decide how to interpret the
>> data that is coming in. To make the probing more well-behaved, another
>> approach would be:
>>
>> * Bind a platform_driver to compatible="arm,amba" (or whatever we
>> ? had in the binding).
>>
>> * In that driver, do nothing except register an amba_device as a child.
>>
>> This would create a somewhat deeper device hierarchy, but be still
>> completely logical: you have a device that cannot be probed (identified
>> simply by its register space), which can be probed internally because
>> the registers actually have a meaning.
>
> Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless
> to me to create a device just to create another device. amba_bus is already
> a bit strange in that it is not really a bus type, but a certain class of
> peripherals.
>
> I'd like to hear Grant's thoughts on this.

AMBA and platform_devices are "special" in that they don't have
requirements on their parent device.  I see absolutely zero issue with
having platform_device and amba_device as siblings.

g.

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Arnd Bergmann]

On Wednesday 01 June 2011, Grant Likely wrote:
> >
> > Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless
> > to me to create a device just to create another device. amba_bus is already
> > a bit strange in that it is not really a bus type, but a certain class of
> > peripherals.
> >
> > I'd like to hear Grant's thoughts on this.
> 
> AMBA and platform_devices are "special" in that they don't have
> requirements on their parent device.  I see absolutely zero issue with
> having platform_device and amba_device as siblings.

Hmm, if we accept that platform and AMBA devices are just different from
all the others, maybe we can do a simpler solution and just hardcode that
difference in the of_platform_bus_create function. It probably doesn't
hurt if we grow a handful of these at most, as long as we wouldn't add
code for a lot of different bus types to the common probing function.

	Arnd

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Rob Herring]

On 06/01/2011 04:29 PM, Arnd Bergmann wrote:
> On Wednesday 01 June 2011, Grant Likely wrote:
>>>
>>> Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless
>>> to me to create a device just to create another device. amba_bus is already
>>> a bit strange in that it is not really a bus type, but a certain class of
>>> peripherals.
>>>
>>> I'd like to hear Grant's thoughts on this.
>>
>> AMBA and platform_devices are "special" in that they don't have
>> requirements on their parent device.  I see absolutely zero issue with
>> having platform_device and amba_device as siblings.
>
> Hmm, if we accept that platform and AMBA devices are just different from
> all the others, maybe we can do a simpler solution and just hardcode that
> difference in the of_platform_bus_create function. It probably doesn't
> hurt if we grow a handful of these at most, as long as we wouldn't add
> code for a lot of different bus types to the common probing function.
>
> 	Arnd

The simplest solutions are usually the best. With that, the 2nd patch 
becomes:

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b785be..6955932 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -21,6 +21,7 @@
  #include <linux/of_irq.h>
  #include <linux/of_platform.h>
  #include <linux/platform_device.h>
+#include <linux/amba/bus.h>

  static int of_dev_node_match(struct device *dev, void *data)
  {
@@ -234,6 +235,11 @@ static int of_platform_bus_create(struct 
device_node *bus,
                 return 0;
         }

+       if (of_device_is_compatible(bus, "arm,amba-device")) {
+               of_amba_device_create(bus, parent);
+               return 0;
+       }
+
         dev = of_platform_device_create(bus, NULL, parent);
         if (!dev || !of_match_node(matches, bus))
                 return 0;

[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan

[Arnd Bergmann]

On Wednesday 08 June 2011 05:12:47 Rob Herring wrote:
> The simplest solutions are usually the best. With that, the 2nd patch 
> becomes:
>
>   static int of_dev_node_match(struct device *dev, void *data)
>   {
> @@ -234,6 +235,11 @@ static int of_platform_bus_create(struct 
> device_node *bus,
>                  return 0;
>          }
> 
> +       if (of_device_is_compatible(bus, "arm,amba-device")) {
> +               of_amba_device_create(bus, parent);
> +               return 0;
> +       }
> +

Sounds good to me, just remind me what of_amba_device_create does when
amba is disabled. I guess it should do nothing in that case, there
can be no amba drivers loaded, so the device is clearly not needed.

	Arnd