[PATCH RFC 0/2] Move dai-link level properties away from dai subnodes

[PATCH RFC 0/2] Move dai-link level properties away from dai subnodes

[Jyri Sarha]

This patch is implemented on top of late patches from Jean-Francois
Moine [1]. 

The patches implement the main part of the simple-card changes
discussed in alsa-devel mailing list.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074592.html

Best regards,
Jyri

Jyri Sarha (2):
  ASoC: core: Update snd_soc_of_parse_daifmt() interface
  ASoC: simple-card: Move dai-link level properties away from dai
    subnodes

 .../devicetree/bindings/sound/simple-card.txt      |   91 ++++----
 include/sound/soc.h                                |    4 +-
 sound/soc/generic/simple-card.c                    |  237 ++++++++++++--------
 sound/soc/soc-core.c                               |    8 +-
 4 files changed, 201 insertions(+), 139 deletions(-)

-- 
1.7.9.5

[PATCH RFC 1/2] ASoC: core: Update snd_soc_of_parse_daifmt() interface

[Jyri Sarha]

Adds struct device_node **bitclkmaster and struct device_node **framemaster
function parameters. With the new syntax bitclock-master and frame-master
properties can explicitly indicate the dai-link bit-clock and frame masters
with a phandle.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 include/sound/soc.h  |    4 +++-
 sound/soc/soc-core.c |    8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index f7de629..e40713a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1228,7 +1228,9 @@ int snd_soc_of_parse_tdm_slot(struct device_node *np,
 int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 				   const char *propname);
 unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
-				     const char *prefix);
+				     const char *prefix,
+				     struct device_node **bitclkmaster,
+				     struct device_node **framemaster);
 int snd_soc_of_get_dai_name(struct device_node *of_node,
 			    const char **dai_name);
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b322cf2..7979dcc 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4560,7 +4560,9 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
 
 unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
-				     const char *prefix)
+				     const char *prefix,
+				     struct device_node **bitclkmaster,
+				     struct device_node **framemaster)
 {
 	int ret, i;
 	char prop[128];
@@ -4643,9 +4645,13 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 	 */
 	snprintf(prop, sizeof(prop), "%sbitclock-master", prefix);
 	bit = !!of_get_property(np, prop, NULL);
+	if (bit && bitclkmaster)
+		*bitclkmaster = of_parse_phandle(np, prop, 0);
 
 	snprintf(prop, sizeof(prop), "%sframe-master", prefix);
 	frame = !!of_get_property(np, prop, NULL);
+	if (frame && framemaster)
+		*framemaster = of_parse_phandle(np, prop, 0);
 
 	switch ((bit << 4) + frame) {
 	case 0x11:
-- 
1.7.9.5

[PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Jyri Sarha]

The properties like format, bitclock-master, frame-master,
bitclock-inversion, and frame-inversion should be common to the dais
connected with a dai-link. For bitclock-master and frame-master
properties to be unambiguous they need to indicate the mastering dai
node with a phandle.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/sound/simple-card.txt      |   91 ++++----
 sound/soc/generic/simple-card.c                    |  237 ++++++++++++--------
 2 files changed, 191 insertions(+), 137 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index 131aa2a..3cafa9e 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -1,6 +1,6 @@
 Simple-Card:
 
-Simple-Card specifies audio DAI connection of SoC <-> codec.
+Simple-Card specifies audio DAI connections of SoC <-> codec.
 
 Required properties:
 
@@ -8,28 +8,54 @@ Required properties:
 
 Optional properties:
 
-- simple-audio-card,name		: User specified audio sound card name, one string
+- simple-audio-card,name		: User specified audio sound card name,
+one string
 					  property.
-- simple-audio-card,format		: CPU/CODEC common audio format.
-					  "i2s", "right_j", "left_j" , "dsp_a"
-					  "dsp_b", "ac97", "pdm", "msb", "lsb"
 - simple-audio-card,widgets		: Please refer to widgets.txt.
 - simple-audio-card,routing		: A list of the connections between audio components.
 					  Each entry is a pair of strings, the first being the
 					  connection's sink, the second being the connection's
 					  source.
-- dai-tdm-slot-num			: Please refer to tdm-slot.txt.
-- dai-tdm-slot-width			: Please refer to tdm-slot.txt.
+Optional subnodes:
+
+- simple-audio-card,dai-link		: Container for dai-link level
+					  properties and the CPU and CODEC
+					  sub-nodes. This container may be
+					  omitted when the card has only one
+					  DAI link. See the examples and the
+					  section bellow.
+
+Dai-link subnode properties and subnodes:
+
+If dai-link subnode is omitted and the subnode properties are directly
+under "sound"-node the subnode property and subnode names have to be
+prefixed with "simple-audio-card,"-prefix.
 
-Required subnodes:
+Requred dai-link subnodes:
 
-- simple-audio-card,dai-link		: container for the CPU and CODEC sub-nodes
-					  This container may be omitted when the
-					  card has only one DAI link.
-					  See the examples.
+- cpu					: CPU   sub-node
+- codec					: CODEC sub-node
 
-- simple-audio-card,cpu			: CPU   sub-node
-- simple-audio-card,codec		: CODEC sub-node
+Optional dai-link subnode properties:
+
+- format				: CPU/CODEC common audio format.
+					  "i2s", "right_j", "left_j" , "dsp_a"
+					  "dsp_b", "ac97", "pdm", "msb", "lsb"
+- frame-master				: Indicates dai-link frame master.
+					  phandle to a cpu or codec subnode.
+- bitclock-master			: Indicates dai-link bit clock master.
+					  phandle to a cpu or codec subnode.
+- bitclock-inversion			: bool property. Add this if the
+					  dai-link uses bit clock inversion.
+- frame-inversion			: bool property. Add this if the
+					  dai-link uses frame clock inversion.
+
+For backward compatibility the frame-master and bitclock-master
+properties can be used as booleans in codec subnode to indicate if the
+codec is the dai-link frame or bit clock master. In this case the same
+properties should not be present at dai-link level and the
+bitclock-inversion and frame-inversion properties should also be placed
+in the codec node if needed.
 
 Required CPU/CODEC subnodes properties:
 
@@ -37,29 +63,21 @@ Required CPU/CODEC subnodes properties:
 
 Optional CPU/CODEC subnodes properties:
 
-- format				: CPU/CODEC specific audio format if needed.
-					  see simple-audio-card,format
-- frame-master				: bool property. add this if subnode is frame master
-- bitclock-master			: bool property. add this if subnode is bitclock master
-- bitclock-inversion			: bool property. add this if subnode has clock inversion
-- frame-inversion			: bool property. add this if subnode has frame inversion
+- dai-tdm-slot-num			: Please refer to tdm-slot.txt.
+- dai-tdm-slot-width			: Please refer to tdm-slot.txt.
 - clocks / system-clock-frequency	: specify subnode's clock if needed.
 					  it can be specified via "clocks" if system has
 					  clock node (= common clock), or "system-clock-frequency"
 					  (if system doens't support common clock)
 
-Note:
- * For 'format', 'frame-master', 'bitclock-master', 'bitclock-inversion' and
-   'frame-inversion', the simple card will use the settings of CODEC for both
-   CPU and CODEC sides as we need to keep the settings identical for both ends
-   of the link.
-
 Example 1 - single DAI link:
 
 sound {
 	compatible = "simple-audio-card";
 	simple-audio-card,name = "VF610-Tower-Sound-Card";
 	simple-audio-card,format = "left_j";
+	simple-audio-card,bitclock-master = <&dailink0_master>;
+	simple-audio-card,frame-master = <&dailink0_master>;
 	simple-audio-card,widgets =
 		"Microphone", "Microphone Jack",
 		"Headphone", "Headphone Jack",
@@ -69,17 +87,12 @@ sound {
 		"Headphone Jack", "HP_OUT",
 		"External Speaker", "LINE_OUT";
 
-	dai-tdm-slot-num = <2>;
-	dai-tdm-slot-width = <8>;
-
 	simple-audio-card,cpu {
 		sound-dai = <&sh_fsi2 0>;
 	};
 
-	simple-audio-card,codec {
+	dailink0_master: simple-audio-card,codec {
 		sound-dai = <&ak4648>;
-		bitclock-master;
-		frame-master;
 		clocks = <&osc>;
 	};
 };
@@ -105,31 +118,31 @@ Example 2 - many DAI links:
 sound {
 	compatible = "simple-audio-card";
 	simple-audio-card,name = "Cubox Audio";
-	simple-audio-card,format = "i2s";
 
 	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
-		simple-audio-card,cpu {
+		format = "i2s";
+		cpu {
 			sound-dai = <&audio1 0>;
 		};
-		simple-audio-card,codec {
+		codec {
 			sound-dai = <&tda998x 0>;
 		};
 	};
 
 	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
-		simple-audio-card,cpu {
+		cpu {
 			sound-dai = <&audio1 1>;
 		};
-		simple-audio-card,codec {
+		codec {
 			sound-dai = <&tda998x 1>;
 		};
 	};
 
 	simple-audio-card,dai-link@2 {		/* S/PDIF - S/PDIF */
-		simple-audio-card,cpu {
+		cpu {
 			sound-dai = <&audio1 1>;
 		};
-		simple-audio-card,codec {
+		codec {
 			sound-dai = <&spdif_codec>;
 		};
 	};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 21f1ccb..f4663d0 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,6 +8,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#define DEBUG
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/module.h>
@@ -88,7 +89,6 @@ 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,
-			      unsigned int daifmt,
 			      struct asoc_simple_dai *dai,
 			      const struct device_node **p_node,
 			      const char **name)
@@ -117,14 +117,6 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 		return ret;
 
 	/*
-	 * bitclock-inversion, frame-inversion
-	 * bitclock-master,    frame-master
-	 * and specific "format" if it has
-	 */
-	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
-	dai->fmt |= daifmt;
-
-	/*
 	 * dai->sysclk come from
 	 *  "clocks = <&xxx>" (if system has common clock)
 	 *  or "system-clock-frequency = <xxx>"
@@ -151,37 +143,134 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	return 0;
 }
 
-static int simple_card_cpu_codec_of(struct device_node *node,
-				int daifmt,
-				struct snd_soc_dai_link *dai_link,
-				struct simple_dai_props *dai_props)
+static int simple_card_dai_link_of(struct device_node *node,
+				   struct device *dev,
+				   struct snd_soc_dai_link *dai_link,
+				   struct simple_dai_props *dai_props)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
+	struct device_node *bitclkmaster = NULL;
+	struct device_node *framemaster = NULL;
+	unsigned int daifmt;
+	char *name;
+	char prop[128];
+	char *prefix = "";
 	int ret;
 
-	/* CPU sub-node */
-	ret = -EINVAL;
-	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np) {
-		ret = asoc_simple_card_sub_parse_of(np, daifmt,
-						&dai_props->cpu_dai,
-						&dai_link->cpu_of_node,
-						&dai_link->cpu_dai_name);
-		of_node_put(np);
+	if (!strcmp("sound", node->name))
+		prefix = "simple-audio-card,";
+
+	daifmt = snd_soc_of_parse_daifmt(node, prefix,
+					 &bitclkmaster, &framemaster);
+	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
+
+	snprintf(prop, sizeof(prop), "%scpu", prefix);
+	np = of_get_child_by_name(node, prop);
+	if (!np) {
+		ret = -EINVAL;
+		dev_err(dev, "%s: Can't find simple-audio-card,cpu DT node\n",
+			__func__);
+		goto dai_link_of_err;
 	}
+
+	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
+					    &dai_link->cpu_of_node,
+					    &dai_link->cpu_dai_name);
 	if (ret < 0)
-		return ret;
+		goto dai_link_of_err;
+
+	dai_props->cpu_dai.fmt = daifmt;
+	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
+	case 0x11:
+		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
+		break;
+	case 0x10:
+		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
+		break;
+	case 0x01:
+		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
+		break;
+	default:
+		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
+		break;
+	}
 
-	/* CODEC sub-node */
-	ret = -EINVAL;
-	np = of_get_child_by_name(node, "simple-audio-card,codec");
-	if (np) {
-		ret = asoc_simple_card_sub_parse_of(np, daifmt,
-						&dai_props->codec_dai,
-						&dai_link->codec_of_node,
-						&dai_link->codec_dai_name);
-		of_node_put(np);
+	of_node_put(np);
+	snprintf(prop, sizeof(prop), "%scodec", prefix);
+	np = of_get_child_by_name(node, prop);
+	if (!np) {
+		ret = -EINVAL;
+		dev_err(dev, "%s: Can't find simple-audio-card,codec DT node\n",
+			__func__);
+		goto dai_link_of_err;
 	}
+
+	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
+					    &dai_link->codec_of_node,
+					    &dai_link->codec_dai_name);
+	if (ret < 0)
+		goto dai_link_of_err;
+
+	if (!bitclkmaster && !framemaster) {
+		/* Master setting not found from dai_link level, revert back
+		   to legacy DT parsing and take settings from codec node. */
+		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
+			__func__);
+		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
+			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
+			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
+	} else {
+		dai_props->codec_dai.fmt = daifmt;
+		switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
+		case 0x11:
+			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
+			break;
+		case 0x10:
+			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
+			break;
+		case 0x01:
+			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
+			break;
+		default:
+			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
+			break;
+		}
+	}
+
+	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;
+
+	/* 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,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", dai_link->cpu_dai_name,
+				dai_link->codec_dai_name);
+	dai_link->name = dai_link->stream_name = name;
+
+	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
+	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
+		dai_link->cpu_dai_name,
+		dai_props->cpu_dai.fmt,
+		dai_props->cpu_dai.sysclk);
+	dev_dbg(dev, "\tcodec : %s / %04x / %d\n",
+		dai_link->codec_dai_name,
+		dai_props->codec_dai.fmt,
+		dai_props->codec_dai.sysclk);
+
+dai_link_of_err:
+	if (np)
+		of_node_put(np);
+	if (bitclkmaster)
+		of_node_put(bitclkmaster);
+	if (framemaster)
+		of_node_put(framemaster);
 	return ret;
 }
 
@@ -192,18 +281,11 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 {
 	struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
 	struct simple_dai_props *dai_props = priv->dai_props;
-	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 */
-	daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
-		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
-
 	/* off-codec widgets */
 	if (of_property_read_bool(node, "simple-audio-card,widgets")) {
 		ret = snd_soc_of_parse_audio_simple_widgets(&priv->snd_card,
@@ -220,71 +302,30 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 			return ret;
 	}
 
-	/* loop on the DAI links */
-	np = NULL;
-	for (;;) {
-		if (multi) {
-			np = of_get_next_child(node, np);
-			if (!np)
-				break;
-		}
-
-		ret = simple_card_cpu_codec_of(multi ? np : node,
-					daifmt, dai_link, dai_props);
-		if (ret < 0)
-			goto err;
-
-		/*
-		 * overwrite cpu_dai->fmt as its DAIFMT_MASTER bit is based on CODEC
-		 * while the other bits should be identical unless buggy SW/HW design.
-		 */
-		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt;
+	dev_dbg(dev, "New simple-card: %s\n", priv->snd_card.name ?
+		priv->snd_card.name : "");
 
-		if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
-			ret = -EINVAL;
-			goto err;
+	if (multi) {
+		struct device_node *np = NULL;
+		int i;
+		for (i = 0; (np = of_get_next_child(node, np)); i++) {
+			dev_dbg(dev, "\tlink %d:\n", i);
+			ret = simple_card_dai_link_of(np, dev, dai_link + i,
+						      dai_props + i);
+			of_node_put(np);
+			if (ret < 0)
+				return ret;
 		}
-
-		/* 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;
-
-		if (!multi)
-			break;
-
-		dai_link++;
-		dai_props++;
+	} else {
+		ret = simple_card_dai_link_of(node, dev, dai_link, dai_props);
+		if (ret < 0)
+			return ret;
 	}
 
-	/* card name is created from CPU/CODEC dai name */
-	dai_link = priv->snd_card.dai_link;
 	if (!priv->snd_card.name)
-		priv->snd_card.name = dai_link->name;
-
-	dev_dbg(dev, "card-name : %s\n", priv->snd_card.name);
-	dev_dbg(dev, "platform : %04x\n", daifmt);
-	dai_props = priv->dai_props;
-	dev_dbg(dev, "cpu : %s / %04x / %d\n",
-		dai_link->cpu_dai_name,
-		dai_props->cpu_dai.fmt,
-		dai_props->cpu_dai.sysclk);
-	dev_dbg(dev, "codec : %s / %04x / %d\n",
-		dai_link->codec_dai_name,
-		dai_props->codec_dai.fmt,
-		dai_props->codec_dai.sysclk);
+		priv->snd_card.name = priv->snd_card.dai_link->name;
 
 	return 0;
-
-err:
-	of_node_put(np);
-	return ret;
 }
 
 /* update the reference count of the devices nodes at end of probe */
-- 
1.7.9.5

Re: [PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Jean-Francois Moine]

On Fri, 21 Mar 2014 18:47:23 +0200
Jyri Sarha <jsarha@ti.com> wrote:

> The properties like format, bitclock-master, frame-master,
> bitclock-inversion, and frame-inversion should be common to the dais
> connected with a dai-link. For bitclock-master and frame-master
> properties to be unambiguous they need to indicate the mastering dai
> node with a phandle.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../devicetree/bindings/sound/simple-card.txt      |   91 ++++----
>  sound/soc/generic/simple-card.c                    |  237 ++++++++++++--------
>  2 files changed, 191 insertions(+), 137 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> index 131aa2a..3cafa9e 100644
> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -1,6 +1,6 @@
>  Simple-Card:
>  
> -Simple-Card specifies audio DAI connection of SoC <-> codec.
> +Simple-Card specifies audio DAI connections of SoC <-> codec.
>  
>  Required properties:
>  
> @@ -8,28 +8,54 @@ Required properties:
>  
>  Optional properties:
>  
> -- simple-audio-card,name		: User specified audio sound card name, one string
> +- simple-audio-card,name		: User specified audio sound card name,
> +one string
>  					  property.
> -- simple-audio-card,format		: CPU/CODEC common audio format.
> -					  "i2s", "right_j", "left_j" , "dsp_a"
> -					  "dsp_b", "ac97", "pdm", "msb", "lsb"
>  - simple-audio-card,widgets		: Please refer to widgets.txt.
>  - simple-audio-card,routing		: A list of the connections between audio components.
>  					  Each entry is a pair of strings, the first being the
>  					  connection's sink, the second being the connection's
>  					  source.
> -- dai-tdm-slot-num			: Please refer to tdm-slot.txt.
> -- dai-tdm-slot-width			: Please refer to tdm-slot.txt.
> +Optional subnodes:
> +
> +- simple-audio-card,dai-link		: Container for dai-link level
> +					  properties and the CPU and CODEC
> +					  sub-nodes. This container may be
> +					  omitted when the card has only one
> +					  DAI link. See the examples and the
> +					  section bellow.
> +
> +Dai-link subnode properties and subnodes:
> +
> +If dai-link subnode is omitted and the subnode properties are directly
> +under "sound"-node the subnode property and subnode names have to be
> +prefixed with "simple-audio-card,"-prefix.
>  
> -Required subnodes:
> +Requred dai-link subnodes:

Typo: 'Required'

>  
> -- simple-audio-card,dai-link		: container for the CPU and CODEC sub-nodes
> -					  This container may be omitted when the
> -					  card has only one DAI link.
> -					  See the examples.
> +- cpu					: CPU   sub-node
> +- codec					: CODEC sub-node
>  
> -- simple-audio-card,cpu			: CPU   sub-node
> -- simple-audio-card,codec		: CODEC sub-node
> +Optional dai-link subnode properties:
> +
> +- format				: CPU/CODEC common audio format.
> +					  "i2s", "right_j", "left_j" , "dsp_a"
> +					  "dsp_b", "ac97", "pdm", "msb", "lsb"
> +- frame-master				: Indicates dai-link frame master.
> +					  phandle to a cpu or codec subnode.
> +- bitclock-master			: Indicates dai-link bit clock master.
> +					  phandle to a cpu or codec subnode.
> +- bitclock-inversion			: bool property. Add this if the
> +					  dai-link uses bit clock inversion.
> +- frame-inversion			: bool property. Add this if the
> +					  dai-link uses frame clock inversion.
> +
> +For backward compatibility the frame-master and bitclock-master
> +properties can be used as booleans in codec subnode to indicate if the
> +codec is the dai-link frame or bit clock master. In this case the same
> +properties should not be present at dai-link level and the
> +bitclock-inversion and frame-inversion properties should also be placed
> +in the codec node if needed.
>  
>  Required CPU/CODEC subnodes properties:
>  
	[snip]
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 21f1ccb..f4663d0 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,6 +8,7 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +#define DEBUG

Should be removed.

>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> @@ -88,7 +89,6 @@ 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,
> -			      unsigned int daifmt,
>  			      struct asoc_simple_dai *dai,
>  			      const struct device_node **p_node,
>  			      const char **name)
> @@ -117,14 +117,6 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  		return ret;
>  
>  	/*
> -	 * bitclock-inversion, frame-inversion
> -	 * bitclock-master,    frame-master
> -	 * and specific "format" if it has
> -	 */
> -	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> -	dai->fmt |= daifmt;
> -
> -	/*
>  	 * dai->sysclk come from
>  	 *  "clocks = <&xxx>" (if system has common clock)
>  	 *  or "system-clock-frequency = <xxx>"
> @@ -151,37 +143,134 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  	return 0;
>  }
>  
> -static int simple_card_cpu_codec_of(struct device_node *node,
> -				int daifmt,
> -				struct snd_soc_dai_link *dai_link,
> -				struct simple_dai_props *dai_props)
> +static int simple_card_dai_link_of(struct device_node *node,
> +				   struct device *dev,
> +				   struct snd_soc_dai_link *dai_link,
> +				   struct simple_dai_props *dai_props)
>  {
> -	struct device_node *np;
> +	struct device_node *np = NULL;
> +	struct device_node *bitclkmaster = NULL;
> +	struct device_node *framemaster = NULL;
> +	unsigned int daifmt;
> +	char *name;
> +	char prop[128];
> +	char *prefix = "";
>  	int ret;
>  
> -	/* CPU sub-node */
> -	ret = -EINVAL;
> -	np = of_get_child_by_name(node, "simple-audio-card,cpu");
> -	if (np) {
> -		ret = asoc_simple_card_sub_parse_of(np, daifmt,
> -						&dai_props->cpu_dai,
> -						&dai_link->cpu_of_node,
> -						&dai_link->cpu_dai_name);
> -		of_node_put(np);
> +	if (!strcmp("sound", node->name))
> +		prefix = "simple-audio-card,";
> +
> +	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> +					 &bitclkmaster, &framemaster);
> +	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> +
> +	snprintf(prop, sizeof(prop), "%scpu", prefix);
> +	np = of_get_child_by_name(node, prop);
> +	if (!np) {
> +		ret = -EINVAL;
> +		dev_err(dev, "%s: Can't find simple-audio-card,cpu DT node\n",
> +			__func__);
> +		goto dai_link_of_err;
>  	}
> +
> +	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
> +					    &dai_link->cpu_of_node,
> +					    &dai_link->cpu_dai_name);
>  	if (ret < 0)
> -		return ret;
> +		goto dai_link_of_err;
> +
> +	dai_props->cpu_dai.fmt = daifmt;
> +	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
> +	case 0x11:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +		break;
> +	case 0x10:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +		break;
> +	case 0x01:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +		break;
> +	default:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +		break;
> +	}
>  
> -	/* CODEC sub-node */
> -	ret = -EINVAL;
> -	np = of_get_child_by_name(node, "simple-audio-card,codec");
> -	if (np) {
> -		ret = asoc_simple_card_sub_parse_of(np, daifmt,
> -						&dai_props->codec_dai,
> -						&dai_link->codec_of_node,
> -						&dai_link->codec_dai_name);
> -		of_node_put(np);
> +	of_node_put(np);
> +	snprintf(prop, sizeof(prop), "%scodec", prefix);
> +	np = of_get_child_by_name(node, prop);
> +	if (!np) {
> +		ret = -EINVAL;
> +		dev_err(dev, "%s: Can't find simple-audio-card,codec DT node\n",
> +			__func__);
> +		goto dai_link_of_err;
>  	}
> +
> +	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
> +					    &dai_link->codec_of_node,
> +					    &dai_link->codec_dai_name);
> +	if (ret < 0)
> +		goto dai_link_of_err;
> +
> +	if (!bitclkmaster && !framemaster) {
> +		/* Master setting not found from dai_link level, revert back
> +		   to legacy DT parsing and take settings from codec node. */
> +		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
> +			__func__);
> +		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
> +			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
> +			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);

We are here each time there is no bitclock-master and no frame-master,
i.e. each time for me. It could be simpler to keep the first 'daifmt'
instead of parsing again.

> +	} else {
> +		dai_props->codec_dai.fmt = daifmt;
> +		switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
> +		case 0x11:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +			break;
> +		case 0x10:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +			break;
> +		case 0x01:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +			break;
> +		default:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +			break;
> +		}
> +	}
> +
> +	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;
> +
> +	/* 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,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", dai_link->cpu_dai_name,
> +				dai_link->codec_dai_name);
> +	dai_link->name = dai_link->stream_name = name;
> +
> +	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
> +	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
> +		dai_link->cpu_dai_name,
> +		dai_props->cpu_dai.fmt,
> +		dai_props->cpu_dai.sysclk);
> +	dev_dbg(dev, "\tcodec : %s / %04x / %d\n",
> +		dai_link->codec_dai_name,
> +		dai_props->codec_dai.fmt,
> +		dai_props->codec_dai.sysclk);
> +
> +dai_link_of_err:
> +	if (np)
> +		of_node_put(np);
> +	if (bitclkmaster)
> +		of_node_put(bitclkmaster);
> +	if (framemaster)
> +		of_node_put(framemaster);
>  	return ret;
>  }
>  
> @@ -192,18 +281,11 @@ static int asoc_simple_card_parse_of(struct device_node *node,
>  {
>  	struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
>  	struct simple_dai_props *dai_props = priv->dai_props;
> -	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 */
> -	daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
> -		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> -
>  	/* off-codec widgets */
>  	if (of_property_read_bool(node, "simple-audio-card,widgets")) {
>  		ret = snd_soc_of_parse_audio_simple_widgets(&priv->snd_card,
> @@ -220,71 +302,30 @@ static int asoc_simple_card_parse_of(struct device_node *node,
>  			return ret;
>  	}
>  
> -	/* loop on the DAI links */
> -	np = NULL;
> -	for (;;) {
> -		if (multi) {
> -			np = of_get_next_child(node, np);
> -			if (!np)
> -				break;
> -		}
> -
> -		ret = simple_card_cpu_codec_of(multi ? np : node,
> -					daifmt, dai_link, dai_props);
> -		if (ret < 0)
> -			goto err;
> -
> -		/*
> -		 * overwrite cpu_dai->fmt as its DAIFMT_MASTER bit is based on CODEC
> -		 * while the other bits should be identical unless buggy SW/HW design.
> -		 */
> -		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt;
> +	dev_dbg(dev, "New simple-card: %s\n", priv->snd_card.name ?
> +		priv->snd_card.name : "");
>  
> -		if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
> -			ret = -EINVAL;
> -			goto err;
> +	if (multi) {
> +		struct device_node *np = NULL;
> +		int i;
> +		for (i = 0; (np = of_get_next_child(node, np)); i++) {
> +			dev_dbg(dev, "\tlink %d:\n", i);
> +			ret = simple_card_dai_link_of(np, dev, dai_link + i,
> +						      dai_props + i);

I feel that my loop was quicker:

		struct device_node *np = NULL;

		for (;;) {
			np = of_get_next_child(node, np);
			if (!np)
				break;
			dev_dbg(dev, "\tlink %d:\n", dai_link - priv->snd_card.dai_link);
			ret = simple_card_dai_link_of(np, dev, dai_link++,
						      dai_props++);


> +			of_node_put(np);
> +			if (ret < 0)
> +				return ret;

The np reference count is updated in of_get_next_child(), so:

			if (ret < 0) {
				of_node_put(np);
				return ret;
			}

Otherwise, it works for me.

Tested-by: Jean-Francois Moine <moinejf@free.fr>

>  		}
> -
> -		/* 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;
> -
> -		if (!multi)
> -			break;
> -
> -		dai_link++;
> -		dai_props++;
> +	} else {
> +		ret = simple_card_dai_link_of(node, dev, dai_link, dai_props);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> -	/* card name is created from CPU/CODEC dai name */
> -	dai_link = priv->snd_card.dai_link;
>  	if (!priv->snd_card.name)
> -		priv->snd_card.name = dai_link->name;
> -
> -	dev_dbg(dev, "card-name : %s\n", priv->snd_card.name);
> -	dev_dbg(dev, "platform : %04x\n", daifmt);
> -	dai_props = priv->dai_props;
> -	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> -		dai_link->cpu_dai_name,
> -		dai_props->cpu_dai.fmt,
> -		dai_props->cpu_dai.sysclk);
> -	dev_dbg(dev, "codec : %s / %04x / %d\n",
> -		dai_link->codec_dai_name,
> -		dai_props->codec_dai.fmt,
> -		dai_props->codec_dai.sysclk);
> +		priv->snd_card.name = priv->snd_card.dai_link->name;
>  
>  	return 0;
> -
> -err:
> -	of_node_put(np);
> -	return ret;
>  }
>  
>  /* update the reference count of the devices nodes at end of probe */

-- 
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: [PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Jyri Sarha]

On 03/23/2014 11:54 AM, Jean-Francois Moine wrote:
> On Fri, 21 Mar 2014 18:47:23 +0200
> Jyri Sarha <jsarha@ti.com> wrote:
>
>> The properties like format, bitclock-master, frame-master,
>> bitclock-inversion, and frame-inversion should be common to the dais
>> connected with a dai-link. For bitclock-master and frame-master
>> properties to be unambiguous they need to indicate the mastering dai
>> node with a phandle.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
[...]
>> -Required subnodes:
>> +Requred dai-link subnodes:
>
> Typo: 'Required'
>

Fixed. Thanks!

[...]
>> --- a/sound/soc/generic/simple-card.c
>> +++ b/sound/soc/generic/simple-card.c
>> @@ -8,6 +8,7 @@
>>    * it under the terms of the GNU General Public License version 2 as
>>    * published by the Free Software Foundation.
>>    */
>> +#define DEBUG
>
> Should be removed.
>

Removed. Thanks!

[...]
>> +	if (!bitclkmaster && !framemaster) {
>> +		/* Master setting not found from dai_link level, revert back
>> +		   to legacy DT parsing and take settings from codec node. */
>> +		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
>> +			__func__);
>> +		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
>> +			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
>> +			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
>
> We are here each time there is no bitclock-master and no frame-master,
> i.e. each time for me. It could be simpler to keep the first 'daifmt'
> instead of parsing again.
>

Sorry, I missed this and comments bellow first time around. I'll make 
another patch set shortly.

Adding another check to test if we are parsing top level sound node 
would save you from the legacy mode parsing. I'll do that and update the
DT binding document accordingly.

[...]
>> +	if (multi) {
>> +		struct device_node *np = NULL;
>> +		int i;
>> +		for (i = 0; (np = of_get_next_child(node, np)); i++) {
>> +			dev_dbg(dev, "\tlink %d:\n", i);
>> +			ret = simple_card_dai_link_of(np, dev, dai_link + i,
>> +						      dai_props + i);
>
> I feel that my loop was quicker:
>

I was targeting for readability rather than speed. I doubt the 
difference is significant since most string processing is anyway taking 
most of the time anyway.

> 		struct device_node *np = NULL;
>
> 		for (;;) {
> 			np = of_get_next_child(node, np);
> 			if (!np)
> 				break;
> 			dev_dbg(dev, "\tlink %d:\n", dai_link - priv->snd_card.dai_link);
> 			ret = simple_card_dai_link_of(np, dev, dai_link++,
> 						      dai_props++);
>
>
>> +			of_node_put(np);
>> +			if (ret < 0)
>> +				return ret;
>
> The np reference count is updated in of_get_next_child(), so:
>
> 			if (ret < 0) {
> 				of_node_put(np);
> 				return ret;
> 			}
>

Oops, need to fix that. Thanks !

> Otherwise, it works for me.
>
> Tested-by: Jean-Francois Moine <moinejf@free.fr>
>

Best regards,
Jyri

Re: [alsa-devel] [PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Kuninori Morimoto]

Hi Jyri

> The properties like format, bitclock-master, frame-master,
> bitclock-inversion, and frame-inversion should be common to the dais
> connected with a dai-link. For bitclock-master and frame-master
> properties to be unambiguous they need to indicate the mastering dai
> node with a phandle.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
(snip)
>  	/*
> -	 * bitclock-inversion, frame-inversion
> -	 * bitclock-master,    frame-master
> -	 * and specific "format" if it has
> -	 */
> -	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> -	dai->fmt |= daifmt;
> -
> -	/*
>  	 * dai->sysclk come from
>  	 *  "clocks = <&xxx>" (if system has common clock)
>  	 *  or "system-clock-frequency = <xxx>"

[1/2] patch exchanged snd_soc_of_parse_daifmt() parameter,
but, user code exchanged in [2/2].
It breaks git-bisect.

> +	dai_props->cpu_dai.fmt = daifmt;
> +	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
> +	case 0x11:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +		break;
> +	case 0x10:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +		break;
> +	case 0x01:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +		break;
> +	default:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +		break;
> +	}

The user of snd_soc_of_parse_daifmt() is only simple-card (?)
I think SND_SOC_DAIFMT_CBxx_CFx cared by snd_soc_of_parse_daifmt() somehow
is very reasonable.


Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Jyri Sarha]

On 03/24/2014 02:05 AM, Kuninori Morimoto wrote:
>
> Hi Jyri
>
>> The properties like format, bitclock-master, frame-master,
>> bitclock-inversion, and frame-inversion should be common to the dais
>> connected with a dai-link. For bitclock-master and frame-master
>> properties to be unambiguous they need to indicate the mastering dai
>> node with a phandle.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
> (snip)
>>   	/*
>> -	 * bitclock-inversion, frame-inversion
>> -	 * bitclock-master,    frame-master
>> -	 * and specific "format" if it has
>> -	 */
>> -	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
>> -	dai->fmt |= daifmt;
>> -
>> -	/*
>>   	 * dai->sysclk come from
>>   	 *  "clocks = <&xxx>" (if system has common clock)
>>   	 *  or "system-clock-frequency = <xxx>"
>
> [1/2] patch exchanged snd_soc_of_parse_daifmt() parameter,
> but, user code exchanged in [2/2].
> It breaks git-bisect.
>

Fixed that. Thanks!

>> +	dai_props->cpu_dai.fmt = daifmt;
>> +	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
>> +	case 0x11:
>> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
>> +		break;
>> +	case 0x10:
>> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
>> +		break;
>> +	case 0x01:
>> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
>> +		break;
>> +	default:
>> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
>> +		break;
>> +	}
>
> The user of snd_soc_of_parse_daifmt() is only simple-card (?)
> I think SND_SOC_DAIFMT_CBxx_CFx cared by snd_soc_of_parse_daifmt() somehow
> is very reasonable.
>

Yes, that is what git grep tells me. snd_soc_of_parse_daifmt() can still 
take care of SND_SOC_DAIFMT_CB?_CF? parsing in simple cases. I just felt 
that adding the full phandle parsing to the function would break its 
genericity and make the function parameters hard to understand.

Best regards,
Jyri