[alsa-devel] [PATCH 0/2][RFC] ASoC: soc-core: modern style dai_link control
Hi Mark
These are [RFC] patches which adds new rule for modern style dai_link. Many sound are using CPU component as Platform component, and then it has below settings on legacy style dai_link
dai_link->platform_of_node = dai_link->cpu_of_node
But, this duplicated component will be just ignored because it is already selected when CPU component binding. We want to allow "no Platform" on modern style dai_link.
And, current legacy style dai_link allows no Platform settings, then, soc-core will select snd-soc-dummy platform for it automatically. In modern style, we need to have CPU/Codec/Platform as array/pointer style. Thus, this kind of "implicit" selection will be just un-understandable.
These 2 patches adds new rule for modern style, and have no effect to legacy style. But, I can't test for *all* board/situation. Thus, I added [RFC] for these.
Kuninori Morimoto (2): ASoC: soc-core: allow no Platform on dai_link ASoC: soc-core: no more implicit snd-soc-dummy platform on modern dai_link
include/sound/soc.h | 2 +- sound/soc/generic/simple-card-utils.c | 4 ++ sound/soc/soc-core.c | 80 ++++++++++++++++++++++------------- 3 files changed, 56 insertions(+), 30 deletions(-)
Hi Mark, again
These are [RFC] patches which adds new rule for modern style dai_link. Many sound are using CPU component as Platform component, and then it has below settings on legacy style dai_link
dai_link->platform_of_node = dai_link->cpu_of_node
But, this duplicated component will be just ignored because it is already selected when CPU component binding. We want to allow "no Platform" on modern style dai_link.
And, current legacy style dai_link allows no Platform settings, then, soc-core will select snd-soc-dummy platform for it automatically. In modern style, we need to have CPU/Codec/Platform as array/pointer style. Thus, this kind of "implicit" selection will be just un-understandable.
These 2 patches adds new rule for modern style, and have no effect to legacy style. But, I can't test for *all* board/situation. Thus, I added [RFC] for these.
And, I think these are no special effect on Codec<->Codec. But, need to check/test.
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Many drivers are constituting "Platform == CPU", so it has below settings on current "legacy style".
dai_link->platform_of_node = dai_link->cpu_of_node;
Because of it, soc_bind_dai_link() will pick-up "CPU component" as "Platform component", and try to add it to snd_soc_pcm_runtime. But it will be ignored, because it is already added when CPU bindings.
This kind of "Platform" settings is pointless. This patch allows "no Platform" for "modern style" (= snd_soc_dai_link_component).
Unfortunately, there are some sound card drivers which doesn't have platform_name, platform_of_node for legacy style. Of course it doesn't have platforms either for modern style. But, it is assuming that "snd_soc_dummy platform" is automatically selected by soc-core. Because of it, we need to judge that dai_link is using "legacy style" or "modern style" somehow. no Platform settings on "legacy style" -> select snd_soc_dummy no Platform settings on "modern style" -> real "no Platform"
If sound card dirver is using "modern style", both codec/platform are using it. So this patch judges it as "modern style" if there is no settings for codec_name codec_of_node codec_dai_name platform_name platform_of_node
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 2 +- sound/soc/soc-core.c | 71 ++++++++++++++++++++++++++++++++++------------------ 2 files changed, 48 insertions(+), 25 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 9568968..c18e076 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -957,7 +957,7 @@ struct snd_soc_dai_link { /* * 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 - * do not need a platform. + * do not need a platform. In such case, platforms are not mandatory. */ const char *platform_name; struct device_node *platform_of_node; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index d6b5edb..cefd121 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -768,6 +768,9 @@ static int snd_soc_is_matching_component( { struct device_node *component_of_node;
+ if (!dlc) + return 0; + component_of_node = component->dev->of_node; if (!component_of_node && component->dev->parent) component_of_node = component->dev->parent->of_node; @@ -1052,6 +1055,15 @@ static int snd_soc_init_platform(struct snd_soc_card *card, * see * soc.h :: struct snd_soc_dai_link */ + + /* it is already using modern style */ + if (!dai_link->codec_name && + !dai_link->codec_of_node && + !dai_link->codec_dai_name && + !dai_link->platform_name && + !dai_link->platform_of_node) + goto init_end; + /* convert Legacy platform link */ if (!platform || dai_link->legacy_platform) { platform = devm_kzalloc(card->dev, @@ -1068,6 +1080,7 @@ static int snd_soc_init_platform(struct snd_soc_card *card, platform->dai_name = NULL; }
+init_end: /* if there's no platform we match on the empty platform */ if (!platform->name && !platform->of_node) @@ -1151,32 +1164,38 @@ static int soc_init_dai_link(struct snd_soc_card *card, } }
- /* FIXME */ - if (link->num_platforms > 1) { - dev_err(card->dev, - "ASoC: multi platform is not yet supported %s\n", - link->name); - return -EINVAL; - } - /* - * Platform may be specified by either name or OF node, but - * can be left unspecified, and a dummy platform will be used. + * Platform may be specified by either name or OF node, + * or no Platform. + * + * FIXME + * + * We need multi-platform support */ - if (link->platforms->name && link->platforms->of_node) { - dev_err(card->dev, - "ASoC: Both platform name/of_node are set for %s\n", - link->name); - return -EINVAL; - } + if (link->num_platforms > 0) {
- /* - * Defer card registartion if platform dai component is not added to - * component list. - */ - if ((link->platforms->of_node || link->platforms->name) && - !soc_find_component(link->platforms->of_node, link->platforms->name)) - return -EPROBE_DEFER; + if (link->num_platforms > 1) { + dev_err(card->dev, + "ASoC: multi platform is not yet supported %s\n", + link->name); + return -EINVAL; + } + + if (link->platforms->name && link->platforms->of_node) { + dev_err(card->dev, + "ASoC: Both platform name/of_node are set for %s\n", + link->name); + return -EINVAL; + } + + /* + * Defer card registartion if platform dai component is not added to + * component list. + */ + if ((link->platforms->of_node || link->platforms->name) && + !soc_find_component(link->platforms->of_node, link->platforms->name)) + return -EPROBE_DEFER; + }
/* * CPU device may be specified by either name or OF node, but @@ -1975,7 +1994,11 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dev_err(card->dev, "init platform error"); continue; } - dai_link->platforms->name = component->name; + if (dai_link->platforms) + dai_link->platforms->name = component->name; + else + dev_err(card->dev, "no platform was selected %s\n", + dai_link->name);
/* convert non BE into BE */ dai_link->no_pcm = 1;
If sound card dirver is using "modern style", both codec/platform are using it. So this patch judges it as "modern style" if there is no settings for codec_name codec_of_node codec_dai_name platform_name platform_of_node
Doesn't this prevent a gradual dailink transition where e.g. the platform_name is removed first and then the codec_name/codec_dai_name is removed second?
I started doing the transition in steps changing all dailinks with platform and codec/codec_dai names at once is quite invasive and possibly error prone. Specifically for Intel machine drivers, the codec names are heavily tweaked to align with the actual ACPI name, not the hard-coded one, and that should be tested independently if possible. Same for codec_dai_names, they depend on quirks.
-Pierre
Hi Pierre-Louis
If sound card dirver is using "modern style", both codec/platform are using it. So this patch judges it as "modern style" if there is no settings for codec_name codec_of_node codec_dai_name platform_name platform_of_node
Doesn't this prevent a gradual dailink transition where e.g. the platform_name is removed first and then the codec_name/codec_dai_name is removed second?
I'm thinking transition to modern from legacy for "normal drivers" will be like this
1) will happen after multi-CPU was supported 2) Transit everything (= CPU/Codec/Platform) by 1 patch for each drivers, not gradual transition (= Mark / Lars doesn't like this style). 3) If you need/want to gradual transition (= like simple-card, audio-graph), unfortunately, it will be full responsibility for your action.
Yes, it is selfish, but is very difficult to prevent all cases in this case. So I think we need to have some rule for transition. Or, other idea is that transit all drivers first without this patch, and add support "no Platform" 2nd. In this case, it will be easier, but will needs many patches. I'm not sure which one is best.
I started doing the transition in steps changing all dailinks with platform and codec/codec_dai names at once is quite invasive and possibly error prone. Specifically for Intel machine drivers, the codec names are heavily tweaked to align with the actual ACPI name, not the hard-coded one, and that should be tested independently if possible. Same for codec_dai_names, they depend on quirks.
Yeah agree. I think transit to modern style on magical (= non hard-coded) platform will be trouble... I'm thinking that transition patch need to be tested/confirmed before removing legacy style for its safety
1) transit "hard-coded" platform first, and confirm modern style behavior. 2) if no problem happen on 1) step, transit "magical" platform 2nd step. will have few/some problem, fixup step-by-step.
These are my opinion, but I want to know your and Mark's idea. I can adjust/follow to it.
Best regards --- Kuninori Morimoto
On 1/30/19 7:46 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis
If sound card dirver is using "modern style", both codec/platform are using it. So this patch judges it as "modern style" if there is no settings for codec_name codec_of_node codec_dai_name platform_name platform_of_node
Doesn't this prevent a gradual dailink transition where e.g. the platform_name is removed first and then the codec_name/codec_dai_name is removed second?
I'm thinking transition to modern from legacy for "normal drivers" will be like this
- will happen after multi-CPU was supported
- Transit everything (= CPU/Codec/Platform) by 1 patch for each drivers, not gradual transition (= Mark / Lars doesn't like this style).
- If you need/want to gradual transition (= like simple-card, audio-graph), unfortunately, it will be full responsibility for your action.
Yes, it is selfish, but is very difficult to prevent all cases in this case. So I think we need to have some rule for transition. Or, other idea is that transit all drivers first without this patch, and add support "no Platform" 2nd. In this case, it will be easier, but will needs many patches. I'm not sure which one is best.
I started doing the transition in steps changing all dailinks with platform and codec/codec_dai names at once is quite invasive and possibly error prone. Specifically for Intel machine drivers, the codec names are heavily tweaked to align with the actual ACPI name, not the hard-coded one, and that should be tested independently if possible. Same for codec_dai_names, they depend on quirks.
Yeah agree. I think transit to modern style on magical (= non hard-coded) platform will be trouble... I'm thinking that transition patch need to be tested/confirmed before removing legacy style for its safety
- transit "hard-coded" platform first, and confirm modern style behavior.
- if no problem happen on 1) step, transit "magical" platform 2nd step. will have few/some problem, fixup step-by-step.
These are my opinion, but I want to know your and Mark's idea. I can adjust/follow to it.
The problem is that it's not just a renaming, there are multiple cases where the codec_name, codec_dai_name fields are explicitly handled. See my initial (compile-tested only) changes at [1]
[1] https://github.com/plbossart/sound/commits/fix/codec-legacy-dai
Hi Pierre
Thank you for your feedback
codec_name codec_of_node codec_dai_name platform_name platform_of_node
(snip)
The problem is that it's not just a renaming, there are multiple cases where the codec_name, codec_dai_name fields are explicitly handled. See my initial (compile-tested only) changes at [1]
Thank you for (off-list) explanation. I think "no Platform" and "no implicit snd-soc-dummy" idea itself is not so bad, but yes... indeed validation might be not easy for some platforms/boards...
I think "transit to modern" is just "rename", but "no Platform / no implicit dummy" are "new feature". Let's separate these. I want to know Mark's opinion
Hi Pierre
codec_name codec_of_node codec_dai_name platform_name platform_of_node
(snip)
The problem is that it's not just a renaming, there are multiple cases where the codec_name, codec_dai_name fields are explicitly handled. See my initial (compile-tested only) changes at [1]
BTW, do you think having macro for it can help us ? This is just sample, it is including modern style multi-CPU so far, but, conversion will be a little bit easier. It is using samsung/bells.c driver for single CPU/Codec/Platform sample. We can adjust for multi CPU/Codec/Platform, I guess.
----------- diff --git a/include/sound/soc.h b/include/sound/soc.h index c18e076..0796b15 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -919,6 +919,8 @@ struct snd_soc_dai_link { */ const char *cpu_dai_name;
+ struct snd_soc_dai_link_component *cpus; + unsigned int num_cpus; /* * codec_name * codec_of_node @@ -1031,6 +1033,40 @@ struct snd_soc_dai_link { struct list_head list; /* DAI link list of the soc card */ struct snd_soc_dobj dobj; /* For topology */ }; +#define LINK_SET_LATER { } +#define LINK_CPU(_dai) { .dai_name = _dai, } +#define LINK_CODEC(_name, _dai) { .name = _name, .dai_name = _dai, } +#define LINK_PLATFORM(_name) { .name = _name } + +#define SND_SOC_DAI_LINK_COMPONENT_CPU(name) \ + struct snd_soc_dai_link_component name##_cpu[] +#define SND_SOC_DAI_LINK_COMPONENT_CODEC(name) \ + struct snd_soc_dai_link_component name##_codec[] +#define SND_SOC_DAI_LINK_COMPONENT_PLATFORM(name) \ + struct snd_soc_dai_link_component name##_platform[] + +#define SND_SOC_DAI_LINK_COMPONENT_CC(name, cpu, codec) \ + SND_SOC_DAI_LINK_COMPONENT_CPU(name) = {cpu};\ + SND_SOC_DAI_LINK_COMPONENT_CODEC(codec) = {codec} + +#define SND_SOC_DAI_LINK_COMPONENT_CCP(name, cpu, codec, platform) \ + SND_SOC_DAI_LINK_COMPONENT_CPU(name) = {cpu}; \ + SND_SOC_DAI_LINK_COMPONENT_CODEC(name) = {codec}; \ + SND_SOC_DAI_LINK_COMPONENT_PLATFORM(name) = {platform} + +#define SND_SOC_LINK_CPU(name) .cpus = name, .num_cpus = ARRAY_SIZE(name) +#define SND_SOC_LINK_CODEC(name) .codecs = name, .num_codecs = ARRAY_SIZE(name) +#define SND_SOC_LINK_PLATFORM(name) .platforms = name, .num_platforms = ARRAY_SIZE(name) + +#define SND_SOC_LINK_CC(name) \ + SND_SOC_LINK_CPU(name##_cpu), \ + SND_SOC_LINK_CODEC(name##_codec) + +#define SND_SOC_LINK_CCP(name) \ + SND_SOC_LINK_CPU(name##_cpu), \ + SND_SOC_LINK_CODEC(name##_codec), \ + SND_SOC_LINK_PLATFORM(name##_platform) + #define for_each_link_codecs(link, i, codec) \ for ((i) = 0; \ ((i) < link->num_codecs) && ((codec) = &link->codecs[i]); \ diff --git a/sound/soc/samsung/bells.c b/sound/soc/samsung/bells.c index 0e66cd8..4ec2f5b 100644 --- a/sound/soc/samsung/bells.c +++ b/sound/soc/samsung/bells.c @@ -317,14 +317,16 @@ static struct snd_soc_dai_link bells_dai_wm5102[] = { }, };
+SND_SOC_DAI_LINK_COMPONENT_CCP(cpu_dsp, + LINK_CPU("samsung-i2s.0"), + LINK_CODEC("spi0.0", "wm0010-sdi1"), + LINK_PLATFORM("samsung-i2s.0")); + static struct snd_soc_dai_link bells_dai_wm5110[] = { { .name = "CPU-DSP", .stream_name = "CPU-DSP", - .cpu_dai_name = "samsung-i2s.0", - .codec_dai_name = "wm0010-sdi1", - .platform_name = "samsung-i2s.0", - .codec_name = "spi0.0", + SND_SOC_LINK_CCP(cpu_dsp), .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM, },
On Fri, Feb 01, 2019 at 09:56:05AM +0900, Kuninori Morimoto wrote:
I think "transit to modern" is just "rename", but "no Platform / no implicit dummy" are "new feature". Let's separate these. I want to know Mark's opinion
I agree that these are different things, though if we allow things to sit for too long it's going to be confusing so ideally they should happen close together.
Hi Mark
I think "transit to modern" is just "rename", but "no Platform / no implicit dummy" are "new feature". Let's separate these. I want to know Mark's opinion
I agree that these are different things, though if we allow things to sit for too long it's going to be confusing so ideally they should happen close together.
Yes, agree. I think we can do like this
v5.x : multi CPU support v5.x + 1 : transit to modern for CPU/Codec/Platform on all drivers v5.x + 2 : add "no Platform / no implicit dummy" support
If there is no objection about "no Platform / no implicit dummy"...
Best regards --- Kuninori Morimoto
On Tue, Feb 12, 2019 at 10:03:32AM +0900, Kuninori Morimoto wrote:
Yes, agree. I think we can do like this
v5.x : multi CPU support v5.x + 1 : transit to modern for CPU/Codec/Platform on all drivers v5.x + 2 : add "no Platform / no implicit dummy" support
If there is no objection about "no Platform / no implicit dummy"...
That sounds like a reasonable plan; I don't think we actually need to wait for a new kernel version between each of these if things happen to go faster but equally if we do that's also fine.
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current ALSA SoC will automatically selects snd-soc-dummy platform by snd_soc_init_platform() if dai_link doesn't select specified platform at legacy style dai_link. Current ALSA SoC is supporting both "legacy style" (= xxx_name, xxx_of_node, xxx_dai_name) and "modern style" (= snd_soc_dai_link_component). This "implicit snd-soc-dummy platform selection" should be prohibited on "modern style". This patch tidyup existing glue code, and doesn't allow it on modern style any more.
Because of these background, this patch also need to update simple card which is only user of modern style Platfrom.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/generic/simple-card-utils.c | 4 ++++ sound/soc/soc-core.c | 17 ++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 5c1424f..21cffdb1 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -399,6 +399,10 @@ void asoc_simple_card_canonicalize_platform(struct snd_soc_dai_link *dai_link) /* Assumes platform == cpu */ if (!dai_link->platforms->of_node) dai_link->platforms->of_node = dai_link->cpu_of_node; + + /* select snd_soc_dummy */ + if (!dai_link->platforms->of_node) + dai_link->platforms->name = "snd-soc-dummy"; } EXPORT_SYMBOL_GPL(asoc_simple_card_canonicalize_platform);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index cefd121..00018b0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1062,7 +1062,7 @@ static int snd_soc_init_platform(struct snd_soc_card *card, !dai_link->codec_dai_name && !dai_link->platform_name && !dai_link->platform_of_node) - goto init_end; + return 0;
/* convert Legacy platform link */ if (!platform || dai_link->legacy_platform) { @@ -1078,13 +1078,12 @@ static int snd_soc_init_platform(struct snd_soc_card *card, platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL; - }
-init_end: - /* if there's no platform we match on the empty platform */ - if (!platform->name && - !platform->of_node) - platform->name = "snd-soc-dummy"; + /* if there's no platform we match on the empty platform */ + if (!platform->name && + !platform->of_node) + platform->name = "snd-soc-dummy"; + }
return 0; } @@ -1181,9 +1180,9 @@ static int soc_init_dai_link(struct snd_soc_card *card, return -EINVAL; }
- if (link->platforms->name && link->platforms->of_node) { + if (!!link->platforms->name == !!link->platforms->of_node) { dev_err(card->dev, - "ASoC: Both platform name/of_node are set for %s\n", + "ASoC: Neither/Both platform name/of_node are set for %s\n", link->name); return -EINVAL; }
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart