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

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 */