[RFC][PATCH 0/2] ASoC: add DT support on simple-card

[RFC][PATCH 0/2] ASoC: add DT support on simple-card

[Kuninori Morimoto]

Hi Mark

These patches add DT support for soc-simple-card driver.

Kuninori Morimoto (2):
      ASoC: add snd_soc_of_parse_daifmt()
      ASoC: simple-card: add DT support

 include/sound/soc.h             |    2 +
 sound/soc/generic/simple-card.c |   90 +++++++++++++++++++++++++++++++++++++--
 sound/soc/soc-core.c            |   72 +++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+), 3 deletions(-)

1st patch adds snd_soc_of_parse_daifmt() to select SND_SOC_DAIFMT_xxx,
but I'm not sure whether this is good idea.
So these are [RFC] patches.

I thought that it will be trouble if...
  1) platform/driver DT used snd_fmt = <0xYYYY> style for SND_SOC_DAIFMT_xxx
  2) someone might update SND_SOC_DAIFMT_xxx value for some reasons.
  3) bootloader should update DT value according to kernel version if 2) happened

I put snd_soc_of_parse_daifmt() on soc-core.c because I thought that other driver can reuse it.
I need your comment for it.

Best regards
---
Kuninori Morimoto

[RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Kuninori Morimoto]

ALSA SoC system is using SND_SOC_DAIFMT_xxx flags on each platform,
and its value might be updated for some reason.
This means that if platform is using Device Tree
and if it gets parameter value directly,
it is difficult to keep compatible on each platform Device Tree.
This patch adds snd_soc_of_parse_daifmt() to solve this issue.
Each platform can use [prefix]snd,soc,daifmt,xxx to set
SND_SOC_DAIFMT_XXX on Device Tree.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |    2 ++
 sound/soc/soc-core.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 91244a0..af64632 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1168,6 +1168,8 @@ int snd_soc_of_parse_card_name(struct snd_soc_card *card,
 			       const char *propname);
 int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 				   const char *propname);
+int snd_soc_of_parse_daifmt(struct device_node *np,
+			    const char *prefix, unsigned int *fmt);
 
 #include <sound/soc-dai.h>
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 9c768bc..0142742 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4179,6 +4179,78 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
 
+static int __snd_soc_of_parse_daifmt(struct device_node *np,
+				     const char *prefix, unsigned int *fmt,
+				     const char *propname, unsigned int val)
+{
+	char str[128];
+	int ret;
+
+	snprintf(str, 128, "%ssnd,soc,daifmt,%s", prefix, propname);
+	ret = of_property_read_bool(np, str);
+	if (ret)
+		*fmt |= val;
+
+	return ret;
+}
+
+int snd_soc_of_parse_daifmt(struct device_node *np,
+			    const char *prefix, unsigned int *fmt)
+{
+	int ret = 0;
+	char pre[] = "";
+
+	if (!prefix)
+		prefix = pre;
+
+	/*
+	 * it will find "[prefix]snd,soc,daifmt,xxx" from device_node,
+	 * and set SND_SOC_DAIFMT_XXX
+	 */
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "i2s", SND_SOC_DAIFMT_I2S);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "left_j", SND_SOC_DAIFMT_LEFT_J);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "dsp_a", SND_SOC_DAIFMT_DSP_A);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "dsp_b", SND_SOC_DAIFMT_DSP_B);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "ac97", SND_SOC_DAIFMT_AC97);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "pdm", SND_SOC_DAIFMT_PDM);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "msb", SND_SOC_DAIFMT_MSB);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "lsb", SND_SOC_DAIFMT_LSB);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "cont", SND_SOC_DAIFMT_CONT);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "gated", SND_SOC_DAIFMT_GATED);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "nb_nf", SND_SOC_DAIFMT_NB_NF);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "nb_if", SND_SOC_DAIFMT_NB_IF);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "ib_nf", SND_SOC_DAIFMT_IB_NF);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "ib_if", SND_SOC_DAIFMT_IB_IF);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "cbm_cfm", SND_SOC_DAIFMT_CBM_CFM);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "cbs_cfm", SND_SOC_DAIFMT_CBS_CFM);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "cbm_cfs", SND_SOC_DAIFMT_CBM_CFS);
+	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
+					 "cbs_cfs", SND_SOC_DAIFMT_CBS_CFS);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt);
+
+
 static int __init snd_soc_init(void)
 {
 #ifdef CONFIG_DEBUG_FS
-- 
1.7.9.5

[RFC][PATCH 2/2] ASoC: simple-card: add DT support

[Kuninori Morimoto]

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/simple-card.c |   90 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b4b4cab..a59e88c 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -47,12 +48,89 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static void asoc_simple_card_parse_of(struct device_node *np,
+				      struct asoc_simple_card_info *cinfo,
+				      struct device *dev)
+{
+	struct asoc_simple_dai_init_info *iinfo;
+	unsigned int cpu_daifmt = 0;
+	unsigned int codec_daifmt = 0;
+	unsigned int sysclk = 0;
+
+	/*
+	 * it will find
+	 *
+	 * iinfo,cpu,snd,soc,daifmt,xxx
+	 * iinfo,codec,snd,soc,daifmt,xxx
+	 * iinfo,sysclk
+	 */
+	snd_soc_of_parse_daifmt(np, "iinfo,cpu,",	&cpu_daifmt);
+	snd_soc_of_parse_daifmt(np, "iinfo,codec,",	&codec_daifmt);
+	of_property_read_u32(np, "iinfo,sysclk",	&sysclk);
+
+	if (cpu_daifmt || codec_daifmt || sysclk) {
+		iinfo = devm_kzalloc(dev, sizeof(*iinfo), GFP_KERNEL);
+		if (!iinfo)
+			return;
+
+		cinfo->init		= iinfo;
+		iinfo->cpu_daifmt	= cpu_daifmt;
+		iinfo->codec_daifmt	= codec_daifmt;
+		iinfo->sysclk		= sysclk;
+	}
+
+	/*
+	 * it will find
+	 *
+	 * cinfo,xxx
+	 */
+	of_property_read_string(np, "cinfo,name",	&cinfo->name);
+	of_property_read_string(np, "cinfo,card",	&cinfo->card);
+	of_property_read_string(np, "cinfo,cpu_dai",	&cinfo->cpu_dai);
+	of_property_read_string(np, "cinfo,codec",	&cinfo->codec);
+	of_property_read_string(np, "cinfo,platform",	&cinfo->platform);
+	of_property_read_string(np, "cinfo,codec_dai",	&cinfo->codec_dai);
+
+	/*
+	 * debug info
+	 */
+	if (cinfo->name)
+		dev_dbg(dev, "name      = %s\n", cinfo->name);
+	if (cinfo->card)
+		dev_dbg(dev, "card      = %s\n", cinfo->card);
+	if (cinfo->cpu_dai)
+		dev_dbg(dev, "cpu_dai   = %s\n", cinfo->cpu_dai);
+	if (cinfo->codec)
+		dev_dbg(dev, "codec     = %s\n", cinfo->codec);
+	if (cinfo->platform)
+		dev_dbg(dev, "platform  = %s\n", cinfo->platform);
+	if (cinfo->codec_dai)
+		dev_dbg(dev, "codec_dai = %s\n", cinfo->codec_dai);
+	if (iinfo && iinfo->cpu_daifmt)
+		dev_dbg(dev, "cpu_daifmt = %08x\n", iinfo->cpu_daifmt);
+	if (iinfo && iinfo->codec_daifmt)
+		dev_dbg(dev, "codec_daifmt = %08x\n", iinfo->codec_daifmt);
+	if (iinfo && iinfo->sysclk)
+		dev_dbg(dev, "iinfo,sysclk = %d\n", iinfo->sysclk);
+}
+
 static int asoc_simple_card_probe(struct platform_device *pdev)
 {
-	struct asoc_simple_card_info *cinfo = pdev->dev.platform_data;
+	struct asoc_simple_card_info *cinfo;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+
+	cinfo = NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo)
+			asoc_simple_card_parse_of(np, cinfo, dev);
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
 
 	if (!cinfo) {
-		dev_err(&pdev->dev, "no info for asoc-simple-card\n");
+		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
 	}
 
@@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	    !cinfo->codec	||
 	    !cinfo->platform	||
 	    !cinfo->codec_dai) {
-		dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n");
+		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
 
@@ -99,9 +177,15 @@ static int asoc_simple_card_remove(struct platform_device *pdev)
 	return snd_soc_unregister_card(&cinfo->snd_card);
 }
 
+static const struct of_device_id asoc_simple_of_match[] = {
+	{ .compatible = "asoc,simple-card", },
+	{},
+};
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

Re: [RFC][PATCH 2/2] ASoC: simple-card: add DT support

[Stephen Warren]

On 11/28/2012 09:32 PM, Kuninori Morimoto wrote:

This could benefit from a patch description.

A file in Documentation/devicetree/bindings is required to specify the
format of the device tree.

Property name prefixes such as "cinfo," aren't very descriptive; what
does that mean?

A property name prefix such as "asoc," sounds Linux- (ASoC-) specific;
DT is supposed to represent HW, and hence shouldn't be influenced by OS
naming, etc.

Looking at the code, I think the machine driver is binding to the other
components by string name. With DT, it should be using phandles to point
at them instead.

The sysclk value that's parsed from DT doesn't appear to be used. How
does clocking work with this driver; what about when the sample-rate
changes, etc.?

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Stephen Warren]

On 11/28/2012 09:31 PM, Kuninori Morimoto wrote:
> ALSA SoC system is using SND_SOC_DAIFMT_xxx flags on each platform,
> and its value might be updated for some reason.
> This means that if platform is using Device Tree
> and if it gets parameter value directly,
> it is difficult to keep compatible on each platform Device Tree.
> This patch adds snd_soc_of_parse_daifmt() to solve this issue.
> Each platform can use [prefix]snd,soc,daifmt,xxx to set
> SND_SOC_DAIFMT_XXX on Device Tree.

> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c

> +static int __snd_soc_of_parse_daifmt(struct device_node *np,
> +				     const char *prefix, unsigned int *fmt,
> +				     const char *propname, unsigned int val)
> +{
> +	char str[128];
> +	int ret;
> +
> +	snprintf(str, 128, "%ssnd,soc,daifmt,%s", prefix, propname);
> +	ret = of_property_read_bool(np, str);
> +	if (ret)
> +		*fmt |= val;
> +
> +	return ret;
> +}
> +
> +int snd_soc_of_parse_daifmt(struct device_node *np,
> +			    const char *prefix, unsigned int *fmt)
> +{
> +	int ret = 0;
> +	char pre[] = "";
> +
> +	if (!prefix)
> +		prefix = pre;
> +
> +	/*
> +	 * it will find "[prefix]snd,soc,daifmt,xxx" from device_node,
> +	 * and set SND_SOC_DAIFMT_XXX
> +	 */
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "i2s", SND_SOC_DAIFMT_I2S);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "left_j", SND_SOC_DAIFMT_LEFT_J);
...

I think it'd be more typical to represent as a single integer property,
where the value is an enumeration indicating the type.

But isn't there a lot more to the DAI format than just the format enum
itself?

Re: [RFC][PATCH 2/2] ASoC: simple-card: add DT support

[Kuninori Morimoto]

Hi Stephen

> This could benefit from a patch description.
> 
> A file in Documentation/devicetree/bindings is required to specify the
> format of the device tree.
> 
> Property name prefixes such as "cinfo," aren't very descriptive; what
> does that mean?
> 
> A property name prefix such as "asoc," sounds Linux- (ASoC-) specific;
> DT is supposed to represent HW, and hence shouldn't be influenced by OS
> naming, etc.
>
> Looking at the code, I think the machine driver is binding to the other
> components by string name. With DT, it should be using phandles to point
> at them instead.

I see. I fix it up.
Thank you for your description.

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Mark Brown]

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

On Wed, Nov 28, 2012 at 10:21:59PM -0700, Stephen Warren wrote:

> But isn't there a lot more to the DAI format than just the format enum
> itself?

There is but it should be OK to represent that as separate properties
rather than trying to glom all the combinations into a single property.
For the frame inversion flags we can assume a reasonable default of non
inverted so it seems OK to leave them for now.

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

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

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Kuninori Morimoto]

Hi Stephen

> I think it'd be more typical to represent as a single integer property,
> where the value is an enumeration indicating the type.

Sorry, I couldn't understand correctly this.
Do you mean like this ?

  snd.soc.daifmt.format      = <3> /* SND_SOC_DAIFMT_LEFT_J */
  snd.soc.daifmt.clock_gate  = <1> /* SND_SOC_DAIFMT_CONT */
  snd.soc.daifmt.inversion   = <3> /* SND_SOC_DAIFMT_IB_NF */
  snd.soc.daifmt.hw_clock    = <2> /* SND_SOC_DAIFMT_CBS_CFM */

I added SND_SOC_DAIFMT_xxx here,
but this style is not readable/understandable for me.
And it is difficult to keep compatible if the number was changed.
(never happen ? I'm not sure)

How about to use string ?

  snd.soc.daifmt.format      = "left_j"
  snd.soc.daifmt.clock_gate  = "cont"
  snd.soc.daifmt.inversion   = "ib_nf"
  snd.soc.daifmt.hw_clock    = "cbs_cfm"

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH 2/2] ASoC: simple-card: add DT support

[Daniel Mack]

Hi Kuninori,

On 29.11.2012 05:32, Kuninori Morimoto wrote:
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/generic/simple-card.c |   90 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index b4b4cab..a59e88c 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -47,12 +48,89 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static void asoc_simple_card_parse_of(struct device_node *np,
> +				      struct asoc_simple_card_info *cinfo,
> +				      struct device *dev)
> +{
> +	struct asoc_simple_dai_init_info *iinfo;
> +	unsigned int cpu_daifmt = 0;
> +	unsigned int codec_daifmt = 0;
> +	unsigned int sysclk = 0;
> +
> +	/*
> +	 * it will find
> +	 *
> +	 * iinfo,cpu,snd,soc,daifmt,xxx
> +	 * iinfo,codec,snd,soc,daifmt,xxx
> +	 * iinfo,sysclk
> +	 */
> +	snd_soc_of_parse_daifmt(np, "iinfo,cpu,",	&cpu_daifmt);
> +	snd_soc_of_parse_daifmt(np, "iinfo,codec,",	&codec_daifmt);
> +	of_property_read_u32(np, "iinfo,sysclk",	&sysclk);
> +
> +	if (cpu_daifmt || codec_daifmt || sysclk) {
> +		iinfo = devm_kzalloc(dev, sizeof(*iinfo), GFP_KERNEL);
> +		if (!iinfo)
> +			return;
> +
> +		cinfo->init		= iinfo;
> +		iinfo->cpu_daifmt	= cpu_daifmt;
> +		iinfo->codec_daifmt	= codec_daifmt;
> +		iinfo->sysclk		= sysclk;
> +	}
> +
> +	/*
> +	 * it will find
> +	 *
> +	 * cinfo,xxx
> +	 */
> +	of_property_read_string(np, "cinfo,name",	&cinfo->name);
> +	of_property_read_string(np, "cinfo,card",	&cinfo->card);
> +	of_property_read_string(np, "cinfo,cpu_dai",	&cinfo->cpu_dai);
> +	of_property_read_string(np, "cinfo,codec",	&cinfo->codec);
> +	of_property_read_string(np, "cinfo,platform",	&cinfo->platform);
> +	of_property_read_string(np, "cinfo,codec_dai",	&cinfo->codec_dai);


CPUs, codecs and platforms should be referenced by phandles rather than
by string. The ASoC core is well prepared for this, by using the
dai_link's *_of_node members.


> +	/*
> +	 * debug info
> +	 */
> +	if (cinfo->name)
> +		dev_dbg(dev, "name      = %s\n", cinfo->name);
> +	if (cinfo->card)
> +		dev_dbg(dev, "card      = %s\n", cinfo->card);
> +	if (cinfo->cpu_dai)
> +		dev_dbg(dev, "cpu_dai   = %s\n", cinfo->cpu_dai);
> +	if (cinfo->codec)
> +		dev_dbg(dev, "codec     = %s\n", cinfo->codec);
> +	if (cinfo->platform)
> +		dev_dbg(dev, "platform  = %s\n", cinfo->platform);
> +	if (cinfo->codec_dai)
> +		dev_dbg(dev, "codec_dai = %s\n", cinfo->codec_dai);
> +	if (iinfo && iinfo->cpu_daifmt)
> +		dev_dbg(dev, "cpu_daifmt = %08x\n", iinfo->cpu_daifmt);
> +	if (iinfo && iinfo->codec_daifmt)
> +		dev_dbg(dev, "codec_daifmt = %08x\n", iinfo->codec_daifmt);
> +	if (iinfo && iinfo->sysclk)
> +		dev_dbg(dev, "iinfo,sysclk = %d\n", iinfo->sysclk);
> +}
> +
>  static int asoc_simple_card_probe(struct platform_device *pdev)
>  {
> -	struct asoc_simple_card_info *cinfo = pdev->dev.platform_data;
> +	struct asoc_simple_card_info *cinfo;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +
> +	cinfo = NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo)
> +			asoc_simple_card_parse_of(np, cinfo, dev);
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
>  
>  	if (!cinfo) {
> -		dev_err(&pdev->dev, "no info for asoc-simple-card\n");
> +		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
>  	}
>  
> @@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	    !cinfo->codec	||
>  	    !cinfo->platform	||
>  	    !cinfo->codec_dai) {
> -		dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n");
> +		dev_err(dev, "insufficient asoc_simple_card_info settings\n");

That's an unrelated change that should probably go into a separate commit.


Daniel

Re: [RFC][PATCH 2/2] ASoC: simple-card: add DT support

[Kuninori Morimoto]

Hi Daniel

Thank you for checking this patch.

> > +	of_property_read_string(np, "cinfo,name",	&cinfo->name);
> > +	of_property_read_string(np, "cinfo,card",	&cinfo->card);
> > +	of_property_read_string(np, "cinfo,cpu_dai",	&cinfo->cpu_dai);
> > +	of_property_read_string(np, "cinfo,codec",	&cinfo->codec);
> > +	of_property_read_string(np, "cinfo,platform",	&cinfo->platform);
> > +	of_property_read_string(np, "cinfo,codec_dai",	&cinfo->codec_dai);
> 
> 
> CPUs, codecs and platforms should be referenced by phandles rather than
> by string. The ASoC core is well prepared for this, by using the
> dai_link's *_of_node members.

OK. I fix it in v2

> > @@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
> >  	    !cinfo->codec	||
> >  	    !cinfo->platform	||
> >  	    !cinfo->codec_dai) {
> > -		dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n");
> > +		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
> 
> That's an unrelated change that should probably go into a separate commit.

OK. I'll do it in v2

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Stephen Warren]

On 11/29/2012 05:35 PM, Kuninori Morimoto wrote:
> 
> Hi Stephen
> 
>> I think it'd be more typical to represent as a single integer property,
>> where the value is an enumeration indicating the type.
> 
> Sorry, I couldn't understand correctly this.
> Do you mean like this ?
>
>   snd.soc.daifmt.format      = <3> /* SND_SOC_DAIFMT_LEFT_J */
>   snd.soc.daifmt.clock_gate  = <1> /* SND_SOC_DAIFMT_CONT */
>   snd.soc.daifmt.inversion   = <3> /* SND_SOC_DAIFMT_IB_NF */
>   snd.soc.daifmt.hw_clock    = <2> /* SND_SOC_DAIFMT_CBS_CFM */

Sorry for the slow response.

What I meant was that rather than:

> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "i2s", SND_SOC_DAIFMT_I2S);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "left_j", SND_SOC_DAIFMT_LEFT_J);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "dsp_a", SND_SOC_DAIFMT_DSP_A);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "dsp_b", SND_SOC_DAIFMT_DSP_B);

I'd expect to see something more like:

fmt = of_property_read_u32_array(np, "bit-format");

But perhaps your binding is representing the set of possible legal
formats? I had assumed it was attempting to represent the single
specific format to choose.

Related, I also wasn't suggesting combining format, clock-gate,
inversion, ... into a single property.

> I added SND_SOC_DAIFMT_xxx here,
> but this style is not readable/understandable for me.
> And it is difficult to keep compatible if the number was changed.
> (never happen ? I'm not sure)

Well, once a DT binding is created, you can't change the numbers, or you
would break the ability for an old DT to work with a newer kernel.

> How about to use string ?
> 
>   snd.soc.daifmt.format      = "left_j"
>   snd.soc.daifmt.clock_gate  = "cont"
>   snd.soc.daifmt.inversion   = "ib_nf"
>   snd.soc.daifmt.hw_clock    = "cbs_cfm"

That's probably the best we can do for now. Using a pre-processor would
be best:

#define SND_SOC_DAIFMT_LEFT_J 3

snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;

... but we can't do that yet...

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Kuninori Morimoto]

Hi Stephen
Cc  Mark

Thank you for your reply

> > +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> > +					 "i2s", SND_SOC_DAIFMT_I2S);
> > +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> > +					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
> > +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> > +					 "left_j", SND_SOC_DAIFMT_LEFT_J);
> > +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> > +					 "dsp_a", SND_SOC_DAIFMT_DSP_A);
> > +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> > +					 "dsp_b", SND_SOC_DAIFMT_DSP_B);
> 
> I'd expect to see something more like:
> 
> fmt = of_property_read_u32_array(np, "bit-format");

Ahh... I see.

> Well, once a DT binding is created, you can't change the numbers, or you
> would break the ability for an old DT to work with a newer kernel.

OK. I understand.

> > How about to use string ?
> > 
> >   snd.soc.daifmt.format      = "left_j"
> >   snd.soc.daifmt.clock_gate  = "cont"
> >   snd.soc.daifmt.inversion   = "ib_nf"
> >   snd.soc.daifmt.hw_clock    = "cbs_cfm"
> 
> That's probably the best we can do for now. Using a pre-processor would
> be best:
> 
> #define SND_SOC_DAIFMT_LEFT_J 3
> 
> snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;
> 
> ... but we can't do that yet...

Thank you.

I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style

	[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";

I would like to know your opinion.

----------------------------------------------
This patch adds snd_soc_of_parse_daifmt() and supports below style on DT.

	[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 91244a0..af64632 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1168,6 +1168,8 @@ int snd_soc_of_parse_card_name(struct snd_soc_card *card,
 			       const char *propname);
 int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 				   const char *propname);
+int snd_soc_of_parse_daifmt(struct device_node *np,
+			    const char *prefix, unsigned int *fmt);
 
 #include <sound/soc-dai.h>
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 9c768bc..6cac98a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4179,6 +4179,74 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
 
+int snd_soc_of_parse_daifmt(struct device_node *np,
+			    const char *prefix, unsigned int *fmt)
+{
+	int num, i, j;
+	int ret;
+	char prop[128];
+	unsigned int format;
+	const char *str;
+	struct {
+		char *name;
+		unsigned int val;
+		unsigned int msk;
+	} of_parse_table[] = {
+	{ "i2s",	SND_SOC_DAIFMT_I2S,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "right_j",	SND_SOC_DAIFMT_RIGHT_J,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "left_j",	SND_SOC_DAIFMT_LEFT_J,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "dsp_a",	SND_SOC_DAIFMT_DSP_A,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "dsp_b",	SND_SOC_DAIFMT_DSP_B,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "ac97",	SND_SOC_DAIFMT_AC97,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "pdm",	SND_SOC_DAIFMT_PDM,	SND_SOC_DAIFMT_FORMAT_MASK},
+	{ "msb",	SND_SOC_DAIFMT_MSB,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "lsb",	SND_SOC_DAIFMT_LSB,	SND_SOC_DAIFMT_FORMAT_MASK },
+	{ "cont",	SND_SOC_DAIFMT_CONT,	SND_SOC_DAIFMT_CLOCK_MASK },
+	{ "gated",	SND_SOC_DAIFMT_GATED,	SND_SOC_DAIFMT_CLOCK_MASK },
+		/*	SND_SOC_DAIFMT_NB_NF is default */
+	{ "nb_if",	SND_SOC_DAIFMT_NB_IF,	SND_SOC_DAIFMT_INV_MASK },
+	{ "ib_nf",	SND_SOC_DAIFMT_IB_NF,	SND_SOC_DAIFMT_INV_MASK },
+	{ "ib_if",	SND_SOC_DAIFMT_IB_IF,	SND_SOC_DAIFMT_INV_MASK },
+	{ "cbm_cfm",	SND_SOC_DAIFMT_CBM_CFM,	SND_SOC_DAIFMT_MASTER_MASK },
+	{ "cbs_cfm",	SND_SOC_DAIFMT_CBS_CFM,	SND_SOC_DAIFMT_MASTER_MASK },
+	{ "cbm_cfs",	SND_SOC_DAIFMT_CBM_CFS,	SND_SOC_DAIFMT_MASTER_MASK },
+	{ "cbs_cfs",	SND_SOC_DAIFMT_CBS_CFS,	SND_SOC_DAIFMT_MASTER_MASK },
+	};
+
+	if (!prefix)
+		prefix = "";
+
+	/*
+	 * it finds "[prefix]snd,soc,daifmt = xxx" from device_node,
+	 * and set SND_SOC_DAIFMT_XXX
+	 */
+	snprintf(prop, sizeof(prop), "%ssnd,soc,daifmt", prefix);
+	num = of_property_count_strings(np, prop);
+	if (num < 0)
+		return num;
+
+	format = 0;
+	for (i = 0; i < num; i++) {
+		ret = of_property_read_string_index(np, prop, i, &str);
+		if (ret < 0)
+			return ret;
+
+		for (j = 0; j < ARRAY_SIZE(of_parse_table); j++) {
+			if (strcmp(str, of_parse_table[j].name) == 0) {
+				if (format & of_parse_table[j].msk)
+					return -EINVAL;
+
+				format |= of_parse_table[j].val;
+			}
+		}
+	}
+
+	*fmt = format;
+
+	return num;
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt);
+
 static int __init snd_soc_init(void)
 {
 #ifdef CONFIG_DEBUG_FS

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Stephen Warren]

On 12/05/2012 12:55 AM, Kuninori Morimoto wrote:
> 
> Hi Stephen
> Cc  Mark
> 
> Thank you for your reply
> 
>>> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
>>> +					 "i2s", SND_SOC_DAIFMT_I2S);
>>> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
>>> +					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
>>> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
>>> +					 "left_j", SND_SOC_DAIFMT_LEFT_J);
>>> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
>>> +					 "dsp_a", SND_SOC_DAIFMT_DSP_A);
>>> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
>>> +					 "dsp_b", SND_SOC_DAIFMT_DSP_B);
>>
>> I'd expect to see something more like:
>>
>> fmt = of_property_read_u32_array(np, "bit-format");
> 
> Ahh... I see.
> 
>> Well, once a DT binding is created, you can't change the numbers, or you
>> would break the ability for an old DT to work with a newer kernel.
> 
> OK. I understand.
> 
>>> How about to use string ?
>>>
>>>   snd.soc.daifmt.format      = "left_j"
>>>   snd.soc.daifmt.clock_gate  = "cont"
>>>   snd.soc.daifmt.inversion   = "ib_nf"
>>>   snd.soc.daifmt.hw_clock    = "cbs_cfm"
>>
>> That's probably the best we can do for now. Using a pre-processor would
>> be best:
>>
>> #define SND_SOC_DAIFMT_LEFT_J 3
>>
>> snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;
>>
>> ... but we can't do that yet...
> 
> Thank you.
> 
> I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style
> 
> 	[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";

I assume you mean i2s not i2c there.

That seems to be overloading one property so that it contains a lot of
separate data items. I'd expect separate properties for the format, the
bitclock inversion, the frame inversion, the bitclock master, and the
frame master.

Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Kuninori Morimoto]

Hi Stephen

> > I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style
> > 
> > 	[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";
> 
> I assume you mean i2s not i2c there.

Oops, indeed.

> That seems to be overloading one property so that it contains a lot of
> separate data items. I'd expect separate properties for the format, the
> bitclock inversion, the frame inversion, the bitclock master, and the
> frame master.

OK. I'll try fix it.

Best regards
---
Kuninori Morimoto