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

The reference count of some device nodes is not correctly reset at end of card probe.

Subject: [PATCH v2 3/4] ASoC: simple-card: accept many DAI links

Some simple audio cards may have many DAI links.
This patch extends the simple-card driver for handling such cards.

Signed-off-by Jean-Francois Moine <[email protected]>

---
 sound/soc/generic/simple-card.c | 132 ++++++++++++++++++++++++++-------------

(quoted diff) +0 -1

-
 1 file changed, 85 insertions(+), 47 deletions(-)

sound/soc/generic/simple-card.c +4 -1

index 2710b52..8188c34 100644
@@ -107,6 +107,9 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	if (ret < 0)
 		return ret;
 
+	if (!dai)
+		return 0;
+
 	/* parse TDM slot */
 	ret = snd_soc_of_parse_tdm_slot(np, &dai->slots, &dai->slot_width);
 	if (ret)

@@ -154,7 +157,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 */

sound/soc/generic/simple-card.c +7 -4

 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");

@@ -179,50 +182,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;

sound/soc/generic/simple-card.c +34 -27

 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
-		of_node_put(np);
-	}
-	if (ret < 0)
-		return ret;
+		if (ret < 0)
+			goto err;
 
-	/* 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");
+			ret = -EINVAL;
+			goto err;
+		}
 		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)
+			goto err;
+
+		if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
+			ret = -EINVAL;
+			goto err;
+		}
 
-	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;
+		priv->snd_card.name = dai_link->name;
 
-	/* simple-card assumes platform == cpu */
-	dai_link->platform_of_node = dai_link->cpu_of_node;
-
-	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",

sound/soc/generic/simple-card.c +4 -11

 		dai_link->cpu_dai_name,

@@ -233,18 +253,11 @@ static int asoc_simple_card_parse_of(struct device_node
 *node,
 		priv->codec_dai.fmt,
 		priv->codec_dai.sysclk);
 
-	/*
-	 * soc_bind_dai_link() will check cpu name
-	 * after of_node matching if dai_link has cpu_dai_name.
-	 * but, it will never match if name was created by fmt_single_name()
-	 * remove cpu_dai_name to escape name matching.
-	 * see
-	 *	fmt_single_name()
-	 *	fmt_multiple_name()
-	 */
-	dai_link->cpu_dai_name = NULL;
-
 	return 0;
+
+err:
+	of_node_put(np);
+	return ret;
 }
 

/* update the reference count of the devices nodes at end of probe */

sound/soc/generic/simple-card.c +14 -2

@@ -274,10 +287,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)

sound/soc/generic/simple-card.c +14 -1

 		return -ENOMEM;

@@ -289,7 +314,7 @@ static int asoc_simple_card_probe(struct platform_device
 *pdev)
 	priv->snd_card.dev = dev;
 	dai_link = priv->dai_link;
 	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)) {

@@ -299,6 +324,19 @@ static int asoc_simple_card_probe(struct platform_device
 *pdev)
 				dev_err(dev, "parse error %d\n", ret);
 			goto err;
 		}
+
+		/*
+		 * soc_bind_dai_link() will check cpu name
+		 * after of_node matching if dai_link has cpu_dai_name.
+		 * but, it will never match if name was created by
 fmt_single_name()
+		 * remove cpu_dai_name to escape name matching.
+		 * see
+		 *	fmt_single_name()
+		 *	fmt_multiple_name()
+		 */
+		if (num_links == 1)
+			dai_link->cpu_dai_name = NULL;
+
 	} else {

struct asoc_simple_card_info *cinfo;

--
1.9.0

I'm not sure why only the first link needs parsing the DAIFMT ?

On my LS1 platform there are two CODECs and two CPU SAI deivces, SGTL5000 <---> SAI
CS42888 <---> ESAI

Could the many dai links feature support this case?

I my understanding is correct here, this patch will support the Board, in which board there maybe only one CODEC supporting many DAIs...

Some simple audio cards may have many DAI links.
This patch extends the simple-card driver for handling such cards.

Signed-off-by Jean-Francois Moine <[email protected]>

---

Quoted 5 lines · Jean-Francois Moine

Some simple audio cards may have many DAI links.
This patch extends the simple-card driver for handling such cards.

Signed-off-by Jean-Francois Moine <[email protected]>

---

[...]

Why can't you just use two simple-card instances in your setup?

You need to update the DT-binding document too when you are changing the binding. That way it would also be easier to follow what you are trying to accomplish here.

The sysclk and tdm properties appear to only work in the dais of the first dai-link and __asoc_simple_card_dai_init() will not be called for other dais at all. The DT-binding of this implementation would look pretty hairy to me.

Quoted 9 lines · On Fri, 14 Mar 2014 13:16:12 +0200 Jyri Sarha <[email protected]> wrote:

Quoted 5 lines · Jean-Francois Moine

Some simple audio cards may have many DAI links.
This patch extends the simple-card driver for handling such cards.

Signed-off-by Jean-Francois Moine <[email protected]>

---

[...]

Why can't you just use two simple-card instances in your setup?

My machine has only one audio device (kirkwood). This one has two outputs. The first one (I2S) is connected to the HDMI transmitter, and the other one (S/PDIF) is connected to both the HDMI transmitter and the S/PDIF connector. There can be only one playback stream, either via I2S, or S/PDIF, or both, with the final output either to HDMI or S/PDIF or both:

           /--> I2S -----+-> HDMI
platform -+             /
           \-> S/PDIF -+
                        \--> S/PDIF

DPCM permits to activate both I2S and S/PDIF, but it does not handle the format and rate constraints (both outputs are always activated with the platform constraints).

So, the actual solution I use is 3 DAI links (= 3 PCMs):
- HDMI via I2S
- HDMI via S/PDIF (no S/PDIF output)
- S/PDIF via S/PDIF (no HDMI output)

Quoted 3 lines

You need to update the DT-binding document too when you are changing the binding. That way it would also be easier to follow what you are trying to accomplish here.

Some people want to have 2 different patchs: one for the code and the other one for the DT change. Some other people like you want only one patch. Which is the right way?

Quoted 4 lines

The sysclk and tdm properties appear to only work in the dais of the first dai-link and __asoc_simple_card_dai_init() will not be called for other dais at all. The DT-binding of this implementation would look pretty hairy to me.

I will add an other dynamic structure for these properties.

Quoted 4 lines · Jean-Francois Moine

So, the actual solution I use is 3 DAI links (= 3 PCMs):
- HDMI via I2S
- HDMI via S/PDIF (no S/PDIF output)
- S/PDIF via S/PDIF (no HDMI output)

Maybe a HW specific machine-driver would serve your case better. Of course it would be great to have a generic implementation that can handle this case too, but I am not sure if it should be called a simple-card anymore :).

Quoted 7 lines

Quoted 3 lines

You need to update the DT-binding document too when you are changing the binding. That way it would also be easier to follow what you are trying to accomplish here.

Some people want to have 2 different patchs: one for the code and the other one for the DT change. Some other people like you want only one patch. Which is the right way?

I did not mean it should be in the same patch. Part of the same series is fine with me. It is up to maintainer to decide these things anyway. I would just like to compare the document with the implementation.