From: Lars-Peter Clausen <lars@metafoo.de>
To: Benoit Cousson <bcousson@baylibre.com>
Cc: Fabien Parent <fparent@baylibre.com>,
alsa-devel@alsa-project.org, broonie@kernel.org,
lgirdwood@gmail.com, Misael Lopez Cruz <misael.lopez@ti.com>
Subject: Re: [RFT v2 6/6] ASoC: core: Add support for DAI multicodec
Date: Sat, 22 Mar 2014 09:33:35 +0100 [thread overview]
Message-ID: <532D4ADF.7030804@metafoo.de> (raw)
In-Reply-To: <1395415650-20045-7-git-send-email-bcousson@baylibre.com>
On 03/21/2014 04:27 PM, Benoit Cousson wrote:
> From: Misael Lopez Cruz <misael.lopez@ti.com>
>
> DAI link assumes a one to one mapping between CPU DAI and CODEC. In
> some cases, the same CPU DAI can be connected to several codecs.
> This is the case for example, if you connect two mono codecs to the
> same I2S link in order to have a stereo card.
> The current ASoC implementation does not allow such setup.
>
> Add support for DAI links composed of a single CPU DAI and multiple
> CODECs. Sound cards have to pass the CODECs array in the corresponding
> DAI link through a new 'snd_soc_dai_link_codec' struct. Each CODEC in
> this array is described in the same manner single CODEC DAIs are
> (either DT/OF node or codec_name).
>
> CPU DAI in a multicodec DAI link can have more channels than what each
> CODEC has. The number of channels each CODEC is responsible for is
> machine specific, hence it's fixed up in machine drivers in a similar
> way to what DPCM does.
>
> Fix a trailing space in the header as well.
This has been a feature that has been missing for quite a while, thanks for
taking care of this. There are a few things that need to be fixed, but
nothing that's not fixable.
I'd expect that after this patch has been applied there are no more usages
of rtd->codec_dai. There are some though.
soc_pcm_params_symmetry() and soc_pcm_has_symmetry(), this should be trivial
just loop over all the codecs.
rtd_get_codec_widget() this should be slightly restructured and merged with
rtd_get_cpu_widget() to have a single function that takes a DAI and returns
the widget. And then in dpcm_prune_paths() loop over all the codecs and only
prune if neither of them are in the list.
dpcm_get_be() loop over all the codecs if you find one where the widget
matches return the be.
soc_dpcm_be_digital_mute() mute all codecs.
snd_soc_dapm_connect_dai_link_widgets() needs to add a route for each codec
DAI to the CPU DAI
A few places in soc-compress.c, but which should all be very similar to
soc-pcm.c
I haven't fully understood how exactly the channel fixup works, so I didn't
comment on it yet.
>
> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
> [fparent@baylibre.com: Adapt to 3.14+]
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> [bcousson@baylibre.com: Change tdm mask fixup, add missing iteration
> over codecs array]
> Signed-off-by: Benoit Cousson <bcousson@baylibre.com>
> ---
> include/sound/soc-dai.h | 5 +
> include/sound/soc.h | 16 ++
> sound/soc/soc-core.c | 356 ++++++++++++++++++++++++++-------------
> sound/soc/soc-dapm.c | 39 +++--
> sound/soc/soc-pcm.c | 438 +++++++++++++++++++++++++++++++++---------------
> 5 files changed, 596 insertions(+), 258 deletions(-)
>
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index 2f66d5e..5391e70 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -274,6 +274,11 @@ struct snd_soc_dai {
> struct snd_soc_codec *codec;
> struct snd_soc_component *component;
>
> + /* CODEC TDM slot masks and params (for fixup) */
> + unsigned int tx_mask;
> + unsigned int rx_mask;
> + struct snd_pcm_hw_params params[2];
> +
> struct snd_soc_card *card;
>
> struct list_head list;
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 0b83168..fd14ec6 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -830,6 +830,15 @@ struct snd_soc_platform_driver {
> int (*bespoke_trigger)(struct snd_pcm_substream *, int);
> };
>
> +struct snd_soc_dai_link_codec {
For the sake of symmetry maybe name this snd_soc_dai_link_component and drop
the 'codec_' prefix in front of the struct fields. There is no reason why
this couldn't be used for CPU dais as well at some point.
> + const char *codec_name;
> + const struct device_node *codec_of_node;
> + const char *codec_dai_name;
I'd like to see this split up into the descriptive part that holds the name,
of_node etc, and the runtime data that holds the pointer to the DAIs. The
descriptive part goes in the dai_link struct the. The pointers to the DAIs
go into the snd_soc_pcm_runtime struct. This is how it is used the only
place where you need both is in soc_bind_dai_link.
> +
> + struct snd_soc_codec *codec;
We are trying to remove the asymmetry between the CPU side DAI and the CODEC
side DAI of the DAI link. E.g. the cpu_dai can also be a CODEC. The only
reason why there still is a codec pointer in the snd_soc_pcm_runtime struct
is because there are still a few places where it is used (and for auxdevs).
But we shouldn't mimic this for new code. Just use codec_dai->codec instead
when you actually need a pointer to the codec.
> + struct snd_soc_dai *codec_dai;
> +};
> +
> struct snd_soc_platform {
> const char *name;
> int id;
> @@ -878,6 +887,10 @@ struct snd_soc_dai_link {
> const struct device_node *codec_of_node;
> /* You MUST specify the DAI name within the codec */
> const char *codec_dai_name;
> +
> + struct snd_soc_dai_link_codec *codecs;
> + int num_codecs;
unsigned int
> +
> /*
> * You MAY specify the link's platform/PCM/DMA driver, either by
> * device name, or by DT/OF node, but not both. Some forms of link
> @@ -1067,6 +1080,9 @@ struct snd_soc_pcm_runtime {
> struct snd_soc_dai *codec_dai;
> struct snd_soc_dai *cpu_dai;
>
> + struct snd_soc_dai_link_codec *codecs;
With the changes suggested above this simply becomes:
struct snd_soc_dai **codec_dais;
Which should also make the code shorter at places since
> + int num_codecs;
unsigned int
> +
> struct delayed_work delayed_work;
> #ifdef CONFIG_DEBUG_FS
> struct dentry *debugfs_dpcm_root;
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index cfa481e..6b6fc39 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
[...]
> + /* Find CODEC from registered CODECs */
> + for (i = 0; i < rtd->num_codecs; i++) {
> + codecs[i].codec = soc_find_codec(codecs[i].codec_of_node,
> + codecs[i].codec_name);
> + if (!codecs[i].codec) {
> + dev_err(card->dev, "ASoC: CODEC %s not registered\n",
> + codecs[i].codec_name);
> + return -EPROBE_DEFER;
> + }
> }
>
> - /* Find CODEC DAI from registered list */
> - rtd->codec_dai = soc_find_codec_dai(rtd->codec,
> - dai_link->codec_dai_name);
> - if (!rtd->codec_dai) {
> - dev_err(card->dev, "ASoC: CODEC DAI %s not registered\n",
> - dai_link->codec_dai_name);
> - return -EPROBE_DEFER;
> + for (i = 0; i < rtd->num_codecs; i++) {
> + codecs[i].codec_dai = soc_find_codec_dai(
> + codecs[i].codec,
> + codecs[i].codec_dai_name);
> + if (!codecs[i].codec_dai) {
> + dev_err(card->dev, "ASoC: CODEC DAI %s not registered\n",
> + codecs[i].codec_dai_name);
> + return -EPROBE_DEFER;
> + }
> }
I suggest to restructure this to:
for (i = 0; i < rtd->num_codecs; i++) {
rtd->codec_dais[i] = soc_find_codec_dai(codecs[i]);
if (!rtd->codecs_dais[i])
...
}
And in soc_find_codec_dai lookup both the CODEC and the DAI
[...]
> +static int soc_dai_link_init(struct snd_soc_card *card, int num)
> {
> struct snd_soc_dai_link *dai_link = &card->dai_link[num];
> struct snd_soc_pcm_runtime *rtd = &card->rtd[num];
> - const char *temp;
> - int ret;
> + struct snd_soc_dai_link_codec *codecs = rtd->codecs;
> + const char **temp;
> + int i, ret;
>
> rtd->card = card;
>
> - /* machine controls, routes and widgets are not prefixed */
> - temp = codec->name_prefix;
> - codec->name_prefix = NULL;
> + temp = devm_kzalloc(card->dev, rtd->num_codecs * sizeof(char *),
> + GFP_KERNEL);
There is a patch http://permalink.gmane.org/gmane.linux.alsa.devel/120606
which removes the temporary name_prefix = NULL. Having that patch applied
first should make this a lot simpler. I don't think you'd even need patch 5
then since this doesn't need to actually loop over all the CODECs anymore.
> + if (!temp)
> + return -ENOMEM;
> +
> + for (i = 0; i < rtd->num_codecs; i++) {
> + /* Make sure all DAPM widgets are instantiated */
> + snd_soc_dapm_new_widgets(codecs[i].codec->dapm.card);
We remove this per component snd_soc_dapm_new_widgets() calls a while ago,
there is now only one call to snd_soc_dapm_new_widgets() once all components
have been initialized.
> +
> + /* machine controls, routes and widgets are not prefixed */
> + temp[i] = codecs[i].codec->name_prefix;
> + codecs[i].codec->name_prefix = NULL;
> + }
>
> /* do machine specific initialization */
> if (dai_link->init) {
> @@ -1318,15 +1364,15 @@ static int soc_dai_link_init(struct snd_soc_card *card,
> return ret;
> }
>
> - codec->name_prefix = temp;
> + for (i = 0; i < rtd->num_codecs; i++)
> + codecs[i].codec->name_prefix = temp[i];
>
> - rtd->codec = codec;
> + devm_kfree(card->dev, temp);
>
> return 0;
> }
>
[...]
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index c8a780d..6c6b7bea 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
[...]
> -static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
> - int event)
> +static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd,
> + struct snd_soc_dai *cpu_dai,
> + struct snd_soc_dai *codec_dai,
> + int stream, int event)
> {
> -
> struct snd_soc_dapm_widget *w_cpu, *w_codec;
> - struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
>
> if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> w_cpu = cpu_dai->playback_widget;
> @@ -3582,9 +3593,15 @@ void snd_soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
> int event)
> {
> struct snd_soc_card *card = rtd->card;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + struct snd_soc_dai *codec_dai;
> + int i;
>
> mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> - soc_dapm_stream_event(rtd, stream, event);
> + for (i = 0; i < rtd->num_codecs; i++) {
> + codec_dai = rtd->codecs[i].codec_dai;
> + soc_dapm_stream_event(rtd, cpu_dai, codec_dai, stream, event);
This needs some refactoring. The looping over all the CODEC should be moved
into soc_dapm_stream_event(). We don't want to call seperatly for each CODEC
dapm_power_widgets().
> + }
> mutex_unlock(&card->dapm_mutex);
> }
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 330eaf0..01d6dd0 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
[...]
> @@ -78,24 +83,30 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
> {
[...]
> }
>
> +
Extra newline
> /**
> * snd_soc_runtime_ignore_pmdown_time() - Check whether to ignore the power down delay
> * @rtd: The ASoC PCM runtime that should be checked.
> @@ -107,11 +118,16 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
> */
> bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd)
> {
> + struct snd_soc_dai_link_codec *codecs = rtd->codecs;
> + int i, ignore = 0;
> +
> if (!rtd->pmdown_time || rtd->dai_link->ignore_pmdown_time)
> return true;
>
> - return rtd->cpu_dai->component->ignore_pmdown_time &&
> - rtd->codec_dai->component->ignore_pmdown_time;
> + for (i = 0; (i < rtd->num_codecs) && !ignore; i++)
> + ignore |= codecs[i].codec_dai->component->ignore_pmdown_time;
This should be "ignore = 1" and then ignore &= ... for each CODEC. We only
want to ignore the power down delay if all components agree that they do not
need it.
> +
> + return rtd->cpu_dai->component->ignore_pmdown_time && ignore;
> }
>
> /**
> @@ -310,32 +326,66 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
> }
> }
[...]
> - soc_pcm_apply_msb(substream, codec_dai);
> + soc_pcm_apply_msb(substream, codecs[0].codec_dai);
I think how this should work is that there should only be a msb constraint
if all the CODEC DAIs have a msb contraint and then it should be the maximum
over all the contraints.
> soc_pcm_apply_msb(substream, cpu_dai);
>
[...]
> /*
> * Called by ALSA when the hardware params are set by application. This
> * function can also be called multiple times and can allocate buffers
> @@ -667,9 +766,9 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> {
> struct snd_soc_pcm_runtime *rtd = substream->private_data;
> struct snd_soc_platform *platform = rtd->platform;
> + struct snd_soc_dai_link_codec *codecs = rtd->codecs;
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
> - int ret = 0;
> + int i, ret = 0;
>
> mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>
> @@ -686,13 +785,39 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> }
> }
>
> - if (codec_dai->driver->ops && codec_dai->driver->ops->hw_params) {
> - ret = codec_dai->driver->ops->hw_params(substream, params, codec_dai);
> - if (ret < 0) {
> - dev_err(codec_dai->dev, "ASoC: can't set %s hw params:"
> - " %d\n", codec_dai->name, ret);
> - goto codec_err;
> + for (i = 0; i < rtd->num_codecs; i++) {
> + struct snd_soc_dai *codec_dai = codecs[i].codec_dai;
> + struct snd_pcm_hw_params *codec_params;
> +
> + codec_params = &codec_dai->params[substream->stream];
> +
> + /* copy params for each codec */
> + memcpy(codec_params, params, sizeof(struct snd_pcm_hw_params));
> +
> + /* fixup params based on TDM slot masks */
> + if (codec_dai->tx_mask)
> + soc_pcm_codec_params_fixup(codec_params,
> + codec_dai->tx_mask);
> + if (codec_dai->rx_mask)
> + soc_pcm_codec_params_fixup(codec_params,
> + codec_dai->rx_mask);
> +
> + if (codec_dai->driver->ops &&
> + codec_dai->driver->ops->hw_params) {
> + ret = codec_dai->driver->ops->hw_params(substream,
> + codec_params, codec_dai);
> + if (ret < 0) {
> + dev_err(codec_dai->dev,
> + "ASoC: can't set %s hw params: %d\n",
> + codec_dai->name, ret);
> + goto codec_err;
> + }
> }
> +
> + codec_dai->rate = params_rate(codec_params);
> + codec_dai->channels = params_channels(codec_params);
> + codec_dai->sample_bits = snd_pcm_format_physical_width(
> + params_format(codec_params));
Those should only be set if all of the hw_params calls succeeded.
> }
>
> if (cpu_dai->driver->ops && cpu_dai->driver->ops->hw_params) {
[...]
> @@ -764,16 +892,21 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> cpu_dai->sample_bits = 0;
> }
>
> - if (codec_dai->active == 1) {
> - codec_dai->rate = 0;
> - codec_dai->channels = 0;
> - codec_dai->sample_bits = 0;
> + for (i = 0; i < rtd->num_codecs; i++) {
> + codec_dai = codecs[i].codec_dai;
> + if (codec_dai->active == 1)
Missing brackets
> + codec_dai->rate = 0;
> + codec_dai->channels = 0;
> + codec_dai->sample_bits = 0;
> }
>
[...]
> @@ -2193,22 +2353,29 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
> int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> {
> struct snd_soc_platform *platform = rtd->platform;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
> + struct snd_soc_dai_link_codec *codecs = rtd->codecs;
> + struct snd_soc_dai *codec_dai;
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> struct snd_pcm *pcm;
> char new_name[64];
> int ret = 0, playback = 0, capture = 0;
> + int i;
>
> if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> playback = rtd->dai_link->dpcm_playback;
> capture = rtd->dai_link->dpcm_capture;
> } else {
> - if (codec_dai->driver->playback.channels_min &&
> - cpu_dai->driver->playback.channels_min)
> - playback = 1;
> - if (codec_dai->driver->capture.channels_min &&
> - cpu_dai->driver->capture.channels_min)
> - capture = 1;
> + for (i = 0; i < rtd->num_codecs; i++) {
> + codec_dai = codecs[i].codec_dai;
> + if (codec_dai->driver->playback.channels_min &&
> + cpu_dai->driver->playback.channels_min)
> + playback++;
> + if (codec_dai->driver->capture.channels_min &&
> + cpu_dai->driver->capture.channels_min)
> + capture++;
> + }
> + capture = (rtd->num_codecs == capture);
> + playback = (rtd->num_codecs == playback);
Hm... I'd say the runtime should support playback or capture if at least one
of the codecs support playback or capture. Not only if all of them support it.
> }
>
> if (rtd->dai_link->playback_only) {
[...]
next prev parent reply other threads:[~2014-03-22 8:32 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-21 15:27 [RFT v2 0/6] ASoC: core: Add support for DAI multicodec Benoit Cousson
2014-03-21 15:27 ` [RFT v2 1/6] ASoC: core: Add helpers for codec and codec_dai search Benoit Cousson
2014-03-21 15:27 ` [RFT v2 2/6] ASoC: core: Add helpers for codec DAI probe & remove Benoit Cousson
2014-03-21 15:27 ` [RFT v2 3/6] ASoC: core: Add helper for DAI widgets linking Benoit Cousson
2014-03-21 15:27 ` [RFT v2 4/6] ASoC: core: Add function for ac97 codec registration Benoit Cousson
2014-03-21 15:27 ` [RFT v2 5/6] ASoC: core: Add helpers for dai link and aux dev init Benoit Cousson
2014-04-14 19:39 ` Mark Brown
2014-04-14 20:12 ` Benoit Cousson
2014-03-21 15:27 ` [RFT v2 6/6] ASoC: core: Add support for DAI multicodec Benoit Cousson
2014-03-22 8:33 ` Lars-Peter Clausen [this message]
2014-03-25 13:10 ` Benoit Cousson
2014-03-25 17:01 ` Lars-Peter Clausen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=532D4ADF.7030804@metafoo.de \
--to=lars@metafoo.de \
--cc=alsa-devel@alsa-project.org \
--cc=bcousson@baylibre.com \
--cc=broonie@kernel.org \
--cc=fparent@baylibre.com \
--cc=lgirdwood@gmail.com \
--cc=misael.lopez@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.