Messages in this thread Patch in this message | | | Date | Fri, 26 Oct 2012 08:12:38 +0100 | From | Srinivas KANDAGATLA <> | Subject | Re: [RFC PATCH 3.7.0-rc2] dt: match id-table before creating platform device |
| |
On 23/10/12 14:15, Rob Herring wrote: re-sending my reply again, as it did not appear in my inbox from dt mailing list. > Adding lkml. DT patches should go to both lists. > > On 10/23/2012 05:30 AM, Srinivas KANDAGATLA wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> >> >> As part of of_platform_populate call, the existing code iterates each >> child node and then creates a platform device for each child, however >> there is bug in the code which does not check the match table before >> creating the platform device. This might result creating two platfrom >> devices and also invoking driver probe twice, which is incorrect. >> >> This patch moves a existing of_match_node check to start of the function >> to fix the bug, doing this way will return immediately without creating >> any datastructures if the child does not match the supplied match-table. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com> >> --- >> drivers/of/platform.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index b80891b..1aaa560 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -367,6 +367,9 @@ static int of_platform_bus_create(struct device_node *bus, >> return 0; >> } >> >> + if (!of_match_node(matches, bus)) >> + return 0; >> + > This is not right. This function is recursive and this change would > break that.
You are correct, this change might break the functionality.
> Perhaps we could only call of_platform_device_create_pdata > if !of_match_node instead, but I'm not completely sure that would be the > right thing to do.
I did try to do the same thing in the patch. May be I should have moved check just before calling of_platform_device_create_pdata?
> There's also some historical things we have to > support which is why we have of_platform_populate and of_platform_bus_probe.
m just trying to understand the difference between of_platform_populate and of_platform_bus_probe. Looking at the function documentation, which states of_platform_bus_probe will only create children of the root which are selected by the @matches argument.
of_platform_populate walks the device tree and creates devices from nodes. It differs in that it follows the modern convention of requiring all device nodes to have a 'compatible' property, and it is suitable for creating devices which are children of the root node.
Lets say If we call of_platform_populate(NULL, match_table, NULL, NULL) on a device trees like the below with struct of_device_id match_table[] = { { .compatible = "simple-bus", } {} };
parent@0{ compatible = "xxx,parent1", "simple-bus"; ... child@0 { compatible = "xxx,child0", "simple-bus"; ... }; child@1 { compatible = "xxx,child1"; ... }; child@2 { compatible = "xxx,child2", "simple-bus"; ... }; };
of_platform_bus_probe would create platform-devices for parent@0, child@0and child@2 where as of_platform_populate would create platform-devices for parent@0, child@0, child@1 and child@2 nodes.
So the question is why do we need to have @matches argument to of_platform_populate in the first place, if it creates all the devices by walking the dt nodes?
It is bit confusing, As some platforms use of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL) assuming that only matching nodes will end up having platform device. Also some platforms use of_platform_bus_probe(NULL, match_table, NULL), where match table is of_default_bus_match_table.
Am not 100% sure what is the right solution, but I think lot of platforms would want behavior like of_platform_bus_probe which takes lookups aswell.
IMO, we could do two things to avoid this confusion in future and achieve the expected behaviour.
1. Remove matches from of_platform_populate 2. add Lookup argument to of_platform_bus_probe
??
--srini
> Rob > >> auxdata = of_dev_lookup(lookup, bus); >> if (auxdata) { >> bus_id = auxdata->name; >> @@ -379,7 +382,7 @@ static int of_platform_bus_create(struct device_node *bus, >> } >> >> dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); >> - if (!dev || !of_match_node(matches, bus)) >> + if (!dev) >> return 0; >> >> for_each_child_of_node(bus, child) { >> > >
| |