| Message ID | 87zhp144vs.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: modern style CPU | expand |
On Mon, Apr 08, 2019 at 11:31:01AM +0900, Kuninori Morimoto wrote: > > Hi Mark > > These are RFC of modern style CPU support. > > modern style dai_link needs CPU/Codec/Platform array and its size, > but it will be very ugly code. To avoid it, 3) adds macro. > > 4) - 7) are sample code for switching to modern style dai_link. > 4), 5) are good sample for single CPU/Codec/Platform. > 6), 7) are good sample for multi Codec. > Almost all sound cards will get similar patch. > I think these can work, but I can't test these. > So, it is very useful if someone can test these. > If these are all OK (= test/review), I can post patch-set > for all sound cards. This looks basically fine to me, hopefully someone will be able to help out with the testing - thanks for your persistence with this work!
Hi Mark > > These are RFC of modern style CPU support. > > > > modern style dai_link needs CPU/Codec/Platform array and its size, > > but it will be very ugly code. To avoid it, 3) adds macro. > > > > 4) - 7) are sample code for switching to modern style dai_link. > > 4), 5) are good sample for single CPU/Codec/Platform. > > 6), 7) are good sample for multi Codec. > > > Almost all sound cards will get similar patch. > > I think these can work, but I can't test these. > > So, it is very useful if someone can test these. > > If these are all OK (= test/review), I can post patch-set > > for all sound cards. > > This looks basically fine to me, hopefully someone will be able to help > out with the testing - thanks for your persistence with this work! Thanks. I will post these patches at this or next week Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis I have 1 question about modern style > > > These are RFC of modern style CPU support. > > > > > > modern style dai_link needs CPU/Codec/Platform array and its size, > > > but it will be very ugly code. To avoid it, 3) adds macro. > > > > > > 4) - 7) are sample code for switching to modern style dai_link. > > > 4), 5) are good sample for single CPU/Codec/Platform. > > > 6), 7) are good sample for multi Codec. > > > > > Almost all sound cards will get similar patch. > > > I think these can work, but I can't test these. > > > So, it is very useful if someone can test these. > > > If these are all OK (= test/review), I can post patch-set > > > for all sound cards. > > > > This looks basically fine to me, hopefully someone will be able to help > > out with the testing - thanks for your persistence with this work! I know you are already working for it for Intel https://github.com/plbossart/sound/commits/fix/codec-legacy-dai and, it will be conflict to my posted patch style. https://patchwork.kernel.org/patch/10888741/ https://patchwork.kernel.org/patch/10888745/ So, I can avoid to posting patch for Intel, then, you need to update it again for modern style CPU. Or, I can update Intel by using above macro. What is your opinion ? Thank you for your help !! Best regards --- Kuninori Morimoto
> I know you are already working for it for Intel > > https://github.com/plbossart/sound/commits/fix/codec-legacy-dai > > and, it will be conflict to my posted patch style. > > https://patchwork.kernel.org/patch/10888741/ > https://patchwork.kernel.org/patch/10888745/ > > So, I can avoid to posting patch for Intel, > then, you need to update it again for modern style CPU. > Or, I can update Intel by using above macro. > What is your opinion ? I must admit I didn't look at the new style. With all due respect I am not sold, it's becoming a lot less intuitive and readable, e.g.: +SND_SOC_DAI_LINK_CCP(aif1, + SND_SOC_DAI_LINK(SDL_CPU("samsung-i2s.0")), + SND_SOC_DAI_LINK(SDL_CODEC("wm8994-codec", "wm8994-aif1")), + SND_SOC_DAI_LINK(SDL_PLATFORM("samsung-i2s.0"))); + +SND_SOC_DAI_LINK_CCP(fifo_tx, + SND_SOC_DAI_LINK(SDL_CPU("samsung-i2s-sec")), + SND_SOC_DAI_LINK(SDL_CODEC("wm8994-codec", "wm8994-aif1")), + SND_SOC_DAI_LINK(SDL_PLATFORM("samsung-i2s-sec"))); + static struct snd_soc_dai_link smdk_dai[] = { { /* Primary DAI i/f */ .name = "WM8994 AIF1", .stream_name = "Pri_Dai", - .cpu_dai_name = "samsung-i2s.0", - .codec_dai_name = "wm8994-aif1", - .platform_name = "samsung-i2s.0", - .codec_name = "wm8994-codec", .init = smdk_wm8994_init_paiftx, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM, .ops = &smdk_ops, + SND_SOC_LINK_CCP(aif1), is this really the new direction? Even the acronyms are not simple, it took me 15mn to figure out that CCP stood for CPU/Codec/Platform and I couldn't figure out what SDL means. The multiple repetitions of SND_SOC_DAI_LINK is also misleading, it's just a property of the *same* dailink that you handle. I am even more nervous since we have a need to explicitly some cpu and codec dai names depending on quirks, with the additional abstraction it'll become plain unreadable - or we need new helpers.
Hi xxx Hi Pierre-Louis Thank you for your feedback > +SND_SOC_DAI_LINK_CCP(aif1, > + SND_SOC_DAI_LINK(SDL_CPU("samsung-i2s.0")), > + SND_SOC_DAI_LINK(SDL_CODEC("wm8994-codec", "wm8994-aif1")), > + SND_SOC_DAI_LINK(SDL_PLATFORM("samsung-i2s.0"))); (snip) > static struct snd_soc_dai_link smdk_dai[] = { > { /* Primary DAI i/f */ > .name = "WM8994 AIF1", > .stream_name = "Pri_Dai", > - .cpu_dai_name = "samsung-i2s.0", > - .codec_dai_name = "wm8994-aif1", > - .platform_name = "samsung-i2s.0", > - .codec_name = "wm8994-codec", > .init = smdk_wm8994_init_paiftx, > .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > SND_SOC_DAIFMT_CBM_CFM, > .ops = &smdk_ops, > + SND_SOC_LINK_CCP(aif1), > > is this really the new direction? > Even the acronyms are not simple, it took me 15mn to figure out that > CCP stood for CPU/Codec/Platform and I couldn't figure out what SDL > means. The reason of "CCP" (= CPU/CODEC/PLATFORM) was to avoid long naming. and SDL is acronyms of "Snd soc Dai Link". but yes, it is un-understandable. It should be more understandable (and possibly short) naming. > The multiple repetitions of SND_SOC_DAI_LINK is also > misleading, it's just a property of the *same* dailink that you > handle. it is needed to handle both single/multi CPU/Codec/Platform. But yes, it has naming issue. > I am even more nervous since we have a need to explicitly some cpu and > codec dai names depending on quirks, with the additional abstraction > it'll become plain unreadable - or we need new helpers. Hmm, OK. I will reconsider about macro. I'm happy if you can review it again. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis Thank you for your feedback > +SND_SOC_DAI_LINK_CCP(aif1, > + SND_SOC_DAI_LINK(SDL_CPU("samsung-i2s.0")), > + SND_SOC_DAI_LINK(SDL_CODEC("wm8994-codec", "wm8994-aif1")), > + SND_SOC_DAI_LINK(SDL_PLATFORM("samsung-i2s.0"))); (snip) > static struct snd_soc_dai_link smdk_dai[] = { > { /* Primary DAI i/f */ > .name = "WM8994 AIF1", > .stream_name = "Pri_Dai", > - .cpu_dai_name = "samsung-i2s.0", > - .codec_dai_name = "wm8994-aif1", > - .platform_name = "samsung-i2s.0", > - .codec_name = "wm8994-codec", > .init = smdk_wm8994_init_paiftx, > .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > SND_SOC_DAIFMT_CBM_CFM, > .ops = &smdk_ops, > + SND_SOC_LINK_CCP(aif1), > > is this really the new direction? > Even the acronyms are not simple, it took me 15mn to figure out that > CCP stood for CPU/Codec/Platform and I couldn't figure out what SDL > means. The reason of "CCP" (= CPU/CODEC/PLATFORM) was to avoid long naming. and SDL is acronyms of "Snd soc Dai Link". but yes, it is un-understandable. It should be more understandable (and possibly short) naming. > The multiple repetitions of SND_SOC_DAI_LINK is also > misleading, it's just a property of the *same* dailink that you > handle. it is needed to handle both single/multi CPU/Codec/Platform. But yes, it has naming issue. > I am even more nervous since we have a need to explicitly some cpu and > codec dai names depending on quirks, with the additional abstraction > it'll become plain unreadable - or we need new helpers. Hmm, OK. I will reconsider about macro. I'm happy if you can review it again. Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, Apr 17, 2019 at 03:26:48PM +0900, Kuninori Morimoto wrote: > > +SND_SOC_DAI_LINK_CCP(aif1, > > + SND_SOC_DAI_LINK(SDL_CPU("samsung-i2s.0")), > > + SND_SOC_DAI_LINK(SDL_CODEC("wm8994-codec", "wm8994-aif1")), > > + SND_SOC_DAI_LINK(SDL_PLATFORM("samsung-i2s.0"))); > > is this really the new direction? > > Even the acronyms are not simple, it took me 15mn to figure out that > > CCP stood for CPU/Codec/Platform and I couldn't figure out what SDL > > means. > The reason of "CCP" (= CPU/CODEC/PLATFORM) was to avoid long naming. > and SDL is acronyms of "Snd soc Dai Link". but yes, it is un-understandable. > It should be more understandable (and possibly short) naming. I think you're right to want a shorter name but Pierre's probably right that that particular name isn't the best... I'm thinking it might be good to look at the fully expanded definitions and then work back from there, or perhaps just stick with them for a while (since the compact forms are hopefully not a requirement for legibility, even if they are a useful improvement).
Hi Mark > I think you're right to want a shorter name but Pierre's probably right > that that particular name isn't the best... I'm thinking it might be > good to look at the fully expanded definitions and then work back from > there, or perhaps just stick with them for a while (since the compact > forms are hopefully not a requirement for legibility, even if they are a > useful improvement). OK. I'm happy to consider about it. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Mark, Pierre-Louis > > I think you're right to want a shorter name but Pierre's probably right > > that that particular name isn't the best... I'm thinking it might be > > good to look at the fully expanded definitions and then work back from > > there, or perhaps just stick with them for a while (since the compact > > forms are hopefully not a requirement for legibility, even if they are a > > useful improvement). These are proposal of modern style macro v2. Any opinions are welcome. v1 -> v2 - Naming fixup - No difference between with / without Platform (= no more xx_CC and xx_CCP) - It can handle each CPU/Codec/Platform manually (if you want) /* Macro */ #define SND_DAILINK_REG(cpu, codec, platform) \ .cpus = cpu, \ .num_cpus = ARRAY_SIZE(cpu), \ .codecs = codec, \ .num_codecs = ARRAY_SIZE(codec), \ .platforms = platform, \ .num_platforms = ARRAY_SIZE(platform) #define SND_DAILINK_REGS(name) \ .cpus = name##_cpus, \ .num_cpus = ARRAY_SIZE(name##_cpus), \ .codecs = name##_codecs, \ .num_codecs = ARRAY_SIZE(name##_codecs), \ .platforms = name##_platforms, \ .num_platforms = ARRAY_SIZE(name##_platforms) #define SND_DAILINK_DEF(name, def...) \ static struct snd_soc_dai_link_component name[] = def #define SND_DAILINK_DEFS(name, cpu, codec, platform) \ SND_DAILINK_DEF(name##_cpus, cpu); \ SND_DAILINK_DEF(name##_codecs, codec); \ SND_DAILINK_DEF(name##_platforms, platform) #define DAILINK_COMPONENT_ARRAY(param...) { param } #define CPU_COMPONENT(_dai) { .dai_name = _dai, } #define CODEC_COMPONENT(_name, _dai) { .name = _name, .dai_name = _dai, } #define PLATFORM_COMPONENT(_name) { .name = _name } #define NO_DAILINK_COMPONENT() { } struct snd_soc_dai_link_component null_dailink_component[0]; /* Sample1. Single CPU/Codec/Platform */ SND_DAILINK_DEFS(test, DAILINK_COMPONENT_ARRAY(CPU_COMPONENT("cpu_dai")), DAILINK_COMPONENT_ARRAY(CODEC_COMPONENT("codec", "codec_dai")), DAILINK_COMPONENT_ARRAY(PLATFORM_COMPONENT("platform"))); struct snd_soc_dai_link link = { ... SND_DAILINK_REGS(test), }; /* Sample2. Multi CPU/Codec, No Platform */ SND_DAILINK_DEFS(test, DAILINK_COMPONENT_ARRAY(CPU_COMPONENT("cpu_dai1"), CPU_COMPONENT("cpu_dai2")), DAILINK_COMPONENT_ARRAY(CODEC_COMPONENT("codec1", "codec_dai1"), CODEC_COMPONENT("codec2", "codec_dai2")), NO_DAILINK_COMPONENT()); struct snd_soc_dai_link link = { ... SND_DAILINK_REGS(test), }; /* Sample3. Define each CPU/Codec/Platform manually */ SND_DAILINK_DEF(test_cpu, DAILINK_COMPONENT_ARRAY(CPU_COMPONENT("cpu_dai1"), CPU_COMPONENT("cpu_dai2"))); SND_DAILINK_DEF(test_codec, DAILINK_COMPONENT_ARRAY(CODEC_COMPONENT("codec1", "codec_dai1"), CODEC_COMPONENT("codec2", "codec_dai2"))); SND_DAILINK_DEF(test_platform, DAILINK_COMPONENT_ARRAY(PLATFORM_COMPONENT("platform"))); struct snd_soc_dai_link link = { ... SND_DAILINK_REG(test_cpu, test_codec, test_platform), }; /* Sample4. Sample3 + no platform */ struct snd_soc_dai_link link = { ... SND_DAILINK_REG(test_cpu, test_codec, null_dailink_component), }; Thank you for your help !! Best regards --- Kuninori Morimoto