Adding lkml. DT patches should go to both lists.
On 10/23/2012 05:30 AM, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <[email protected]>
>
> 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 <[email protected]>
> ---
> 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. 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. There's also some historical things we have to
support which is why we have of_platform_populate and of_platform_bus_probe.
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) {
>
On 23/10/12 14:15, Rob Herring wrote:
> Adding lkml. DT patches should go to both lists.
>
> On 10/23/2012 05:30 AM, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.
Am 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_probewould 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 confusion, 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.
IMO, we could do two things to avoid this confusion in future.
1. Remove matches from of_platform_populate
2. add Lookup argument to of_platform_bus_probe
What do you think?
--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) {
>>
>
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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) {
>>
>
>
On Fri, 26 Oct 2012 08:12:38 +0100, Srinivas KANDAGATLA <[email protected]> wrote:
> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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?
No, the current code is correct. The purpose of the match table passed
to of_platform_populate is to figure out which nodes are bus nodes that
need to be recursed into.
However, *every single* child node of the root when
of_platform_populate() is called will be used to create a
platform_device. This is by design.
So, if somewhere is calling of_platform_populate() with the expectation
that it will only create devices for a subset of the nodes, then that
code is working on an incorrect assumption.
>
> > 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?
of_platform_populate will create devices for all the children of
child@0 and child@2 also. The intent is for generic board support to
call of_platform_populate() on the root of the tree and have all the
nodes with compatible properties *and all the children of simple memory
mapped busses* created into devices. This is normally what we want;
particularly for new board support.
>
> 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.
of_platform_bus_probe() is old code used by most of the powerpc
platforms. Don't use it for new board support.
g.