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

[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 1/3] ARM: shmobile: armadillo800eva: Properly specify HDMI audio link format

[Kuninori Morimoto]

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

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>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
same as original patch

 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 7eac846..ac8e91d 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.9.1

[PATCH 2/3] ARM: shmobile: armadillo800eva: fix clock inversion

[Kuninori Morimoto]

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

When operating in left-justfied mode both the frame-clock and the
bit-clock need to be inverted to be standards compliant.

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

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
removed FSI driver fixup parts

 arch/arm/mach-shmobile/board-armadillo800eva.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index ac8e91d..bf37e3c 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 = {
-- 
1.9.1

[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: [alsa-devel] [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: [alsa-devel] [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
>

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

[Mark Brown]

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

On Tue, Mar 24, 2015 at 01:05:02AM +0000, Kuninori Morimoto wrote:

> So, I think keeping current FSI driver as-is is more safety for old boards.
> I tested these patches on latest Mark's branch 

Simon, are these OK for me to apply or do you want to apply them?

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

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

[Simon Horman]

Hi Mark,

On Thu, Mar 26, 2015 at 10:36:28AM -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 01:05:02AM +0000, Kuninori Morimoto wrote:
> 
> > So, I think keeping current FSI driver as-is is more safety for old boards.
> > I tested these patches on latest Mark's branch 
> 
> Simon, are these OK for me to apply or do you want to apply them?

These changes do not appear to conflict with anything I have queued up for
v4.1 so assuming that you are targeting that release please feel free to
take them. (My only worry here is about conflicts.)

Acked-by: Simon Horman <horms+renesas@verge.net.au>

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

[Mark Brown]

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

On Tue, Mar 24, 2015 at 01:05:02AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark, Lars
> Cc Simon

Applied all, thanks.

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