| Message ID | 87lfyfe4r3.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: modern dai_link style support | expand |
Hmm, guess reviewing 001 proved redundant after all. Unless I got it wrong, you are removing code implemented in that very patch (the 001). Any chance for eliminating ping-pong effect and doing the "right" changes from the get-go? Especially the renames are confusing here (s/cleanup_platform/cleanup_legacy/) if you intend to remove them soon after. If there is no other way around it and solution is accepted, a note, perhaps in 001 would be helpful for future readers. Czarek >From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > >All drivers switched to modern style dai_link >(= struct snd_soc_dai_link_component). >Let's remove legacy style dai_link. > >Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >--- > include/sound/soc.h | 65 ++------------------ > sound/soc/soc-core.c | 165 +++------------------------------------------------ > 2 files changed, 12 insertions(+), 218 deletions(-) > >diff --git a/include/sound/soc.h b/include/sound/soc.h >index 0fa79b8..055e6d0 100644 >--- a/include/sound/soc.h >+++ b/include/sound/soc.h >@@ -901,77 +901,33 @@ struct snd_soc_dai_link { > const char *stream_name; /* Stream name */ > > /* >- * cpu_name >- * cpu_of_node >- * cpu_dai_name >- * >- * These are legacy style, and will be replaced to >- * modern style (= snd_soc_dai_link_component) in the future, >- * but, not yet supported so far. >- * If modern style was supported for CPU, all driver will switch >- * to use it, and, legacy style code will be removed from ALSA SoC. >- */ >- /* > * You MAY specify the link's CPU-side device, either by device name, > * or by DT/OF node, but not both. If this information is omitted, > * the CPU-side DAI is matched using .cpu_dai_name only, which >hence > * must be globally unique. These fields are currently typically used > * only for codec to codec links, or systems using device tree. > */ >- const char *cpu_name; >- struct device_node *cpu_of_node; > /* > * You MAY specify the DAI name of the CPU DAI. If this information is > * omitted, the CPU-side DAI is matched using >.cpu_name/.cpu_of_node > * only, which only works well when that device exposes a single DAI. > */ >- const char *cpu_dai_name; >- > struct snd_soc_dai_link_component *cpus; > unsigned int num_cpus; > > /* >- * codec_name >- * codec_of_node >- * codec_dai_name >- * >- * These are legacy style, it will be converted to modern style >- * (= snd_soc_dai_link_component) automatically in soc-core >- * if driver is using legacy style. >- * Driver shouldn't use both legacy and modern style in the same time. >- * If modern style was supported for CPU, all driver will switch >- * to use it, and, legacy style code will be removed from ALSA SoC. >- */ >- /* > * You MUST specify the link's codec, either by device name, or by > * DT/OF node, but not both. > */ >- const char *codec_name; >- 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_component *codecs; > unsigned int num_codecs; > > /* >- * platform_name >- * platform_of_node >- * >- * These are legacy style, it will be converted to modern style >- * (= snd_soc_dai_link_component) automatically in soc-core >- * if driver is using legacy style. >- * Driver shouldn't use both legacy and modern style in the same time. >- * If modern style was supported for CPU, all driver will switch >- * to use it, and, legacy style code will be removed from ALSA SoC. >- */ >- /* > * 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. > */ >- const char *platform_name; >- struct device_node *platform_of_node; > struct snd_soc_dai_link_component *platforms; > unsigned int num_platforms; > >@@ -1033,13 +989,6 @@ struct snd_soc_dai_link { > /* Do not create a PCM for this DAI link (Backend link) */ > unsigned int ignore:1; > >- /* >- * This driver uses legacy platform naming. Set by the core, machine >- * drivers should not modify this value. >- */ >- unsigned int legacy_platform:1; >- unsigned int legacy_cpu:1; >- > struct list_head list; /* DAI link list of the soc card */ > struct snd_soc_dobj dobj; /* For topology */ > }; >@@ -1699,15 +1648,11 @@ int >snd_soc_fixup_dai_links_platform_name(struct snd_soc_card *card, > if (!name) > return -ENOMEM; > >- if (dai_link->platforms) >- /* only single platform is supported for now */ >- dai_link->platforms->name = name; >- else >- /* >- * legacy mode, this case will be removed when all >- * derivers are switched to modern style dai_link. >- */ >- dai_link->platform_name = name; >+ if (!dai_link->platforms) >+ return -EINVAL; >+ >+ /* only single platform is supported for now */ >+ dai_link->platforms->name = name; > } > > return 0; >diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >index e069dfb..b28dda9 100644 >--- a/sound/soc/soc-core.c >+++ b/sound/soc/soc-core.c >@@ -1052,167 +1052,18 @@ static void soc_remove_dai_links(struct >snd_soc_card *card) > } > } > >-static int snd_soc_init_cpu(struct snd_soc_card *card, >- struct snd_soc_dai_link *dai_link) >-{ >- struct snd_soc_dai_link_component *cpu = dai_link->cpus; >- >- /* >- * REMOVE ME >- * >- * This is glue code for Legacy vs Modern dai_link. >- * This function will be removed if all derivers are switched to >- * modern style dai_link. >- * Driver shouldn't use both legacy and modern style in the same time. >- * see >- * soc.h :: struct snd_soc_dai_link >- */ >- /* convert Legacy platform link */ >- if (!cpu) { >- cpu = devm_kzalloc(card->dev, >- sizeof(struct snd_soc_dai_link_component), >- GFP_KERNEL); >- if (!cpu) >- return -ENOMEM; >- >- dai_link->cpus = cpu; >- dai_link->num_cpus = 1; >- dai_link->legacy_cpu = 1; >- >- cpu->name = dai_link->cpu_name; >- cpu->of_node = dai_link->cpu_of_node; >- cpu->dai_name = dai_link->cpu_dai_name; >- } >- >- if (!dai_link->cpus) { >- dev_err(card->dev, "ASoC: DAI link has no CPUs\n"); >- return -EINVAL; >- } >- >- return 0; >-} >- >-static int snd_soc_init_platform(struct snd_soc_card *card, >- struct snd_soc_dai_link *dai_link) >-{ >- struct snd_soc_dai_link_component *platform = dai_link->platforms; >- >- /* >- * REMOVE ME >- * >- * This is glue code for Legacy vs Modern dai_link. >- * This function will be removed if all derivers are switched to >- * modern style dai_link. >- * Driver shouldn't use both legacy and modern style in the same time. >- * see >- * soc.h :: struct snd_soc_dai_link >- */ >- /* convert Legacy platform link */ >- if (!platform) { >- platform = devm_kzalloc(card->dev, >- sizeof(struct snd_soc_dai_link_component), >- GFP_KERNEL); >- if (!platform) >- return -ENOMEM; >- >- dai_link->platforms = platform; >- dai_link->num_platforms = 1; >- dai_link->legacy_platform = 1; >- platform->name = dai_link->platform_name; >- platform->of_node = dai_link->platform_of_node; >- platform->dai_name = NULL; >- } >- >- /* if there's no platform we match on the empty platform */ >- if (!platform->name && >- !platform->of_node) >- platform->name = "snd-soc-dummy"; >- >- return 0; >-} >- >-static void soc_cleanup_legacy(struct snd_soc_card *card) >-{ >- struct snd_soc_dai_link *link; >- int i; >- /* >- * FIXME >- * >- * this function should be removed with snd_soc_init_platform >- */ >- >- for_each_card_prelinks(card, i, link) { >- if (link->legacy_platform) { >- link->legacy_platform = 0; >- link->platforms = NULL; >- } >- if (link->legacy_cpu) { >- link->legacy_cpu = 0; >- link->cpus = NULL; >- } >- } >-} >- >-static int snd_soc_init_multicodec(struct snd_soc_card *card, >- struct snd_soc_dai_link *dai_link) >-{ >- /* >- * REMOVE ME >- * >- * This is glue code for Legacy vs Modern dai_link. >- * This function will be removed if all derivers are switched to >- * modern style dai_link. >- * Driver shouldn't use both legacy and modern style in the same time. >- * see >- * soc.h :: struct snd_soc_dai_link >- */ >- >- /* Legacy codec/codec_dai link is a single entry in multicodec */ >- if (dai_link->codec_name || dai_link->codec_of_node || >- dai_link->codec_dai_name) { >- dai_link->num_codecs = 1; >- >- dai_link->codecs = devm_kzalloc(card->dev, >- sizeof(struct snd_soc_dai_link_component), >- GFP_KERNEL); >- if (!dai_link->codecs) >- return -ENOMEM; >- >- dai_link->codecs[0].name = dai_link->codec_name; >- dai_link->codecs[0].of_node = dai_link->codec_of_node; >- dai_link->codecs[0].dai_name = dai_link->codec_dai_name; >- } >- >- if (!dai_link->codecs) { >- dev_err(card->dev, "ASoC: DAI link has no CODECs\n"); >- return -EINVAL; >- } >- >- return 0; >-} >+static struct snd_soc_dai_link_component dummy_link = COMP_DUMMY(); > > static int soc_init_dai_link(struct snd_soc_card *card, > struct snd_soc_dai_link *link) > { >- int i, ret; >+ int i; > struct snd_soc_dai_link_component *codec; > >- ret = snd_soc_init_cpu(card, link); >- if (ret) { >- dev_err(card->dev, "ASoC: failed to init cpu\n"); >- return ret; >- } >- >- ret = snd_soc_init_platform(card, link); >- if (ret) { >- dev_err(card->dev, "ASoC: failed to init multiplatform\n"); >- return ret; >- } >- >- ret = snd_soc_init_multicodec(card, link); >- if (ret) { >- dev_err(card->dev, "ASoC: failed to init multicodec\n"); >- return ret; >+ /* default Platform */ >+ if (!link->platforms || !link->num_platforms) { >+ link->platforms = &dummy_link; >+ link->num_platforms = 1; > } > > for_each_link_codecs(link, i, codec) { >@@ -2059,7 +1910,7 @@ static void soc_check_tplg_fes(struct snd_soc_card >*card) > card->dai_link[i].name); > > /* override platform component */ >- if (snd_soc_init_platform(card, dai_link) < 0) { >+ if (!dai_link->platforms) { > dev_err(card->dev, "init platform error"); > continue; > } >@@ -2110,7 +1961,6 @@ static int soc_cleanup_card_resources(struct >snd_soc_card *card) > /* remove and free each DAI */ > soc_remove_dai_links(card); > soc_remove_pcm_runtimes(card); >- soc_cleanup_legacy(card); > > /* remove auxiliary devices */ > soc_remove_aux_devices(card); >@@ -2867,7 +2717,6 @@ int snd_soc_register_card(struct snd_soc_card >*card) > > ret = soc_init_dai_link(card, link); > if (ret) { >- soc_cleanup_legacy(card); > dev_err(card->dev, "ASoC: failed to init link %s\n", > link->name); > mutex_unlock(&client_mutex); >-- >2.7.4
Please don't top-post. On 6/6/19 1:25 PM, Rojewski, Cezary wrote: > Hmm, guess reviewing 001 proved redundant after all. Unless I got it wrong, you are removing code implemented in that very patch (the 001). Not quite. There was already code to convert codecs and platforms to the new representation but the cpu part was missing. The first patch only deals with cpu dais. The last patch removes all the conversions for codec/platform/cpu and uses the new representation across the board, so there's more code removed in the last patch than added in the first. > Any chance for eliminating ping-pong effect and doing the "right" changes from the get-go? Especially the renames are confusing here (s/cleanup_platform/cleanup_legacy/) if you intend to remove them soon after. Using a ping-pong analogy for a 146-patch series is pushing it. It's first make then break to avoid bisect issues. And the names match what is used in the existing code. maybe the naming isn't to your liking but it's what has been used for a while. Note that the last patch is going to break all the non-upstream machine drivers so you will have quite a bit of work to do on your own when you rebase. > > If there is no other way around it and solution is accepted, a note, perhaps in 001 would be helpful for future readers. > > Czarek > >> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> >> All drivers switched to modern style dai_link >> (= struct snd_soc_dai_link_component). >> Let's remove legacy style dai_link. >> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> --- >> include/sound/soc.h | 65 ++------------------ >> sound/soc/soc-core.c | 165 +++------------------------------------------------ >> 2 files changed, 12 insertions(+), 218 deletions(-) >> >> diff --git a/include/sound/soc.h b/include/sound/soc.h >> index 0fa79b8..055e6d0 100644 >> --- a/include/sound/soc.h >> +++ b/include/sound/soc.h >> @@ -901,77 +901,33 @@ struct snd_soc_dai_link { >> const char *stream_name; /* Stream name */ >> >> /* >> - * cpu_name >> - * cpu_of_node >> - * cpu_dai_name >> - * >> - * These are legacy style, and will be replaced to >> - * modern style (= snd_soc_dai_link_component) in the future, >> - * but, not yet supported so far. >> - * If modern style was supported for CPU, all driver will switch >> - * to use it, and, legacy style code will be removed from ALSA SoC. >> - */ >> - /* >> * You MAY specify the link's CPU-side device, either by device name, >> * or by DT/OF node, but not both. If this information is omitted, >> * the CPU-side DAI is matched using .cpu_dai_name only, which >> hence >> * must be globally unique. These fields are currently typically used >> * only for codec to codec links, or systems using device tree. >> */ >> - const char *cpu_name; >> - struct device_node *cpu_of_node; >> /* >> * You MAY specify the DAI name of the CPU DAI. If this information is >> * omitted, the CPU-side DAI is matched using >> .cpu_name/.cpu_of_node >> * only, which only works well when that device exposes a single DAI. >> */ >> - const char *cpu_dai_name; >> - >> struct snd_soc_dai_link_component *cpus; >> unsigned int num_cpus; >> >> /* >> - * codec_name >> - * codec_of_node >> - * codec_dai_name >> - * >> - * These are legacy style, it will be converted to modern style >> - * (= snd_soc_dai_link_component) automatically in soc-core >> - * if driver is using legacy style. >> - * Driver shouldn't use both legacy and modern style in the same time. >> - * If modern style was supported for CPU, all driver will switch >> - * to use it, and, legacy style code will be removed from ALSA SoC. >> - */ >> - /* >> * You MUST specify the link's codec, either by device name, or by >> * DT/OF node, but not both. >> */ >> - const char *codec_name; >> - 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_component *codecs; >> unsigned int num_codecs; >> >> /* >> - * platform_name >> - * platform_of_node >> - * >> - * These are legacy style, it will be converted to modern style >> - * (= snd_soc_dai_link_component) automatically in soc-core >> - * if driver is using legacy style. >> - * Driver shouldn't use both legacy and modern style in the same time. >> - * If modern style was supported for CPU, all driver will switch >> - * to use it, and, legacy style code will be removed from ALSA SoC. >> - */ >> - /* >> * 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. >> */ >> - const char *platform_name; >> - struct device_node *platform_of_node; >> struct snd_soc_dai_link_component *platforms; >> unsigned int num_platforms; >> >> @@ -1033,13 +989,6 @@ struct snd_soc_dai_link { >> /* Do not create a PCM for this DAI link (Backend link) */ >> unsigned int ignore:1; >> >> - /* >> - * This driver uses legacy platform naming. Set by the core, machine >> - * drivers should not modify this value. >> - */ >> - unsigned int legacy_platform:1; >> - unsigned int legacy_cpu:1; >> - >> struct list_head list; /* DAI link list of the soc card */ >> struct snd_soc_dobj dobj; /* For topology */ >> }; >> @@ -1699,15 +1648,11 @@ int >> snd_soc_fixup_dai_links_platform_name(struct snd_soc_card *card, >> if (!name) >> return -ENOMEM; >> >> - if (dai_link->platforms) >> - /* only single platform is supported for now */ >> - dai_link->platforms->name = name; >> - else >> - /* >> - * legacy mode, this case will be removed when all >> - * derivers are switched to modern style dai_link. >> - */ >> - dai_link->platform_name = name; >> + if (!dai_link->platforms) >> + return -EINVAL; >> + >> + /* only single platform is supported for now */ >> + dai_link->platforms->name = name; >> } >> >> return 0; >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >> index e069dfb..b28dda9 100644 >> --- a/sound/soc/soc-core.c >> +++ b/sound/soc/soc-core.c >> @@ -1052,167 +1052,18 @@ static void soc_remove_dai_links(struct >> snd_soc_card *card) >> } >> } >> >> -static int snd_soc_init_cpu(struct snd_soc_card *card, >> - struct snd_soc_dai_link *dai_link) >> -{ >> - struct snd_soc_dai_link_component *cpu = dai_link->cpus; >> - >> - /* >> - * REMOVE ME >> - * >> - * This is glue code for Legacy vs Modern dai_link. >> - * This function will be removed if all derivers are switched to >> - * modern style dai_link. >> - * Driver shouldn't use both legacy and modern style in the same time. >> - * see >> - * soc.h :: struct snd_soc_dai_link >> - */ >> - /* convert Legacy platform link */ >> - if (!cpu) { >> - cpu = devm_kzalloc(card->dev, >> - sizeof(struct snd_soc_dai_link_component), >> - GFP_KERNEL); >> - if (!cpu) >> - return -ENOMEM; >> - >> - dai_link->cpus = cpu; >> - dai_link->num_cpus = 1; >> - dai_link->legacy_cpu = 1; >> - >> - cpu->name = dai_link->cpu_name; >> - cpu->of_node = dai_link->cpu_of_node; >> - cpu->dai_name = dai_link->cpu_dai_name; >> - } >> - >> - if (!dai_link->cpus) { >> - dev_err(card->dev, "ASoC: DAI link has no CPUs\n"); >> - return -EINVAL; >> - } >> - >> - return 0; >> -} >> - >> -static int snd_soc_init_platform(struct snd_soc_card *card, >> - struct snd_soc_dai_link *dai_link) >> -{ >> - struct snd_soc_dai_link_component *platform = dai_link->platforms; >> - >> - /* >> - * REMOVE ME >> - * >> - * This is glue code for Legacy vs Modern dai_link. >> - * This function will be removed if all derivers are switched to >> - * modern style dai_link. >> - * Driver shouldn't use both legacy and modern style in the same time. >> - * see >> - * soc.h :: struct snd_soc_dai_link >> - */ >> - /* convert Legacy platform link */ >> - if (!platform) { >> - platform = devm_kzalloc(card->dev, >> - sizeof(struct snd_soc_dai_link_component), >> - GFP_KERNEL); >> - if (!platform) >> - return -ENOMEM; >> - >> - dai_link->platforms = platform; >> - dai_link->num_platforms = 1; >> - dai_link->legacy_platform = 1; >> - platform->name = dai_link->platform_name; >> - platform->of_node = dai_link->platform_of_node; >> - platform->dai_name = NULL; >> - } >> - >> - /* if there's no platform we match on the empty platform */ >> - if (!platform->name && >> - !platform->of_node) >> - platform->name = "snd-soc-dummy"; >> - >> - return 0; >> -} >> - >> -static void soc_cleanup_legacy(struct snd_soc_card *card) >> -{ >> - struct snd_soc_dai_link *link; >> - int i; >> - /* >> - * FIXME >> - * >> - * this function should be removed with snd_soc_init_platform >> - */ >> - >> - for_each_card_prelinks(card, i, link) { >> - if (link->legacy_platform) { >> - link->legacy_platform = 0; >> - link->platforms = NULL; >> - } >> - if (link->legacy_cpu) { >> - link->legacy_cpu = 0; >> - link->cpus = NULL; >> - } >> - } >> -} >> - >> -static int snd_soc_init_multicodec(struct snd_soc_card *card, >> - struct snd_soc_dai_link *dai_link) >> -{ >> - /* >> - * REMOVE ME >> - * >> - * This is glue code for Legacy vs Modern dai_link. >> - * This function will be removed if all derivers are switched to >> - * modern style dai_link. >> - * Driver shouldn't use both legacy and modern style in the same time. >> - * see >> - * soc.h :: struct snd_soc_dai_link >> - */ >> - >> - /* Legacy codec/codec_dai link is a single entry in multicodec */ >> - if (dai_link->codec_name || dai_link->codec_of_node || >> - dai_link->codec_dai_name) { >> - dai_link->num_codecs = 1; >> - >> - dai_link->codecs = devm_kzalloc(card->dev, >> - sizeof(struct snd_soc_dai_link_component), >> - GFP_KERNEL); >> - if (!dai_link->codecs) >> - return -ENOMEM; >> - >> - dai_link->codecs[0].name = dai_link->codec_name; >> - dai_link->codecs[0].of_node = dai_link->codec_of_node; >> - dai_link->codecs[0].dai_name = dai_link->codec_dai_name; >> - } >> - >> - if (!dai_link->codecs) { >> - dev_err(card->dev, "ASoC: DAI link has no CODECs\n"); >> - return -EINVAL; >> - } >> - >> - return 0; >> -} >> +static struct snd_soc_dai_link_component dummy_link = COMP_DUMMY(); >> >> static int soc_init_dai_link(struct snd_soc_card *card, >> struct snd_soc_dai_link *link) >> { >> - int i, ret; >> + int i; >> struct snd_soc_dai_link_component *codec; >> >> - ret = snd_soc_init_cpu(card, link); >> - if (ret) { >> - dev_err(card->dev, "ASoC: failed to init cpu\n"); >> - return ret; >> - } >> - >> - ret = snd_soc_init_platform(card, link); >> - if (ret) { >> - dev_err(card->dev, "ASoC: failed to init multiplatform\n"); >> - return ret; >> - } >> - >> - ret = snd_soc_init_multicodec(card, link); >> - if (ret) { >> - dev_err(card->dev, "ASoC: failed to init multicodec\n"); >> - return ret; >> + /* default Platform */ >> + if (!link->platforms || !link->num_platforms) { >> + link->platforms = &dummy_link; >> + link->num_platforms = 1; >> } >> >> for_each_link_codecs(link, i, codec) { >> @@ -2059,7 +1910,7 @@ static void soc_check_tplg_fes(struct snd_soc_card >> *card) >> card->dai_link[i].name); >> >> /* override platform component */ >> - if (snd_soc_init_platform(card, dai_link) < 0) { >> + if (!dai_link->platforms) { >> dev_err(card->dev, "init platform error"); >> continue; >> } >> @@ -2110,7 +1961,6 @@ static int soc_cleanup_card_resources(struct >> snd_soc_card *card) >> /* remove and free each DAI */ >> soc_remove_dai_links(card); >> soc_remove_pcm_runtimes(card); >> - soc_cleanup_legacy(card); >> >> /* remove auxiliary devices */ >> soc_remove_aux_devices(card); >> @@ -2867,7 +2717,6 @@ int snd_soc_register_card(struct snd_soc_card >> *card) >> >> ret = soc_init_dai_link(card, link); >> if (ret) { >> - soc_cleanup_legacy(card); >> dev_err(card->dev, "ASoC: failed to init link %s\n", >> link->name); >> mutex_unlock(&client_mutex); >> -- >> 2.7.4 > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi Rojewski Cc Pierre-Louis Thank you for your review, explanation, feedback. > > Hmm, guess reviewing 001 proved redundant after all. Unless I got it wrong, you are removing code implemented in that very patch (the 001). > > Not quite. There was already code to convert codecs and platforms to > the new representation but the cpu part was missing. The first patch > only deals with cpu dais. The last patch removes all the conversions > for codec/platform/cpu and uses the new representation across the > board, so there's more code removed in the last patch than added in > the first. > > > Any chance for eliminating ping-pong effect and doing the "right" changes from the get-go? Especially the renames are confusing here (s/cleanup_platform/cleanup_legacy/) if you intend to remove them soon after. > > Using a ping-pong analogy for a 146-patch series is pushing it. It's > first make then break to avoid bisect issues. And the names match what > is used in the existing code. maybe the naming isn't to your liking > but it's what has been used for a while. > > Note that the last patch is going to break all the non-upstream > machine drivers so you will have quite a bit of work to do on your own > when you rebase. This patch-set moves from "legacy style" to "modern style", [001] added glue code, and [146] removed "legacy style". I believe this is needed to support more complex multiple connection device, like multi-CPU / multi-Platform, etc, etc... As Pierre-Louis said, unfortunately last patch removes "legacy style" from ALSA SoC. I'm sorry to bother you, but, I / upstream can't care about out-of-tree code, unfortunately... Thank you for your help !! Best regards --- Kuninori Morimoto
On Thu, Jun 06, 2019 at 02:08:50PM -0500, Pierre-Louis Bossart wrote: > Please don't top-post. > > On 6/6/19 1:25 PM, Rojewski, Cezary wrote: > > Hmm, guess reviewing 001 proved redundant after all. Unless I got it wrong, you are removing code implemented in that very patch (the 001). > > Any chance for eliminating ping-pong effect and doing the "right" changes from the get-go? Especially the renames are confusing here (s/cleanup_platform/cleanup_legacy/) if you intend to remove them soon after. Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > Using a ping-pong analogy for a 146-patch series is pushing it. It's first > make then break to avoid bisect issues. And the names match what is used in > the existing code. maybe the naming isn't to your liking but it's what has > been used for a while. > Note that the last patch is going to break all the non-upstream machine > drivers so you will have quite a bit of work to do on your own when you > rebase. Right, avoiding build breaks is important here - it helps future bisectability if we don't have commits (and especially long serieses of commits) that just randomly fail to build. That's *way* more useful than dropping the initial patch would've been. > > If there is no other way around it and solution is accepted, a note, perhaps in 001 would be helpful for future readers. There's no guarantee that a patch series will be applied at once, if there's a problem part way through the series the earlier bits might get applied to save future review.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 0fa79b8..055e6d0 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -901,77 +901,33 @@ struct snd_soc_dai_link { const char *stream_name; /* Stream name */ /* - * cpu_name - * cpu_of_node - * cpu_dai_name - * - * These are legacy style, and will be replaced to - * modern style (= snd_soc_dai_link_component) in the future, - * but, not yet supported so far. - * If modern style was supported for CPU, all driver will switch - * to use it, and, legacy style code will be removed from ALSA SoC. - */ - /* * You MAY specify the link's CPU-side device, either by device name, * or by DT/OF node, but not both. If this information is omitted, * the CPU-side DAI is matched using .cpu_dai_name only, which hence * must be globally unique. These fields are currently typically used * only for codec to codec links, or systems using device tree. */ - const char *cpu_name; - struct device_node *cpu_of_node; /* * You MAY specify the DAI name of the CPU DAI. If this information is * omitted, the CPU-side DAI is matched using .cpu_name/.cpu_of_node * only, which only works well when that device exposes a single DAI. */ - const char *cpu_dai_name; - struct snd_soc_dai_link_component *cpus; unsigned int num_cpus; /* - * codec_name - * codec_of_node - * codec_dai_name - * - * These are legacy style, it will be converted to modern style - * (= snd_soc_dai_link_component) automatically in soc-core - * if driver is using legacy style. - * Driver shouldn't use both legacy and modern style in the same time. - * If modern style was supported for CPU, all driver will switch - * to use it, and, legacy style code will be removed from ALSA SoC. - */ - /* * You MUST specify the link's codec, either by device name, or by * DT/OF node, but not both. */ - const char *codec_name; - 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_component *codecs; unsigned int num_codecs; /* - * platform_name - * platform_of_node - * - * These are legacy style, it will be converted to modern style - * (= snd_soc_dai_link_component) automatically in soc-core - * if driver is using legacy style. - * Driver shouldn't use both legacy and modern style in the same time. - * If modern style was supported for CPU, all driver will switch - * to use it, and, legacy style code will be removed from ALSA SoC. - */ - /* * 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. */ - const char *platform_name; - struct device_node *platform_of_node; struct snd_soc_dai_link_component *platforms; unsigned int num_platforms; @@ -1033,13 +989,6 @@ struct snd_soc_dai_link { /* Do not create a PCM for this DAI link (Backend link) */ unsigned int ignore:1; - /* - * This driver uses legacy platform naming. Set by the core, machine - * drivers should not modify this value. - */ - unsigned int legacy_platform:1; - unsigned int legacy_cpu:1; - struct list_head list; /* DAI link list of the soc card */ struct snd_soc_dobj dobj; /* For topology */ }; @@ -1699,15 +1648,11 @@ int snd_soc_fixup_dai_links_platform_name(struct snd_soc_card *card, if (!name) return -ENOMEM; - if (dai_link->platforms) - /* only single platform is supported for now */ - dai_link->platforms->name = name; - else - /* - * legacy mode, this case will be removed when all - * derivers are switched to modern style dai_link. - */ - dai_link->platform_name = name; + if (!dai_link->platforms) + return -EINVAL; + + /* only single platform is supported for now */ + dai_link->platforms->name = name; } return 0; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e069dfb..b28dda9 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1052,167 +1052,18 @@ static void soc_remove_dai_links(struct snd_soc_card *card) } } -static int snd_soc_init_cpu(struct snd_soc_card *card, - struct snd_soc_dai_link *dai_link) -{ - struct snd_soc_dai_link_component *cpu = dai_link->cpus; - - /* - * REMOVE ME - * - * This is glue code for Legacy vs Modern dai_link. - * This function will be removed if all derivers are switched to - * modern style dai_link. - * Driver shouldn't use both legacy and modern style in the same time. - * see - * soc.h :: struct snd_soc_dai_link - */ - /* convert Legacy platform link */ - if (!cpu) { - cpu = devm_kzalloc(card->dev, - sizeof(struct snd_soc_dai_link_component), - GFP_KERNEL); - if (!cpu) - return -ENOMEM; - - dai_link->cpus = cpu; - dai_link->num_cpus = 1; - dai_link->legacy_cpu = 1; - - cpu->name = dai_link->cpu_name; - cpu->of_node = dai_link->cpu_of_node; - cpu->dai_name = dai_link->cpu_dai_name; - } - - if (!dai_link->cpus) { - dev_err(card->dev, "ASoC: DAI link has no CPUs\n"); - return -EINVAL; - } - - return 0; -} - -static int snd_soc_init_platform(struct snd_soc_card *card, - struct snd_soc_dai_link *dai_link) -{ - struct snd_soc_dai_link_component *platform = dai_link->platforms; - - /* - * REMOVE ME - * - * This is glue code for Legacy vs Modern dai_link. - * This function will be removed if all derivers are switched to - * modern style dai_link. - * Driver shouldn't use both legacy and modern style in the same time. - * see - * soc.h :: struct snd_soc_dai_link - */ - /* convert Legacy platform link */ - if (!platform) { - platform = devm_kzalloc(card->dev, - sizeof(struct snd_soc_dai_link_component), - GFP_KERNEL); - if (!platform) - return -ENOMEM; - - dai_link->platforms = platform; - dai_link->num_platforms = 1; - dai_link->legacy_platform = 1; - platform->name = dai_link->platform_name; - platform->of_node = dai_link->platform_of_node; - platform->dai_name = NULL; - } - - /* if there's no platform we match on the empty platform */ - if (!platform->name && - !platform->of_node) - platform->name = "snd-soc-dummy"; - - return 0; -} - -static void soc_cleanup_legacy(struct snd_soc_card *card) -{ - struct snd_soc_dai_link *link; - int i; - /* - * FIXME - * - * this function should be removed with snd_soc_init_platform - */ - - for_each_card_prelinks(card, i, link) { - if (link->legacy_platform) { - link->legacy_platform = 0; - link->platforms = NULL; - } - if (link->legacy_cpu) { - link->legacy_cpu = 0; - link->cpus = NULL; - } - } -} - -static int snd_soc_init_multicodec(struct snd_soc_card *card, - struct snd_soc_dai_link *dai_link) -{ - /* - * REMOVE ME - * - * This is glue code for Legacy vs Modern dai_link. - * This function will be removed if all derivers are switched to - * modern style dai_link. - * Driver shouldn't use both legacy and modern style in the same time. - * see - * soc.h :: struct snd_soc_dai_link - */ - - /* Legacy codec/codec_dai link is a single entry in multicodec */ - if (dai_link->codec_name || dai_link->codec_of_node || - dai_link->codec_dai_name) { - dai_link->num_codecs = 1; - - dai_link->codecs = devm_kzalloc(card->dev, - sizeof(struct snd_soc_dai_link_component), - GFP_KERNEL); - if (!dai_link->codecs) - return -ENOMEM; - - dai_link->codecs[0].name = dai_link->codec_name; - dai_link->codecs[0].of_node = dai_link->codec_of_node; - dai_link->codecs[0].dai_name = dai_link->codec_dai_name; - } - - if (!dai_link->codecs) { - dev_err(card->dev, "ASoC: DAI link has no CODECs\n"); - return -EINVAL; - } - - return 0; -} +static struct snd_soc_dai_link_component dummy_link = COMP_DUMMY(); static int soc_init_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *link) { - int i, ret; + int i; struct snd_soc_dai_link_component *codec; - ret = snd_soc_init_cpu(card, link); - if (ret) { - dev_err(card->dev, "ASoC: failed to init cpu\n"); - return ret; - } - - ret = snd_soc_init_platform(card, link); - if (ret) { - dev_err(card->dev, "ASoC: failed to init multiplatform\n"); - return ret; - } - - ret = snd_soc_init_multicodec(card, link); - if (ret) { - dev_err(card->dev, "ASoC: failed to init multicodec\n"); - return ret; + /* default Platform */ + if (!link->platforms || !link->num_platforms) { + link->platforms = &dummy_link; + link->num_platforms = 1; } for_each_link_codecs(link, i, codec) { @@ -2059,7 +1910,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) card->dai_link[i].name); /* override platform component */ - if (snd_soc_init_platform(card, dai_link) < 0) { + if (!dai_link->platforms) { dev_err(card->dev, "init platform error"); continue; } @@ -2110,7 +1961,6 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card) /* remove and free each DAI */ soc_remove_dai_links(card); soc_remove_pcm_runtimes(card); - soc_cleanup_legacy(card); /* remove auxiliary devices */ soc_remove_aux_devices(card); @@ -2867,7 +2717,6 @@ int snd_soc_register_card(struct snd_soc_card *card) ret = soc_init_dai_link(card, link); if (ret) { - soc_cleanup_legacy(card); dev_err(card->dev, "ASoC: failed to init link %s\n", link->name); mutex_unlock(&client_mutex);