[PATCH v3 1/4] ASoC: simple-card: Simplify code

[PATCH v3 1/4] ASoC: simple-card: Simplify code

[Jean-Francois Moine]

The global DAI format is used only in the function
asoc_simple_card_parse_of(). So, move it from the private data
to the stack.

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

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index dcf37fb..ca7e63e 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -20,7 +20,6 @@
 
 struct simple_card_data {
 	struct snd_soc_card snd_card;
-	unsigned int daifmt;
 	struct asoc_simple_dai cpu_dai;
 	struct asoc_simple_dai codec_dai;
 	struct snd_soc_dai_link snd_link;
@@ -154,13 +153,14 @@ 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;
+	unsigned int daifmt;
 	int ret;
 
 	/* parsing the card name from DT */
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
 
 	/* get CPU/CODEC common format via simple-audio-card,format */
-	priv->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
+	daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
 		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
 
 	/* off-codec widgets */
@@ -183,7 +183,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
 	if (np) {
-		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
+		ret = asoc_simple_card_sub_parse_of(np, daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
@@ -196,7 +196,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,codec");
 	if (np) {
-		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
+		ret = asoc_simple_card_sub_parse_of(np, daifmt,
 						  &priv->codec_dai,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
@@ -223,7 +223,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	dai_link->platform_of_node = dai_link->cpu_of_node;
 
 	dev_dbg(dev, "card-name : %s\n", name);
-	dev_dbg(dev, "platform : %04x\n", priv->daifmt);
+	dev_dbg(dev, "platform : %04x\n", daifmt);
 	dev_dbg(dev, "cpu : %s / %04x / %d\n",
 		dai_link->cpu_dai_name,
 		priv->cpu_dai.fmt,
-- 
1.9.0

[PATCH v3 2/4] ASoC: simple-card: dynamically allocate the DAI link and properties

[Jean-Francois Moine]

The DAI link array and the properties (fmt, sysclk slots) are
hard-coded for a single CPU / CODEC link.

This patch dynamically allocates the DAI link array and the
properties with the aim of supporting many DAI links.

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

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index ca7e63e..a55dc46 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -20,9 +20,11 @@
 
 struct simple_card_data {
 	struct snd_soc_card snd_card;
-	struct asoc_simple_dai cpu_dai;
-	struct asoc_simple_dai codec_dai;
-	struct snd_soc_dai_link snd_link;
+	struct simple_dais {
+		struct asoc_simple_dai cpu_dai;
+		struct asoc_simple_dai codec_dai;
+	} *dais;
+	struct snd_soc_dai_link dai_link[];	/* dynamically allocated */
 };
 
 static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
@@ -70,11 +72,11 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	struct snd_soc_dai *cpu = rtd->cpu_dai;
 	int ret;
 
-	ret = __asoc_simple_card_dai_init(codec, &priv->codec_dai);
+	ret = __asoc_simple_card_dai_init(codec, &priv->dais->codec_dai);
 	if (ret < 0)
 		return ret;
 
-	ret = __asoc_simple_card_dai_init(cpu, &priv->cpu_dai);
+	ret = __asoc_simple_card_dai_init(cpu, &priv->dais->cpu_dai);
 	if (ret < 0)
 		return ret;
 
@@ -184,7 +186,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
 	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, daifmt,
-						  &priv->cpu_dai,
+						  &priv->dais->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
 		of_node_put(np);
@@ -197,7 +199,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	np = of_get_child_by_name(node, "simple-audio-card,codec");
 	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, daifmt,
-						  &priv->codec_dai,
+						  &priv->dais->codec_dai,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
 		of_node_put(np);
@@ -226,12 +228,12 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	dev_dbg(dev, "platform : %04x\n", daifmt);
 	dev_dbg(dev, "cpu : %s / %04x / %d\n",
 		dai_link->cpu_dai_name,
-		priv->cpu_dai.fmt,
-		priv->cpu_dai.sysclk);
+		priv->dais->cpu_dai.fmt,
+		priv->dais->cpu_dai.sysclk);
 	dev_dbg(dev, "codec : %s / %04x / %d\n",
 		dai_link->codec_dai_name,
-		priv->codec_dai.fmt,
-		priv->codec_dai.sysclk);
+		priv->dais->codec_dai.fmt,
+		priv->dais->codec_dai.sysclk);
 
 	/*
 	 * soc_bind_dai_link() will check cpu name
@@ -276,7 +278,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;
 
@@ -285,10 +289,17 @@ 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 = priv->dai_link;
 	priv->snd_card.dai_link = dai_link;
 	priv->snd_card.num_links = 1;
 
+	/* get room for the other properties */
+	priv->dais = devm_kzalloc(dev,
+			sizeof(*priv->dais),
+			GFP_KERNEL);
+	if (!priv->dais)
+		return -ENOMEM;
+
 	if (np && of_device_is_available(np)) {
 
 		ret = asoc_simple_card_parse_of(np, priv, dev);
@@ -322,13 +333,13 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 		dai_link->codec_name	= cinfo->codec;
 		dai_link->cpu_dai_name	= cinfo->cpu_dai.name;
 		dai_link->codec_dai_name = cinfo->codec_dai.name;
-		memcpy(&priv->cpu_dai, &cinfo->cpu_dai,
-						sizeof(priv->cpu_dai));
-		memcpy(&priv->codec_dai, &cinfo->codec_dai,
-						sizeof(priv->codec_dai));
+		memcpy(&priv->dais->cpu_dai, &cinfo->cpu_dai,
+						sizeof(priv->dais->cpu_dai));
+		memcpy(&priv->dais->codec_dai, &cinfo->codec_dai,
+						sizeof(priv->dais->codec_dai));
 
-		priv->cpu_dai.fmt	|= cinfo->daifmt;
-		priv->codec_dai.fmt	|= cinfo->daifmt;
+		priv->dais->cpu_dai.fmt		|= cinfo->daifmt;
+		priv->dais->codec_dai.fmt	|= cinfo->daifmt;
 	}
 
 	/*
-- 
1.9.0

[PATCH v3 3/4] ASoC: simple-card: Handle many DAI links

[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 <moinejf@free.fr>
---
 sound/soc/generic/simple-card.c | 134 +++++++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 51 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index a55dc46..ffb9be3 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -70,13 +70,14 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 				snd_soc_card_get_drvdata(rtd->card);
 	struct snd_soc_dai *codec = rtd->codec_dai;
 	struct snd_soc_dai *cpu = rtd->cpu_dai;
-	int ret;
+	int num, ret;
 
-	ret = __asoc_simple_card_dai_init(codec, &priv->dais->codec_dai);
+	num = rtd - rtd->card->rtd;
+	ret = __asoc_simple_card_dai_init(codec, &priv->dais[num].codec_dai);
 	if (ret < 0)
 		return ret;
 
-	ret = __asoc_simple_card_dai_init(cpu, &priv->dais->cpu_dai);
+	ret = __asoc_simple_card_dai_init(cpu, &priv->dais[num].cpu_dai);
 	if (ret < 0)
 		return ret;
 
@@ -156,7 +157,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	struct device_node *np;
 	char *name;
 	unsigned int daifmt;
-	int ret;
+	int num, ret;
 
 	/* parsing the card name from DT */
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
@@ -181,50 +182,63 @@ 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;
+	for (num = 0; ; num++, dai_link++) {
+		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");
+			ret = -EINVAL;
+			goto err;
+		}
 		ret = asoc_simple_card_sub_parse_of(np, daifmt,
-						  &priv->dais->cpu_dai,
+						  &priv->dais[num].cpu_dai,
 						  &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, daifmt,
-						  &priv->dais->codec_dai,
+						  &priv->dais[num].codec_dai,
 						  &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;
+	}
 
 	/* 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", daifmt);
 	dev_dbg(dev, "cpu : %s / %04x / %d\n",
 		dai_link->cpu_dai_name,
@@ -235,18 +249,11 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 		priv->dais->codec_dai.fmt,
 		priv->dais->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 */
@@ -276,10 +283,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;
@@ -291,11 +310,11 @@ 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;
 
 	/* get room for the other properties */
 	priv->dais = devm_kzalloc(dev,
-			sizeof(*priv->dais),
+			sizeof(*priv->dais) * num_links,
 			GFP_KERNEL);
 	if (!priv->dais)
 		return -ENOMEM;
@@ -308,6 +327,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

[PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

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

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/sound/simple-card.txt      | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index b30c222..a872e7b 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -26,6 +26,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
@@ -43,7 +46,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";
@@ -88,3 +91,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>;
+	};
+};
-- 
1.9.0

[PATCH v3 0/4] ASoC: simple-card: multi DAI links extension

[Jean-Francois Moine]

This patch series extends the simple card driver to handle
many DAI links as this exists in the Cubox audio subsystem.

-v3
	- remove 'Fix the reference count of device nodes'
		which is applied (Mark Brown)
	- new patch 'Simplify code'
	- dynamically allocate and use properties for all DAI links
		(Jyri Sarha and Li Xiubo)
- v2
	- change subject/comment about device node reference count
		(Mark Brown)
	- use a null size array instead of an implicit area for the DAI links
		(Li Xiubo)
	- update the reference count of the device node at end of probe

Jean-Francois Moine (4):
  ASoC: simple-card: Simplify code
  ASoC: simple-card: dynamically allocate the DAI link and properties
  ASoC: simple-card: Handle 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                    | 181 +++++++++++++--------
 2 files changed, 145 insertions(+), 70 deletions(-)

-- 
1.9.0

RE: [PATCH v3 0/4] ASoC: simple-card: multi DAI links extension

[Li.Xiubo]

> Subject: [PATCH v3 0/4] ASoC: simple-card: multi DAI links extension
> 
> This patch series extends the simple card driver to handle
> many DAI links as this exists in the Cubox audio subsystem.
> 
> -v3
> 	- remove 'Fix the reference count of device nodes'
> 		which is applied (Mark Brown)
> 	- new patch 'Simplify code'
> 	- dynamically allocate and use properties for all DAI links
> 		(Jyri Sarha and Li Xiubo)


This patch series looks good to me.

For this patch series:
Reviewed-by: Xiubo Li <Li.Xiubo@freescale.com>


Thanks,

--
Best Regards,
Xiubo


> - v2
> 	- change subject/comment about device node reference count
> 		(Mark Brown)
> 	- use a null size array instead of an implicit area for the DAI links
> 		(Li Xiubo)
> 	- update the reference count of the device node at end of probe
> 
> Jean-Francois Moine (4):
>   ASoC: simple-card: Simplify code
>   ASoC: simple-card: dynamically allocate the DAI link and properties
>   ASoC: simple-card: Handle 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                    | 181 +++++++++++++-------
> -
>  2 files changed, 145 insertions(+), 70 deletions(-)
> 
> --
> 1.9.0
> 
> 

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

[Jyri Sarha]

On 03/15/2014 01:09 PM, Jean-Francois Moine wrote:
> The DAI link array and the properties (fmt, sysclk slots) are
> hard-coded for a single CPU / CODEC link.
>
> This patch dynamically allocates the DAI link array and the
> properties with the aim of supporting many DAI links.
>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   sound/soc/generic/simple-card.c | 49 +++++++++++++++++++++++++----------------
>   1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index ca7e63e..a55dc46 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -20,9 +20,11 @@
>
>   struct simple_card_data {
>   	struct snd_soc_card snd_card;
> -	struct asoc_simple_dai cpu_dai;
> -	struct asoc_simple_dai codec_dai;
> -	struct snd_soc_dai_link snd_link;
> +	struct simple_dais {
> +		struct asoc_simple_dai cpu_dai;
> +		struct asoc_simple_dai codec_dai;
> +	} *dais;
> +	struct snd_soc_dai_link dai_link[];	/* dynamically allocated */
>   };
>

This is only an implementation detail, but wouldn't it produce a cleaner 
implementation if you would write the above structure like this:

   struct simple_card_data {
   	struct snd_soc_card snd_card;
	struct simple_dai_links {
		struct snd_soc_dai_link dai_link;
		struct asoc_simple_dai cpu_dai;
		struct asoc_simple_dai codec_dai;
	} *dai_links;
   };

or even

   struct simple_card_data {
   	struct snd_soc_card snd_card;
	struct simple_dai_links {
		struct snd_soc_dai_link dai_link;
		struct asoc_simple_dai cpu_dai;
		struct asoc_simple_dai codec_dai;
	} dai_links[]; /* dynamically allocated */
   };

But, as said this only an implementation detail.

Best regards,
Jyri

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 2014-03-15 13:30, Jean-Francois Moine wrote:
> There may be many couples of CPU/CODEC DAI links.
> The example 2 is extracted from the Cubox DT.
> 
> Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> ---
[...]

This binding forces all the dai links to share the same card level 
properties. I find it problematic in these cases:

>> - simple-audio-card,format		: CPU/CODEC common audio format.
>> 					  "i2s", "right_j", "left_j" , "dsp_a"
>> 					  "dsp_b", "ac97", "pdm", "msb", "lsb"

The code allows having the format parameter in sub nodes too, but the 
document does no mention it. Adding a mention would solve this problem 
at least partly.

Neither does the document mention that 
"simple-audio-card,bitclock-inversion" and 
"simple-audio-card,frame-inversion" are also allowed in card level. 
Currently the code uses simple bit-wise-or from card-level and dai-level 
daifmt parameters, which may lead to broken daifmt setting. However, 
this on directly related to this patch.

>> - dai-tdm-slot-num			: Please refer to tdm-slot.txt.
>> - dai-tdm-slot-width			: Please refer to tdm-slot.txt.

These properties are actually only taken from sub-nodes so the document 
is broken but the code is ok.

In general this binding would look better if another sub-node level 
would added for dai-link related properties, including the cpu and codec 
sub-nodes, but that would break the backwards compatibility.

Best regards,
Jyri
--
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 v3 2/4] ASoC: simple-card: dynamically allocate the DAI link and properties

[Jean-Francois Moine]

On Mon, 17 Mar 2014 11:29:42 +0200
Jyri Sarha <jsarha@ti.com> wrote:

> On 03/15/2014 01:09 PM, Jean-Francois Moine wrote:
> > The DAI link array and the properties (fmt, sysclk slots) are
> > hard-coded for a single CPU / CODEC link.
> >
> > This patch dynamically allocates the DAI link array and the
> > properties with the aim of supporting many DAI links.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >   sound/soc/generic/simple-card.c | 49 +++++++++++++++++++++++++----------------
> >   1 file changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> > index ca7e63e..a55dc46 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -20,9 +20,11 @@
> >
> >   struct simple_card_data {
> >   	struct snd_soc_card snd_card;
> > -	struct asoc_simple_dai cpu_dai;
> > -	struct asoc_simple_dai codec_dai;
> > -	struct snd_soc_dai_link snd_link;
> > +	struct simple_dais {
> > +		struct asoc_simple_dai cpu_dai;
> > +		struct asoc_simple_dai codec_dai;
> > +	} *dais;
> > +	struct snd_soc_dai_link dai_link[];	/* dynamically allocated */
> >   };
> >
> 
> This is only an implementation detail, but wouldn't it produce a cleaner 
> implementation if you would write the above structure like this:
> 
>    struct simple_card_data {
>    	struct snd_soc_card snd_card;
> 	struct simple_dai_links {
> 		struct snd_soc_dai_link dai_link;
> 		struct asoc_simple_dai cpu_dai;
> 		struct asoc_simple_dai codec_dai;
> 	} *dai_links;
>    };
> 
> or even
> 
>    struct simple_card_data {
>    	struct snd_soc_card snd_card;
> 	struct simple_dai_links {
> 		struct snd_soc_dai_link dai_link;
> 		struct asoc_simple_dai cpu_dai;
> 		struct asoc_simple_dai codec_dai;
> 	} dai_links[]; /* dynamically allocated */
>    };
> 
> But, as said this only an implementation detail.

Jyri,

No, this would not work. The DAI link in the struct snd_soc_card is an
array of struct snd_soc_dai_link. There cannot be anything between the
elements!

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Jyri Sarha]

On 03/17/2014 12:23 PM, Jean-Francois Moine wrote:
> Jyri,
>
> No, this would not work. The DAI link in the struct snd_soc_card is an
> array of struct snd_soc_dai_link. There cannot be anything between the
> elements!

Oh.. did not go deep enough. Forget about this comment.

Best regards,
Jyri

Re: [PATCH v3 1/4] ASoC: simple-card: Simplify code

[Mark Brown]

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

On Sat, Mar 15, 2014 at 11:32:42AM +0100, Jean-Francois Moine wrote:
> The global DAI format is used only in the function
> asoc_simple_card_parse_of(). So, move it from the private data
> to the stack.

Applied, thanks.

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

Re: [PATCH v3 2/4] ASoC: simple-card: dynamically allocate the DAI link and properties

[Mark Brown]

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

On Sat, Mar 15, 2014 at 12:09:39PM +0100, Jean-Francois Moine wrote:
> The DAI link array and the properties (fmt, sysclk slots) are
> hard-coded for a single CPU / CODEC link.

Applied, thanks.

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

Re: [PATCH v3 3/4] ASoC: simple-card: Handle many DAI links

[Mark Brown]

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

On Sat, Mar 15, 2014 at 12:26:54PM +0100, Jean-Francois Moine wrote:
> Some simple audio cards may have many DAI links.
> This patch extends the simple-card driver for handling such cards.

This appears to be updating the device tree binding but there is no
documentation update to go with it.  Given that there are exactly two
DAI subnodes specified explicitly by name in the binding it's not at all
clear how this is intended to work.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
> There may be many couples of CPU/CODEC DAI links.
> The example 2 is extracted from the Cubox DT.

Oh, here's some documentation - please include the documentation
before the code, without the documentation the reader is going to have
no idea what the code is supposed to be implementing.

> +  There may be one or many couples (simple-audio-card,cpu, simple-audio-card,codec)
> +  (see example 2).

This doesn't mention how they're matched up.

> +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>;
> +	};

So, this is not exactly pretty as a binding.  I would expect to see the
links explicitly represented in the DT so you see the two DAIs in each
link grouped into a container, the above isn't very easy to read and as
Jyri says this lack of clarity also causes practical problems in that
there's nowhere to place link specific parameters.

I think what I'd expect to see here is that the simple card can either
be a container with a link in it directly or a container of links.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

On Mon, 17 Mar 2014 16:43:39 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
	[snip]
> > +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>;
> > +	};
> 
> So, this is not exactly pretty as a binding.  I would expect to see the
> links explicitly represented in the DT so you see the two DAIs in each
> link grouped into a container, the above isn't very easy to read and as
> Jyri says this lack of clarity also causes practical problems in that
> there's nowhere to place link specific parameters.
> 
> I think what I'd expect to see here is that the simple card can either
> be a container with a link in it directly or a container of links.

I agree. I see two possible syntaxes:

1) keep the same definitions in the containers:

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Cubox Audio";

	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
		simple-audio-card,cpu {
			sound-dai = <&audio1 0>;
			format = "i2s";
		};
		simple-audio-card,codec {
			sound-dai = <&tda998x 0>;
		};
	};

	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
		simple-audio-card,cpu {
			sound-dai = <&audio1 1>;
		};
		simple-audio-card,codec {
			sound-dai = <&tda998x 1>;
		}
	};
	...

2) new definitions in the container

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Cubox Audio";

	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
		format = "i2s";
		cpu-dai  = <&audio1 0>;
		codec-dai = <&tda998x 0>;
	};

	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
		cpu-dai = <&audio1 1>;
		codec-dai = <&tda998x 1>;
	};
	...

The 2nd syntax is simpler and clearer, but the properties of the CPU
DAI and of the CODEC DAI are the same in the container (format, clock
and PCM slots). Is this a problem?

BTW, there is a 'dai_fmt' in the DAI link, but this format is not used
in the simple-card. Why?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Tue, Mar 18, 2014 at 09:17:28AM +0100, Jean-Francois Moine wrote:

> The 2nd syntax is simpler and clearer, but the properties of the CPU
> DAI and of the CODEC DAI are the same in the container (format, clock
> and PCM slots). Is this a problem?

Yes, things like clocks might be different.  We need a way to specify
DAI specific properties.

> BTW, there is a 'dai_fmt' in the DAI link, but this format is not used
> in the simple-card. Why?

Could you be more specific please?  If you mean the dai_fmt feature of
the generic dai_link struct then because of the confusion about what
the DAI formats mean which you'll have seen on the list.

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

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

Re: [PATCH v3 2/4] ASoC: simple-card: dynamically allocate the DAI link and properties

[Mark Brown]

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

On Sat, Mar 15, 2014 at 12:09:39PM +0100, Jean-Francois Moine wrote:
> The DAI link array and the properties (fmt, sysclk slots) are
> hard-coded for a single CPU / CODEC link.

Sorry, I dropped this patch - it conflicts with Nicolin's patch to force
the two ends of the DAI link to have the same format which probably
isn't the ideal fix but at least allows sensible looking DTs to be
written and parsed.  Nicolin is on holiday and I don't have any systems
which use simple-card.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/18/2014 10:17 AM, Jean-Francois Moine wrote:
> On Mon, 17 Mar 2014 16:43:39 +0000
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>> On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
> 	[snip]
...

>
> I agree. I see two possible syntaxes:
>
> 1) keep the same definitions in the containers:
>
> sound {
> 	compatible = "simple-audio-card";
> 	simple-audio-card,name = "Cubox Audio";
>
> 	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 0>;
> 			format = "i2s";
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&tda998x 0>;
> 		};
> 	};
>
> 	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 1>;
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&tda998x 1>;
> 		}
> 	};
> 	...
>

I vote for the version above. As Mark said there is need for dai 
specific properties.

While we are at it we could update the bitclock-master and frame-master 
syntax to be like this:

bitclock-master = "cpu"
frame-master = "codec"

With the above explicit definition all the daifmt settings could be 
defined in link level. For backwards compatibility we could still define 
that omitting the value equals "codec" and omitting the property equals 
"cpu".

It may sometimes be helpful to allow overwriting link level settings in 
dai level. In order to do that it should be possible to write all daifmt 
settings explicitly like this:

bitclock-inversion = <0>; /* <0> = no bitclock-inversion */

If backward compatibility is necessary we could recognize the syntax 
version from the existence dai-link node.

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Simple Audio";
	simple-audio-card,widgets = ...
	simple-audio-card,routing = ...

	simple-audio-card,dai-link@0 {		/* I2S - codec */
		format = "i2s";
		bitclock-master = "codec";
		frame-master = "codec";
		bitclock-inversion = <1>;
		simple-audio-card,cpu {
			sound-dai = <&audio1 0>;
			bitclock-inversion = <0>;
		};
		simple-audio-card,codec {
			sound-dai = <&codec 0>;
			system-clock-frequency = <12000000>;
		};
	};
...

I can participate in the implementation too.

Best regards,
Jyro
--
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 v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 12:08:55PM +0200, Jyri Sarha wrote:

> While we are at it we could update the bitclock-master and frame-master
> syntax to be like this:

> bitclock-master = "cpu"
> frame-master = "codec"

> With the above explicit definition all the daifmt settings could be defined
> in link level. For backwards compatibility we could still define that
> omitting the value equals "codec" and omitting the property equals "cpu".

It seems it'd be a bit more idiomatic to do that with a phandle rather
than with a string in order to allow extensions for things like TDM (the
I2S to mono speaker driver use case for example).

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

On Wed, 19 Mar 2014 12:08:55 +0200
Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> wrote:

> sound {
> 	compatible = "simple-audio-card";
> 	simple-audio-card,name = "Simple Audio";
> 	simple-audio-card,widgets = ...
> 	simple-audio-card,routing = ...
> 
> 	simple-audio-card,dai-link@0 {		/* I2S - codec */
> 		format = "i2s";
> 		bitclock-master = "codec";
> 		frame-master = "codec";
> 		bitclock-inversion = <1>;
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 0>;
> 			bitclock-inversion = <0>;
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&codec 0>;
> 			system-clock-frequency = <12000000>;
> 		};
> 	};
> ...
> 
> I can participate in the implementation too.

Thanks. I will prepare the multi DAI links and send you the first
patches.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
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 v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 03:46 PM, Mark Brown wrote:
> On Wed, Mar 19, 2014 at 12:08:55PM +0200, Jyri Sarha wrote:
>
>> While we are at it we could update the bitclock-master and frame-master
>> syntax to be like this:
>
>> bitclock-master = "cpu"
>> frame-master = "codec"
>
>> With the above explicit definition all the daifmt settings could be defined
>> in link level. For backwards compatibility we could still define that
>> omitting the value equals "codec" and omitting the property equals "cpu".
>
> It seems it'd be a bit more idiomatic to do that with a phandle rather
> than with a string in order to allow extensions for things like TDM (the
> I2S to mono speaker driver use case for example).
>

You mean a like this:

sound {
     compatible = "simple-audio-card";
     simple-audio-card,name = "Simple Audio";
     simple-audio-card,widgets = ...
     simple-audio-card,routing = ...

     simple-audio-card,dai-link@0 {        /* I2S - codec */
         format = "i2s";
         bitclock-master = <&codec 0>
         frame-master = <&codec 0>;
         bitclock-inversion = <1>;
         simple-audio-card,cpu {
             sound-dai = <&audio1 0>;
             bitclock-inversion = <0>;
         };
         simple-audio-card,codec {
             sound-dai = <&codec 0>;
             system-clock-frequency = <12000000>;
         };
     };
...

Yep, that makes sense when considering tdm setups with multiple codecs 
on the same wires.

Best regards,
Jyri

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Lars-Peter Clausen]

On 03/19/2014 11:08 AM, Jyri Sarha wrote:
[...]
> It may sometimes be helpful to allow overwriting link level settings in dai
> level. In order to do that it should be possible to write all daifmt
> settings explicitly like this:
>
> bitclock-inversion = <0>; /* <0> = no bitclock-inversion */

When does this make sense? Either the bitclock is inverted for all of them 
or for none.

--
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 v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 08:32:06PM +0200, Jyri Sarha wrote:
> On 03/19/2014 03:46 PM, Mark Brown wrote:

> >It seems it'd be a bit more idiomatic to do that with a phandle rather
> >than with a string in order to allow extensions for things like TDM (the
> >I2S to mono speaker driver use case for example).

> You mean a like this:

> sound {

>         bitclock-master = <&codec 0>
>         frame-master = <&codec 0>;
>         bitclock-inversion = <1>;
>         simple-audio-card,cpu {

Possibly referring to the subnodes rather than the device but yeah.

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

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

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
> On 03/19/2014 11:08 AM, Jyri Sarha wrote:
> [...]
>> It may sometimes be helpful to allow overwriting link level settings
>> in dai
>> level. In order to do that it should be possible to write all daifmt
>> settings explicitly like this:
>>
>> bitclock-inversion = <0>; /* <0> = no bitclock-inversion */
>
> When does this make sense? Either the bitclock is inverted for all of
> them or for none.
>

Definition of clock signal and it's inversion varies between chip 
manufacturers and sometimes it may not be possible to get all the dai 
drivers to work identically in this respect. Because of this in some 
cases there may be a need to set the inversion bit only at one end of 
the link.

I think there was an example of this in some mail regarding the 
simple-card DT-bidings, but I can't find it ATM.

Best regards,
Jyri

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:

> >When does this make sense? Either the bitclock is inverted for all of
> >them or for none.

> Definition of clock signal and it's inversion varies between chip
> manufacturers and sometimes it may not be possible to get all the dai
> drivers to work identically in this respect. Because of this in some cases
> there may be a need to set the inversion bit only at one end of the link.

No, Linux has a definition of all the clock modes which applies to all
devices regardless of what the manufacturer documents in their
datasheet.

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

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Lars-Peter Clausen]

On 03/19/2014 08:21 PM, Mark Brown wrote:
> On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
>> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
>
>>> When does this make sense? Either the bitclock is inverted for all of
>>> them or for none.
>
>> Definition of clock signal and it's inversion varies between chip
>> manufacturers and sometimes it may not be possible to get all the dai
>> drivers to work identically in this respect. Because of this in some cases
>> there may be a need to set the inversion bit only at one end of the link.
>
> No, Linux has a definition of all the clock modes which applies to all
> devices regardless of what the manufacturer documents in their
> datasheet.
>

Yep. The clock properties are well defined for the different modes that can 
be specified in the format property. It's up to the driver to translate this 
to driver specific settings. If two drivers behave differently for the same 
mode one of them (or both) are broken.

We should probably add the definitions for the different formats to the DT 
bindings. E.g. what is default, what is inverted polarity, etc.

- Lars
--
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: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 09:31 PM, Lars-Peter Clausen wrote:
> On 03/19/2014 08:21 PM, Mark Brown wrote:
>> On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
>>> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
>>
>>>> When does this make sense? Either the bitclock is inverted for all of
>>>> them or for none.
>>
>>> Definition of clock signal and it's inversion varies between chip
>>> manufacturers and sometimes it may not be possible to get all the dai
>>> drivers to work identically in this respect. Because of this in some
>>> cases
>>> there may be a need to set the inversion bit only at one end of the
>>> link.
>>
>> No, Linux has a definition of all the clock modes which applies to all
>> devices regardless of what the manufacturer documents in their
>> datasheet.
>>
>
> Yep. The clock properties are well defined for the different modes that
> can be specified in the format property. It's up to the driver to
> translate this to driver specific settings. If two drivers behave
> differently for the same mode one of them (or both) are broken.
>

After a little thinking it is clear to me too that only reason to have 
this overwrite capability is a badly written dai driver. Even with buggy 
HW it should always be possible to present the working modes in daifmt 
terms. Ok, let's remove it.

Here is an updated DT example with updated phandle for master settings:

sound {
     compatible = "simple-audio-card";
     simple-audio-card,name = "Simple Audio";
     simple-audio-card,widgets = ...
     simple-audio-card,routing = ...

     simple-audio-card,dai-link@0 {        /* I2S - codec */
         format = "i2s";
         bitclock-master = <&dai_link_master>
         frame-master = <&dai_link_master>;
         bitclock-inversion;
         simple-audio-card,cpu {
             sound-dai = <&audio1 0>;
         };
         dai_link_master: simple-audio-card,codec {
             sound-dai = <&codec 0>;
             system-clock-frequency = <12000000>;
         };
     };
...


> We should probably add the definitions for the different formats to the
> DT bindings. E.g. what is default, what is inverted polarity, etc.
>

That is a good idea.

Best regards,
Jyri
--
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