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-justfied mode both the frame-clock and the bit-clock need to be inverted to be standards compliant.

This means also 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 --- 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?

Changes since v1: * Fix whitespace --- 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..6783d3f 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;

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 --- Changes since v1: * dev_dbg(dai_fmt) --- 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 f7c6734..3efd947 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,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);

/* @@ -572,14 +559,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);

Hi Lars

Thank you for your patch

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

No changes since v1.

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",

I guess if you want to use common .daifmt, it should be same as "codec" side. (cpu side "S" means, codec / daifmt side "M") So, this mean above should be

@@ -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_CBM_CFM, <= .cpu_dai = { .name = "fsib-dai", - .fmt = SND_SOC_DAIFMT_CBS_CFS, },

c7a507eea1db1430476289f525f9c853d5d485e8 (ASoC: fsi: fixup SND_SOC_DAIFMT_CBx_CFx flags) can be good example ? (I wonder what happen on armadillo800eva ?)

Best regards --- Kuninori Morimoto

Hi Lars, Simon

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

I tested these 3 patches on armadillo800eva

[PATCH v2 1/3] ARM: shmobile: armadillo800eva: Properly specify HDMI audio link format [PATCH v2 2/3] ASoC: sh: fsi: Fix clock inversion [PATCH v2 3/3] ASoC: simple-card: Remove support for setting differing DAI formats

Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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. We used this inversion flags on each board for historical reasons (copy-paste), 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. armadillo800eva works well with below patch (as 2/3 patch)

-------------- 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 = { --------------