diff mbox series

[v2,114/146] ASoC: soc-topology: use modern dai_link style

Message ID 87v9xjfjgo.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show
Series ASoC: modern dai_link style support | expand

Commit Message

Kuninori Morimoto June 6, 2019, 4:19 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

ASoC is now supporting modern style dai_link
(= snd_soc_dai_link_component) for CPU/Codec/Platform.
This patch switches to use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-topology.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Amadeusz Sławiński June 6, 2019, 1:17 p.m. UTC | #1 
On 06 Jun 2019 13:19:14 +0900
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> ASoC is now supporting modern style dai_link
> (= snd_soc_dai_link_component) for CPU/Codec/Platform.
> This patch switches to use it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-topology.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 3299ebb..f485f7f 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -560,7 +560,7 @@ static void remove_link(struct snd_soc_component
> *comp, 
>  	kfree(link->name);
>  	kfree(link->stream_name);
> -	kfree(link->cpu_dai_name);
> +	kfree(link->cpus->dai_name);
>  
>  	list_del(&dobj->list);
>  	snd_soc_remove_dai_link(comp->card, link);
> @@ -1879,12 +1879,22 @@ static int soc_tplg_fe_link_create(struct
> soc_tplg *tplg, struct snd_soc_tplg_pcm *pcm)
>  {
>  	struct snd_soc_dai_link *link;
> +	struct snd_soc_dai_link_component *dlc;
>  	int ret;
>  
> -	link = kzalloc(sizeof(struct snd_soc_dai_link), GFP_KERNEL);
> +	/* link + cpu + codec */
> +	link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)),
> GFP_KERNEL); if (link == NULL)
>  		return -ENOMEM;
>  
> +	dlc = (struct snd_soc_dai_link_component *)(link + 1);
> +
> +	link->cpus	= &dlc[0];
> +	link->codecs	= &dlc[1];

While I understand what is going on here, I find this bit ugly.
Can it perhaps be changed to something like:

	link = kzalloc(sizeof(*link), GFP_KERNEL);
	if (link == NULL)
		return -ENOMEM;
	link->cpus = kzalloc(sizeof(*dlc), GFP_KERNEL);
	if (link->cpus == NULL) {
		ret = -ENOMEM;
		goto err;
	}
	link->codecs = kzalloc(sizeof(*dlc), GFP_KERNEL);
	if (link->cpus == NULL) {
		ret = -ENOMEM;
		goto err;
	}

(...)
err:
	kfree(link->cpus);
	kfree(link);
	return ret;

While it has a bit more of boiler plate it's easier to follow.

And while I look at soc_tplg_fe_link_create() it could use some
more memory checks, but it's a topic for separate patch.

> +
> +	link->num_cpus	 = 1;
> +	link->num_codecs = 1;
> +
>  	if (strlen(pcm->pcm_name)) {
>  		link->name = kstrdup(pcm->pcm_name, GFP_KERNEL);
>  		link->stream_name = kstrdup(pcm->pcm_name,
> GFP_KERNEL); @@ -1892,10 +1902,10 @@ static int
> soc_tplg_fe_link_create(struct soc_tplg *tplg, link->id =
> le32_to_cpu(pcm->pcm_id); 
>  	if (strlen(pcm->dai_name))
> -		link->cpu_dai_name = kstrdup(pcm->dai_name,
> GFP_KERNEL);
> +		link->cpus->dai_name = kstrdup(pcm->dai_name,
> GFP_KERNEL); 
> -	link->codec_name = "snd-soc-dummy";
> -	link->codec_dai_name = "snd-soc-dummy-dai";
> +	link->codecs->name = "snd-soc-dummy";
> +	link->codecs->dai_name = "snd-soc-dummy-dai";
>  
>  	/* enable DPCM */
>  	link->dynamic = 1;
> @@ -1912,7 +1922,7 @@ static int soc_tplg_fe_link_create(struct
> soc_tplg *tplg, dev_err(tplg->comp->dev, "ASoC: FE link loading
> failed\n"); kfree(link->name);
>  		kfree(link->stream_name);
> -		kfree(link->cpu_dai_name);
> +		kfree(link->cpus->dai_name);
>  		kfree(link);
>  		return ret;
>  	}
Pierre-Louis Bossart June 6, 2019, 2:03 p.m. UTC | #2 
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index 3299ebb..f485f7f 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -560,7 +560,7 @@ static void remove_link(struct snd_soc_component
>> *comp,
>>   	kfree(link->name);
>>   	kfree(link->stream_name);
>> -	kfree(link->cpu_dai_name);
>> +	kfree(link->cpus->dai_name);
>>   
>>   	list_del(&dobj->list);
>>   	snd_soc_remove_dai_link(comp->card, link);
>> @@ -1879,12 +1879,22 @@ static int soc_tplg_fe_link_create(struct
>> soc_tplg *tplg, struct snd_soc_tplg_pcm *pcm)
>>   {
>>   	struct snd_soc_dai_link *link;
>> +	struct snd_soc_dai_link_component *dlc;
>>   	int ret;
>>   
>> -	link = kzalloc(sizeof(struct snd_soc_dai_link), GFP_KERNEL);
>> +	/* link + cpu + codec */
>> +	link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)),
>> GFP_KERNEL); if (link == NULL)
>>   		return -ENOMEM;
>>   
>> +	dlc = (struct snd_soc_dai_link_component *)(link + 1);
>> +
>> +	link->cpus	= &dlc[0];
>> +	link->codecs	= &dlc[1];
> 
> While I understand what is going on here, I find this bit ugly.

It's not so bad and it avoid multiple tests and tags that are just as 
ugly IMHO.

> Can it perhaps be changed to something like:
> 
> 	link = kzalloc(sizeof(*link), GFP_KERNEL);
> 	if (link == NULL)
> 		return -ENOMEM;
> 	link->cpus = kzalloc(sizeof(*dlc), GFP_KERNEL);
> 	if (link->cpus == NULL) {
> 		ret = -ENOMEM;
> 		goto err;
> 	}
> 	link->codecs = kzalloc(sizeof(*dlc), GFP_KERNEL);
> 	if (link->cpus == NULL) {
> 		ret = -ENOMEM;
> 		goto err;
> 	}
> 
> (...)
> err:
> 	kfree(link->cpus);
> 	kfree(link);
> 	return ret;
> 
> While it has a bit more of boiler plate it's easier to follow.
> 
> And while I look at soc_tplg_fe_link_create() it could use some
> more memory checks, but it's a topic for separate patch.
Kuninori Morimoto June 7, 2019, 12:27 a.m. UTC | #3 
Hi Amadeusz

Thank you for your feedback

> >> +	/* link + cpu + codec */
> >> +	link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)),
> >> GFP_KERNEL); if (link == NULL)
> >>   		return -ENOMEM;
> >>   +	dlc = (struct snd_soc_dai_link_component *)(link + 1);
> >> +
> >> +	link->cpus	= &dlc[0];
> >> +	link->codecs	= &dlc[1];
> > 
> > While I understand what is going on here, I find this bit ugly.
> 
> It's not so bad and it avoid multiple tests and tags that are just as
> ugly IMHO.

Yeah, it is using a little bit tricky method.

> > Can it perhaps be changed to something like:
> > 
> > 	link = kzalloc(sizeof(*link), GFP_KERNEL);
> > 	if (link == NULL)
> > 		return -ENOMEM;
> > 	link->cpus = kzalloc(sizeof(*dlc), GFP_KERNEL);
> > 	if (link->cpus == NULL) {
> > 		ret = -ENOMEM;
> > 		goto err;
> > 	}
> > 	link->codecs = kzalloc(sizeof(*dlc), GFP_KERNEL);
> > 	if (link->cpus == NULL) {
> > 		ret = -ENOMEM;
> > 		goto err;
> > 	}

From "logic" point of view, I think this patch has no bug,
but from "readable code" point of view,
I have no objection to exchange like above.

I'm not familiar with this driver,
so, can you handle it by incremental patch if you want?

Thank you for your help !!
Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 3299ebb..f485f7f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -560,7 +560,7 @@  static void remove_link(struct snd_soc_component *comp,
 
 	kfree(link->name);
 	kfree(link->stream_name);
-	kfree(link->cpu_dai_name);
+	kfree(link->cpus->dai_name);
 
 	list_del(&dobj->list);
 	snd_soc_remove_dai_link(comp->card, link);
@@ -1879,12 +1879,22 @@  static int soc_tplg_fe_link_create(struct soc_tplg *tplg,
 	struct snd_soc_tplg_pcm *pcm)
 {
 	struct snd_soc_dai_link *link;
+	struct snd_soc_dai_link_component *dlc;
 	int ret;
 
-	link = kzalloc(sizeof(struct snd_soc_dai_link), GFP_KERNEL);
+	/* link + cpu + codec */
+	link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL);
 	if (link == NULL)
 		return -ENOMEM;
 
+	dlc = (struct snd_soc_dai_link_component *)(link + 1);
+
+	link->cpus	= &dlc[0];
+	link->codecs	= &dlc[1];
+
+	link->num_cpus	 = 1;
+	link->num_codecs = 1;
+
 	if (strlen(pcm->pcm_name)) {
 		link->name = kstrdup(pcm->pcm_name, GFP_KERNEL);
 		link->stream_name = kstrdup(pcm->pcm_name, GFP_KERNEL);
@@ -1892,10 +1902,10 @@  static int soc_tplg_fe_link_create(struct soc_tplg *tplg,
 	link->id = le32_to_cpu(pcm->pcm_id);
 
 	if (strlen(pcm->dai_name))
-		link->cpu_dai_name = kstrdup(pcm->dai_name, GFP_KERNEL);
+		link->cpus->dai_name = kstrdup(pcm->dai_name, GFP_KERNEL);
 
-	link->codec_name = "snd-soc-dummy";
-	link->codec_dai_name = "snd-soc-dummy-dai";
+	link->codecs->name = "snd-soc-dummy";
+	link->codecs->dai_name = "snd-soc-dummy-dai";
 
 	/* enable DPCM */
 	link->dynamic = 1;
@@ -1912,7 +1922,7 @@  static int soc_tplg_fe_link_create(struct soc_tplg *tplg,
 		dev_err(tplg->comp->dev, "ASoC: FE link loading failed\n");
 		kfree(link->name);
 		kfree(link->stream_name);
-		kfree(link->cpu_dai_name);
+		kfree(link->cpus->dai_name);
 		kfree(link);
 		return ret;
 	}