[alsa-devel] [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) - 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(-)
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,
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.
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; }
/*
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
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!
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
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.
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.
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;
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.
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>; + }; +};
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@free.fr
[...]
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
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.
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?
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.
On 03/18/2014 10:17 AM, Jean-Francois Moine wrote:
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]
...
I agree. I see two possible syntaxes:
- 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
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).
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
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.
On Wed, 19 Mar 2014 12:08:55 +0200 Jyri Sarha jsarha@ti.com 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.
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.
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
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.
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
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
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
(Mark Brown)
- change subject/comment about device node reference count
(Li Xiubo)
- use a null size array instead of an implicit area for the DAI links
- 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
participants (5)
-
Jean-Francois Moine -
Jyri Sarha -
Lars-Peter Clausen -
Li.Xiubo＠freescale.com -
Mark Brown