[alsa-devel] [PATCH 0/3] ARM: shmobile: armadillo: fixup CPU settings
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(-)
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
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
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
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),
[...]
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.
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
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
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?
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>
On Tue, Mar 24, 2015 at 01:05:02AM +0000, Kuninori Morimoto wrote:
Hi Mark, Lars Cc Simon
Applied all, thanks.
participants (4)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Mark Brown -
Simon Horman