Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Herve Codina]

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