[PATCH v5 0/4] ASoC: simple-card: Add multi-CODEC support

[PATCH v5 0/4] ASoC: simple-card: Add multi-CODEC support

[Jean-Francois Moine]

This patch set adds multi-CODEC support to the simple card.

v5: accept only "sound-dai" as the dai link property (Arnd Bergmann)
v4: (Mark Brown)
	- document the new core function
	- calculation of the number of codecs moved to the core function
	- split the patch about simple-card.c
	- less code changes
	- remove the cast of pointers
	- less changes in the DT binding example 2
v3: rebase on broonie git 2014/11/03
v2: accept list of CODECs in 'sound-dais' (Benoit Cousson)

Jean-Francois Moine (4):
  ASoC: core: add multi-codec support in DT
  ASoC: simple-card: Remove useless function argument
  ASoC: simple-card: add multi-CODECs in DT
  ASoC: simple-card: Remove useless check

 .../devicetree/bindings/sound/simple-card.txt      |  18 +---
 include/sound/soc.h                                |   3 +
 sound/soc/generic/simple-card.c                    |  87 ++++++++--------
 sound/soc/soc-core.c                               | 110 ++++++++++++++++++---
 4 files changed, 149 insertions(+), 69 deletions(-)

-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/4] ASoC: core: add multi-codec support in DT

[Jean-Francois Moine]

This patch exports a core function which handles the DT description
of multi-codec links (as: "sound-dai = <&hdmi 0>, <&spdif_codec>;")
and creates a CODEC component array in the DAI link.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 include/sound/soc.h  |   3 ++
 sound/soc/soc-core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 98 insertions(+), 15 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 13e1648..8426a4f 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1492,6 +1492,9 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 				     struct device_node **framemaster);
 int snd_soc_of_get_dai_name(struct device_node *of_node,
 			    const char **dai_name);
+int snd_soc_of_get_dai_link_codecs(struct device *dev,
+				   struct device_node *of_node,
+				   struct snd_soc_dai_link *dai_link);
 
 #include <sound/soc-dai.h>
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index cc52ea1..e2ed55d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3392,36 +3392,30 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt);
 
-int snd_soc_of_get_dai_name(struct device_node *of_node,
-			    const char **dai_name)
+static int snd_soc_get_dai_name(struct of_phandle_args *args,
+				const char **dai_name)
 {
 	struct snd_soc_component *pos;
-	struct of_phandle_args args;
-	int ret;
-
-	ret = of_parse_phandle_with_args(of_node, "sound-dai",
-					 "#sound-dai-cells", 0, &args);
-	if (ret)
-		return ret;
-
-	ret = -EPROBE_DEFER;
+	int ret = -EPROBE_DEFER;
 
 	mutex_lock(&client_mutex);
 	list_for_each_entry(pos, &component_list, list) {
-		if (pos->dev->of_node != args.np)
+		if (pos->dev->of_node != args->np)
 			continue;
 
 		if (pos->driver->of_xlate_dai_name) {
-			ret = pos->driver->of_xlate_dai_name(pos, &args, dai_name);
+			ret = pos->driver->of_xlate_dai_name(pos,
+							     args,
+							     dai_name);
 		} else {
 			int id = -1;
 
-			switch (args.args_count) {
+			switch (args->args_count) {
 			case 0:
 				id = 0; /* same as dai_drv[0] */
 				break;
 			case 1:
-				id = args.args[0];
+				id = args->args[0];
 				break;
 			default:
 				/* not supported */
@@ -3443,6 +3437,21 @@ int snd_soc_of_get_dai_name(struct device_node *of_node,
 		break;
 	}
 	mutex_unlock(&client_mutex);
+	return ret;
+}
+
+int snd_soc_of_get_dai_name(struct device_node *of_node,
+			    const char **dai_name)
+{
+	struct of_phandle_args args;
+	int ret;
+
+	ret = of_parse_phandle_with_args(of_node, "sound-dai",
+					 "#sound-dai-cells", 0, &args);
+	if (ret)
+		return ret;
+
+	ret = snd_soc_get_dai_name(&args, dai_name);
 
 	of_node_put(args.np);
 
@@ -3450,6 +3459,77 @@ int snd_soc_of_get_dai_name(struct device_node *of_node,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_get_dai_name);
 
+/*
+ * snd_soc_of_get_dai_link_codecs - Parse a list of CODECs in the devicetree
+ * @dev: Card device
+ * @of_node: Device node
+ * @dai_link: DAI link
+ *
+ * Builds an array of CODEC DAI components from the DAI link property
+ * 'sound-dai'.
+ * The array is set in the DAI link and the number of DAIs is set accordingly.
+ * The device nodes in the array (of_node) must be dereferenced by the caller.
+ *
+ * Returns 0 for success
+ */
+int snd_soc_of_get_dai_link_codecs(struct device *dev,
+				   struct device_node *of_node,
+				   struct snd_soc_dai_link *dai_link)
+{
+	struct of_phandle_args args;
+	struct snd_soc_dai_link_component *component;
+	char *name;
+	int index, num_codecs, ret;
+
+	/* Count the number of CODECs */
+	name = "sound-dai";
+	num_codecs = of_count_phandle_with_args(of_node, name,
+						"#sound-dai-cells");
+	if (num_codecs <= 0) {
+		if (num_codecs == -ENOENT)
+			dev_err(dev, "No 'sound-dai' property\n");
+		else
+			dev_err(dev, "Bad phandle in 'sound-dai'\n");
+		return num_codecs;
+	}
+	component = devm_kzalloc(dev,
+				 sizeof *component * num_codecs,
+				 GFP_KERNEL);
+	if (!component)
+		return -ENOMEM;
+	dai_link->codecs = component;
+	dai_link->num_codecs = num_codecs;
+
+	/* Parse the list */
+	for (index = 0, component = dai_link->codecs;
+	     index < dai_link->num_codecs;
+	     index++, component++) {
+		ret = of_parse_phandle_with_args(of_node, name,
+						 "#sound-dai-cells",
+						  index, &args);
+		if (ret)
+			goto err;
+		component->of_node = args.np;
+		ret = snd_soc_get_dai_name(&args, &component->dai_name);
+		if (ret < 0)
+			goto err;
+	}
+	return 0;
+err:
+	for (index = 0, component = dai_link->codecs;
+	     index < dai_link->num_codecs;
+	     index++, component++) {
+		if (!component->of_node)
+			break;
+		of_node_put(component->of_node);
+		component->of_node = NULL;
+	}
+	dai_link->codecs = NULL;
+	dai_link->num_codecs = 0;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_get_dai_link_codecs);
+
 static int __init snd_soc_init(void)
 {
 #ifdef CONFIG_DEBUG_FS
-- 
2.1.3

Re: [PATCH v5 1/4] ASoC: core: add multi-codec support in DT

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 282 bytes --]

On Tue, Nov 25, 2014 at 01:16:12PM +0100, Jean-Francois Moine wrote:
> This patch exports a core function which handles the DT description
> of multi-codec links (as: "sound-dai = <&hdmi 0>, <&spdif_codec>;")
> and creates a CODEC component array in the DAI link.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v5 2/4] ASoC: simple-card: Remove useless function argument

[Jean-Francois Moine]

The device node pointer 'cpu' is not used in the function
asoc_simple_card_parse_daifmt().

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 sound/soc/generic/simple-card.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 3e3fec4..ece22d5 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -232,7 +232,6 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 
 static int asoc_simple_card_parse_daifmt(struct device_node *node,
 					 struct simple_card_data *priv,
-					 struct device_node *cpu,
 					 struct device_node *codec,
 					 char *prefix, int idx)
 {
@@ -309,7 +308,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	}
 
 	ret = asoc_simple_card_parse_daifmt(node, priv,
-					    cpu, codec, prefix, idx);
+					    codec, prefix, idx);
 	if (ret < 0)
 		goto dai_link_of_err;
 
-- 
2.1.3

Re: [PATCH v5 2/4] ASoC: simple-card: Remove useless function argument

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

On Tue, Nov 25, 2014 at 01:22:45PM +0100, Jean-Francois Moine wrote:
> The device node pointer 'cpu' is not used in the function
> asoc_simple_card_parse_daifmt().

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v5 3/4] ASoC: simple-card: add multi-CODECs in DT

[Jean-Francois Moine]

This patch allows many CODECs per link to be defined in the device tree.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 .../devicetree/bindings/sound/simple-card.txt      | 18 ++---
 sound/soc/generic/simple-card.c                    | 81 ++++++++++++----------
 2 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index c3cba60..461abf0 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -65,7 +65,8 @@ properties should also be placed in the codec node if needed.
 
 Required CPU/CODEC subnodes properties:
 
-- sound-dai				: phandle and port of CPU/CODEC
+- sound-dai				: phandle and port of CPU
+					  or list of phandle and port of CODECs
 
 Optional CPU/CODEC subnodes properties:
 
@@ -119,7 +120,7 @@ sh_fsi2: sh_fsi2@ec230000 {
 	interrupts = <0 146 0x4>;
 };
 
-Example 2 - many DAI links:
+Example 2 - many DAI links and multi-CODECs:
 
 sound {
 	compatible = "simple-audio-card";
@@ -135,21 +136,12 @@ sound {
 		};
 	};
 
-	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
+	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI & S/PDIF */
 		cpu {
 			sound-dai = <&audio1 1>;
 		};
 		codec {
-			sound-dai = <&tda998x 1>;
-		};
-	};
-
-	simple-audio-card,dai-link@2 {		/* S/PDIF - S/PDIF */
-		cpu {
-			sound-dai = <&audio1 1>;
-		};
-		codec {
-			sound-dai = <&spdif_codec>;
+			sound-dai = <&tda998x 1>, <&spdif_codec>;
 		};
 	};
 };
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index ece22d5..61f9500 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -172,34 +172,12 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 static int
 asoc_simple_card_sub_parse_of(struct device_node *np,
 			      struct asoc_simple_dai *dai,
-			      struct device_node **p_node,
-			      const char **name,
-			      int *args_count)
+			      struct device_node *dai_np)
 {
-	struct of_phandle_args args;
 	struct clk *clk;
 	u32 val;
 	int ret;
 
-	/*
-	 * Get node via "sound-dai = <&phandle port>"
-	 * it will be used as xxx_of_node on soc_bind_dai_link()
-	 */
-	ret = of_parse_phandle_with_args(np, "sound-dai",
-					 "#sound-dai-cells", 0, &args);
-	if (ret)
-		return ret;
-
-	*p_node = args.np;
-
-	if (args_count)
-		*args_count = args.args_count;
-
-	/* Get dai->name */
-	ret = snd_soc_of_get_dai_name(np, name);
-	if (ret < 0)
-		return ret;
-
 	/* Parse TDM slot */
 	ret = snd_soc_of_parse_tdm_slot(np, &dai->slots, &dai->slot_width);
 	if (ret)
@@ -222,7 +200,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	} else if (!of_property_read_u32(np, "system-clock-frequency", &val)) {
 		dai->sysclk = val;
 	} else {
-		clk = of_clk_get(args.np, 0);
+		clk = of_clk_get(dai_np, 0);
 		if (!IS_ERR(clk))
 			dai->sysclk = clk_get_rate(clk);
 	}
@@ -286,10 +264,12 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
 	struct device_node *cpu = NULL;
 	struct device_node *codec = NULL;
+	struct snd_soc_dai_link_component *component;
+	struct of_phandle_args args;
 	char *name;
 	char prop[128];
 	char *prefix = "";
-	int ret, cpu_args;
+	int ret, cpu_args, i;
 
 	/* For single DAI link & old style of DT node */
 	if (is_top_level_node)
@@ -312,16 +292,30 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (ret < 0)
 		goto dai_link_of_err;
 
+	/* Get the CPU node and name */
+	ret = of_parse_phandle_with_args(cpu, "sound-dai",
+					 "#sound-dai-cells", 0, &args);
+	if (ret)
+		goto dai_link_of_err;
+	dai_link->cpu_of_node = args.np;
+	cpu_args = args.args_count;
+
+	ret = snd_soc_of_get_dai_name(cpu, &dai_link->cpu_dai_name);
+	if (ret < 0)
+		goto dai_link_of_err;
+
 	ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
-					    &dai_link->cpu_of_node,
-					    &dai_link->cpu_dai_name,
-					    &cpu_args);
+					    dai_link->cpu_of_node);
+	if (ret < 0)
+		goto dai_link_of_err;
+
+	/* Get the node and name of the CODEC(s) */
+	ret = snd_soc_of_get_dai_link_codecs(dev, codec, dai_link);
 	if (ret < 0)
 		goto dai_link_of_err;
 
 	ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai,
-					    &dai_link->codec_of_node,
-					    &dai_link->codec_dai_name, NULL);
+					    dai_link->codecs[0].of_node);
 	if (ret < 0)
 		goto dai_link_of_err;
 
@@ -336,10 +330,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	/* DAI link 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,
+			    strlen(dai_link->codecs[0].dai_name) + 2,
 			    GFP_KERNEL);
 	sprintf(name, "%s-%s", dai_link->cpu_dai_name,
-				dai_link->codec_dai_name);
+				dai_link->codecs[0].dai_name);
 	dai_link->name = dai_link->stream_name = name;
 	dai_link->ops = &asoc_simple_card_ops;
 	dai_link->init = asoc_simple_card_dai_init;
@@ -350,7 +344,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 		dai_props->cpu_dai.fmt,
 		dai_props->cpu_dai.sysclk);
 	dev_dbg(dev, "\tcodec : %s / %04x / %d\n",
-		dai_link->codec_dai_name,
+		dai_link->codecs[0].dai_name,
 		dai_props->codec_dai.fmt,
 		dai_props->codec_dai.sysclk);
 
@@ -365,8 +359,18 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	 */
 	if (!cpu_args)
 		dai_link->cpu_dai_name = NULL;
+	goto out;
 
 dai_link_of_err:
+	for (i = 0, component = dai_link->codecs;
+	     i < dai_link->num_codecs;
+	     i++, component++) {
+		if (!component->of_node)
+			break;
+		of_node_put(component->of_node);
+		component->of_node = NULL;
+	}
+out:
 	of_node_put(cpu);
 	of_node_put(codec);
 
@@ -456,13 +460,20 @@ static int asoc_simple_card_unref(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
 	struct snd_soc_dai_link *dai_link;
-	int num_links;
+	struct snd_soc_dai_link_component *component;
+	int num_links, i;
 
 	for (num_links = 0, dai_link = card->dai_link;
 	     num_links < card->num_links;
 	     num_links++, dai_link++) {
 		of_node_put(dai_link->cpu_of_node);
-		of_node_put(dai_link->codec_of_node);
+		for (i = 0, component = dai_link->codecs;
+		     i < dai_link->num_codecs;
+		     i++, component++) {
+			if (!component->of_node)
+				break;
+			of_node_put(component->of_node);
+		}
 	}
 	return 0;
 }
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/4] ASoC: simple-card: add multi-CODECs in DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Tue, Nov 25, 2014 at 01:30:14PM +0100, Jean-Francois Moine wrote:

> This patch allows many CODECs per link to be defined in the device tree.

It's also quite big and fiddly and hard to read, the changes that are
being made aren't blindingly obvious and there's quite a few of them.
As I've said before it's really importat that changes are clear and easy
to read, if the code is complex or surprising then the changelog needs
to be that bit more detailed to make thigs clear.  Things like talking
about why the code is being moved out and how it is being transformed
would be really helpful with this one, it's not enough to know the
overall goal of the patch, I also need to know how the patch is intended
to achieve that.

I think this is mostly OK but a couple of things...

> -Example 2 - many DAI links:
> +Example 2 - many DAI links and multi-CODECs:

I'd be much happier with a new example here rather than modifying the
old one.

> @@ -365,8 +359,18 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>  	 */
>  	if (!cpu_args)
>  		dai_link->cpu_dai_name = NULL;
> +	goto out;
>  
>  dai_link_of_err:

This goto out thing here is messy, it's not our normal coding style and
is error prone - better to just duplicate a small amount of cleanup.

> +	for (i = 0, component = dai_link->codecs;
> +	     i < dai_link->num_codecs;
> +	     i++, component++) {
> +		if (!component->of_node)
> +			break;

What's this break doing here, why might we be missing a node and why
should we skip all remaining components rather than just this one as a
result - a continue would be less surprising.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v5 4/4] ASoC: simple-card: Remove useless check

[Jean-Francois Moine]

The CPU and CODEC names are checked when getting the DAI names.
There is no need to check them once more.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 sound/soc/generic/simple-card.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 61f9500..c1ba78b 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -319,11 +319,6 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (ret < 0)
 		goto dai_link_of_err;
 
-	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
-		ret = -EINVAL;
-		goto dai_link_of_err;
-	}
-
 	/* Simple Card assumes platform == cpu */
 	dai_link->platform_of_node = dai_link->cpu_of_node;
 
-- 
2.1.3