On Wed, Jun 1, 2011 at 10:52 AM, Rob Herring <[email protected]> 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. _______________________________________________ devicetree-discuss mailing list [email protected] https://lists.ozlabs.org/listinfo/devicetree-discuss
