[PATCH v3] ASoC: simple-card: add Device Tree support

[PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information,
and .of_xlate_dai_name support on each component driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v1 -> v2

 - add common clock support, system-clock-frequency can over-write it
 - add some comment on code

v2 -> v3

 - add devicetree ML address

 .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
 sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
 2 files changed, 226 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..75bbc5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,85 @@
+Simple-Card:
+
+Required properties:
+
+- compatible				: "simple-audio"
+- simple-audio,card-name		: simple-audio card name
+- simple-audio,cpu			: CPU sub-node, see below
+- simple-audio,codec			: CODEC sub-node, see below
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common format, see below
+
+Required cpu/codec subnode properties:
+
+- sound-dai				: phandle and port for CPU/CODEC
+- #sound-dai-cells			: sound-dai phandle's node
+
+Optional subnode properties:
+
+- format				: specific format if needed, see below
+- frame-master				: frame master
+- bitclock-master			: bitclock master
+- bitclock-inversion			: clock inversion
+- frame-inversion			: frame inversion
+- clocks				: phandle for system clock rate
+- system-clock-frequency		: system clock rate
+					  it will overwrite clocks's rate
+
+simple-audio,format
+	"i2s"
+	"right_j"
+	"left_j"
+	"dsp_a"
+	"dsp_b"
+	"ac97"
+	"pdm"
+	"msb"
+	"lsb"
+
+Example:
+
+clock {
+	osc: oscillator {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <11289600>;
+	};
+};
+
+sound {
+	compatible = "simple-audio";
+
+	simple-audio,card-name = "FSI2A-AK4648";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+		/* it can use this instead of clocks
+		 * system-clock-frequency = <11289600>; */
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@0x12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@0xec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 8c49147..62befbd 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+__asoc_simple_card_parse_of(struct device_node *np,
+			    struct asoc_simple_dai *dai,
+			    struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * 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);
+	if (!*node)
+		return -ENODEV;
+
+	of_node_put(*node);
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/* dai->sysclk via "clolks = <xxx>" */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		dai->sysclk = 0;
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	/*
+	 * overwrite dai->sysclk if it has
+	 * "system-clock-frequency = <xxx>"
+	 */
+	of_property_read_u32(np,
+			     "system-clock-frequency",
+			     &dai->sysclk);
+
+	return 0;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	/* get card name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	info->name = info->card;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU/CODEC sub-node */
+	for_each_child_of_node(node, np) {
+		if (0 == strcmp("simple-audio,cpu", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->cpu_dai,
+							  of_cpu);
+		if (0 == strcmp("simple-audio,codec", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->codec_dai,
+							  of_codec);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x / %p\n",
+		info->daifmt,
+		*of_platform);
+	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk,
+		*of_cpu);
+	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk,
+		*of_codec);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -64,10 +190,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -81,6 +207,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -102,10 +231,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Thu, Oct 03, 2013 at 05:04:41PM -0700, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information,
> and .of_xlate_dai_name support on each component driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

CCing in the DT maintainers directly.

> ---
> v1 -> v2
> 
>  - add common clock support, system-clock-frequency can over-write it
>  - add some comment on code
> 
> v2 -> v3
> 
>  - add devicetree ML address
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
>  sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
>  2 files changed, 226 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name
> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node
> +
> +Optional subnode properties:
> +
> +- format				: specific format if needed, see below
> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +- bitclock-inversion			: clock inversion
> +- frame-inversion			: frame inversion
> +- clocks				: phandle for system clock rate
> +- system-clock-frequency		: system clock rate
> +					  it will overwrite clocks's rate
> +
> +simple-audio,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"
> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +		/* it can use this instead of clocks
> +		 * system-clock-frequency = <11289600>; */
> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {
> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {
> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai,
> +			    struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	of_node_put(*node);
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/* dai->sysclk via "clolks = <xxx>" */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		dai->sysclk = 0;
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	/*
> +	 * overwrite dai->sysclk if it has
> +	 * "system-clock-frequency = <xxx>"
> +	 */
> +	of_property_read_u32(np,
> +			     "system-clock-frequency",
> +			     &dai->sysclk);
> +
> +	return 0;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	/* get card name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU/CODEC sub-node */
> +	for_each_child_of_node(node, np) {
> +		if (0 == strcmp("simple-audio,cpu", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->cpu_dai,
> +							  of_cpu);
> +		if (0 == strcmp("simple-audio,codec", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->codec_dai,
> +							  of_codec);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x / %p\n",
> +		info->daifmt,
> +		*of_platform);
> +	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk,
> +		*of_cpu);
> +	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk,
> +		*of_codec);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -64,10 +190,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -81,6 +207,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -102,10 +231,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 
> 

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

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

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Rutland]

Hi,

On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information,
> and .of_xlate_dai_name support on each component driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 
>  - add common clock support, system-clock-frequency can over-write it
>  - add some comment on code
> 
> v2 -> v3
> 
>  - add devicetree ML address
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
>  sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
>  2 files changed, 226 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:

It's really difficult to review this without a description. Please explain what
this binding represents.

What hardware is this applicable to?

Does this inherit from some class of binding?

> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name

What's this used for?

> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below

Describe these as required subnodes. Nodes are not properties.

> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node

This description makes no sense, the organisation seems structurally wrong.

What does this mean? What does it affect?

> +
> +Optional subnode properties:
> +
> +- format				: specific format if needed, see below
> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +- bitclock-inversion			: clock inversion
> +- frame-inversion			: frame inversion

What do these mean? Repeating the name without a dash is completely unhelpful.
Describe what these imply.

What type are they?

Which subnode(s) do they apply to?

> +- clocks				: phandle for system clock rate

Just one clock?

Nit: clocks are specified with a phandle + clock-specifier pair, not just a phandle.

> +- system-clock-frequency		: system clock rate
> +					  it will overwrite clocks's rate

This seems very odd.

Why do you want to overwrite a clock's rate?

> +
> +simple-audio,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"

What do these mean? Why are they not described when the property was defined above?

Does this also apply for the "format" property?

> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +		/* it can use this instead of clocks
> +		 * system-clock-frequency = <11289600>; */

This just confuses matters. If ou want to show this off, have two examples.

> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {

Nit: remove the 0x on the unit-address

> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {

Again, fix the unit-address please.

> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai,
> +			    struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	of_node_put(*node);

Why?

You're refrering to it, so why do you not want to have it refcounted?

It could disappear under your feet.

> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/* dai->sysclk via "clolks = <xxx>" */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		dai->sysclk = 0;
> +	else
> +		dai->sysclk = clk_get_rate(clk);

This seems like an odd assumption to make. Why not error?

> +
> +	/*
> +	 * overwrite dai->sysclk if it has
> +	 * "system-clock-frequency = <xxx>"
> +	 */
> +	of_property_read_u32(np,
> +			     "system-clock-frequency",
> +			     &dai->sysclk);

Is dai->sysclk defined as a u32?

> +
> +	return 0;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	/* get card name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;

What if the string is not in the DT?

Does the core code handle these being NULL?

> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU/CODEC sub-node */
> +	for_each_child_of_node(node, np) {
> +		if (0 == strcmp("simple-audio,cpu", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->cpu_dai,
> +							  of_cpu);
> +		if (0 == strcmp("simple-audio,codec", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->codec_dai,
> +							  of_codec);

of_get_child_by_name?

> +		if (ret < 0)
> +			return ret;
> +	}

What if there were no children?

> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x / %p\n",
> +		info->daifmt,
> +		*of_platform);

Why is the pointer helpful, rather than the info it points to?

Thanks,
Mark.

[PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v3 -> v4

 - explain detail of each properties on simple-card.txt
 - fixup odd examples on simple-card.txt
 - remove "simple-card,card-name". create it from cpu/codec name
 - use of_get_child_by_name()
 - remove odd pointer info from dev_dbg()
 - remove subnode format which are no longer needed

This is based on asoc/topic/simple branch

 .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
 sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
 2 files changed, 223 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..4871e91
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,73 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio"
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio,cpu			: CPU   sub-node
+- simple-audio,codec			: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- frame-master				: bool property. add this if subnode was frame master
+- bitclock-master			: bool property. add this if subnode was bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- clocks / system-clock-frequency	: specify subnode's clock if needed.
+					  it can be specified via "clocks" if system has clock node,
+                                          or "system-clock-frequency" if system doesn't have it.
+
+Example:
+
+clock {
+	osc: oscillator {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <11289600>;
+	};
+};
+
+sound {
+	compatible = "simple-audio";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&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>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..c0fb635 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * 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);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +244,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi

I would like to know the current status of this patch

> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> v3 -> v4
> 
>  - explain detail of each properties on simple-card.txt
>  - fixup odd examples on simple-card.txt
>  - remove "simple-card,card-name". create it from cpu/codec name
>  - use of_get_child_by_name()
>  - remove odd pointer info from dev_dbg()
>  - remove subnode format which are no longer needed
> 
> This is based on asoc/topic/simple branch
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..4871e91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC
> +
> +Optional CPU/CODEC subnodes properties:
> +
> +- frame-master				: bool property. add this if subnode was frame master
> +- bitclock-master			: bool property. add this if subnode was bitclock master
> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> +					  it can be specified via "clocks" if system has clock node,
> +                                          or "system-clock-frequency" if system doesn't have it.
> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +	};
> +};
> +
> +&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>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index b2fbb70..c0fb635 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,7 +8,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x\n", info->daifmt);
> +	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk);
> +	dev_dbg(dev, "codec : %s / %04x / %d\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -107,10 +244,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 194 bytes --]

On Thu, Nov 14, 2013 at 09:13:24PM -0800, Kuninori Morimoto wrote:

> I would like to know the current status of this patch

I'm still hoping that some of the DT maintainers will review things.

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

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Fri, Oct 25, 2013 at 03:14:20AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v3 -> v4
> 
>  - explain detail of each properties on simple-card.txt
>  - fixup odd examples on simple-card.txt
>  - remove "simple-card,card-name". create it from cpu/codec name
>  - use of_get_child_by_name()
>  - remove odd pointer info from dev_dbg()
>  - remove subnode format which are no longer needed
> 
> This is based on asoc/topic/simple branch
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..4871e91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC

Is there a class binding for audio devices this derives from?

> +
> +Optional CPU/CODEC subnodes properties:

Do these all apply to both sub-nodes?

> +- frame-master				: bool property. add this if subnode was frame master
> +- bitclock-master			: bool property. add this if subnode was bitclock master

s/was/is/

> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> +					  it can be specified via "clocks" if system has clock node,
> +                                          or "system-clock-frequency" if system doesn't have it.

What does "if system doesn't have it" mean? If it doesn't have a clock,
how does said non-existent clock have a frequency?

It would be possible to use a fixed-clock in place of
system-clock-frequency, which would make the binding more consistent and
the driver simpler, at the cost of making the dt marginally more
complex.

> +
> +Example:
> +
> +clock {

Why is this container here? It's confusing and unnecessary.

> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};

[...]

> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */

This comment looks confusing to me. I'm not sure it's all that helpful.

> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);

As a general note, I'm surprised there isn't a helper that does all of
the above, from of_parse_phandle to here.

> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"

s/clolks/clocks/

> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);

What it this isn't present?

> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;

Why? What does this imply?

Thanks,
Mark.

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

Thank you for your feedback

> > +- frame-master				: bool property. add this if subnode was frame master
> > +- bitclock-master			: bool property. add this if subnode was bitclock master
> 
> s/was/is/

Oops, I will fix it on v5

> > +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> > +- frame-inversion			: bool property. add this if subnode has frame inversion
> > +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> > +					  it can be specified via "clocks" if system has clock node,
> > +                                          or "system-clock-frequency" if system doesn't have it.
> 
> What does "if system doesn't have it" mean? If it doesn't have a clock,
> how does said non-existent clock have a frequency?

It means "if system doesn't support common clock".
I will fix it

> > +static int
> > +asoc_simple_card_sub_parse_of(struct device_node *np,
> > +			      struct asoc_simple_dai *dai,
> > +			      struct device_node **node)
> > +{
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	/*
> > +	 * 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);
> > +	if (!*node)
> > +		return -ENODEV;
> > +
> > +	/* get dai->name */
> > +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> > +	if (ret < 0)
> > +		goto parse_error;
> > +
> > +	/*
> > +	 * bitclock-inversion, frame-inversion
> > +	 * bitclock-master,    frame-master
> > +	 * and specific "format" if it has
> > +	 */
> 
> This comment looks confusing to me. I'm not sure it's all that helpful.
> 
> > +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> 
> As a general note, I'm surprised there isn't a helper that does all of
> the above, from of_parse_phandle to here.

snd_soc_of_parse_daifmt() is the helper fucntion,
and above comment is explaining it.
Or, am I misunderstanding your review ?

> > +	/*
> > +	 * dai->sysclk come from
> > +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> 
> s/clolks/clocks/

Grr, thank you

> > +	clk = of_clk_get(np, 0);
> > +	if (IS_ERR(clk))
> > +		of_property_read_u32(np,
> > +				     "system-clock-frequency",
> > +				     &dai->sysclk);
> 
> What it this isn't present?

If sysclk doesn't have common clock support

> > +	/* simple-card assumes platform == cpu */
> > +	*of_platform = *of_cpu;
> 
> Why? What does this imply?

This is the one of reason why this driver is "simple" card

Best regards
---
Kuninori Morimoto

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Mon, Nov 18, 2013 at 12:42:23AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> Thank you for your feedback
> 
> > > +- frame-master				: bool property. add this if subnode was frame master
> > > +- bitclock-master			: bool property. add this if subnode was bitclock master
> > 
> > s/was/is/
> 
> Oops, I will fix it on v5
> 
> > > +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> > > +- frame-inversion			: bool property. add this if subnode has frame inversion
> > > +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> > > +					  it can be specified via "clocks" if system has clock node,
> > > +                                          or "system-clock-frequency" if system doesn't have it.
> > 
> > What does "if system doesn't have it" mean? If it doesn't have a clock,
> > how does said non-existent clock have a frequency?
> 
> It means "if system doesn't support common clock".
> I will fix it

When you say "doesn't support common clock", you mean the code for that
platform is incompatible with the common clock framework? It seems very
messy to allow a Linux-internal implementation detail (which is expected
to change) to leak into a binding...

> 
> > > +static int
> > > +asoc_simple_card_sub_parse_of(struct device_node *np,
> > > +			      struct asoc_simple_dai *dai,
> > > +			      struct device_node **node)
> > > +{
> > > +	struct clk *clk;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * 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);
> > > +	if (!*node)
> > > +		return -ENODEV;
> > > +
> > > +	/* get dai->name */
> > > +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> > > +	if (ret < 0)
> > > +		goto parse_error;
> > > +
> > > +	/*
> > > +	 * bitclock-inversion, frame-inversion
> > > +	 * bitclock-master,    frame-master
> > > +	 * and specific "format" if it has
> > > +	 */
> > 
> > This comment looks confusing to me. I'm not sure it's all that helpful.
> > 
> > > +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> > 
> > As a general note, I'm surprised there isn't a helper that does all of
> > the above, from of_parse_phandle to here.
> 
> snd_soc_of_parse_daifmt() is the helper fucntion,
> and above comment is explaining it.
> Or, am I misunderstanding your review ?
> 

What I meant was that I am surprised there isn't a helper doing all of:
* of_parse_phandle
* snd_soc_of_get_dai_name
* snd_soc_of_parse_daifmt

It looks like a common pattern that could be factored out.

> > > +	/*
> > > +	 * dai->sysclk come from
> > > +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> > 
> > s/clolks/clocks/
> 
> Grr, thank you
> 
> > > +	clk = of_clk_get(np, 0);
> > > +	if (IS_ERR(clk))
> > > +		of_property_read_u32(np,
> > > +				     "system-clock-frequency",
> > > +				     &dai->sysclk);
> > 
> > What it this isn't present?
> 
> If sysclk doesn't have common clock support

Huh? That's not what I asked.

What if the dt has neither a clock or a system-clock-frequency property?

> 
> > > +	/* simple-card assumes platform == cpu */
> > > +	*of_platform = *of_cpu;
> > 
> > Why? What does this imply?
> 
> This is the one of reason why this driver is "simple" card

The issue here is that I'm almost totally unfamiliar with audio
hardware, nor the ASoC bindings. If this makes sense to you and Mark
Brown then I'm happy to continue not understanding either. :)

Thanks,
Mark.

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Mon, Nov 18, 2013 at 11:36:18AM +0000, Mark Rutland wrote:

> What I meant was that I am surprised there isn't a helper doing all of:
> * of_parse_phandle
> * snd_soc_of_get_dai_name
> * snd_soc_of_parse_daifmt

> It looks like a common pattern that could be factored out.

This is the factoring out of the common pattern, other drivers will only
need the compatible string to work out the format.

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

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

Thank you for your feedback

> > It means "if system doesn't support common clock".
> > I will fix it
> 
> When you say "doesn't support common clock", you mean the code for that
> platform is incompatible with the common clock framework? It seems very
> messy to allow a Linux-internal implementation detail (which is expected
> to change) to leak into a binding...

Some CPU doesn't support common clock, like PowerPC (?)
This is Mark (Brown) comment

--------------------
So, ideally.  However we have to consider the fact that the clock API
isn't reliably available makes this harder than it should be.  Even
among the DT using platforms at least PowerPC still uses a custom clock
API.  We could just use this as a carrot to push people to convert
though.
---------------------


> > > > +		of_property_read_u32(np,
> > > > +				     "system-clock-frequency",
> > > > +				     &dai->sysclk);
> > > 
> > > What it this isn't present?
> > 
> > If sysclk doesn't have common clock support
> 
> Huh? That's not what I asked.
> 
> What if the dt has neither a clock or a system-clock-frequency property?

OK, sorry for my English

This happen if cpu/codec was slave, not strange things.
Please check "Example". in there, "simple-audio,codec" has clocks
but, "simple-audio,cpu" doesn't have it, and "simple-audio,codec" has
"bitclock-master" and "frame-master";
This case, codec chip creates audio play/capture clock from system clock,
and "cpu" use it.
This is the reason why the name is "system-clock-frequency" instead of "clock-frequency"
The image is like this

 clocks /    +- simple card --+
system clock |                |-> playback
-------------+-[codec]--[cpu] |
             |                |<- capture
             +----------------+

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Tue, Nov 19, 2013 at 02:03:21AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> Thank you for your feedback
> 
> > > It means "if system doesn't support common clock".
> > > I will fix it
> > 
> > When you say "doesn't support common clock", you mean the code for that
> > platform is incompatible with the common clock framework? It seems very
> > messy to allow a Linux-internal implementation detail (which is expected
> > to change) to leak into a binding...
> 
> Some CPU doesn't support common clock, like PowerPC (?)
> This is Mark (Brown) comment
> 
> --------------------
> So, ideally.  However we have to consider the fact that the clock API
> isn't reliably available makes this harder than it should be.  Even
> among the DT using platforms at least PowerPC still uses a custom clock
> API.  We could just use this as a carrot to push people to convert
> though.
> ---------------------

I would be happier if we could unify the common clock infrastructure, it
would make this kind of thing a lot lessy messy. However, I'm not
against the system-clock-frequency property for now.

> 
> 
> > > > > +		of_property_read_u32(np,
> > > > > +				     "system-clock-frequency",
> > > > > +				     &dai->sysclk);
> > > > 
> > > > What it this isn't present?
> > > 
> > > If sysclk doesn't have common clock support
> > 
> > Huh? That's not what I asked.
> > 
> > What if the dt has neither a clock or a system-clock-frequency property?
> 
> OK, sorry for my English

Sorry for the confusion, I'll try to be less ambiguous in future :)

What I was trying to get at here is that if there is neither a clock or
a system-clock-frequency property in the device tree, dai->sysclk will
not have been initialised in this path. Is this a valid case, and will
dai->sysclk have a well-defined, sane value?

Thanks,
Mark.

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

> > --------------------
> > So, ideally.  However we have to consider the fact that the clock API
> > isn't reliably available makes this harder than it should be.  Even
> > among the DT using platforms at least PowerPC still uses a custom clock
> > API.  We could just use this as a carrot to push people to convert
> > though.
> > ---------------------
> 
> I would be happier if we could unify the common clock infrastructure, it
> would make this kind of thing a lot lessy messy. However, I'm not
> against the system-clock-frequency property for now.

Thank you

> > OK, sorry for my English
> 
> Sorry for the confusion, I'll try to be less ambiguous in future :)
> 
> What I was trying to get at here is that if there is neither a clock or
> a system-clock-frequency property in the device tree, dai->sysclk will
> not have been initialised in this path. Is this a valid case, and will
> dai->sysclk have a well-defined, sane value?

My understanding, this "dai" itself is created by devm_kzalloc()
So, default dai->sysclk is 0.
And, if there is no clocks, no system-clock-frequency property,
it try of_property_read_u32() side. but it will do nothing to dai->sysclk
in such case. so dai->sysclk is still 0, and it is very sane on this driver.
Is this good answer ?

+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Thu, Nov 21, 2013 at 12:12:13AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> > > --------------------
> > > So, ideally.  However we have to consider the fact that the clock API
> > > isn't reliably available makes this harder than it should be.  Even
> > > among the DT using platforms at least PowerPC still uses a custom clock
> > > API.  We could just use this as a carrot to push people to convert
> > > though.
> > > ---------------------
> > 
> > I would be happier if we could unify the common clock infrastructure, it
> > would make this kind of thing a lot lessy messy. However, I'm not
> > against the system-clock-frequency property for now.
> 
> Thank you
> 
> > > OK, sorry for my English
> > 
> > Sorry for the confusion, I'll try to be less ambiguous in future :)
> > 
> > What I was trying to get at here is that if there is neither a clock or
> > a system-clock-frequency property in the device tree, dai->sysclk will
> > not have been initialised in this path. Is this a valid case, and will
> > dai->sysclk have a well-defined, sane value?
> 
> My understanding, this "dai" itself is created by devm_kzalloc()
> So, default dai->sysclk is 0.
> And, if there is no clocks, no system-clock-frequency property,
> it try of_property_read_u32() side. but it will do nothing to dai->sysclk
> in such case. so dai->sysclk is still 0, and it is very sane on this driver.
> Is this good answer ?

That sounds fine to me. Just wanted to make sure. :)

Thanks,
Mark.

[PATCH v5] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v4 -> v5

 - fixup spell miss
 - removed un-needed "clock node" example from simple-card.txt
 - add explain that clocks can be used if system has "common clock"

 .../devicetree/bindings/sound/simple-card.txt      |   66 ++++++++
 sound/soc/generic/simple-card.c                    |  157 +++++++++++++++++++-
 2 files changed, 217 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..615a655
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,66 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio"
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio,cpu			: CPU   sub-node
+- simple-audio,codec			: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- frame-master				: bool property. add this if subnode is frame master
+- bitclock-master			: bool property. add this if subnode is bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- 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 can't use it.
+
+Example:
+
+sound {
+	compatible = "simple-audio";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&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>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..da1fd7e 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,145 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * 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);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clocks = <&xxx>" (if system has common clock)
+	 *  or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +204,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +221,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +245,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi

I would like to know current status of this patch

> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> v4 -> v5
> 
>  - fixup spell miss
>  - removed un-needed "clock node" example from simple-card.txt
>  - add explain that clocks can be used if system has "common clock"
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   66 ++++++++
>  sound/soc/generic/simple-card.c                    |  157 +++++++++++++++++++-
>  2 files changed, 217 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..615a655
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,66 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC
> +
> +Optional CPU/CODEC subnodes properties:
> +
> +- frame-master				: bool property. add this if subnode is frame master
> +- bitclock-master			: bool property. add this if subnode is bitclock master
> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- 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 can't use it.
> +
> +Example:
> +
> +sound {
> +	compatible = "simple-audio";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +	};
> +};
> +
> +&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>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index b2fbb70..da1fd7e 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,7 +8,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -57,11 +58,145 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * 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);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clocks = <&xxx>" (if system has common clock)
> +	 *  or "system-clock-frequency = <xxx>"
> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x\n", info->daifmt);
> +	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk);
> +	dev_dbg(dev, "codec : %s / %04x / %d\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -69,10 +204,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -86,6 +221,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -107,10 +245,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

On Thu, Nov 28, 2013 at 05:11:34PM -0800, Kuninori Morimoto wrote:

> I would like to know current status of this patch

Still hoping for DT review :/

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

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Rob Herring]

On Fri, Nov 15, 2013 at 9:50 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Oct 25, 2013 at 03:14:20AM +0100, Kuninori Morimoto wrote:
>> Support for loading the simple-card module via DeviceTree.
>> It requests CPU/CODEC information.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
>> ---
>> v3 -> v4
>>
>>  - explain detail of each properties on simple-card.txt
>>  - fixup odd examples on simple-card.txt
>>  - remove "simple-card,card-name". create it from cpu/codec name
>>  - use of_get_child_by_name()
>>  - remove odd pointer info from dev_dbg()
>>  - remove subnode format which are no longer needed
>>
>> This is based on asoc/topic/simple branch
>>
>>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>>  2 files changed, 223 insertions(+), 6 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
>> new file mode 100644
>> index 0000000..4871e91
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
>> @@ -0,0 +1,73 @@
>> +Simple-Card:
>> +
>> +Simple-Card specifies audio DAI connection of SoC <-> codec.
>> +
>> +Required properties:
>> +
>> +- compatible                         : "simple-audio"

Still not really a fan of this generic name. Can we define in the
description above what simple means.

>> +
>> +Optional properties:
>> +
>> +- simple-audio,format                        : CPU/CODEC common audio format.
>> +                                     "i2s", "right_j", "left_j" , "dsp_a"
>> +                                     "dsp_b", "ac97", "pdm", "msb", "lsb"
>> +Required subnodes:
>> +
>> +- simple-audio,cpu                   : CPU   sub-node
>> +- simple-audio,codec                 : CODEC sub-node
>> +
>> +Required CPU/CODEC subnodes properties:
>> +
>> +- sound-dai                          : phandle and port of CPU/CODEC
>
> Is there a class binding for audio devices this derives from?
>
>> +
>> +Optional CPU/CODEC subnodes properties:
>
> Do these all apply to both sub-nodes?
>
>> +- frame-master                               : bool property. add this if subnode was frame master
>> +- bitclock-master                    : bool property. add this if subnode was bitclock master
>
> s/was/is/
>
>> +- bitclock-inversion                 : bool property. add this if subnode has clock inversion
>> +- frame-inversion                    : bool property. add this if subnode has frame inversion
>> +- clocks / system-clock-frequency    : specify subnode's clock if needed.
>> +                                       it can be specified via "clocks" if system has clock node,
>> +                                          or "system-clock-frequency" if system doesn't have it.
>
> What does "if system doesn't have it" mean? If it doesn't have a clock,
> how does said non-existent clock have a frequency?
>
> It would be possible to use a fixed-clock in place of
> system-clock-frequency, which would make the binding more consistent and
> the driver simpler, at the cost of making the dt marginally more
> complex.

Just plain "clock-frequency" is fairly standard, so please use that
instead. Unless there is need for a fixed-clock to be routed to
several nodes, then I think using the more simple clock-frequency here
is fine.

Can both sub-nodes really have different clocks? Seems like that would
exceed the definition of simple.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

On Mon, Nov 18, 2013 at 08:12:06AM -0600, Rob Herring wrote:

> Can both sub-nodes really have different clocks? Seems like that would
> exceed the definition of simple.

In theory, though it would be unusual.  What is more normal that the
clock is only connected to one of the nodes and we need to know which it
is.

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

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob, Mark

> >> +Required properties:
> >> +
> >> +- compatible                         : "simple-audio"
> 
> Still not really a fan of this generic name. Can we define in the
> description above what simple means.

So, how about "simple-audio-card" ?

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v4 -> v5

 - fixup spell miss
 - removed un-needed "clock node" example from simple-card.txt
 - add explain that clocks can be used if system has "common clock"

v5 -> v6

 - simple-audio => simple-audio-card

 .../devicetree/bindings/sound/simple-card.txt      |   68 +++++++++
 sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
 2 files changed, 218 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..769a346
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,68 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio-card"
+
+Optional properties:
+
+- simple-audio-card,format		: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio-card,cpu			: CPU   sub-node
+- simple-audio-card,codec		: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- format				: CPU/CODEC specific audio format if needed.
+					  see simple-audio-card,format
+- frame-master				: bool property. add this if subnode is frame master
+- bitclock-master			: bool property. add this if subnode is bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- 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)
+
+Example:
+
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,format = "left_j";
+
+	simple-audio-card,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio-card,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&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>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..7a9b6b4 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * 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);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clocks = <&xxx>" (if system has common clock)
+	 *  or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio-card,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio-card,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio-card,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +244,17 @@ 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 = "simple-audio-card", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Wed, Nov 20, 2013 at 03:25:02PM +0900, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.

I had a look at the recent DT reviews and it seems like the review here
is both getting slower and focusing on more and more minor issues so I
think that really everything is OK here and I've applied this.

Thanks for your hard work and perseverence with this.

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

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

Re: [alsa-devel] [PATCH v6] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark

> > Support for loading the simple-card module via DeviceTree.
> > It requests CPU/CODEC information.
> 
> I had a look at the recent DT reviews and it seems like the review here
> is both getting slower and focusing on more and more minor issues so I
> think that really everything is OK here and I've applied this.
> 
> Thanks for your hard work and perseverence with this.

Thank you !!
Phew, we spent 1 year for this support ;P

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Rob Herring]

On Mon, Nov 18, 2013 at 8:36 PM, Kuninori Morimoto
<kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
>
> Hi Rob, Mark
>
>> >> +Required properties:
>> >> +
>> >> +- compatible                         : "simple-audio"
>>
>> Still not really a fan of this generic name. Can we define in the
>> description above what simple means.
>
> So, how about "simple-audio-card" ?

That's missing my point. First, I think you should be defining the
actual h/w in the DT and doing the mapping of that to a simple audio
driver in the kernel. Otherwise how do you fix some quirk on a
particular platform later on without updating the DTB? I'm fine with
this being the default compatible string, but you should also require
a more specific name. Perhaps it is just <soc>-simple-audio or
<board>-simple-audio.

Second, you need to define in this binding document what simple means.
What properties of the audio subsystem make it simple? The h/w has and
doesn't have what? How do I decide if my platform can or should use
this binding?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, Nov 20, 2013 at 08:20:19AM -0600, Rob Herring wrote:

> Second, you need to define in this binding document what simple means.
> What properties of the audio subsystem make it simple? The h/w has and
> doesn't have what? How do I decide if my platform can or should use
> this binding?

I think it's reasonable to just say that a simple device is one that can
use this binding and that if that is not possible we need to make a
taste based decision at the time between extending this binding or using
a new one.

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

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob

> >> Still not really a fan of this generic name. Can we define in the
> >> description above what simple means.
> >
> > So, how about "simple-audio-card" ?
> 
> That's missing my point. First, I think you should be defining the
> actual h/w in the DT and doing the mapping of that to a simple audio
> driver in the kernel. Otherwise how do you fix some quirk on a
> particular platform later on without updating the DTB? I'm fine with
> this being the default compatible string, but you should also require
> a more specific name. Perhaps it is just <soc>-simple-audio or
> <board>-simple-audio.
> 
> Second, you need to define in this binding document what simple means.
> What properties of the audio subsystem make it simple? The h/w has and
> doesn't have what? How do I decide if my platform can or should use
> this binding?

Basically, on ASoC case, SoC/board needs <soc>-<codec>-audio-card
(= not simple card) for matching each other, and this is start point.
This means we need many <soc>-<codec>-audio-card.c driver.
But, in some case, the difference between
<socA>-<codecA> <-> <socA>-<codecB> <-> <socB>-<codecA> <-> <socB>-<codecB>
was just "name". creating too many such driver was not sane for me.
This simple-audio is used in such case.
Of course we can update simple-audio feature
(if it is very simple/common feature)
but, if you need <soc/board>-audio-card which needs special feature,
you need to create such driver without using simple-card.
This is very normal approach on ASoC and there are many such driver.

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob

ping ?

> > >> Still not really a fan of this generic name. Can we define in the
> > >> description above what simple means.
> > >
> > > So, how about "simple-audio-card" ?
> > 
> > That's missing my point. First, I think you should be defining the
> > actual h/w in the DT and doing the mapping of that to a simple audio
> > driver in the kernel. Otherwise how do you fix some quirk on a
> > particular platform later on without updating the DTB? I'm fine with
> > this being the default compatible string, but you should also require
> > a more specific name. Perhaps it is just <soc>-simple-audio or
> > <board>-simple-audio.
> > 
> > Second, you need to define in this binding document what simple means.
> > What properties of the audio subsystem make it simple? The h/w has and
> > doesn't have what? How do I decide if my platform can or should use
> > this binding?
> 
> Basically, on ASoC case, SoC/board needs <soc>-<codec>-audio-card
> (= not simple card) for matching each other, and this is start point.
> This means we need many <soc>-<codec>-audio-card.c driver.
> But, in some case, the difference between
> <socA>-<codecA> <-> <socA>-<codecB> <-> <socB>-<codecA> <-> <socB>-<codecB>
> was just "name". creating too many such driver was not sane for me.
> This simple-audio is used in such case.
> Of course we can update simple-audio feature
> (if it is very simple/common feature)
> but, if you need <soc/board>-audio-card which needs special feature,
> you need to create such driver without using simple-card.
> This is very normal approach on ASoC and there are many such driver.
> 
> Best regards
> ---
> Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Thu, Oct 24, 2013 at 06:17:59PM +0100, Mark Rutland wrote:
> On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:

> > +- simple-audio,card-name		: simple-audio card name

> What's this used for?

This serves two useful functions.  One is that this is used for display
to users so they have a friendly name for the sound card (it is
relatively common to have multiple sound cards in the system).  The
other is that it is essentially a compatibility string for configuration
- you get a lot of sound devices that are electrically identical and
hence look the same from a driver point of view but due different
physical form factors should be configured differently.

> > +- format				: specific format if needed, see below
> > +- frame-master				: frame master
> > +- bitclock-master			: bitclock master
> > +- bitclock-inversion			: clock inversion
> > +- frame-inversion			: frame inversion

> What do these mean? Repeating the name without a dash is completely unhelpful.
> Describe what these imply.

These are all boolean propeties.  The meanings should be obvious or at
least very easily discoverable to anyone with any familiarity with audio
hardware; if you can understand what to do with them they should be OK.

> > +- clocks				: phandle for system clock rate

> Just one clock?

This is a limitation from the simple card, anything that needs more
complex clocking should be using a different binding.

> > +- system-clock-frequency		: system clock rate
> > +					  it will overwrite clocks's rate

> This seems very odd.

> Why do you want to overwrite a clock's rate?

It's relatively common to derive the audio clock from a general purpose
programmable clock which needs to be configured appropriately for use.

> > +simple-audio,format
> > +	"i2s"
> > +	"right_j"
> > +	"left_j"
> > +	"dsp_a"
> > +	"dsp_b"
> > +	"ac97"
> > +	"pdm"
> > +	"msb"
> > +	"lsb"

> What do these mean? Why are they not described when the property was defined above?

This is another one where the names should be clear for people familiar
with the hardware, they're well known terms.

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

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

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark (Brown), Mark (Rutland)

I had sent v4 patch, but I removed simple-card,card-name on it.
But, as Mark (Brown) explained, I think simple-card,card-name
is very helpful for users.
I can send v5 patch (with it), or incremental patch if we can have it.
So, I need your comment

> [1  <multipart/signed (7bit)>]
> [1.1  <text/plain; us-ascii (7bit)>]
> On Thu, Oct 24, 2013 at 06:17:59PM +0100, Mark Rutland wrote:
> > On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:
> 
> > > +- simple-audio,card-name		: simple-audio card name
> 
> > What's this used for?
> 
> This serves two useful functions.  One is that this is used for display
> to users so they have a friendly name for the sound card (it is
> relatively common to have multiple sound cards in the system).  The
> other is that it is essentially a compatibility string for configuration
> - you get a lot of sound devices that are electrically identical and
> hence look the same from a driver point of view but due different
> physical form factors should be configured differently.
> 
> > > +- format				: specific format if needed, see below
> > > +- frame-master				: frame master
> > > +- bitclock-master			: bitclock master
> > > +- bitclock-inversion			: clock inversion
> > > +- frame-inversion			: frame inversion
> 
> > What do these mean? Repeating the name without a dash is completely unhelpful.
> > Describe what these imply.
> 
> These are all boolean propeties.  The meanings should be obvious or at
> least very easily discoverable to anyone with any familiarity with audio
> hardware; if you can understand what to do with them they should be OK.
> 
> > > +- clocks				: phandle for system clock rate
> 
> > Just one clock?
> 
> This is a limitation from the simple card, anything that needs more
> complex clocking should be using a different binding.
> 
> > > +- system-clock-frequency		: system clock rate
> > > +					  it will overwrite clocks's rate
> 
> > This seems very odd.
> 
> > Why do you want to overwrite a clock's rate?
> 
> It's relatively common to derive the audio clock from a general purpose
> programmable clock which needs to be configured appropriately for use.
> 
> > > +simple-audio,format
> > > +	"i2s"
> > > +	"right_j"
> > > +	"left_j"
> > > +	"dsp_a"
> > > +	"dsp_b"
> > > +	"ac97"
> > > +	"pdm"
> > > +	"msb"
> > > +	"lsb"
> 
> > What do these mean? Why are they not described when the property was defined above?
> 
> This is another one where the names should be clear for people familiar
> with the hardware, they're well known terms.
> [1.2 Digital signature <application/pgp-signature (7bit)>]
> 
> [2  <text/plain; us-ascii (7bit)>]
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Tue, Oct 29, 2013 at 05:39:17PM -0700, Kuninori Morimoto wrote:

> I had sent v4 patch, but I removed simple-card,card-name on it.
> But, as Mark (Brown) explained, I think simple-card,card-name
> is very helpful for users.
> I can send v5 patch (with it), or incremental patch if we can have it.
> So, I need your comment

Let's review the existing bindings as-is unless there's a need to revise
for some other reason, it's easy enough to add the property back later.

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

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark

> > I had sent v4 patch, but I removed simple-card,card-name on it.
> > But, as Mark (Brown) explained, I think simple-card,card-name
> > is very helpful for users.
> > I can send v5 patch (with it), or incremental patch if we can have it.
> > So, I need your comment
> 
> Let's review the existing bindings as-is unless there's a need to revise
> for some other reason, it's easy enough to add the property back later.

OK, I see
Thank you.

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html