[alsa-devel] [PATCHv2 0/4] simple-card: simplify the code.
And comment and advice are welcome.
Change in v2: - Maintian compatibility with the old DTs.
Change in v1: - Add simple-card dts node patches. - Fix format parsing bug from Jean-Francois's comment. - Rebase to Kuninori-san's newest changes in next branch.
Xiubo Li (4): ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code. ASoC: simple-card: Merge single and muti DAI link(s) code. ASoC: simple-card: Adjust the comments of simple card. ASoC: simple-card: binding: update binding to support the new style.
.../devicetree/bindings/sound/simple-card.txt | 184 ++++++++++++++------- sound/soc/generic/simple-card.c | 131 ++++++++------- 2 files changed, 191 insertions(+), 124 deletions(-)
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 986d2c7..cad2b30 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np, + struct device_node *bitclkmaster, + struct device_node *framemaster) +{ + switch (((np == bitclkmaster) << 4) | (np == framemaster)) { + case 0x11: + return SND_SOC_DAIFMT_CBS_CFS; + case 0x10: + return SND_SOC_DAIFMT_CBS_CFM; + case 0x01: + return SND_SOC_DAIFMT_CBM_CFS; + default: + return SND_SOC_DAIFMT_CBM_CFM; + } + + /* Shouldn't be here */ + return -EINVAL; +} + static int asoc_simple_card_dai_link_of(struct device_node *node, struct device *dev, struct snd_soc_dai_link *dai_link, @@ -172,7 +192,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, struct device_node *np = NULL; struct device_node *bitclkmaster = NULL; struct device_node *framemaster = NULL; - unsigned int daifmt; + unsigned int daifmt, fmt; char *name; char prop[128]; char *prefix = ""; @@ -185,6 +205,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, &bitclkmaster, &framemaster); daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
+ /* Parse CPU DAI sub-node */ snprintf(prop, sizeof(prop), "%scpu", prefix); np = of_get_child_by_name(node, prop); if (!np) { @@ -199,23 +220,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, if (ret < 0) 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; - } - + fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster); + dai_props->cpu_dai.fmt = daifmt | fmt; of_node_put(np); + + /* Parse CODEC DAI sub-node */ snprintf(prop, sizeof(prop), "%scodec", prefix); np = of_get_child_by_name(node, prop); if (!np) { @@ -240,21 +249,9 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, 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; - } + fmt = asoc_simple_card_fmt_master(np, bitclkmaster, + framemaster); + dai_props->codec_dai.fmt = daifmt | fmt; }
if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
On 09/02/2014 02:56 PM, Xiubo Li wrote:
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 986d2c7..cad2b30 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
On Tue, 02 Sep 2014 15:51:41 +0530 Varka Bhadram varkabhadram@gmail.com wrote:
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
I don't see which macros: the values are just 2 booleans.
On 09/02/2014 04:08 PM, Jean-Francois Moine wrote:
On Tue, 02 Sep 2014 15:51:41 +0530 Varka Bhadram varkabhadram@gmail.com wrote:
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
I don't see which macros: the values are just 2 booleans.
I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable names to those...?
At Tue, 02 Sep 2014 16:12:40 +0530, Varka Bhadram wrote:
On 09/02/2014 04:08 PM, Jean-Francois Moine wrote:
On Tue, 02 Sep 2014 15:51:41 +0530 Varka Bhadram varkabhadram@gmail.com wrote:
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
I don't see which macros: the values are just 2 booleans.
I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable names to those...?
The whole switch block is too hackish, makes unnecessarily complex. It can be more strightforwardly like:
if (np == bitclkmaster) return np == framemater ? SND_SOC_DAIFMT_CBS_CFS : SND_SOC_DAIFMT_CBS_CFM; else return np == framemaster ? SND_SOC_DAIFMT_CBM_CFS : SND_SOC_DAIFMT_CBM_CFM;
Or, if you love efficiency and complexity, something like:
#define SND_SOC_DAIFMT(_np, _clk, _frame) \ ((((_np) != (_clk)) | (((_np) != (_frame)) << 1) << 12) + (1 << 12)
Then return SND_SOC_DAIFMT(np, blkclkmaster, framemaster);
Takashi
On Tue, 02 Sep 2014 16:12:40 +0530 Varka Bhadram varkabhadram@gmail.com wrote:
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
I don't see which macros: the values are just 2 booleans.
I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable names to those...?
#define TRUE_TRUE 0x11 #define TRUE_FALSE 0x10 #define FALSE_TRUE 0x01
or
case ((TRUE << 4) | TRUE: ... case ((TRUE << 4) | FALSE: ... case ((FALSE << 4) | TRUE: ...
??
On 09/02/2014 02:09 PM, Jean-Francois Moine wrote:
On Tue, 02 Sep 2014 16:12:40 +0530 Varka Bhadram varkabhadram@gmail.com wrote:
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
It will be nice if we declare the switch case numbers as macros (specific name)...
I don't see which macros: the values are just 2 booleans.
I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable names to those...?
#define TRUE_TRUE 0x11 #define TRUE_FALSE 0x10 #define FALSE_TRUE 0x01
or
case ((TRUE << 4) | TRUE: ... case ((TRUE << 4) | FALSE: ... case ((FALSE << 4) | TRUE: ...
I would vote for this. Even over the options suggested by Takashi, but then again this really a matter of taste.
The fact that frame and bit-clock master boolean values are bundled into a single "enum" field, instead of two dedicated bits, makes all options bit inconvenient.
Best regards, Jyri
On 09/02/2014 12:26 PM, Xiubo Li wrote:
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 986d2c7..cad2b30 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
....
- fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster);
- dai_props->cpu_dai.fmt = daifmt | fmt;
...
fmt = asoc_simple_card_fmt_master(np, bitclkmaster,framemaster);dai_props->codec_dai.fmt = daifmt | fmt;
This won't work. The logic for cpu node needs to be negated for codec node.
Best regards, Jyri
Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
On 09/02/2014 12:26 PM, Xiubo Li wrote:
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
card.c
index 986d2c7..cad2b30 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
....
- fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster);
- dai_props->cpu_dai.fmt = daifmt | fmt;
...
fmt = asoc_simple_card_fmt_master(np, bitclkmaster,framemaster);dai_props->codec_dai.fmt = daifmt | fmt;This won't work. The logic for cpu node needs to be negated for codec node.
Yes, actually it should be.
As my previous patches about this: ---- Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx' mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's frame clock is as master/slave.
So these same DAI formats should be informed to CPU and CODE DAIs at the same time. For the Codec driver will set the bit clock and frame clock as the DAI formats said, but for the CPU driver, if the the bit clock or frame clock is as Codec master, so it should be set CPU DAI device as bit clock or frame clock as slave, and vice versa.
The old code will cause confusion, and we should be clear that the letter 'C' here mean to Codec. ----
For the master format, no matter for CPU or CODEC, it always means Codec is master or slave for bit/frame clock, not means the local DAI device's bit/frame clock as master or slave.
So your CPU DAI device driver should negate this locally as the existed Ones do.
Thanks,
BRs Xiubo
On 09/03/2014 05:37 AM, Li.Xiubo@freescale.com wrote:
Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master()
...
This won't work. The logic for cpu node needs to be negated for codec node.
Yes, actually it should be.
As my previous patches about this:
Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx' mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's frame clock is as master/slave.
So these same DAI formats should be informed to CPU and CODE DAIs at the same time. For the Codec driver will set the bit clock and frame clock as the DAI formats said, but for the CPU driver, if the the bit clock or frame clock is as Codec master, so it should be set CPU DAI device as bit clock or frame clock as slave, and vice versa.
The old code will cause confusion, and we should be clear that the letter 'C' here mean to Codec.
For the master format, no matter for CPU or CODEC, it always means Codec is master or slave for bit/frame clock, not means the local DAI device's bit/frame clock as master or slave.
So your CPU DAI device driver should negate this locally as the existed Ones do.
Yes, but there is double negation in this patch. The switch-case assignments depend on whether the bitclkmaster and framemaster DT-node pointers are compared to a cpu-dai-node or codec-dai-node. When your patch compares the codec-node, it does the decisions like it was a cpu-node, which produces inverted CBM and CFM setting.
However, Kurinori-san's patch fixes this problem because it just uses the daifmt generated by comparing to codec node for both cpu and codec nodes.
The reason why I did the comparison per node basis, was to make the code more ready for tdm setups with multiple codecs on a same wire. But writing code for something that is not really needed yet is usually a bad idea, like it was this time too.
Kurinori-san's version of the fix should be fine and it cleans up the code quite nicely.
Best regards, Jyri
Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
On 09/03/2014 05:37 AM, Li.Xiubo@freescale.com wrote:
Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add
asoc_simple_card_fmt_master() ...
This won't work. The logic for cpu node needs to be negated for codec node.
Yes, actually it should be.
As my previous patches about this:
Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx' mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's frame clock is as master/slave.
So these same DAI formats should be informed to CPU and CODE DAIs at the same time. For the Codec driver will set the bit clock and frame clock as the DAI formats said, but for the CPU driver, if the the bit clock or frame clock is as Codec master, so it should be set CPU DAI device as bit clock or frame clock as slave, and vice versa.
The old code will cause confusion, and we should be clear that the letter 'C' here mean to Codec.
For the master format, no matter for CPU or CODEC, it always means Codec is master or slave for bit/frame clock, not means the local DAI device's bit/frame clock as master or slave.
So your CPU DAI device driver should negate this locally as the existed Ones do.
Yes, but there is double negation in this patch. The switch-case assignments depend on whether the bitclkmaster and framemaster DT-node pointers are compared to a cpu-dai-node or codec-dai-node. When your patch compares the codec-node, it does the decisions like it was a cpu-node, which produces inverted CBM and CFM setting.
However, Kurinori-san's patch fixes this problem because it just uses the daifmt generated by comparing to codec node for both cpu and codec nodes.
The reason why I did the comparison per node basis, was to make the code more ready for tdm setups with multiple codecs on a same wire. But writing code for something that is not really needed yet is usually a bad idea, like it was this time too.
Kurinori-san's version of the fix should be fine and it cleans up the code quite nicely.
Yes, agree.
So I just removed this patch from my patch series list.
Kuninori-san will post his local patch about this later.
Thanks,
BRs Xiubo
Hi Xiubo
I was very surprised about this patch because the idea is same as my local patch (I was planned to send it to ML :)
I attached my local patch to sharing idea.
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
I think this concept is nice, but setting all fmt in this function is good for me see my local patch
---------- From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Thu, 28 Aug 2014 19:20:14 +0900 Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()
Current daifmt setting method in simple-card driver is placed to many places, and using un-readable/confusable method. This patch adds new asoc_simple_card_parse_daifmt() and tidyup code.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index bea5901..c932103 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static int asoc_simple_card_parse_daifmt(struct device_node *node, + struct simple_card_data *priv, + struct device_node *cpu, + struct device_node *codec, + char *prefix, int idx) +{ + struct device *dev = simple_priv_to_dev(priv); + struct device_node *bitclkmaster = NULL; + struct device_node *framemaster = NULL; + struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx); + struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai; + struct asoc_simple_dai *codec_dai = &dai_props->codec_dai; + unsigned int daifmt; + + daifmt = snd_soc_of_parse_daifmt(node, prefix, + &bitclkmaster, &framemaster); + daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK; + + if (strlen(prefix) && !bitclkmaster && !framemaster) { + /* + * No dai-link level and master setting was not found from + * sound node level, revert back to legacy DT parsing and + * take the settings from codec node. + */ + dev_dbg(dev, "Revert to legacy daifmt parsing\n"); + + cpu_dai->fmt = codec_dai->fmt = + snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) | + (daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK); + } else { + + switch (((codec == bitclkmaster) << 4) | (codec == framemaster)) { + case 0x11: + daifmt |= SND_SOC_DAIFMT_CBM_CFM; + break; + case 0x10: + daifmt |= SND_SOC_DAIFMT_CBM_CFS; + break; + case 0x01: + daifmt |= SND_SOC_DAIFMT_CBS_CFM; + break; + default: + daifmt |= SND_SOC_DAIFMT_CBS_CFS; + break; + } + + cpu_dai->fmt = daifmt; + codec_dai->fmt = daifmt; + } + + if (bitclkmaster) + of_node_put(bitclkmaster); + if (framemaster) + of_node_put(framemaster); + + return 0; +} + static int asoc_simple_card_dai_link_of(struct device_node *node, struct simple_card_data *priv, int idx, @@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, struct device *dev = simple_priv_to_dev(priv); struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx); struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx); - struct device_node *np = NULL; - struct device_node *bitclkmaster = NULL; - struct device_node *framemaster = NULL; - unsigned int daifmt; + struct device_node *cpu = NULL; + struct device_node *codec = NULL; char *name; char prop[128]; char *prefix = ""; @@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, if (is_top_level_node) 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) { + cpu = of_get_child_by_name(node, prop); + + snprintf(prop, sizeof(prop), "%scodec", prefix); + codec = of_get_child_by_name(node, prop); + + if (!cpu || !codec) { ret = -EINVAL; dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop); 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); + ret = asoc_simple_card_parse_daifmt(node, priv, + cpu, codec, prefix, idx); if (ret < 0) 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; - } - - 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 %s DT node\n", __func__, prop); + ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai, + &dai_link->cpu_of_node, + &dai_link->cpu_dai_name); + if (ret < 0) goto dai_link_of_err; - }
- ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai, + ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai, &dai_link->codec_of_node, &dai_link->codec_dai_name); if (ret < 0) goto dai_link_of_err;
- if (strlen(prefix) && !bitclkmaster && !framemaster) { - /* No dai-link level and master setting was not found from - sound node level, revert back to legacy DT parsing and - take the 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; @@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, dai_link->cpu_dai_name = NULL;
dai_link_of_err: - if (np) - of_node_put(np); - if (bitclkmaster) - of_node_put(bitclkmaster); - if (framemaster) - of_node_put(framemaster); + if (cpu) + of_node_put(cpu); + if (codec) + of_node_put(codec); return ret; }
---------
Best regards --- Kuninori Morimoto
Hi Kuninori-san,
Yes, I think it make sense to set all fmt in one function, and will Be more readable.
I agree with you, could you please just wait, because there has many Replications and good Ideas about this patch, and I will revise it. Then you can improve it as your patch blow.
Thanks,
BRs Xiubo
Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
Hi Xiubo
I was very surprised about this patch because the idea is same as my local patch (I was planned to send it to ML :)
I attached my local patch to sharing idea.
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
I think this concept is nice, but setting all fmt in this function is good for me see my local patch
From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Thu, 28 Aug 2014 19:20:14 +0900 Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()
Current daifmt setting method in simple-card driver is placed to many places, and using un-readable/confusable method. This patch adds new asoc_simple_card_parse_daifmt() and tidyup code.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index bea5901..c932103 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static int asoc_simple_card_parse_daifmt(struct device_node *node,
struct simple_card_data *priv,struct device_node *cpu,struct device_node *codec,char *prefix, int idx)+{
- struct device *dev = simple_priv_to_dev(priv);
- struct device_node *bitclkmaster = NULL;
- struct device_node *framemaster = NULL;
- struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
- struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
- struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
- unsigned int daifmt;
- daifmt = snd_soc_of_parse_daifmt(node, prefix,
&bitclkmaster, &framemaster);- daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
- if (strlen(prefix) && !bitclkmaster && !framemaster) {
/** No dai-link level and master setting was not found from* sound node level, revert back to legacy DT parsing and* take the settings from codec node.*/dev_dbg(dev, "Revert to legacy daifmt parsing\n");cpu_dai->fmt = codec_dai->fmt =snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);- } else {
switch (((codec == bitclkmaster) << 4) | (codec == framemaster)){
case 0x11:daifmt |= SND_SOC_DAIFMT_CBM_CFM;break;case 0x10:daifmt |= SND_SOC_DAIFMT_CBM_CFS;break;case 0x01:daifmt |= SND_SOC_DAIFMT_CBS_CFM;break;default:daifmt |= SND_SOC_DAIFMT_CBS_CFS;break;}cpu_dai->fmt = daifmt;codec_dai->fmt = daifmt;- }
- if (bitclkmaster)
of_node_put(bitclkmaster);- if (framemaster)
of_node_put(framemaster);- return 0;
+}
static int asoc_simple_card_dai_link_of(struct device_node *node, struct simple_card_data *priv, int idx, @@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, struct device *dev = simple_priv_to_dev(priv); struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx); struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
- struct device_node *np = NULL;
- struct device_node *bitclkmaster = NULL;
- struct device_node *framemaster = NULL;
- unsigned int daifmt;
- struct device_node *cpu = NULL;
- struct device_node *codec = NULL; char *name; char prop[128]; char *prefix = "";
@@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, if (is_top_level_node) 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) {
- cpu = of_get_child_by_name(node, prop);
- snprintf(prop, sizeof(prop), "%scodec", prefix);
- codec = of_get_child_by_name(node, prop);
- if (!cpu || !codec) { ret = -EINVAL; dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop); 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);
- ret = asoc_simple_card_parse_daifmt(node, priv,
cpu, codec, prefix, idx);
- 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;- }
- 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 %s DT node\n", __func__, prop);
- ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
&dai_link->cpu_of_node,&dai_link->cpu_dai_name);- if (ret < 0) goto dai_link_of_err;
}
ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
- ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai, &dai_link->codec_of_node, &dai_link->codec_dai_name); if (ret < 0) goto dai_link_of_err;
- if (strlen(prefix) && !bitclkmaster && !framemaster) {
/* No dai-link level and master setting was not found fromsound node level, revert back to legacy DT parsing andtake the 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;
@@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, dai_link->cpu_dai_name = NULL;
dai_link_of_err:
- if (np)
of_node_put(np);- if (bitclkmaster)
of_node_put(bitclkmaster);- if (framemaster)
of_node_put(framemaster);
- if (cpu)
of_node_put(cpu);- if (codec)
of_node_put(codec);}
Best regards
Kuninori Morimoto
Hi Xiubo
Yes, I think it make sense to set all fmt in one function, and will Be more readable.
I agree with you, could you please just wait, because there has many Replications and good Ideas about this patch, and I will revise it. Then you can improve it as your patch blow.
Thank you for your help I don't care so much about my local patch. You can re-use it if you want. Of course I can do it if you want
Best regards --- Kuninori Morimoto
Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
Hi Xiubo
Yes, I think it make sense to set all fmt in one function, and will Be more readable.
I agree with you, could you please just wait, because there has many Replications and good Ideas about this patch, and I will revise it. Then you can improve it as your patch blow.
Thank you for your help I don't care so much about my local patch. You can re-use it if you want. Of course I can do it if you want
Please send it out of your local patch.
Please also consider the ideas about Jyri, Jean-Francios, Varka and Takashi's advice as previous emails about my patch.
BRs Xiubo
Hi Kuninori-san,
Yes your patch has fixed the bug Jyri has pointed out.
So I has discard my [PATCHv2 1/4] patch.
Please send your patch out to replace this one.
Thanks,
BRs Xiubo
-----Original Message----- From: Xiubo Li-B47053 Sent: Wednesday, September 03, 2014 10:22 AM To: 'Kuninori Morimoto' Cc: broonie@kernel.org; perex@perex.cz; lgirdwood@gmail.com; tiwai@suse.de; moinejf@free.fr; andrew@lunn.ch; kuninori.morimoto.gx@renesas.com; jsarha@ti.com; devicetree@vger.kernel.org; alsa-devel@alsa-project.org; robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org; linux- kernel@vger.kernel.org Subject: RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
Hi Kuninori-san,
Yes, I think it make sense to set all fmt in one function, and will Be more readable.
I agree with you, could you please just wait, because there has many Replications and good Ideas about this patch, and I will revise it. Then you can improve it as your patch blow.
Thanks,
BRs Xiubo
Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
Hi Xiubo
I was very surprised about this patch because the idea is same as my local patch (I was planned to send it to ML :)
I attached my local patch to sharing idea.
+static inline unsigned int +asoc_simple_card_fmt_master(struct device_node *np,
struct device_node *bitclkmaster,struct device_node *framemaster)+{
- switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
- case 0x11:
return SND_SOC_DAIFMT_CBS_CFS;- case 0x10:
return SND_SOC_DAIFMT_CBS_CFM;- case 0x01:
return SND_SOC_DAIFMT_CBM_CFS;- default:
return SND_SOC_DAIFMT_CBM_CFM;- }
- /* Shouldn't be here */
- return -EINVAL;
+}
I think this concept is nice, but setting all fmt in this function is good for me see my local patch
From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Thu, 28 Aug 2014 19:20:14 +0900 Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()
Current daifmt setting method in simple-card driver is placed to many places, and using un-readable/confusable method. This patch adds new asoc_simple_card_parse_daifmt() and tidyup code.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
card.c
index bea5901..c932103 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return 0; }
+static int asoc_simple_card_parse_daifmt(struct device_node *node,
struct simple_card_data *priv,struct device_node *cpu,struct device_node *codec,char *prefix, int idx)+{
- struct device *dev = simple_priv_to_dev(priv);
- struct device_node *bitclkmaster = NULL;
- struct device_node *framemaster = NULL;
- struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
- struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
- struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
- unsigned int daifmt;
- daifmt = snd_soc_of_parse_daifmt(node, prefix,
&bitclkmaster, &framemaster);- daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
- if (strlen(prefix) && !bitclkmaster && !framemaster) {
/** No dai-link level and master setting was not found from* sound node level, revert back to legacy DT parsing and* take the settings from codec node.*/dev_dbg(dev, "Revert to legacy daifmt parsing\n");cpu_dai->fmt = codec_dai->fmt =snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);- } else {
switch (((codec == bitclkmaster) << 4) | (codec == framemaster)){
case 0x11:daifmt |= SND_SOC_DAIFMT_CBM_CFM;break;case 0x10:daifmt |= SND_SOC_DAIFMT_CBM_CFS;break;case 0x01:daifmt |= SND_SOC_DAIFMT_CBS_CFM;break;default:daifmt |= SND_SOC_DAIFMT_CBS_CFS;break;}cpu_dai->fmt = daifmt;codec_dai->fmt = daifmt;- }
- if (bitclkmaster)
of_node_put(bitclkmaster);- if (framemaster)
of_node_put(framemaster);- return 0;
+}
static int asoc_simple_card_dai_link_of(struct device_node *node, struct simple_card_data *priv, int idx, @@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, struct device *dev = simple_priv_to_dev(priv); struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx); struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
- struct device_node *np = NULL;
- struct device_node *bitclkmaster = NULL;
- struct device_node *framemaster = NULL;
- unsigned int daifmt;
- struct device_node *cpu = NULL;
- struct device_node *codec = NULL; char *name; char prop[128]; char *prefix = "";
@@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, if (is_top_level_node) 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) {
- cpu = of_get_child_by_name(node, prop);
- snprintf(prop, sizeof(prop), "%scodec", prefix);
- codec = of_get_child_by_name(node, prop);
- if (!cpu || !codec) { ret = -EINVAL; dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop); 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);
- ret = asoc_simple_card_parse_daifmt(node, priv,
cpu, codec, prefix, idx);
- 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;- }
- 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 %s DT node\n", __func__, prop);
- ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
&dai_link->cpu_of_node,&dai_link->cpu_dai_name);- if (ret < 0) goto dai_link_of_err;
}
ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
- ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai, &dai_link->codec_of_node, &dai_link->codec_dai_name); if (ret < 0) goto dai_link_of_err;
- if (strlen(prefix) && !bitclkmaster && !framemaster) {
/* No dai-link level and master setting was not found fromsound node level, revert back to legacy DT parsing andtake the 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;
@@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, dai_link->cpu_dai_name = NULL;
dai_link_of_err:
- if (np)
of_node_put(np);- if (bitclkmaster)
of_node_put(bitclkmaster);- if (framemaster)
of_node_put(framemaster);
- if (cpu)
of_node_put(cpu);- if (codec)
of_node_put(codec);}
Best regards
Kuninori Morimoto
Hi Xiubo
Yes your patch has fixed the bug Jyri has pointed out.
So I has discard my [PATCHv2 1/4] patch.
Please send your patch out to replace this one.
(snip)
Please send it out of your local patch.
Please also consider the ideas about Jyri, Jean-Francios, Varka and Takashi's advice as previous emails about my patch.
OK, will do. To avoid confusion/conflict, I will post it after Mark applied it. Because many simple-card patches are posted in these days...
Best regards --- Kuninori Morimoto
Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.
Hi Xiubo
Yes your patch has fixed the bug Jyri has pointed out.
So I has discard my [PATCHv2 1/4] patch.
Please send your patch out to replace this one.
(snip)
Please send it out of your local patch.
Please also consider the ideas about Jyri, Jean-Francios, Varka and Takashi's advice as previous emails about my patch.
OK, will do. To avoid confusion/conflict, I will post it after Mark applied it. Because many simple-card patches are posted in these days...
Yes, Get it.
Thanks,
BRs Xiubo
On Tue, 02 Sep 2014 21:13:52 -0700 (PDT) Kuninori Morimoto kuninori.morimoto.gx@gmail.com wrote:
OK, will do. To avoid confusion/conflict, I will post it after Mark applied it. Because many simple-card patches are posted in these days...
Yes, I have one more awaiting, about multi-CODECs...
This patch will split the DT node into old style and new style: The new style will merge the single DAI link and muti DAI links code together, the new style will be easier to add muti DAI links from old single DAI link DTs.
This patch will maintian compatibility with the old DTs.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/generic/simple-card.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index cad2b30..667fa49 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -198,6 +198,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, char *prefix = ""; int ret;
+ /* For single DAI link & old style of DT node */ if (is_top_level_node) prefix = "simple-audio-card,";
@@ -306,14 +307,16 @@ dai_link_of_err:
static int asoc_simple_card_parse_of(struct device_node *node, struct simple_card_data *priv, - struct device *dev, - int multi) + struct device *dev) { struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link; struct simple_dai_props *dai_props = priv->dai_props; u32 val; int ret;
+ if (!node) + return -EINVAL; + /* parsing the card name from DT */ snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
@@ -341,7 +344,8 @@ static int asoc_simple_card_parse_of(struct device_node *node, dev_dbg(dev, "New simple-card: %s\n", priv->snd_card.name ? priv->snd_card.name : "");
- if (multi) { + /* Single/Muti DAI link(s) & New style of DT node */ + if (of_get_child_by_name(node, "simple-audio-card,dai-link")) { struct device_node *np = NULL; int i = 0;
@@ -358,6 +362,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, i++; } } else { + /* For single DAI link & old style of DT node */ ret = asoc_simple_card_dai_link_of(node, dev, dai_link, dai_props, true); if (ret < 0) @@ -397,16 +402,13 @@ 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 num_links, multi, ret; + int num_links, ret;
/* get the number of DAI links */ - if (np && of_get_child_by_name(np, "simple-audio-card,dai-link")) { + if (np && of_get_child_by_name(np, "simple-audio-card,dai-link")) num_links = of_get_child_count(np); - multi = 1; - } else { + else num_links = 1; - multi = 0; - }
/* allocate the private data and the DAI link array */ priv = devm_kzalloc(dev, @@ -433,7 +435,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
if (np && of_device_is_available(np)) {
- ret = asoc_simple_card_parse_of(np, priv, dev, multi); + ret = asoc_simple_card_parse_of(np, priv, dev); if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(dev, "parse error %d\n", ret);
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/generic/simple-card.c | 48 ++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 667fa49..e6976a0 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -120,7 +120,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np, int ret;
/* - * get node via "sound-dai = <&phandle port>" + * Get node via "sound-dai = <&phandle port>" * it will be used as xxx_of_node on soc_bind_dai_link() */ node = of_parse_phandle(np, "sound-dai", 0); @@ -128,19 +128,19 @@ asoc_simple_card_sub_parse_of(struct device_node *np, return -ENODEV; *p_node = node;
- /* get dai->name */ + /* Get dai->name */ ret = snd_soc_of_get_dai_name(np, name); if (ret < 0) return ret;
- /* parse TDM slot */ + /* Parse TDM slot */ ret = snd_soc_of_parse_tdm_slot(np, &dai->slots, &dai->slot_width); if (ret) return ret;
/* - * dai->sysclk come from - * "clocks = <&xxx>" (if system has common clock) + * Parse dai->sysclk come from "clocks = <&xxx>" + * (if system has common clock) * or "system-clock-frequency = <xxx>" * or device's module clock. */ @@ -241,9 +241,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, goto dai_link_of_err;
if (strlen(prefix) && !bitclkmaster && !framemaster) { - /* No dai-link level and master setting was not found from - sound node level, revert back to legacy DT parsing and - take the settings from codec node. */ + /* + * No DAI link level and master setting was found + * from sound node level, revert back to legacy DT + * parsing and take the 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 = @@ -260,10 +262,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, goto dai_link_of_err; }
- /* simple-card assumes platform == cpu */ + /* Simple Card assumes platform == cpu */ dai_link->platform_of_node = dai_link->cpu_of_node;
- /* Link name is created from CPU/CODEC dai name */ + /* DAI link name is created from CPU/CODEC dai name */ name = devm_kzalloc(dev, strlen(dai_link->cpu_dai_name) + strlen(dai_link->codec_dai_name) + 2, @@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, dai_props->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 + * In 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. Please see: * fmt_single_name() * fmt_multiple_name() */ @@ -317,10 +319,10 @@ static int asoc_simple_card_parse_of(struct device_node *node, if (!node) return -EINVAL;
- /* parsing the card name from DT */ + /* Parse the card name from DT */ snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
- /* off-codec widgets */ + /* The off-codec widgets */ if (of_property_read_bool(node, "simple-audio-card,widgets")) { ret = snd_soc_of_parse_audio_simple_widgets(&priv->snd_card, "simple-audio-card,widgets"); @@ -375,7 +377,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, return 0; }
-/* update the reference count of the devices nodes at end of probe */ +/* Decrease the reference count of the device nodes */ static int asoc_simple_card_unref(struct platform_device *pdev) { struct snd_soc_card *card = platform_get_drvdata(pdev); @@ -404,29 +406,27 @@ static int asoc_simple_card_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; int num_links, ret;
- /* get the number of DAI links */ + /* Get the number of DAI links */ if (np && of_get_child_by_name(np, "simple-audio-card,dai-link")) num_links = of_get_child_count(np); else num_links = 1;
- /* allocate the private data and the DAI link array */ + /* Allocate the private data and the DAI link array */ priv = devm_kzalloc(dev, sizeof(*priv) + sizeof(*dai_link) * num_links, GFP_KERNEL); if (!priv) return -ENOMEM;
- /* - * init snd_soc_card - */ + /* Init snd_soc_card */ priv->snd_card.owner = THIS_MODULE; priv->snd_card.dev = dev; dai_link = priv->dai_link; priv->snd_card.dai_link = dai_link; priv->snd_card.num_links = num_links;
- /* get room for the other properties */ + /* Get room for the other properties */ priv->dai_props = devm_kzalloc(dev, sizeof(*priv->dai_props) * num_links, GFP_KERNEL);
On Tue, 2 Sep 2014 17:26:08 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
@@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node, dai_props->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
* In 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. Please see:*/
- fmt_single_name()
- fmt_multiple_name()
The patch done by Kuninori, setting the cpu_dai_name to NULL in all cases, does not work. This sequence should be replaced where is was previously.
Subject: Re: [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.
On Tue, 2 Sep 2014 17:26:08 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
@@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct
device_node *node,
dai_props->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
* In 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. Please see:*/
- fmt_single_name()
- fmt_multiple_name()
The patch done by Kuninori, setting the cpu_dai_name to NULL in all cases, does not work. This sequence should be replaced where is was previously.
If so, it should be another issue here, should we send another patch for It ?
Thanks,
BRs Xiubo
Hi Xiubo
/*
* 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
* In 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. Please see:*/
- fmt_single_name()
- fmt_multiple_name()
The patch done by Kuninori, setting the cpu_dai_name to NULL in all cases, does not work. This sequence should be replaced where is was previously.
If so, it should be another issue here, should we send another patch for It ?
I posted patch yesterday, and Jean tested it
Subject: [PATCH][RFC] ASoC: simple-card: fixup cpu_dai_name clear case Date: Tue, 02 Sep 2014 20:05:32 +0900
Best regards --- Kuninori Morimoto
Subject: Re: [alsa-devel] [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.
Hi Xiubo
/*
* 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 byfmt_single_name()
* remove cpu_dai_name to escape name matching.* see
* In 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. Please see:*/
- fmt_single_name()
- fmt_multiple_name()
The patch done by Kuninori, setting the cpu_dai_name to NULL in all cases, does not work. This sequence should be replaced where is was previously.
If so, it should be another issue here, should we send another patch for It ?
I posted patch yesterday, and Jean tested it
Subject: [PATCH][RFC] ASoC: simple-card: fixup cpu_dai_name clear case Date: Tue, 02 Sep 2014 20:05:32 +0900
Nice.
I think I just missed it.
Thanks,
BRs Xiubo
This update patch will split the DT node into old style and new style: The new style will will be easier to add muti DAI links from old single DAI link DTs.
This patch will maintian compatibility with the old DTs.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- .../devicetree/bindings/sound/simple-card.txt | 184 ++++++++++++++------- 1 file changed, 126 insertions(+), 58 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt index c2e9841..6fb8966 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.txt +++ b/Documentation/devicetree/bindings/sound/simple-card.txt @@ -1,15 +1,19 @@ -Simple-Card: +Device-Tree bindings for Simple Card
Simple-Card specifies audio DAI connections of SoC <-> codec.
-Required properties: +=== Top level's properties and subnodes ===
+*** Required properties *** - compatible : "simple-audio-card"
-Optional properties: - +*** Optional properties *** - 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" + (This is used for single DAI link & old style.) - 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 @@ -17,63 +21,85 @@ Optional properties: source. - simple-audio-card,mclk-fs : Multiplication factor between stream rate and codec mclk. - -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 dai-link subnodes: - -- cpu : CPU sub-node -- codec : CODEC sub-node - -Optional dai-link subnode properties: - +- simple-audio-card,frame-master : Indicates DAI link frame master. One phandle to a cpu + or codec subnode. + (This is used for single DAI link & old style.) +- simple-audio-card,bitclock-master : Indicates DAI link bit clock master. One phandle to a + cpu or codec subnode. + (This is used for single DAI link & old style.) + +*** 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 and using the old + style. See the examples and the section bellow. +- simple-audio-card,cpu : CPU DAI sub-node. + (This is used for single DAI link & old style.) +- simple-audio-card,codec : CODEC DAI sub-node. + (This is used for single DAI link & old style.) + +=== DAI link node's properties and its subnodes === + +*** Required subnodes *** +- cpu : CPU DAI sub-node +- codec : CODEC DAI sub-node + +*** Optional 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. +- frame-master : Indicates DAI link frame master. One phandle to a cpu + or codec subnode. + (This is One boolean property for old style.) +- bitclock-master : Indicates DAI link bit clock master. One phandle to a + cpu or codec subnode. + (This is one boolean property for old style.)
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 there -should be no dai-link node, the same properties should not be present +codec is the DAI link frame or bit clock master. In this case there +should be no DAI link node, the same properties should not be present at sound-node level, and the bitclock-inversion and frame-inversion properties should also be placed in the codec node if needed.
-Required CPU/CODEC subnodes properties:
-- sound-dai : phandle and port of CPU/CODEC +=== CPU/CODEC DAI node's properties and its subnodes ===
-Optional CPU/CODEC subnodes properties: +*** Required properties *** +- sound-dai : One phandle and port of CPU/CODEC
+*** Optional properties *** +- bitclock-inversion : Boolean property. Add this if the DAI device uses bit + clock inversion. +- frame-inversion : Boolean property. Add this if the DAI device uses frame + clock 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) +- clocks / system-clock-frequency : specify CPU/CODEC DAI node'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)
-Example 1 - single DAI link: +=== Examples === +*** CPU & CODEC DAI DT nodes *** +&i2c0 { + ak4648: ak4648@12 { + #sound-dai-cells = <0>; + compatible = "asahi-kasei,ak4648"; + reg = <0x12>; + }; +};
+sh_fsi2: sh_fsi2@ec230000 { + #sound-dai-cells = <1>; + compatible = "renesas,sh_fsi2"; + reg = <0xec230000 0x400>; + interrupt-parent = <&gic>; + interrupts = <0 146 0x4>; +}; + +Example 1 - single DAI link & old style: +bitclock-master and frame-master as phandles. sound { compatible = "simple-audio-card"; simple-audio-card,name = "VF610-Tower-Sound-Card"; @@ -91,6 +117,7 @@ sound {
simple-audio-card,cpu { sound-dai = <&sh_fsi2 0>; + bitclock-inversion; };
dailink0_master: simple-audio-card,codec { @@ -99,24 +126,64 @@ sound { }; };
-&i2c0 { - ak4648: ak4648@12 { - #sound-dai-cells = <0>; - compatible = "asahi-kasei,ak4648"; - reg = <0x12>; +Example 2 - single DAI link & old style: +bitclock-master and frame-master as boolean properties. +sound { + compatible = "simple-audio-card"; + simple-audio-card,name = "VF610-Tower-Sound-Card"; + simple-audio-card,format = "left_j"; + simple-audio-card,widgets = + "Microphone", "Microphone Jack", + "Headphone", "Headphone Jack", + "Speaker", "External Speaker"; + simple-audio-card,routing = + "MIC_IN", "Microphone Jack", + "Headphone Jack", "HP_OUT", + "External Speaker", "LINE_OUT"; + + simple-audio-card,cpu { + sound-dai = <&sh_fsi2 0>; }; -};
-sh_fsi2: sh_fsi2@ec230000 { - #sound-dai-cells = <1>; - compatible = "renesas,sh_fsi2"; - reg = <0xec230000 0x400>; - interrupt-parent = <&gic>; - interrupts = <0 146 0x4>; + simple-audio-card,codec { + sound-dai = <&ak4648>; + clocks = <&osc>; + bitclock-master; + frame-master; + bitclock-inversion; + }; };
-Example 2 - many DAI links: +Example 3 - single DAI link & new style: +sound { + compatible = "simple-audio-card"; + simple-audio-card,name = "VF610-Tower-Sound-Card"; + simple-audio-card,widgets = + "Microphone", "Microphone Jack", + "Headphone", "Headphone Jack", + "Speaker", "External Speaker"; + simple-audio-card,routing = + "MIC_IN", "Microphone Jack", + "Headphone Jack", "HP_OUT", + "External Speaker", "LINE_OUT"; + + simple-audio-card,dai-link { + format = "i2s"; + bitclock-master = <&dailink0_master>; + frame-master = <&dailink0_master>; + cpu { + sound-dai = <&sh_fsi2 0>; + frame-inversion; + }; + dailink0_master: codec { + sound-dai = <&ak4648>; + clocks = <&osc>; + frame-inversion; + }; + }; +};
+Example 4 - many DAI links: sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; @@ -128,6 +195,7 @@ sound { }; codec { sound-dai = <&tda998x 0>; + frame-inversion; }; };
On Tue, 2 Sep 2014 17:26:09 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
+Example 4 - many DAI links: sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; @@ -128,6 +195,7 @@ sound { }; codec { sound-dai = <&tda998x 0>;
frame-inversion;
This is not useful: there is no clock/frame handling in the kirkwood audio controller.
Subject: Re: [PATCHv2 4/4] ASoC: simple-card: binding: update binding to support the new style.
On Tue, 2 Sep 2014 17:26:09 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
+Example 4 - many DAI links: sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; @@ -128,6 +195,7 @@ sound { }; codec { sound-dai = <&tda998x 0>;
frame-inversion;This is not useful: there is no clock/frame handling in the kirkwood audio controller.
Okay, just for one example, if it really matter here I will remove this.
Thanks,
BRs Xiubo
participants (7)
-
Jean-Francois Moine -
Jyri Sarha -
Kuninori Morimoto -
Li.Xiubo＠freescale.com -
Takashi Iwai -
Varka Bhadram -
Xiubo Li