Re: [PATCH 5/5] ASoC: soc.h: don't create dummy Component via COMP_DUMMY()

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 12/4/2023 2:31 AM, Kuninori Morimoto wrote:
> Many ASoC drivers define CPU/Codec/Platform dai_link by below macro.
> 
> 	SND_SOC_DAILINK_DEFS(link,
> (A)		     DAILINK_COMP_ARRAY(COMP_CPU("cpu_dai")),
> (B)		     DAILINK_COMP_ARRAY(COMP_CODEC("codec", "dai1"),
> (B)					COMP_CODEC("codec", "dai2")),
> (C)		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> 
> In this case, this macro will be converted to like below
> 
> 	[o] = static struct snd_soc_dai_link_component
> 
> (A)	[o] link_cpus[]      = {{ .dai_name = "cpu_dai" }};
> (B)	[o] link_codecs[]    = {{ .dai_name = "dai1", .name = "codec" },
> 				{ .dai_name = "dai2", .name = "codec" }}
> (C)	[o] link_platforms[] = {{ }};
> 
> CPU and Codec info will be filled by COMP_CPU() / COMP_CODEC (= A,B),
> and Platform will have empty data by COMP_EMPTY() (= C) in this case.
> 
> Platform empty info will be filled when driver probe()
> (most of case, CPU info will be copied to use soc-generic-dmaengine-pcm).
> 
> For example in case of DPCM FE/BE, it will be like below.
> Codec will be dummy Component / DAI in this case (X).
> 
> 	SND_SOC_DAILINK_DEFS(link,
> 		     DAILINK_COMP_ARRAY(COMP_CPU(...)),
> (X)		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
> 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> 
> (X) part will converted like below
> 
> 	[o] link_codecs[]    = {{ .name = "snd-soc-dummy",
> 				  .dai_name = "snd-soc-dummy-dai", }}
> 
> Even though we already have common asoc_dummy_dlc for dummy
> Component / DAI, this macro will re-create new dummy dlc.
> Some drivers defines many dai_link info via SND_SOC_DAILINK_DEFS(),
> this means many dummy dlc also will be re-created. This is waste of
> memory.
> 
> If we can use existing common asoc_dummy_dlc at (X),
> we can avoid to re-creating dummy dlc, then, we can save the memory.
> 
> At that time, we want to keep existing code as much as possible, because
> too many drivers are using this macro. But because of its original style,
> using common asoc_dummy_dlc from it is very difficult or impossible.
> 
> So let's change the mind. The macro is used like below
> 
> 	SND_SOC_DAILINK_DEFS(link,
> 		     DAILINK_COMP_ARRAY(COMP_CPU(...)),
> (x)		     DAILINK_COMP_ARRAY(COMP_DUMMY()),
> 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> 
> 	static struct snd_soc_dai_link dai_links[] = {
> 	{
> 		.name = ...,
> 		.stream_name = ...,
> (y)		SND_SOC_DAILINK_REG(link),
> 	},
> 
> (y) part will be like below
> 
>          static struct snd_soc_dai_link dai_links[] = {
>          {
>                  .name = ...,
>                  .stream_name = ...,
>   ^		...
>   |		.codecs		= link_codecs,
> (y)		.num_codecs	= ARRAY_SIZE(link_codecs),
>   v		...
> 	}
> 
> This patch try to use trick on COMP_DUMMY()
> 
> -	#define COMP_DUMMY()	{ .name = "snd-soc-dummy", .dai_name = "snd-soc-dummy-dai", }
> +	#define COMP_DUMMY()
> 
> By this tric, (x) part will be like below.
> 
> before
> 	[o] link_codecs[] = {{ .name = "snd-soc-dummy", .dai_name = "snd-soc-dummy-dai", }}
> after
> 	[o] link_codecs[] = { };
> 
> This is same as below
> 
> 	[o] link_codecs[0];
> 
> This means it has pointer (link_codecs), but the array size is 0.
> (y) part will be like below.
> 
> 	static struct snd_soc_dai_link dai_links[] = {
> 	{
> 		...
> 		.codecs		= link_codecs,
> 		.num_codecs	= 0,
> 		...
> 	},
> 
> This is very special settings that normal use usually not do,
> but new macro do.
> We can find this special settings on soc-core.c and fill it as
> "dummy DAI" (= asoc_dummy_dlc). By this tric, we can avoid to re-create
> dummy dlc and save the memory.
> 
> This patch add tric at COMP_DUMMY() and add snd_soc_fill_dummy_dai()
> to fill dummy DAI.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   include/sound/soc.h  |  2 +-
>   sound/soc/soc-core.c | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index f3803c2dc349..7cbe85ca040d 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -938,7 +938,7 @@ snd_soc_link_to_platform(struct snd_soc_dai_link *link, int n) {
>   #define COMP_PLATFORM(_name)		{ .name = _name }
>   #define COMP_AUX(_name)			{ .name = _name }
>   #define COMP_CODEC_CONF(_name)		{ .name = _name }
> -#define COMP_DUMMY()			{ .name = "snd-soc-dummy", .dai_name = "snd-soc-dummy-dai", }
> +#define COMP_DUMMY()			/* see snd_soc_fill_dummy_dai() */

Isn't it effectively making COMP_DUMMY same as COMP_EMPTY, or am I not 
seeing something? I guess next step could be to just remove all 
COMP_DUMMY and replace them with COMP_EMPTY to avoid two definitions 
which are same thing?

>   
>   extern struct snd_soc_dai_link_component null_dailink_component[0];
>   extern struct snd_soc_dai_link_component snd_soc_dummy_dlc;
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 132946f82a29..88de4c5a376f 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -576,6 +576,34 @@ static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
>   	return NULL;
>   }
>   
> +static void snd_soc_fill_dummy_dai(struct snd_soc_card *card)
> +{
> +	struct snd_soc_dai_link *dai_link;
> +	int i;
> +
> +	/*
> +	 * COMP_DUMMY() creates size 0 array for CPU/Codec on dai_link.
> +	 * This function fill it as dummy DAI.
> +	 *
> +	 * size = 0, but has pointer means created by COMP_DUMMY()
> +	 */
> +	for_each_card_prelinks(card, i, dai_link) {
> +		if (dai_link->num_cpus == 0 && dai_link->cpus) {
> +			dai_link->num_cpus	= 1;
> +			dai_link->cpus		= &snd_soc_dummy_dlc;
> +		}
> +		if (dai_link->num_codecs == 0 && dai_link->codecs) {
> +			dai_link->num_codecs	= 1;
> +			dai_link->codecs	= &snd_soc_dummy_dlc;
> +		}
> +		if (dai_link->num_platforms == 0 && dai_link->platforms) {
> +			dev_warn(card->dev, "platform don't need dummy Component/DAI\n");

I would just replace above print with code comment, no need to spam dmesg.

> +			dai_link->num_platforms	= 0;
> +			dai_link->platforms	= NULL;
> +		}
> +	}
> +}
> +
>   static void snd_soc_flush_all_delayed_work(struct snd_soc_card *card)
>   {
>   	struct snd_soc_pcm_runtime *rtd;
> @@ -2131,6 +2159,8 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
>   	mutex_lock(&client_mutex);
>   	snd_soc_card_mutex_lock_root(card);
>   
> +	snd_soc_fill_dummy_dai(card);
> +
>   	snd_soc_dapm_init(&card->dapm, card, NULL);
>   
>   	/* check whether any platform is ignore machine FE and using topology */