diff mbox

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

Message ID 878ue08ngb.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State RFC
Headers show

Commit Message

Kuninori Morimoto April 10, 2015, 7:15 a.m. UTC 
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 ?

---------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Lars-Peter Clausen April 10, 2015, 8:52 a.m. UTC | #1 
[...]
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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