Re: [PATCH RFC 2/2] ASoC: simple-card: Move dai-link level properties away from dai subnodes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Fri, 21 Mar 2014 18:47:23 +0200
Jyri Sarha <jsarha@xxxxxx> wrote:

> The properties like format, bitclock-master, frame-master,
> bitclock-inversion, and frame-inversion should be common to the dais
> connected with a dai-link. For bitclock-master and frame-master
> properties to be unambiguous they need to indicate the mastering dai
> node with a phandle.
> 
> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> ---
>  .../devicetree/bindings/sound/simple-card.txt      |   91 ++++----
>  sound/soc/generic/simple-card.c                    |  237 ++++++++++++--------
>  2 files changed, 191 insertions(+), 137 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> index 131aa2a..3cafa9e 100644
> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -1,6 +1,6 @@
>  Simple-Card:
>  
> -Simple-Card specifies audio DAI connection of SoC <-> codec.
> +Simple-Card specifies audio DAI connections of SoC <-> codec.
>  
>  Required properties:
>  
> @@ -8,28 +8,54 @@ Required properties:
>  
>  Optional properties:
>  
> -- simple-audio-card,name		: User specified audio sound card name, one string
> +- simple-audio-card,name		: User specified audio sound card name,
> +one string
>  					  property.
> -- simple-audio-card,format		: CPU/CODEC common audio format.
> -					  "i2s", "right_j", "left_j" , "dsp_a"
> -					  "dsp_b", "ac97", "pdm", "msb", "lsb"
>  - simple-audio-card,widgets		: Please refer to widgets.txt.
>  - simple-audio-card,routing		: A list of the connections between audio components.
>  					  Each entry is a pair of strings, the first being the
>  					  connection's sink, the second being the connection's
>  					  source.
> -- dai-tdm-slot-num			: Please refer to tdm-slot.txt.
> -- dai-tdm-slot-width			: Please refer to tdm-slot.txt.
> +Optional subnodes:
> +
> +- simple-audio-card,dai-link		: Container for dai-link level
> +					  properties and the CPU and CODEC
> +					  sub-nodes. This container may be
> +					  omitted when the card has only one
> +					  DAI link. See the examples and the
> +					  section bellow.
> +
> +Dai-link subnode properties and subnodes:
> +
> +If dai-link subnode is omitted and the subnode properties are directly
> +under "sound"-node the subnode property and subnode names have to be
> +prefixed with "simple-audio-card,"-prefix.
>  
> -Required subnodes:
> +Requred dai-link subnodes:

Typo: 'Required'

>  
> -- simple-audio-card,dai-link		: container for the CPU and CODEC sub-nodes
> -					  This container may be omitted when the
> -					  card has only one DAI link.
> -					  See the examples.
> +- cpu					: CPU   sub-node
> +- codec					: CODEC sub-node
>  
> -- simple-audio-card,cpu			: CPU   sub-node
> -- simple-audio-card,codec		: CODEC sub-node
> +Optional dai-link subnode properties:
> +
> +- format				: CPU/CODEC common audio format.
> +					  "i2s", "right_j", "left_j" , "dsp_a"
> +					  "dsp_b", "ac97", "pdm", "msb", "lsb"
> +- frame-master				: Indicates dai-link frame master.
> +					  phandle to a cpu or codec subnode.
> +- bitclock-master			: Indicates dai-link bit clock master.
> +					  phandle to a cpu or codec subnode.
> +- bitclock-inversion			: bool property. Add this if the
> +					  dai-link uses bit clock inversion.
> +- frame-inversion			: bool property. Add this if the
> +					  dai-link uses frame clock inversion.
> +
> +For backward compatibility the frame-master and bitclock-master
> +properties can be used as booleans in codec subnode to indicate if the
> +codec is the dai-link frame or bit clock master. In this case the same
> +properties should not be present at dai-link level and the
> +bitclock-inversion and frame-inversion properties should also be placed
> +in the codec node if needed.
>  
>  Required CPU/CODEC subnodes properties:
>  
	[snip]
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 21f1ccb..f4663d0 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,6 +8,7 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +#define DEBUG

Should be removed.

>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> @@ -88,7 +89,6 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  
>  static int
>  asoc_simple_card_sub_parse_of(struct device_node *np,
> -			      unsigned int daifmt,
>  			      struct asoc_simple_dai *dai,
>  			      const struct device_node **p_node,
>  			      const char **name)
> @@ -117,14 +117,6 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  		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->fmt |= daifmt;
> -
> -	/*
>  	 * dai->sysclk come from
>  	 *  "clocks = <&xxx>" (if system has common clock)
>  	 *  or "system-clock-frequency = <xxx>"
> @@ -151,37 +143,134 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  	return 0;
>  }
>  
> -static int simple_card_cpu_codec_of(struct device_node *node,
> -				int daifmt,
> -				struct snd_soc_dai_link *dai_link,
> -				struct simple_dai_props *dai_props)
> +static int simple_card_dai_link_of(struct device_node *node,
> +				   struct device *dev,
> +				   struct snd_soc_dai_link *dai_link,
> +				   struct simple_dai_props *dai_props)
>  {
> -	struct device_node *np;
> +	struct device_node *np = NULL;
> +	struct device_node *bitclkmaster = NULL;
> +	struct device_node *framemaster = NULL;
> +	unsigned int daifmt;
> +	char *name;
> +	char prop[128];
> +	char *prefix = "";
>  	int ret;
>  
> -	/* 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, daifmt,
> -						&dai_props->cpu_dai,
> -						&dai_link->cpu_of_node,
> -						&dai_link->cpu_dai_name);
> -		of_node_put(np);
> +	if (!strcmp("sound", node->name))
> +		prefix = "simple-audio-card,";
> +
> +	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> +					 &bitclkmaster, &framemaster);
> +	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> +
> +	snprintf(prop, sizeof(prop), "%scpu", prefix);
> +	np = of_get_child_by_name(node, prop);
> +	if (!np) {
> +		ret = -EINVAL;
> +		dev_err(dev, "%s: Can't find simple-audio-card,cpu DT node\n",
> +			__func__);
> +		goto dai_link_of_err;
>  	}
> +
> +	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
> +					    &dai_link->cpu_of_node,
> +					    &dai_link->cpu_dai_name);
>  	if (ret < 0)
> -		return ret;
> +		goto dai_link_of_err;
> +
> +	dai_props->cpu_dai.fmt = daifmt;
> +	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
> +	case 0x11:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +		break;
> +	case 0x10:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +		break;
> +	case 0x01:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +		break;
> +	default:
> +		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +		break;
> +	}
>  
> -	/* 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, daifmt,
> -						&dai_props->codec_dai,
> -						&dai_link->codec_of_node,
> -						&dai_link->codec_dai_name);
> -		of_node_put(np);
> +	of_node_put(np);
> +	snprintf(prop, sizeof(prop), "%scodec", prefix);
> +	np = of_get_child_by_name(node, prop);
> +	if (!np) {
> +		ret = -EINVAL;
> +		dev_err(dev, "%s: Can't find simple-audio-card,codec DT node\n",
> +			__func__);
> +		goto dai_link_of_err;
>  	}
> +
> +	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
> +					    &dai_link->codec_of_node,
> +					    &dai_link->codec_dai_name);
> +	if (ret < 0)
> +		goto dai_link_of_err;
> +
> +	if (!bitclkmaster && !framemaster) {
> +		/* Master setting not found from dai_link level, revert back
> +		   to legacy DT parsing and take settings from codec node. */
> +		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
> +			__func__);
> +		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
> +			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
> +			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);

We are here each time there is no bitclock-master and no frame-master,
i.e. each time for me. It could be simpler to keep the first 'daifmt'
instead of parsing again.

> +	} else {
> +		dai_props->codec_dai.fmt = daifmt;
> +		switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
> +		case 0x11:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> +			break;
> +		case 0x10:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> +			break;
> +		case 0x01:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +			break;
> +		default:
> +			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> +			break;
> +		}
> +	}
> +
> +	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
> +			ret = -EINVAL;
> +			goto dai_link_of_err;
> +	}
> +
> +	/* simple-card assumes platform == cpu */
> +	dai_link->platform_of_node = dai_link->cpu_of_node;
> +
> +	/* Link name is created from CPU/CODEC dai name */
> +	name = devm_kzalloc(dev,
> +			    strlen(dai_link->cpu_dai_name)   +
> +			    strlen(dai_link->codec_dai_name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", dai_link->cpu_dai_name,
> +				dai_link->codec_dai_name);
> +	dai_link->name = dai_link->stream_name = name;
> +
> +	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
> +	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
> +		dai_link->cpu_dai_name,
> +		dai_props->cpu_dai.fmt,
> +		dai_props->cpu_dai.sysclk);
> +	dev_dbg(dev, "\tcodec : %s / %04x / %d\n",
> +		dai_link->codec_dai_name,
> +		dai_props->codec_dai.fmt,
> +		dai_props->codec_dai.sysclk);
> +
> +dai_link_of_err:
> +	if (np)
> +		of_node_put(np);
> +	if (bitclkmaster)
> +		of_node_put(bitclkmaster);
> +	if (framemaster)
> +		of_node_put(framemaster);
>  	return ret;
>  }
>  
> @@ -192,18 +281,11 @@ static int asoc_simple_card_parse_of(struct device_node *node,
>  {
>  	struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
>  	struct simple_dai_props *dai_props = priv->dai_props;
> -	struct device_node *np;
> -	char *name;
> -	unsigned int daifmt;
>  	int ret;
>  
>  	/* parsing the card name from DT */
>  	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
>  
> -	/* get CPU/CODEC common format via simple-audio-card,format */
> -	daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
> -		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> -
>  	/* off-codec widgets */
>  	if (of_property_read_bool(node, "simple-audio-card,widgets")) {
>  		ret = snd_soc_of_parse_audio_simple_widgets(&priv->snd_card,
> @@ -220,71 +302,30 @@ static int asoc_simple_card_parse_of(struct device_node *node,
>  			return ret;
>  	}
>  
> -	/* loop on the DAI links */
> -	np = NULL;
> -	for (;;) {
> -		if (multi) {
> -			np = of_get_next_child(node, np);
> -			if (!np)
> -				break;
> -		}
> -
> -		ret = simple_card_cpu_codec_of(multi ? np : node,
> -					daifmt, dai_link, dai_props);
> -		if (ret < 0)
> -			goto err;
> -
> -		/*
> -		 * overwrite cpu_dai->fmt as its DAIFMT_MASTER bit is based on CODEC
> -		 * while the other bits should be identical unless buggy SW/HW design.
> -		 */
> -		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt;
> +	dev_dbg(dev, "New simple-card: %s\n", priv->snd_card.name ?
> +		priv->snd_card.name : "");
>  
> -		if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
> -			ret = -EINVAL;
> -			goto err;
> +	if (multi) {
> +		struct device_node *np = NULL;
> +		int i;
> +		for (i = 0; (np = of_get_next_child(node, np)); i++) {
> +			dev_dbg(dev, "\tlink %d:\n", i);
> +			ret = simple_card_dai_link_of(np, dev, dai_link + i,
> +						      dai_props + i);

I feel that my loop was quicker:

		struct device_node *np = NULL;

		for (;;) {
			np = of_get_next_child(node, np);
			if (!np)
				break;
			dev_dbg(dev, "\tlink %d:\n", dai_link - priv->snd_card.dai_link);
			ret = simple_card_dai_link_of(np, dev, dai_link++,
						      dai_props++);


> +			of_node_put(np);
> +			if (ret < 0)
> +				return ret;

The np reference count is updated in of_get_next_child(), so:

			if (ret < 0) {
				of_node_put(np);
				return ret;
			}

Otherwise, it works for me.

Tested-by: Jean-Francois Moine <moinejf@xxxxxxx>

>  		}
> -
> -		/* simple-card assumes platform == cpu */
> -		dai_link->platform_of_node = dai_link->cpu_of_node;
> -
> -		name = devm_kzalloc(dev,
> -				    strlen(dai_link->cpu_dai_name)   +
> -				    strlen(dai_link->codec_dai_name) + 2,
> -				    GFP_KERNEL);
> -		sprintf(name, "%s-%s", dai_link->cpu_dai_name,
> -					dai_link->codec_dai_name);
> -		dai_link->name = dai_link->stream_name = name;
> -
> -		if (!multi)
> -			break;
> -
> -		dai_link++;
> -		dai_props++;
> +	} else {
> +		ret = simple_card_dai_link_of(node, dev, dai_link, dai_props);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> -	/* card name is created from CPU/CODEC dai name */
> -	dai_link = priv->snd_card.dai_link;
>  	if (!priv->snd_card.name)
> -		priv->snd_card.name = dai_link->name;
> -
> -	dev_dbg(dev, "card-name : %s\n", priv->snd_card.name);
> -	dev_dbg(dev, "platform : %04x\n", daifmt);
> -	dai_props = priv->dai_props;
> -	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> -		dai_link->cpu_dai_name,
> -		dai_props->cpu_dai.fmt,
> -		dai_props->cpu_dai.sysclk);
> -	dev_dbg(dev, "codec : %s / %04x / %d\n",
> -		dai_link->codec_dai_name,
> -		dai_props->codec_dai.fmt,
> -		dai_props->codec_dai.sysclk);
> +		priv->snd_card.name = priv->snd_card.dai_link->name;
>  
>  	return 0;
> -
> -err:
> -	of_node_put(np);
> -	return ret;
>  }
>  
>  /* update the reference count of the devices nodes at end of probe */

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux