Re: [alsa-devel] [PATCH 04/21] ASoC: soc-core: rename soc_init_dai_link() to soc_dai_link_sanity_check()

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 10/10/19 8:19 PM, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis
> 
>>> -	for_each_card_prelinks(card, i, dai_link) {
>>> -		ret = soc_init_dai_link(card, dai_link);
>>> -		if (ret) {
>>> -			dev_err(card->dev, "ASoC: failed to init link %s: %d\n",
>>> -				dai_link->name, ret);
>>> -			mutex_unlock(&client_mutex);
>>> -			return ret;
>>> -		}
>>> -	}
>>
>> This part is difficult to understand.
>>
>> There were two calls to soc_init_dai_link(), here and [2] below.
>> Your patch removes the first call and the for loop, is this
>> intentional and needed?
> 
> soc_init_dai_link() is just sanity_check now.
> In my understanding, it is needed before soc_bind_dai_link().
> 
> Current code is like below.
> (1) and (2) are for care prelink:ed dai_link.
> (A) and (B) are for topology added new dai_link.
> and
> (1) is for (2)
> (A) is for (B)
> 
> 	int snd_soc_instantiate_card()
> 	{
> 		for_each_card_prelinks(...) {
> (1)			ret = soc_init_dai_link(...);
> 			...
> 		}
> 		...
> 		for_each_card_prelinks(...) {
> (2)			ret = soc_bind_dai_link(...);
> 			...
> 		}
> 		...
> 		for_each_card_links(...) {
> 			...
> (A)			ret = soc_init_dai_link(...);
> 			...
> (B)			ret = soc_bind_dai_link(...);
> 		}
> 	}
> 
> It is very confusing/verbose code for me.
> It can be more simple if we can call soc_init_dai_link()
> from soc_bind_dai_link().

ok, the explanations help, maye you can add them to the commit message 
to help explain the intent, e.g.

  Current code is like below.
  (1) and (2) are for care prelink:ed dai_link.
  (A) and (B) are for topology added new dai_link.
  and
  (1) is for (2)
  (A) is for (B)


  		for_each_card_prelinks(...) {
  (2)		 	int snd_soc_instantiate_card()
  	{
  		for_each_card_prelinks(...) {
  (1)			ret = soc_init_dai_link(...);
  			...
  		}
  		...	ret = soc_bind_dai_link(...);
  			...
  		}
  		...
  		for_each_card_links(...) {
  			...
  (A)			ret = soc_init_dai_link(...);
  			...
  (B)			ret = soc_bind_dai_link(...);
  		}


and the new code keeps the same flow/steps but collapses the two calls 
into one

  		for_each_card_prelinks(...) {
  (2)		 	int snd_soc_instantiate_card()
  	{
  		for_each_card_prelinks(...) {
  (1)			ret = soc_bind_dai_link(...);
  			...
  		}
  		...
  		for_each_card_links(...) {
  			
(A) (B)			ret = soc_bind_dai_link(...);
  		}

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel