[RFC PATCH v3 2/2] dt: add custom device creation to platform bus scan
Grant Likely
grant.likely at secretlab.ca
Thu Jun 2 02:58:03 EST 2011
On Wed, Jun 1, 2011 at 10:52 AM, Rob Herring <robherring2 at 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.
More information about the devicetree-discuss
mailing list