[PATCH 1/3] ARM: shmobile: armadillo800eva: Properly specify HDMI audio link format

[PATCH 1/3] ARM: shmobile: armadillo800eva: Properly specify HDMI audio link format

[Lars-Peter Clausen]

The DAI link format should be specified for the whole link rather than just
one component on the link. So move the format specification for the HDMI
audio link from the CPU component to the link itself.

Since the sh-mobile-hdmi DAI driver doesn't implement the set_fmt() callback
in this case there is no functional difference between only specifying the
the format for the CPU side or for the whole link, but the later it will
allow us to remove support for just specifying the format for one component.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/arm/mach-shmobile/board-armadillo800eva.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index 6d949f1..75de26c 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -1040,9 +1040,9 @@ static struct asoc_simple_card_info fsi2_hdmi_info = {
 	.card		= "FSI2B-HDMI",
 	.codec		= "sh-mobile-hdmi",
 	.platform	= "sh_fsi2",
+	.daifmt		= SND_SOC_DAIFMT_CBS_CFS,
 	.cpu_dai = {
 		.name	= "fsib-dai",
-		.fmt	= SND_SOC_DAIFMT_CBS_CFS,
 	},
 	.codec_dai = {
 		.name = "sh_mobile_hdmi-hifi",
-- 
1.8.0

[PATCH 2/3] ASoC: sh: fsi: Fix clock inversion

[Lars-Peter Clausen]

According to the sh7724 hardware user manual (Rev.2.00 Jan 2013) page 1851
to 1856 the FSI bit-clock is inverted to the bit-clock as specified by the
I2S standard. This means the bit clock inversion bit should be set for a
normal I2S clock and should not be set for an inverted I2S clock.

Similarly when operating in left-justified mode both the frame-clock and the
bit-clock need to be inverted to be standards compliant.

This means also that the extra clock inversion setting in the
armadillo800eva machine driver for CPU component should now be removed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
I don't have the hardware but I'd like to get rid of the extra
SND_SOC_DAIFMT_IB_NF in simple-card platform data, so we can remove the fmt
field. Kuninori can you check if this works?
---
 arch/arm/mach-shmobile/board-armadillo800eva.c | 1 -
 sound/soc/sh/fsi.c                             | 7 +++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index 75de26c..36aaeb1 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -1015,7 +1015,6 @@ static struct asoc_simple_card_info fsi_wm8978_info = {
 	.platform	= "sh_fsi2",
 	.daifmt		= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
 	.cpu_dai = {
-		.fmt	= SND_SOC_DAIFMT_IB_NF,
 		.name	= "fsia-dai",
 	},
 	.codec_dai = {
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index b87b22e..dfb17f6 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -1594,6 +1594,12 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 static int fsi_set_fmt_dai(struct fsi_priv *fsi, unsigned int fmt)
 {
+	/*
+	 * FSI bit clock is inverted to the I2S specification, so we invert it
+	 * when a non-inverted I2S clock was requested and vice versa.
+	 */
+	 fsi->bit_clk_inv = !fsi->bit_clk_inv;
+
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
 		fsi->fmt = CR_I2S;
@@ -1602,6 +1608,7 @@ static int fsi_set_fmt_dai(struct fsi_priv *fsi, unsigned int fmt)
 	case SND_SOC_DAIFMT_LEFT_J:
 		fsi->fmt = CR_PCM;
 		fsi->chan_num = 2;
+		fsi->lr_clk_inv = !fsi->lr_clk_inv;
 		break;
 	default:
 		return -EINVAL;
-- 
1.8.0

Re: [PATCH 2/3] ASoC: sh: fsi: Fix clock inversion

[Sergei Shtylyov]

Hello.

On 1/19/2015 12:54 PM, Lars-Peter Clausen wrote:

> According to the sh7724 hardware user manual (Rev.2.00 Jan 2013) page 1851
> to 1856 the FSI bit-clock is inverted to the bit-clock as specified by the
> I2S standard. This means the bit clock inversion bit should be set for a
> normal I2S clock and should not be set for an inverted I2S clock.

> Similarly when operating in left-justified mode both the frame-clock and the
> bit-clock need to be inverted to be standards compliant.

> This means also that the extra clock inversion setting in the
> armadillo800eva machine driver for CPU component should now be removed.

> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

[...]

> diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
> index b87b22e..dfb17f6 100644
> --- a/sound/soc/sh/fsi.c
> +++ b/sound/soc/sh/fsi.c
> @@ -1594,6 +1594,12 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>
>   static int fsi_set_fmt_dai(struct fsi_priv *fsi, unsigned int fmt)
>   {
> +	/*
> +	 * FSI bit clock is inverted to the I2S specification, so we invert it
> +	 * when a non-inverted I2S clock was requested and vice versa.
> +	 */
> +	 fsi->bit_clk_inv = !fsi->bit_clk_inv;

    One space too many here. :-)

[...]

WBR, Sergei

[PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Lars-Peter Clausen]

Having to set different formats on the CPU side and the CODEC side of a DAI
link is usually indication that something is terribly wrong and in most
cases is a result of a broken driver that implements a set_fmt() callback
which does not follow the specification. In the past this feature has been
used to work around broken drivers, rather than fixing them. We don't really
want to encourage this, so remove support for setting different formats on
both ends of the link.

Along the way switch to static DAI format setup by setting the the dai_fmt
field of the snd_soc_dai_link rather than calling snd_soc_dai_fmt().

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/sound/simple_card.h     |  1 -
 sound/soc/generic/simple-card.c | 29 +++++++----------------------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/include/sound/simple_card.h b/include/sound/simple_card.h
index 1255ddb..b9b4f28 100644
--- a/include/sound/simple_card.h
+++ b/include/sound/simple_card.h
@@ -16,7 +16,6 @@
 
 struct asoc_simple_dai {
 	const char *name;
-	unsigned int fmt;
 	unsigned int sysclk;
 	int slots;
 	int slot_width;
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index f7c6734..d90a22e 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -125,14 +125,6 @@ static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
 {
 	int ret;
 
-	if (set->fmt) {
-		ret = snd_soc_dai_set_fmt(dai, set->fmt);
-		if (ret && ret != -ENOTSUPP) {
-			dev_err(dai->dev, "simple-card: set_fmt error\n");
-			goto err;
-		}
-	}
-
 	if (set->sysclk) {
 		ret = snd_soc_dai_set_sysclk(dai, 0, set->sysclk, 0);
 		if (ret && ret != -ENOTSUPP) {
@@ -269,12 +261,10 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 					 struct device_node *codec,
 					 char *prefix, int idx)
 {
+	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx);
 	struct device *dev = simple_priv_to_dev(priv);
 	struct device_node *bitclkmaster = NULL;
 	struct device_node *framemaster = NULL;
-	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
-	struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
-	struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
 	unsigned int daifmt;
 
 	daifmt = snd_soc_of_parse_daifmt(node, prefix,
@@ -289,8 +279,7 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 		 */
 		dev_dbg(dev, "Revert to legacy daifmt parsing\n");
 
-		cpu_dai->fmt = codec_dai->fmt =
-			snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
+		daifmt = snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
 			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
 	} else {
 		if (codec == bitclkmaster)
@@ -299,11 +288,10 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 		else
 			daifmt |= (codec == framemaster) ?
 				SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS;
-
-		cpu_dai->fmt	= daifmt;
-		codec_dai->fmt	= daifmt;
 	}
 
+	dai_link->dai_fmt = daifmt;
+
 	of_node_put(bitclkmaster);
 	of_node_put(framemaster);
 
@@ -379,13 +367,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	dai_link->init = asoc_simple_card_dai_init;
 
 	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
-	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
+	dev_dbg(dev, "\tcpu : %s / %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",
+	dev_dbg(dev, "\tcodec : %s / %d\n",
 		dai_link->codec_dai_name,
-		dai_props->codec_dai.fmt,
 		dai_props->codec_dai.sysclk);
 
 	/*
@@ -572,14 +558,13 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 		dai_link->codec_name	= cinfo->codec;
 		dai_link->cpu_dai_name	= cinfo->cpu_dai.name;
 		dai_link->codec_dai_name = cinfo->codec_dai.name;
+		dai_link->dai_fmt	= cinfo->daifmt;
 		dai_link->init		= asoc_simple_card_dai_init;
 		memcpy(&priv->dai_props->cpu_dai, &cinfo->cpu_dai,
 					sizeof(priv->dai_props->cpu_dai));
 		memcpy(&priv->dai_props->codec_dai, &cinfo->codec_dai,
 					sizeof(priv->dai_props->codec_dai));
 
-		priv->dai_props->cpu_dai.fmt	|= cinfo->daifmt;
-		priv->dai_props->codec_dai.fmt	|= cinfo->daifmt;
 	}
 
 	snd_soc_card_set_drvdata(&priv->snd_card, priv);
-- 
1.8.0

Re: [PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Kuninori Morimoto]

Hi Lars

> Having to set different formats on the CPU side and the CODEC side of a DAI
> link is usually indication that something is terribly wrong and in most
> cases is a result of a broken driver that implements a set_fmt() callback
> which does not follow the specification. In the past this feature has been
> used to work around broken drivers, rather than fixing them. We don't really
> want to encourage this, so remove support for setting different formats on
> both ends of the link.
> 
> Along the way switch to static DAI format setup by setting the the dai_fmt
> field of the snd_soc_dai_link rather than calling snd_soc_dai_fmt().
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
(snip)
> +	dai_link->dai_fmt = daifmt;
> +
>  	of_node_put(bitclkmaster);
>  	of_node_put(framemaster);
>  
> @@ -379,13 +367,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>  	dai_link->init = asoc_simple_card_dai_init;
>  
>  	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
> -	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
> +	dev_dbg(dev, "\tcpu : %s / %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",
> +	dev_dbg(dev, "\tcodec : %s / %d\n",
>  		dai_link->codec_dai_name,
> -		dai_props->codec_dai.fmt,
>  		dai_props->codec_dai.sysclk);

Can you please indicate dai_fmt here anyway ?
I don't care "how to" or "where", but format information is useful for debugging

Best regards
---
Kuninori Morimoto

[PATCH 0/3] ARM: shmobile: armadillo: fixup CPU settings

[Kuninori Morimoto]

Hi Mark, Lars
Cc Simon

These patches had been posted in ALSA/SH-ARM ML in
http://thread.gmane.org/gmane.linux.ports.sh.devel/43079
by Lars. But, he doesn't have armadillo board, and I commented
about FSI driver, and no one update these, these patches were
marooned.

Original 2/3 patch changed behavior for clock inversion on FSI driver.
FSI + wm8978 on armadillo800eva worked without any issues,
but, I don't know how much effect it has for other board (many board are using FSI).
We used this inversion flags on each board for historical reasons,
but, almost all these were not needed (except some picky board) on FSI.
Maybe Lars's 2/3 patch is correct, but, it is difficult to check/confirm for all boards.
And unfortunately, Renesas don't use FSI anymore.
So, I think keeping current FSI driver as-is is more safety for old boards.
I tested these patches on latest Mark's branch 

Lars-Peter Clausen (3):
      ARM: shmobile: armadillo800eva: Properly specify HDMI audio link format
      ARM: shmobile: armadillo800eva: fix clock inversion
      ASoC: simple-card: Remove support for setting differing DAI formats

 arch/arm/mach-shmobile/board-armadillo800eva.c |  3 +--
 include/sound/simple_card.h                    |  1 -
 sound/soc/generic/simple-card.c                | 30 ++++++++----------------------
 3 files changed, 9 insertions(+), 25 deletions(-)

[PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Kuninori Morimoto]

From: Lars-Peter Clausen <lars@metafoo.de>

Having to set different formats on the CPU side and the CODEC side of a DAI
link is usually indication that something is terribly wrong and in most
cases is a result of a broken driver that implements a set_fmt() callback
which does not follow the specification. In the past this feature has been
used to work around broken drivers, rather than fixing them. We don't really
want to encourage this, so remove support for setting different formats on
both ends of the link.

Along the way switch to static DAI format setup by setting the the dai_fmt
field of the snd_soc_dai_link rather than calling snd_soc_dai_fmt().

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
same as original patch

 include/sound/simple_card.h     |  1 -
 sound/soc/generic/simple-card.c | 30 ++++++++----------------------
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/include/sound/simple_card.h b/include/sound/simple_card.h
index 1255ddb..b9b4f28 100644
--- a/include/sound/simple_card.h
+++ b/include/sound/simple_card.h
@@ -16,7 +16,6 @@
 
 struct asoc_simple_dai {
 	const char *name;
-	unsigned int fmt;
 	unsigned int sysclk;
 	int slots;
 	int slot_width;
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index c49a408..33feee9 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -125,14 +125,6 @@ static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
 {
 	int ret;
 
-	if (set->fmt) {
-		ret = snd_soc_dai_set_fmt(dai, set->fmt);
-		if (ret && ret != -ENOTSUPP) {
-			dev_err(dai->dev, "simple-card: set_fmt error\n");
-			goto err;
-		}
-	}
-
 	if (set->sysclk) {
 		ret = snd_soc_dai_set_sysclk(dai, 0, set->sysclk, 0);
 		if (ret && ret != -ENOTSUPP) {
@@ -269,12 +261,10 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 					 struct device_node *codec,
 					 char *prefix, int idx)
 {
+	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx);
 	struct device *dev = simple_priv_to_dev(priv);
 	struct device_node *bitclkmaster = NULL;
 	struct device_node *framemaster = NULL;
-	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
-	struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
-	struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
 	unsigned int daifmt;
 
 	daifmt = snd_soc_of_parse_daifmt(node, prefix,
@@ -289,8 +279,7 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 		 */
 		dev_dbg(dev, "Revert to legacy daifmt parsing\n");
 
-		cpu_dai->fmt = codec_dai->fmt =
-			snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
+		daifmt = snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
 			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
 	} else {
 		if (codec == bitclkmaster)
@@ -299,11 +288,10 @@ static int asoc_simple_card_parse_daifmt(struct device_node *node,
 		else
 			daifmt |= (codec == framemaster) ?
 				SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS;
-
-		cpu_dai->fmt	= daifmt;
-		codec_dai->fmt	= daifmt;
 	}
 
+	dai_link->dai_fmt = daifmt;
+
 	of_node_put(bitclkmaster);
 	of_node_put(framemaster);
 
@@ -384,13 +372,12 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	dai_link->init = asoc_simple_card_dai_init;
 
 	dev_dbg(dev, "\tname : %s\n", dai_link->stream_name);
-	dev_dbg(dev, "\tcpu : %s / %04x / %d\n",
+	dev_dbg(dev, "\tformat : %04x\n", dai_link->dai_fmt);
+	dev_dbg(dev, "\tcpu : %s / %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",
+	dev_dbg(dev, "\tcodec : %s / %d\n",
 		dai_link->codec_dai_name,
-		dai_props->codec_dai.fmt,
 		dai_props->codec_dai.sysclk);
 
 	/*
@@ -577,14 +564,13 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 		dai_link->codec_name	= cinfo->codec;
 		dai_link->cpu_dai_name	= cinfo->cpu_dai.name;
 		dai_link->codec_dai_name = cinfo->codec_dai.name;
+		dai_link->dai_fmt	= cinfo->daifmt;
 		dai_link->init		= asoc_simple_card_dai_init;
 		memcpy(&priv->dai_props->cpu_dai, &cinfo->cpu_dai,
 					sizeof(priv->dai_props->cpu_dai));
 		memcpy(&priv->dai_props->codec_dai, &cinfo->codec_dai,
 					sizeof(priv->dai_props->codec_dai));
 
-		priv->dai_props->cpu_dai.fmt	|= cinfo->daifmt;
-		priv->dai_props->codec_dai.fmt	|= cinfo->daifmt;
 	}
 
 	snd_soc_card_set_drvdata(&priv->snd_card, priv);
-- 
1.9.1

Re: [PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Kuninori Morimoto]

Hi Lars, Mark

> Having to set different formats on the CPU side and the CODEC side of a DAI
> link is usually indication that something is terribly wrong and in most
> cases is a result of a broken driver that implements a set_fmt() callback
> which does not follow the specification. In the past this feature has been
> used to work around broken drivers, rather than fixing them. We don't really
> want to encourage this, so remove support for setting different formats on
> both ends of the link.
> 
> Along the way switch to static DAI format setup by setting the the dai_fmt
> field of the snd_soc_dai_link rather than calling snd_soc_dai_fmt().
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> same as original patch
> 
>  include/sound/simple_card.h     |  1 -
>  sound/soc/generic/simple-card.c | 30 ++++++++----------------------
>  2 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/include/sound/simple_card.h b/include/sound/simple_card.h
> index 1255ddb..b9b4f28 100644
> --- a/include/sound/simple_card.h
> +++ b/include/sound/simple_card.h
> @@ -16,7 +16,6 @@
>  
>  struct asoc_simple_dai {
>  	const char *name;
> -	unsigned int fmt;
>  	unsigned int sysclk;
>  	int slots;
>  	int slot_width;
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index c49a408..33feee9 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -125,14 +125,6 @@ static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
>  {
>  	int ret;
>  
> -	if (set->fmt) {
> -		ret = snd_soc_dai_set_fmt(dai, set->fmt);
> -		if (ret && ret != -ENOTSUPP) {
> -			dev_err(dai->dev, "simple-card: set_fmt error\n");
> -			goto err;
> -		}
> -	}

This patch removed above snd_soc_dai_set_fmt() here,
and samethings is done in snd_soc_instantiate_card().

But, I noticed it breaks set_fmt() and pcm_new() timing.
Before: set_fmt -> pcm_new 
After:  pcm_new -> set_fmt

My driver adds kctrl on pcm_new timing, and it refers
set_fmt's settings. but now, set_fmt happen *after* pcm_new.
(it adds new kctrl if it has SND_SOC_DAIFMT_CBS_CFS)

My solution is these 2
 pattern1) exchange set_fmt/pcm_new timing. see below
 pattern2) exchange kctrl assumption (always set kctrl)

Maybe I should try pattern2 ?

---------------------------------------
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 76bfff2..24d6733 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1604,6 +1604,12 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
                }
        }
 
+       for (i = 0; i < card->num_links; i++) {
+               if (card->dai_link[i].dai_fmt)
+                       snd_soc_runtime_set_dai_fmt(&card->rtd[i],
+                                                   card->dai_link[i].dai_fmt);
+       }
+
        /* probe all DAI links on this card */
        for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
                        order++) {
@@ -1642,12 +1648,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
                snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes,
                                        card->num_of_dapm_routes);
 
-       for (i = 0; i < card->num_links; i++) {
-               if (card->dai_link[i].dai_fmt)
-                       snd_soc_runtime_set_dai_fmt(&card->rtd[i],
-                               card->dai_link[i].dai_fmt);
-       }
-
        snprintf(card->snd_card->shortname, sizeof(card->snd_card->shortname),
                 "%s", card->name);
        snprintf(card->snd_card->longname, sizeof(card->snd_card->longname),

Re: [PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Lars-Peter Clausen]

[...]
> But, I noticed it breaks set_fmt() and pcm_new() timing.
> Before: set_fmt -> pcm_new
> After:  pcm_new -> set_fmt
>
> My driver adds kctrl on pcm_new timing, and it refers
> set_fmt's settings. but now, set_fmt happen *after* pcm_new.
> (it adds new kctrl if it has SND_SOC_DAIFMT_CBS_CFS)

What does that control do? This seems to be a bit of a layering violation to 
create a control in the PCM driver based on the configuration of the DAI link.

>
> My solution is these 2
>   pattern1) exchange set_fmt/pcm_new timing. see below
>   pattern2) exchange kctrl assumption (always set kctrl)
>
> Maybe I should try pattern2 ?
>
> ---------------------------------------
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 76bfff2..24d6733 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1604,6 +1604,12 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
>                  }
>          }
>
> +       for (i = 0; i < card->num_links; i++) {
> +               if (card->dai_link[i].dai_fmt)
> +                       snd_soc_runtime_set_dai_fmt(&card->rtd[i],
> +                                                   card->dai_link[i].dai_fmt);
> +       }
> +

This seems to be to early, the DAI's should at least have been probed. I 
think we should put it in soc_probe_link_dais() after the the dai_link->init 
section.

Re: [alsa-devel] [PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Kuninori Morimoto]

Hi Lars

Thank you for your feedback

> > Before: set_fmt -> pcm_new
> > After:  pcm_new -> set_fmt
> >
> > My driver adds kctrl on pcm_new timing, and it refers
> > set_fmt's settings. but now, set_fmt happen *after* pcm_new.
> > (it adds new kctrl if it has SND_SOC_DAIFMT_CBS_CFS)
> 
> What does that control do? This seems to be a bit of a layering
> violation to create a control in the PCM driver based on the
> configuration of the DAI link.

Our device can support runtime sampling rate convert, and our driver
is supporting it via DPCM. but, this feature needs "clock master".
This control is for it.

> > ---------------------------------------
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index 76bfff2..24d6733 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -1604,6 +1604,12 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
> >                  }
> >          }
> >
> > +       for (i = 0; i < card->num_links; i++) {
> > +               if (card->dai_link[i].dai_fmt)
> > +                       snd_soc_runtime_set_dai_fmt(&card->rtd[i],
> > +                                                   card->dai_link[i].dai_fmt);
> > +       }
> > +
> 
> This seems to be to early, the DAI's should at least have been
> probed. I think we should put it in soc_probe_link_dais() after the
> the dai_link->init section.

Thanks

soc_probe_link_dais() needs many loops
	for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; 
			order++) {
		for (i = 0; i < card->num_links; i++) {

but, it is checking

	if (order != SND_SOC_COMP_ORDER_LAST)
		return 0;

This means we can put it under soc_probe_link_dais()
I can send formal patch if this is OK.

# But, I wonder what is good explain about this patch ...
# indeed I noticed this issue from
# 1efb53a220b78fdfdbb97b726a2156713e75bdab
# (ASoC: simple-card: Remove support for setting differing DAI formats)
# but, it is simple-card user only...

--------
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 76bfff2..9777e78 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1324,6 +1324,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 		}
 	}
 
+	if (dai_link->dai_fmt)
+		snd_soc_runtime_set_dai_fmt(rtd, dai_link->dai_fmt);
+
 	ret = soc_post_component_init(rtd, dai_link->name);
 	if (ret)
 		return ret;
@@ -1642,12 +1645,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes,
 					card->num_of_dapm_routes);
 
-	for (i = 0; i < card->num_links; i++) {
-		if (card->dai_link[i].dai_fmt)
-			snd_soc_runtime_set_dai_fmt(&card->rtd[i],
-				card->dai_link[i].dai_fmt);
-	}
-
 	snprintf(card->snd_card->shortname, sizeof(card->snd_card->shortname),
 		 "%s", card->name);
 	snprintf(card->snd_card->longname, sizeof(card->snd_card->longname),
---------

Best regards
---
Kuninori Morimoto

Re: [PATCH 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

[Lars-Peter Clausen]

On 04/10/2015 11:21 AM, Kuninori Morimoto wrote:
> This means we can put it under soc_probe_link_dais()
> I can send formal patch if this is OK.

Looks perfect.

>
> # But, I wonder what is good explain about this patch ...
> # indeed I noticed this issue from
> # 1efb53a220b78fdfdbb97b726a2156713e75bdab
> # (ASoC: simple-card: Remove support for setting differing DAI formats)
> # but, it is simple-card user only...
>
> --------
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 76bfff2..9777e78 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1324,6 +1324,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
>   		}
>   	}
>
> +	if (dai_link->dai_fmt)
> +		snd_soc_runtime_set_dai_fmt(rtd, dai_link->dai_fmt);
> +
>   	ret = soc_post_component_init(rtd, dai_link->name);
>   	if (ret)
>   		return ret;
> @@ -1642,12 +1645,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
>   		snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes,
>   					card->num_of_dapm_routes);
>
> -	for (i = 0; i < card->num_links; i++) {
> -		if (card->dai_link[i].dai_fmt)
> -			snd_soc_runtime_set_dai_fmt(&card->rtd[i],
> -				card->dai_link[i].dai_fmt);
> -	}
> -
>   	snprintf(card->snd_card->shortname, sizeof(card->snd_card->shortname),
>   		 "%s", card->name);
>   	snprintf(card->snd_card->longname, sizeof(card->snd_card->longname),
> ---------
>
> Best regards
> ---
> Kuninori Morimoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>