On Wed, Jun 1, 2011 at 10:52 AM, Rob Herring [off-list ref] wrote:

On 05/27/2011 07:06 AM, Arnd Bergmann wrote:

quoted

On Thursday 26 May 2011, Rob Herring wrote:

quoted

On 05/26/2011 08:11 AM, Arnd Bergmann wrote:

quoted

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.

quoted

quoted

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.

quoted

@@ -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.