Re: [RFC][PATCH 2/2 v2] ASoC: simple-card: add Device Tree support

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

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

On Mon, Jan 14, 2013 at 06:40:37PM -0800, Kuninori Morimoto wrote:

Sorry about the delay in getting back to you on this.

> +- simple-audio,cpu,dai,clock-gating		: if needed, see below

A lot of these are listed as "if needed" - this means they should be
listed separately as optional properties rather than in the required
properties section.

> +- simple-audio,codec,controller			: phandle for CODEC DAI

It feels like this should just be simple-audio,codec - the controller is
just redudnant.  Though for idiomatic DT we ought to write something
like

	simple-audio,codec {
		simple-audio,dev = &phandle;
		simple-audio,system-clock-frequency = 122880000;
	};

rather than have these very long prefixes to the individual property
names.

> +- simple-audio,codec,dai,name			: simple-audio CODEC DAI name
> +- simple-audio,codec,dai,format			: see below
> +- simple-audio,codec,dai,clock-gating		: if needed, see below
> +- simple-audio,codec,dai,bitclock-inversion	: if needed
> +- simple-audio,codec,dai,bitclock-master	: if needed
> +- simple-audio,codec,dai,frame-inversion	: if needed
> +- simple-audio,codec,dai,frame-master		: if needed
> +- simple-audio,codec,dai,system-clock-frequency	: system clock rate if needed

I'm also thinking that for some of the above properties which really
should be the same for both ends of the link we should just specify
them at the card levle and copy them over.  The format and inversion 
mainly.

> +
> +simple-audio,xxx,dai,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"
> +
> +simple-audio,xxx,dai,clock-gating
> +	"continuous"
> +	"gated"
> +
> +Example:
> +
> +fsi_ak4642 {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,platform,controller = <&sh_fsi2>;
> +
> +	simple-audio,cpu,dai,name = "fsia-dai";
> +	simple-audio,cpu,dai,format = "left_j";
> +
> +	simple-audio,codec,controller = <&ak4648>;
> +	simple-audio,codec,dai,name = "ak4642-hifi";
> +	simple-audio,codec,dai,format = "left_j";
> +	simple-audio,codec,dai,bitclock-master;
> +	simple-audio,codec,dai,frame-master;
> +	simple-audio,codec,dai,system-clock-frequency = <11289600>;
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {
> +	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 6cf8355..72dd8ae 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +53,99 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static struct device_node*
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    const char *node_name1,
> +			    const char *node_name2,
> +			    const char **interface,
> +			    struct asoc_simple_dai *dai)
> +{
> +	struct device_node *node;
> +	char prop[128];
> +
> +	/* node or name is required */
> +	snprintf(prop, sizeof(prop),
> +		 "simple-audio,%s,controller", node_name1);
> +	node = of_parse_phandle(np, prop, 0);
> +	if (node)
> +		of_node_put(node);
> +
> +	/* get "simple-audio,xxx,yyy,name = xxx" */
> +	snprintf(prop, sizeof(prop),
> +		 "simple-audio,%s%s,name", node_name1, node_name2);
> +	of_property_read_string(np, prop, interface);
> +
> +	if (dai) {
> +		/* get "simple-audio,xxx,yyy,formart = xxx" */
> +		snprintf(prop, sizeof(prop),
> +			 "simple-audio,%s%s,", node_name1, node_name2);
> +		dai->fmt = snd_soc_of_parse_daifmt(np, prop);
> +
> +		/* get "simple-audio,xxx,yyy,system-clock-frequency = <xxx>" */
> +		snprintf(prop, sizeof(prop),
> +			 "simple-audio,%s%s,system-clock-frequency",
> +			 node_name1, node_name2);
> +		of_property_read_u32(np, prop, &dai->sysclk);
> +	}
> +
> +	return node;
> +}
> +
> +static void asoc_simple_card_parse_of(struct device_node *np,
> +				      struct asoc_simple_card_info *info,
> +				      struct device *dev,
> +				      struct device_node **of_cpu,
> +				      struct device_node **of_codec,
> +				      struct device_node **of_platform)
> +{
> +	of_property_read_string(np, "simple-audio,card-name", &info->card);
> +	info->name = info->card;
> +
> +	*of_cpu = __asoc_simple_card_parse_of(
> +		np, "cpu", ",dai", &info->cpu_dai.name, &info->cpu_dai);
> +	*of_codec = __asoc_simple_card_parse_of(
> +		np, "codec", ",dai", &info->codec_dai.name, &info->codec_dai);
> +	*of_platform = __asoc_simple_card_parse_of(
> +		np, "platform", "", &info->platform, NULL);
> +
> +	dev_dbg(dev, "card-name  : %s\n", info->card);
> +	dev_dbg(dev, "cpu info   : %s / %x / %d / %p\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk,
> +		*of_cpu);
> +	dev_dbg(dev, "codec_info : %s / %x / %d / %p\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk,
> +		*of_codec);
> +	dev_dbg(dev, "platform_info : %s / %p\n",
> +		info->platform,
> +		*of_platform);
> +}
> +
>  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)
> +			asoc_simple_card_parse_of(np, cinfo, dev,
> +						  &of_cpu,
> +						  &of_codec,
> +						  &of_platform);
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -64,10 +153,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 +170,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,9 +194,15 @@ static int asoc_simple_card_remove(struct platform_device *pdev)
>  	return snd_soc_unregister_card(&cinfo->snd_card);
>  }
>  
> +static const struct of_device_id asoc_simple_of_match[] = {
> +	{ .compatible = "simple-audio", },
> +	{},
> +};
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 

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

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss