Skip to contentSkip to conversation

ASoC: simple-card: DT fix and multi DAI links extension

PATCH Superseded Jean-Francois Moine 3 participants 10 messages Feb 19, 2014 – Feb 25, 2014

Subsystems (4)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover ASoC: simple-card: DT fix and multi DAI links extension

This patch series fixes a small problem about node device locks and also extends the simple card driver to handle many DAI links as this exists in the Cubox audio subsystem.

Jean-Francois Moine (4):
ASoC: simple-card: Fix device node locks
ASoC: simple-card: dynamically allocate the DAI link array
ASoC: simple-card: accept many DAI links
ASoC: simple-card: add DT documentation for multi-DAI links

 .../devicetree/bindings/sound/simple-card.txt      |  34 ++++-
 sound/soc/generic/simple-card.c                    | 148 +++++++++++++++------
 2 files changed, 137 insertions(+), 45 deletions(-)
Signature · 6 lines
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#1 lore raw

PATCH 1/4 ASoC: simple-card: Fix device node locks

Some device nodes stay locked and some other ones are not locked while being used during the card lifetime.

---
 sound/soc/generic/simple-card.c | 51 +++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 12 deletions(-)
sound/soc/generic/simple-card.c +39 -12
index 4fe8abc..8809ab4 100644
@@ -92,7 +92,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	/* get dai->name */
 	ret = snd_soc_of_get_dai_name(np, name);
 	if (ret < 0)
-		goto parse_error;
+		return ret;
 
 	/*
 	 * bitclock-inversion, frame-inversion
@@ -112,7 +112,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 		clk = of_clk_get(np, 0);
 		if (IS_ERR(clk)) {
 			ret = PTR_ERR(clk);
-			goto parse_error;
+			return ret;
 		}
 
 		dai->sysclk = clk_get_rate(clk);
@@ -126,12 +126,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 			dai->sysclk = clk_get_rate(clk);
 	}
 
-	ret = 0;
-
-parse_error:
-	of_node_put(node);
-
-	return ret;
+	return 0;
 }
 
 static int asoc_simple_card_parse_of(struct device_node *node,
@@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	/* CPU sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
+		of_node_put(np);
+	}
 	if (ret < 0)
 		return ret;
 
 	/* CODEC sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,codec");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->codec_dai,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
+		of_node_put(np);
+	}
 	if (ret < 0)
 		return ret;
 
@@ -219,6 +218,26 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	return 0;
 }
 
+static int asoc_simple_card_purge(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct snd_soc_dai_link *dai_link;
+	struct device_node *np;
+	int num_links;
+
+	for (num_links = 0, dai_link = card->dai_link;
+	     num_links < card->num_links;
+	     num_links++, dai_link++) {
+		np = (struct device_node *) dai_link->cpu_of_node;
+		if (np)
+			of_node_put(np);
+		np = (struct device_node *) dai_link->codec_of_node;
+		if (np)
+			of_node_put(np);
+	}
+	return 0;
+}
+
 static int asoc_simple_card_probe(struct platform_device *pdev)
 {
 	struct simple_card_data *priv;
@@ -246,7 +265,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "parse error %d\n", ret);
-			return ret;
+			goto err;
 		}
 	} else {
 		struct asoc_simple_card_info *cinfo;
@@ -287,7 +306,14 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	snd_soc_card_set_drvdata(&priv->snd_card, priv);
 
-	return devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
+	ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
+	if (ret < 0)
+		goto err;
+	return 0;
+
+err:
+	asoc_simple_card_purge(pdev);
+	return ret;
 }
 
 static const struct of_device_id asoc_simple_of_match[] = {
@@ -303,6 +329,7 @@ static struct platform_driver asoc_simple_card = {
 		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
+	.remove		= asoc_simple_card_purge,
 };
 
 module_platform_driver(asoc_simple_card);
Signature · 1 line
1.9.0
#2 lore raw

RE: [PATCH 1/4] ASoC: simple-card: Fix device node locks

sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node · line 173

Quoted 12 lines
sound/soc/generic/simple-card.c +2 -1
@@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node
 *node,
 	/* CPU sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
+		of_node_put(np);

Does the of_node_put(np) is really needed here ?

Quoted 4 lines
sound/soc/generic/simple-card.c +1 -0
+	}
 	if (ret < 0)
 		return ret;
 

sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 185

Quoted 10 lines
sound/soc/generic/simple-card.c +2 -1
 	/* CODEC sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,codec");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->codec_dai,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
+		of_node_put(np);

The same here...

sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 175

Quoted 4 lines
sound/soc/generic/simple-card.c +1 -0
+	}
 	if (ret < 0)
 		return ret;

Thanks,

--
Best Regards,
Xiubo

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

Re: [PATCH 1/4] ASoC: simple-card: Fix device node locks

Quoted 6 lines · On Mon, 24 Feb 2014 02:17:00 +0000 "[email protected]"…
Quoted 6 lines
sound/soc/generic/simple-card.c +0 -1
@@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node
 *node,
 	/* CPU sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np)
Quoted 8 lines

sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 172

Quoted 6 lines
sound/soc/generic/simple-card.c +2 -0
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
+		of_node_put(np);

Does the of_node_put(np) is really needed here ?

	[snip]

Yes, of_get_child_by_name() increments the node refcount and np is not used afterwards.

But, you are right, this creates a bug in the next patch when using of_get_next_child(). I will fix it.

Thanks.

Signature · 2 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
maintainer
#4 lore raw

Re: [PATCH 1/4] ASoC: simple-card: Fix device node locks

In reply to Jean-Francois Moine
Quoted 2 lines · Jean-Francois Moine

Some device nodes stay locked and some other ones are not locked while being used during the card lifetime.

Please pay more attention to describing your patches clearly in changelogs and to splitting them up for review. This is a frequent issue and it does make it much harder to check what's happening.

In this case you're not actually working with locking but rather with reference counting and there's no real description of the actual errors either. Saying something like "function X takes a reference which we need to drop" would help a lot, and it would let us think about if it's sensible for the function to return holding the reference in the first place and how long the reference needs to be held for.

#5 lore raw

PATCH 2/4 ASoC: simple-card: dynamically allocate the DAI link array

In reply to Jean-Francois Moine

The DAI link array is hard-coded as a single CPU / CODEC DAIs link.

This patch allocates this array with the card definition and facilitates handling more links.

---
 sound/soc/generic/simple-card.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
sound/soc/generic/simple-card.c +4 -3
index 8809ab4..a75a8bb 100644
@@ -20,7 +20,6 @@ struct simple_card_data {
 	unsigned int daifmt;
 	struct asoc_simple_dai cpu_dai;
 	struct asoc_simple_dai codec_dai;
-	struct snd_soc_dai_link snd_link;
 };
 
 static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
@@ -246,7 +245,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev,
+			sizeof(*priv) + sizeof(*dai_link),
+			GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -255,7 +256,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	 */
 	priv->snd_card.owner = THIS_MODULE;
 	priv->snd_card.dev = dev;
-	dai_link = &priv->snd_link;
+	dai_link = (struct snd_soc_dai_link *) (priv + 1);
 	priv->snd_card.dai_link = dai_link;
 	priv->snd_card.num_links = 1;
 
Signature · 1 line
1.9.0
#6 lore raw

RE: [PATCH 2/4] ASoC: simple-card: dynamically allocate the DAI link array

Quoted 11 lines
sound/soc/generic/simple-card.c +0 -1
@@ -20,7 +20,6 @@ struct simple_card_data {
 	unsigned int daifmt;
 	struct asoc_simple_dai cpu_dai;
 	struct asoc_simple_dai codec_dai;
-	struct snd_soc_dai_link snd_link;
 };
 
 static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,

sound/soc/generic/simple-card.c static int asoc_simple_card_probe(struct platform_device · line 250

Quoted 7 lines
sound/soc/generic/simple-card.c +2 -1
@@ -246,7 +245,9 @@ static int asoc_simple_card_probe(struct platform_device
 *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev,
+			sizeof(*priv) + sizeof(*dai_link),

This is okey for me.

Well, how about splitting the *priv and *dai_link into two separated memory blocks? As we can get the dai-link pointer via priv->snd_card.dai_link in other places.

IMHO, then the code will be much more simplifier and readable.

Just for one suggestion.

Thanks,

--
Best regards,
Xiubo

Quoted 18 lines
sound/soc/generic/simple-card.c +2 -2
+			GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -255,7 +256,7 @@ static int asoc_simple_card_probe(struct platform_device
 *pdev)
 	 */
 	priv->snd_card.owner = THIS_MODULE;
 	priv->snd_card.dev = dev;
-	dai_link = &priv->snd_link;
+	dai_link = (struct snd_soc_dai_link *) (priv + 1);
 	priv->snd_card.dai_link = dai_link;
 	priv->snd_card.num_links = 1;
 
--

1.9.0

#7 lore raw

Re: [alsa-devel] [PATCH 2/4] ASoC: simple-card: dynamically allocate the DAI link array

Quoted 20 lines · On Mon, 24 Feb 2014 03:32:02 +0000 "[email protected]"…
Quoted 10 lines
sound/soc/generic/simple-card.c +0 -1
@@ -20,7 +20,6 @@ struct simple_card_data {
 	unsigned int daifmt;
 	struct asoc_simple_dai cpu_dai;
 	struct asoc_simple_dai codec_dai;
-	struct snd_soc_dai_link snd_link;
 };
 
 static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,

sound/soc/generic/simple-card.c static int asoc_simple_card_probe(struct platform_device · line 250

Quoted 7 lines
sound/soc/generic/simple-card.c +2 -1
@@ -246,7 +245,9 @@ static int asoc_simple_card_probe(struct platform_device
 *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev,
+			sizeof(*priv) + sizeof(*dai_link),

This is okey for me.

Quoted 5 lines

Well, how about splitting the *priv and *dai_link into two separated memory blocks? As we can get the dai-link pointer via priv->snd_card.dai_link in other places.

IMHO, then the code will be much more simplifier and readable.

It is just a simple optimization: less calls to memory allocation and less code (also, less TLB reload?). I will add more comments.

Signature · 2 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
#8 lore raw

PATCH 3/4 ASoC: simple-card: accept many DAI links

In reply to Jean-Francois Moine

Some simple audio cards may have many DAI links. This patch makes them handled by the simple-card driver.

---
 sound/soc/generic/simple-card.c | 100 ++++++++++++++++++++++++++--------------
 1 file changed, 66 insertions(+), 34 deletions(-)
sound/soc/generic/simple-card.c +66 -34
index a75a8bb..6e0e580 100644
@@ -93,6 +93,9 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	if (ret < 0)
 		return ret;
 
+	if (!dai)
+		return 0;
+
 	/*
 	 * bitclock-inversion, frame-inversion
 	 * bitclock-master,    frame-master
@@ -135,7 +138,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
 	struct device_node *np;
 	char *name;
-	int ret;
+	int first_link, ret;
 
 	/* parsing the card name from DT */
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
@@ -160,50 +163,67 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 			return ret;
 	}
 
-	/* CPU sub-node */
-	ret = -EINVAL;
-	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np) {
+	/* loop on the DAI links */
+	np = NULL;
+	first_link = 1;
+	for (;;) {
+		np = of_get_next_child(node, np);
+		if (!np)
+			break;
+
+		/* CPU sub-node */
+		if (strcmp(np->name, "simple-audio-card,cpu") != 0) {
+			dev_err(dev, "Bad CPU DAI\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
-						  &priv->cpu_dai,
+					first_link ? &priv->cpu_dai : NULL,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
 		of_node_put(np);
-	}
-	if (ret < 0)
-		return ret;
+		if (ret < 0)
+			return ret;
 
-	/* CODEC sub-node */
-	ret = -EINVAL;
-	np = of_get_child_by_name(node, "simple-audio-card,codec");
-	if (np) {
+		/* CODEC sub-node */
+		np = of_get_next_child(node, np);
+		if (strcmp(np->name, "simple-audio-card,codec") != 0) {
+			dev_err(dev, "Bad CODEC DAI\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
-						  &priv->codec_dai,
+					first_link ? &priv->codec_dai : NULL,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
 		of_node_put(np);
-	}
-	if (ret < 0)
-		return ret;
+		if (ret < 0)
+			return ret;
+
+		if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name)
+			return -EINVAL;
 
-	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name)
-		return -EINVAL;
+		/* simple-card assumes platform == cpu */
+		dai_link->platform_of_node = dai_link->cpu_of_node;
+
+		name = devm_kzalloc(dev,
+				    strlen(dai_link->cpu_dai_name)   +
+				    strlen(dai_link->codec_dai_name) + 2,
+				    GFP_KERNEL);
+		sprintf(name, "%s-%s", dai_link->cpu_dai_name,
+					dai_link->codec_dai_name);
+		dai_link->name = dai_link->stream_name = name;
+
+		dai_link++;
+		first_link = 0;
+	}
 
 	/* card name is created from CPU/CODEC dai name */
-	name = devm_kzalloc(dev,
-			    strlen(dai_link->cpu_dai_name)   +
-			    strlen(dai_link->codec_dai_name) + 2,
-			    GFP_KERNEL);
-	sprintf(name, "%s-%s", dai_link->cpu_dai_name,
-				dai_link->codec_dai_name);
+	dai_link = priv->snd_card.dai_link;
 	if (!priv->snd_card.name)
-		priv->snd_card.name = name;
-	dai_link->name = dai_link->stream_name = name;
-
-	/* simple-card assumes platform == cpu */
-	dai_link->platform_of_node = dai_link->cpu_of_node;
+		priv->snd_card.name = dai_link->name;
 
-	dev_dbg(dev, "card-name : %s\n", name);
+	dev_dbg(dev, "card-name : %s\n", priv->snd_card.name);
 	dev_dbg(dev, "platform : %04x\n", priv->daifmt);
 	dev_dbg(dev, "cpu : %s / %04x / %d\n",
 		dai_link->cpu_dai_name,
@@ -243,10 +263,22 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	struct snd_soc_dai_link *dai_link;
 	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
-	int ret;
+	int num_links, ret;
+
+	/* get the number of DAI links */
+	if (np) {
+		num_links = of_get_child_count(np);
+		if (num_links == 0 || (num_links & 1)) {
+			dev_err(&pdev->dev, "Bad number of DAI links\n");
+			return -EINVAL;
+		}
+		num_links /= 2;
+	} else {
+		num_links = 1;
+	}
 
 	priv = devm_kzalloc(dev,
-			sizeof(*priv) + sizeof(*dai_link),
+			sizeof(*priv) + sizeof(*dai_link) * num_links,
 			GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -258,7 +290,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	priv->snd_card.dev = dev;
 	dai_link = (struct snd_soc_dai_link *) (priv + 1);
 	priv->snd_card.dai_link = dai_link;
-	priv->snd_card.num_links = 1;
+	priv->snd_card.num_links = num_links;
 
 	if (np && of_device_is_available(np)) {
 
Signature · 1 line
1.9.0
#9 lore raw

PATCH 4/4 ASoC: simple-card: add DT documentation for multi-DAI links

In reply to Jean-Francois Moine

There may be many couples of CPU/CODEC DAI links.
The example 2 is extracted from the Cubox DT.

---
 .../devicetree/bindings/sound/simple-card.txt      | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/sound/simple-card.txt +33 -1
index 0527358..60306bf 100644
@@ -24,6 +24,9 @@ Required subnodes:
 - simple-audio-card,cpu			: CPU   sub-node
 - simple-audio-card,codec		: CODEC sub-node
 
+  There may be one or many couples (simple-audio-card,cpu, simple-audio-card,codec)
+  (see example 2).
+
 Required CPU/CODEC subnodes properties:
 
 - sound-dai				: phandle and port of CPU/CODEC
@@ -41,7 +44,7 @@ Optional CPU/CODEC subnodes properties:
 					  clock node (= common clock), or "system-clock-frequency"
 					  (if system doens't support common clock)
 
-Example:
+Example 1:
 
 sound {
 	compatible = "simple-audio-card";
@@ -83,3 +86,32 @@ sh_fsi2: sh_fsi2@ec230000 {
 	interrupt-parent = <&gic>;
 	interrupts = <0 146 0x4>;
 };
+
+Example 2:
+
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,name = "Cubox Audio";
+
+	simple-audio-card,cpu@0 {		/* I2S - HDMI */
+		sound-dai = <&audio1 0>;
+		format = "i2s";
+	};
+	simple-audio-card,codec@0 {
+		sound-dai = <&tda998x 0>;
+	};
+
+	simple-audio-card,cpu@1 {		/* S/PDIF - HDMI */
+		sound-dai = <&audio1 1>;
+	};
+	simple-audio-card,codec@1 {
+		sound-dai = <&tda998x 1>;
+	};
+
+	simple-audio-card,cpu@2 {		/* S/PDIF - S/PDIF */
+		sound-dai = <&audio1 1>;
+	};
+	simple-audio-card,codec@2 {
+		sound-dai = <&spdif_codec>;
+	};
+};
Signature · 1 line
1.9.0