On Thu, 25 May 2023 00:01:14 +0000
Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> Hi Herve
>
> Thank you for your reply.
>
> > So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
> > not correct as it has nothing to do with DAI links and must be call once
> > per Card.
>
> My biggest concern is that this code is calling same code multiple times.
> It is easy to forget such thing when updating in this kind of code.
> We don't forget / take mistake if these are merged.
> But we have such code everywhere ;) this is just my concern, not a big deal.
>
> static int __simple_for_each_link (...)
> {
> ...
> => add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
> ...
> }
>
> static int simple_populate_aux(...)
> {
> ...
> => node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");
> ...
> }
>
Well, of_get_child_by_name() is called twice to retrieve the additional-devs
node but for very different reason.
In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI.
In simple_populate_aux() to take care of the devices declared in the node.
I am not sure that we should avoid that.
It will lead to a more complex code and flags just to avoid this call.
Not sure that it will be better.
__simple_for_each_link() is called multiple times and is supposed to look at links.
To avoid the of_get_child_by_name() filter-out call, __simple_for_each_link()
will look at link *and* populate devices calling simple_populate_aux().
And to do that correctly it will use a flag to be sure that simple_populate_aux()
was called only once.
In order to avoid some kind of duplication (at least the node name):
static struct device_node *simple_of_get_add_devs(struct device_node *node)
{
return of_get_child_by_name(node, PREFIX "additional-devs");
}
static int __simple_for_each_link (...)
{
...
=> add_devs = simple_of_get_add_devs(top);
...
}
static int simple_populate_aux(...)
{
...
=> node = simple_of_get_add_devs(dev->of_node);
...
}
Does it look better ?
Best regards,
Hervé
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
