| Message ID | 87zfuesz8y.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | ASoC: Replace dpcm_playback/capture to playback/capture_only | expand |
On 3/31/24 19:30, Kuninori Morimoto wrote: > Current soc_get_playback_capture() (A) is checking playback/capture > availability for DPCM (X) / Normal (Y) / Codec2Codec (Z) connections. > > (A) static int soc_get_playback_capture(...) > { > ... > ^ if (dai_link->dynamic || dai_link->no_pcm) { > | ... > |(a) if (dai_link->dpcm_playback) { > | ... > | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > |(*) ... > | v } > | ... > (X) } > |(b) if (dai_link->dpcm_capture) { > | ... > | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > |(*) ... > | v } > | ... > v } > } else { > ^ ^ /* Adapt stream for codec2codec links */ > |(Z) int cpu_capture = ... > | v int cpu_playback = ... > (Y) > | ^ for_each_rtd_ch_maps(rtd, i, ch_maps) { > |(*) ... > v v } > } > ... > } > > (*) part is checking each DAI's availability. > > At first, (X) part is for DPCM, and it checks playback/capture > availability if dai_link has dpcm_playback/capture flag (a)(b). > But we are already using playback/capture_only flag for Normal (Y) and > Codec2Codec (Z). We can use this flags for DPCM too. > > Before After > dpcm_playback = 1; => /* no flags */ > dpcm_capture = 1; > > dpcm_playback = 1; => playback_only = 1; > > dpcm_capture = 1; => capture_only = 1; > > dpcm_playback = 0; => error > dpcm_capture = 0; > > This patch convert dpcm_ flags to _only flag, and dpcm_ flag will be > removed if all driver switched to _only flags. > > Here, CPU <-> Codec relationship is like this > > DPCM > [CPU/dummy]-[dummy/Codec] > ^^^^ ^^^^^ > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > DPCM part (X) is checking only CPU DAI, and > Normal part (Y) is checking both CPU/Codec DAI > > Here, validation check on dummy DAI is always true, > Therefor we want to expand validation check to all cases. > > One note here is that unfortunately DPCM BE Codec had been no validation > check before, but all cases validation check breaks compatibility on > some vender's devices. Thus this patch ignore it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 90 +++++++++++++++++++-------------------------- > 1 file changed, 38 insertions(+), 52 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 77ee103b7cd1..8761ae8fc05f 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2793,7 +2793,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > int *playback, int *capture) > { > struct snd_soc_dai_link *dai_link = rtd->dai_link; > + struct snd_soc_dai_link_ch_map *ch_maps; > struct snd_soc_dai *cpu_dai; > + struct snd_soc_dai *codec_dai; > + struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc); > + int cpu_playback; > + int cpu_capture; > int has_playback = 0; > int has_capture = 0; > int i; > @@ -2803,65 +2808,46 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > return -EINVAL; > } > > + /* REMOVE ME */ > if (dai_link->dynamic || dai_link->no_pcm) { > - int stream; > - > - if (dai_link->dpcm_playback) { > - stream = SNDRV_PCM_STREAM_PLAYBACK; > - > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - has_playback = 1; > - break; > - } > - } > - if (!has_playback) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support playback for stream %s\n", > - dai_link->stream_name); > - return -EINVAL; > - } > + if (dai_link->dpcm_playback && !dai_link->dpcm_capture) > + dai_link->playback_only = 1; > + if (!dai_link->dpcm_playback && dai_link->dpcm_capture) > + dai_link->capture_only = 1; > + if (!dai_link->dpcm_playback && !dai_link->dpcm_capture) { > + dev_err(rtd->dev, "no dpcm_playback/capture are selected\n"); > + return -EINVAL; > } > - if (dai_link->dpcm_capture) { > - stream = SNDRV_PCM_STREAM_CAPTURE; > + } > > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - has_capture = 1; > - break; > - } > - } > + /* Adapt stream for codec2codec links */ > + cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK); > + cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); > > - if (!has_capture) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support capture for stream %s\n", > - dai_link->stream_name); > - return -EINVAL; > - } > - } > - } else { > - struct snd_soc_dai_link_ch_map *ch_maps; > - struct snd_soc_dai *codec_dai; > - > - /* Adapt stream for codec2codec links */ > - int cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); > - int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK); > + /* > + * see > + * soc.h :: [dai_link->ch_maps Image sample] > + */ > + for_each_rtd_ch_maps(rtd, i, ch_maps) { > + cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu); > + codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec); > > /* > - * see > - * soc.h :: [dai_link->ch_maps Image sample] > + * FIXME > + * > + * DPCM BE Codec has been no checked before. > + * It should be checked, but it breaks compatibility. > + * It ignores BE Codec here, so far. > */ > - for_each_rtd_ch_maps(rtd, i, ch_maps) { > - cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu); > - codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec); > - > - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) > - has_playback = 1; > - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) > - has_capture = 1; > - } > + if (dai_link->no_pcm) > + codec_dai = dummy_dai; > + > + if (snd_soc_dai_stream_valid(cpu_dai, cpu_playback) && > + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK)) > + has_playback = 1; > + if (snd_soc_dai_stream_valid(cpu_dai, cpu_capture) && > + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE)) > + has_capture = 1; > } The problem I have is with the following code (not shown with diff) if (dai_link->playback_only) has_capture = 0; if (dai_link->capture_only) has_playback = 0; So with this grand unification, all the loops above may make a decision that could be overridden by these two branches. This was not the case before for DPCM, all the 'has_capture' and 'has_playback' variables were used as a verification of the dai_link settings with an error thrown e.g. if the dpcm_playback was set without any DAIs supporting playback. Now the dailink settings are used unconditionally. There is one warning added if there are no settings for a dailink, but we've lost the detection of a mismatch between dailink and the set of cpu/codec dais that are part of this dailink.
On 3/31/24 19:32, Kuninori Morimoto wrote: > Historically, ASoC doesn't have validation check for DPCM BE Codec, > but it should have. Current ASoC is ignoring it same as before, > but let's indicate the warning about that. > > This warning and code should be removed and cleanuped if DPCM BE Codec > has necessary settings. > One of the big user which doesn't have it is Intel. > > --- sound/soc/codecs/hda.c --- > > static struct snd_soc_dai_driver card_binder_dai = { > .id = -1, > .name = "codec-probing-DAI", > + .capture.channels_min = 1, > + .playback.channels_min = 1, > }; > > --- sound/pci/hda/patch_hdmi.c --- > > static int generic_hdmi_build_pcms(...) > { > ... > for (...) { > ... > + pstr->channels_min = 1; > } > > return 0; > } > > Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com > Cc: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index ac42c089815b..95a5e28dead3 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2796,7 +2796,6 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > struct snd_soc_dai_link_ch_map *ch_maps; > struct snd_soc_dai *cpu_dai; > struct snd_soc_dai *codec_dai; > - struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc); > int cpu_playback; > int cpu_capture; > int has_playback = 0; > @@ -2817,24 +2816,36 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > * soc.h :: [dai_link->ch_maps Image sample] > */ > for_each_rtd_ch_maps(rtd, i, ch_maps) { > - cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu); > - codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec); > + int cpu_play_t, cpu_capture_t; > + int codec_play_t, codec_capture_t; > + > + cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu); > + codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec); > + > + cpu_play_t = snd_soc_dai_stream_valid(cpu_dai, cpu_playback); > + codec_play_t = snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK); > + > + cpu_capture_t = snd_soc_dai_stream_valid(cpu_dai, cpu_capture); > + codec_capture_t = snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE); > > /* > - * FIXME > + * FIXME / CLEAN-UP-ME > * > * DPCM BE Codec has been no checked before. > * It should be checked, but it breaks compatibility. > * It ignores BE Codec here, so far. > */ > - if (dai_link->no_pcm) > - codec_dai = dummy_dai; > + if ((dai_link->no_pcm) && > + (!codec_play_t && !codec_capture_t)) { > + dev_warn_once(rtd->dev, "DCPM BE Codec has no stream settings (%s)\n", > + codec_dai->name); > + codec_play_t = 1; > + codec_capture_t = 1; > + } > > - if (snd_soc_dai_stream_valid(cpu_dai, cpu_playback) && > - snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK)) > + if (cpu_play_t && codec_play_t) > has_playback = 1; > - if (snd_soc_dai_stream_valid(cpu_dai, cpu_capture) && > - snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE)) > + if (cpu_capture_t && codec_capture_t) > has_capture = 1; > } All that code should be added earlier, and there's still the issue that all this code is now overridden by the dai_link settings.
Hi Pierre-Louis Thank you for your review > The problem I have is with the following code (not shown with diff) > > if (dai_link->playback_only) > has_capture = 0; > > if (dai_link->capture_only) > has_playback = 0; > > So with this grand unification, all the loops above may make a decision > that could be overridden by these two branches. > > This was not the case before for DPCM, all the 'has_capture' and > 'has_playback' variables were used as a verification of the dai_link > settings with an error thrown e.g. if the dpcm_playback was set without > any DAIs supporting playback. I could understand so far. > Now the dailink settings are used unconditionally. There is one warning > added if there are no settings for a dailink, but we've lost the > detection of a mismatch between dailink and the set of cpu/codec dais > that are part of this dailink. But sorry I could understand this. "There is one warning added if there are no settings for a dailink" By [01/16] patch ? I think no warning is added. Or do you mean by [15/16] patch ? "we've lost the detection of a mismatch between dailink and the set of cpu/codec dais that are part of this dailink" Sorry I couldn't understand about this. Which mismatch detection we lost ?? Concrete sample / code / image is very helpful for me to well understanding. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis, again > > The problem I have is with the following code (not shown with diff) > > > > if (dai_link->playback_only) > > has_capture = 0; > > > > if (dai_link->capture_only) > > has_playback = 0; > > > > So with this grand unification, all the loops above may make a decision > > that could be overridden by these two branches. > > > > This was not the case before for DPCM, all the 'has_capture' and > > 'has_playback' variables were used as a verification of the dai_link > > settings with an error thrown e.g. if the dpcm_playback was set without > > any DAIs supporting playback. Hmm... above 2 branches are used for DPCM too before. > > Now the dailink settings are used unconditionally. There is one warning > > added if there are no settings for a dailink, but we've lost the > > detection of a mismatch between dailink and the set of cpu/codec dais > > that are part of this dailink. Does this mean, for example you want to have warning/error by dpcm_playback flag if you are thinking it can use playback , but FE or BE playback was not valid ? If so, yes indeed this patch removes such flags. But I wonder why you want to get it in case of DPCM only ? I can understand if all case want to have it. Then, I think we can check _only for this purpose too ? (A) if dai_link has playback_only -> it should have has_playback (B) if dai_link has capture_only -> it should have has_capture (C) if dai_link don't have both xxx_only -> it should have both has_xxx I think we already have (A)(B) check. We want to add (C) check ? If my understanding was correct, the things dpcm_xxx flag can do is also can do by xxx_only flag. But dpcm_xxx flag is used only DPCM, but xxx_only flag is used on all cases. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/1/24 19:21, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your review > >> The problem I have is with the following code (not shown with diff) >> >> if (dai_link->playback_only) >> has_capture = 0; >> >> if (dai_link->capture_only) >> has_playback = 0; >> >> So with this grand unification, all the loops above may make a decision >> that could be overridden by these two branches. >> >> This was not the case before for DPCM, all the 'has_capture' and >> 'has_playback' variables were used as a verification of the dai_link >> settings with an error thrown e.g. if the dpcm_playback was set without >> any DAIs supporting playback. > > I could understand so far. > >> Now the dailink settings are used unconditionally. There is one warning >> added if there are no settings for a dailink, but we've lost the >> detection of a mismatch between dailink and the set of cpu/codec dais >> that are part of this dailink. > > But sorry I could understand this. > > "There is one warning added if there are no settings for a dailink" > > By [01/16] patch ? I think no warning is added. Or do you mean by [15/16] > patch ? Yes I looked at the entire series, it's just too complicated to look with diff. > > "we've lost the detection of a mismatch between dailink and the > set of cpu/codec dais that are part of this dailink" > > Sorry I couldn't understand about this. > Which mismatch detection we lost ?? Concrete sample / code / image > is very helpful for me to well understanding. With the older code, if the dpcm_playback was set for the dailink but there isn't any dai connected to support playback, an error was thrown. With the new code, if playback_only is set but there isn't any dai connected, there is no error thrown, is there? The point is that these flags are sometimes set in the machine driver, sometimes set in the framework, and the open is which one has the priority.
On 4/1/2024 2:31 AM, Kuninori Morimoto wrote: > soc_get_playback_capture() is now handling DPCM and normal comprehensively > for playback/capture stream. We can use playback/capture_only flag > instead of using dpcm_playback/capture. This patch replace these. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- ... > diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c > index b94835448b1b..34a9b2e52451 100644 > --- a/sound/soc/intel/boards/sof_sdw.c > +++ b/sound/soc/intel/boards/sof_sdw.c > @@ -1151,8 +1151,8 @@ static void init_dai_link(struct device *dev, struct snd_soc_dai_link *dai_links > dai_links->num_cpus = cpus_num; > dai_links->codecs = codecs; > dai_links->num_codecs = codecs_num; > - dai_links->dpcm_playback = playback; > - dai_links->dpcm_capture = capture; > + dai_links->playback_only = !playback; > + dai_links->capture_only = !capture; Above seems weird? Should probably be: dai_links->playback_only = playback && !capture; dai_links->capture_only = capture && !playback; and while at it, I still wonder if it is best way to go about this change, because it causes problems like one above due to need to do boolean logic to know which direction is enabled. I would just modify struct snd_soc_dai_link to have fields like: int playback_enabled; int capture_enabled; which would be far more understandable. And if we don't want to have two variables then perhaps something like: #define ASOC_ENDPOINT_DISABLED BIT(0) #define ASOC_ENDPOINT_PLAYBACK BIT(1) #define ASOC_ENDPOINT_CAPTURE BIT(2) #define ASOC_ENDPOINT_BIDIRECTIONAL (ENDPOINT_PLAYBACK | ENDPOINT_CAPTURE) struct snd_soc_dai_link { (...) int endpoint_type:2; // see ASOC_ENDPOINT (...) }; I like the idea of removing the duplication of variables, but if we are trying to simplify things, let's try to not complicate them at the same time. Thanks, Amadeusz
On 4/2/24 01:43, Kuninori Morimoto wrote: > > Hi Pierre-Louis, again > >>> The problem I have is with the following code (not shown with diff) >>> >>> if (dai_link->playback_only) >>> has_capture = 0; >>> >>> if (dai_link->capture_only) >>> has_playback = 0; >>> >>> So with this grand unification, all the loops above may make a decision >>> that could be overridden by these two branches. >>> >>> This was not the case before for DPCM, all the 'has_capture' and >>> 'has_playback' variables were used as a verification of the dai_link >>> settings with an error thrown e.g. if the dpcm_playback was set without >>> any DAIs supporting playback. > > Hmm... above 2 branches are used for DPCM too before. Not really, playback_only and capture_only were never set so those two branches were always false. > >>> Now the dailink settings are used unconditionally. There is one warning >>> added if there are no settings for a dailink, but we've lost the >>> detection of a mismatch between dailink and the set of cpu/codec dais >>> that are part of this dailink. > > Does this mean, for example you want to have warning/error by dpcm_playback > flag if you are thinking it can use playback , but FE or BE playback was > not valid ? Again we had a verification that if the dpcm_playback was set at the dailink level, it was actually supported by the dais. We seem to have lost this verification. We only have an error when there are no settings at all. > > If so, yes indeed this patch removes such flags. > But I wonder why you want to get it in case of DPCM only ? It's helpful to detect invalid configurations. And I see to reason why the removal of flags should change the functionality. > I can understand if all case want to have it. > > Then, I think we can check _only for this purpose too ? > > (A) if dai_link has playback_only -> it should have has_playback > (B) if dai_link has capture_only -> it should have has_capture > (C) if dai_link don't have both xxx_only -> it should have both has_xxx > > I think we already have (A)(B) check. We want to add (C) check ? > > If my understanding was correct, the things dpcm_xxx flag can do is also > can do by xxx_only flag. But dpcm_xxx flag is used only DPCM, but xxx_only > flag is used on all cases. I am only concerned about mismatches between dailinks and dai capabilities. The logic above is fine, but it's in the scope of the dailink only.
Hi Amadeusz Cc Pierre-Louis Thank you for your review > > - dai_links->dpcm_playback = playback; > > - dai_links->dpcm_capture = capture; > > + dai_links->playback_only = !playback; > > + dai_links->capture_only = !capture; > > Above seems weird? Should probably be: > > dai_links->playback_only = playback && !capture; > dai_links->capture_only = capture && !playback; Ah, yes, indeed. Thank you for pointing it. I will fix it on v3 > and while at it, I still wonder if it is best way to go about this > change, because it causes problems like one above due to need to do > boolean logic to know which direction is enabled. I would just modify > struct snd_soc_dai_link to have fields like: > int playback_enabled; > int capture_enabled; > which would be far more understandable. And if we don't want to have two > variables then perhaps something like: > #define ASOC_ENDPOINT_DISABLED BIT(0) > #define ASOC_ENDPOINT_PLAYBACK BIT(1) > #define ASOC_ENDPOINT_CAPTURE BIT(2) > #define ASOC_ENDPOINT_BIDIRECTIONAL (ENDPOINT_PLAYBACK | ENDPOINT_CAPTURE) (snip) > I like the idea of removing the duplication of variables, but if we are > trying to simplify things, let's try to not complicate them at the same > time. Thank you for your idea. I agree that I don't want things be complicated. Basically, I can agree about your idea, but there is biggest problem to do that in reality. It is too late. If we uses xxx_enabled flag, almost all driver need to be updated, but it doesn't have something "from". DPCM connection have dpcm_xxx, but other connection doesn't. Indeed we can use xxx_only flag, but the user of it is rare case. And if we use xxx_enabled, this means "default disabled". The point is "Which should be default ?" disabled or enabled. If "default is enabled", almost all driver need to nothing, because driver is created to use it. For me, "default enabled" and "need special flag if disabled" is very natural. But if "default is disable", almost all driver need to set xxx_enabled = 1, but it is very verbose for me, and it will be more huge, complicated, and large scope of influence patch than this patch-set. It seems Pierre-Louis have similar opinion of you ? So, alternative idea is have such flag in the function. For driver side point, let's use xxx_only flag. This means "default enabled", "need special flag if disabled". (We want to convert xxx_only to xxx_disabled in the future ?) Pseudo Code bool has_playback, has_capture; bool should_have_playback, should_have_capture; /* default enabled */ should_have_playback = 1; should_have_capture = 1; /* use spacial flag if disabled */ if (dai_links->playback_only) should_have_capture = 0; if (dai_links->capture_only) should_have_playback = 0; /* valid check */ for_each_xxx( ... ) { has_playback = xxx; has_capture = xxx; } /* use spacial flag if disabled */ if (dai_links->playback_only) has_capture = 0; if (dai_links->capture_only) has_playback = 0; /* both disabled is error */ if (!has_playback && !has_capture) { dev_err(...) return -EINVAL; } /* detect mismatch */ if (has_playback != should_have_playback) { dev_err(dev, "It should playback valid, but not") return -EINVAL; } if (has_capture != should_have_capture) { dev_err(dev, "It should capture valid, but not") return -EINVAL; } Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis Thank you for your feedback. I could understand your comment 80%, but not yet 100% > With the older code, if the dpcm_playback was set for the dailink but > there isn't any dai connected to support playback, an error was thrown. > > With the new code, if playback_only is set but there isn't any dai > connected, there is no error thrown, is there? (snip) > Again we had a verification that if the dpcm_playback was set at the > dailink level, it was actually supported by the dais. > > We seem to have lost this verification. We only have an error when there > are no settings at all. Pseudo code of new soc_get_playback_capture() is like this soc_get_playback_capture(...) { ... ^ for_each_rtd_ch_maps(...) { | ... (A) has_playback = xxx; | has_capture = xxx; v } ^ if (dai_link->playback_only) | has_capture = 0; (B) | if (dai_link->capture_only) v has_playback = 0; ^ if (!has_playback && !has_capture) { (C) dev_err(...); v return -EINVAL; } ... } In old/new soc_get_playback_capture(), has_xxx will be set at least if one of rtd connected DAI can handle playback/capture. In new code, it will be handled at (A). And unneeded has_xxx will be removed if xxx_only was set (B) Then, if neither has_xxx was set, it will be error (C) In new code, if playback_only is set but there isn't any dai connected, there is no error thrown, is there? If playback_only was set, has_capture will be removed at (B). And if DAI was not playback-able, this means has_playback was not set at (A). In such case, (C) will indicate error. Same things happen if capture_only too. So, old functions validation still exist in my opinion, but am I misunderstanding ? One note here is that in DPCM case, old function checks CPU only, but new function checks both CPU and Codec. 2nd note is that in current version of patch-set, if dai_link doesn't have xxx_only settings (= it should have both playback/capture), but if DAI has has_playback or has_capture only, it can't detect about it. I suggested it in previous mail, and will fix in v3 > The point is that these flags are sometimes set in the machine driver, > sometimes set in the framework, and the open is which one has the priority. I couldn't understand this. I think "machine driver" = CPU/Codec driver, but what is "these flags" which is sometimes set in machine driver, and sometimes set in framework ?? dpcm_xxx ? xxx_only ?? I don't think framework set these... Or do you mean [09/16] patch (= it will set dai_link->no_pcm) ? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Mon 01 Apr 2024 at 00:27, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Mark > > This is v2 patch-set > > When we use DPCM, we need to set dpcm_playback/capture flag. > If these flag are set, soc_get_playback_capture() will check its > availability, but non DPCM doesn't need such special flags. > > OTOH, it cares playback/capture_only flag. It is needed. > > This patch remove DPCM special flag, and replace it playback/capture_only > flag if needed. Hi Kuninori-san, Thanks for taking the time to clean the dpcm flags. While at it, I wonder if it would be worth taking it a step further. playback_only and capture_only have implication on each other. If one is set, the other can/must not be set. This leads to conditions which can be fairly hard to read and possibly bugs. I had to re-read the meson patch a few times to make sure it still had the same meaning, TBH Wouldn't it be better to replace those 2 flags with a single bitfield ? something like: unsigned int directions; #define PLAYBACK_VALID BIT(0) #define CAPTURE_VALID BIT(1) ... or something similar. I think conditions on the enabled stream would become much clearer like this. The only invalid configuation would be '!directions', which again is easier to read, instead of checking if both flags are set. It would be easy to keep playback_only/capture_only tests, where necessary, with an helper function. What do you think ? Sorry if it is a bit late to discuss this. > > v1 -> v2 > - based on latest ASoC branch > - keep comment on Intel > - tidyup patch title > - tidyup DPCM BE warning output condition > - Add new patch for Document > > Link: https://lore.kernel.org/r/87o7b353of.wl-kuninori.morimoto.gx@renesas.com > > Kuninori Morimoto (16): > ASoC: soc-pcm.c: cleanup soc_get_playback_capture() > ASoC: amd: Replace dpcm_playback/capture to playback/capture_only > ASoC: fsl: Replace dpcm_playback/capture to playback/capture_only > ASoC: sof: Replace dpcm_playback/capture to playback/capture_only > ASoC: meson: Replace dpcm_playback/capture to playback/capture_only > ASoC: Intel: Replace dpcm_playback/capture to playback/capture_only > ASoC: samsung: Replace dpcm_playback/capture to playback/capture_only > ASoC: mediatek: Replace dpcm_playback/capture to playback/capture_only > ASoC: soc-core: Replace dpcm_playback/capture to playback/capture_only > ASoC: soc-topology: Replace dpcm_playback/capture to > playback/capture_only > ASoC: soc-compress: Replace dpcm_playback/capture to > playback/capture_only > ASoC: Intel: avs: boards: Replace dpcm_playback/capture to > playback/capture_only > ASoC: remove snd_soc_dai_link_set_capabilities() > ASoC: soc-pcm: remove dpcm_playback/capture > ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings > ASoC: doc: remove .dpcm_playback/capture flags > > Documentation/sound/soc/dpcm.rst | 14 ++- > include/sound/soc-dai.h | 1 - > include/sound/soc.h | 4 - > sound/soc/amd/acp-da7219-max98357a.c | 20 ++--- > sound/soc/amd/acp-es8336.c | 2 - > sound/soc/amd/acp/acp-mach-common.c | 24 ++--- > sound/soc/amd/acp3x-rt5682-max9836.c | 6 +- > sound/soc/amd/vangogh/acp5x-mach.c | 6 -- > sound/soc/fsl/fsl-asoc-card.c | 16 ++-- > sound/soc/fsl/imx-audmix.c | 6 +- > sound/soc/fsl/imx-card.c | 7 +- > sound/soc/generic/audio-graph-card.c | 2 - > sound/soc/generic/audio-graph-card2.c | 2 - > sound/soc/generic/simple-card.c | 2 - > sound/soc/intel/avs/boards/da7219.c | 2 - > sound/soc/intel/avs/boards/dmic.c | 4 +- > sound/soc/intel/avs/boards/es8336.c | 2 - > sound/soc/intel/avs/boards/hdaudio.c | 4 - > sound/soc/intel/avs/boards/i2s_test.c | 2 - > sound/soc/intel/avs/boards/max98357a.c | 2 +- > sound/soc/intel/avs/boards/max98373.c | 2 - > sound/soc/intel/avs/boards/max98927.c | 2 - > sound/soc/intel/avs/boards/nau8825.c | 2 - > sound/soc/intel/avs/boards/rt274.c | 2 - > sound/soc/intel/avs/boards/rt286.c | 2 - > sound/soc/intel/avs/boards/rt298.c | 2 - > sound/soc/intel/avs/boards/rt5514.c | 2 +- > sound/soc/intel/avs/boards/rt5663.c | 2 - > sound/soc/intel/avs/boards/rt5682.c | 2 - > sound/soc/intel/avs/boards/ssm4567.c | 2 - > sound/soc/intel/boards/bdw-rt5650.c | 4 - > sound/soc/intel/boards/bdw-rt5677.c | 4 - > sound/soc/intel/boards/bdw_rt286.c | 10 +-- > sound/soc/intel/boards/bxt_da7219_max98357a.c | 32 ++++--- > sound/soc/intel/boards/bxt_rt298.c | 26 +++--- > sound/soc/intel/boards/bytcht_cx2072x.c | 6 +- > sound/soc/intel/boards/bytcht_da7213.c | 6 +- > sound/soc/intel/boards/bytcht_es8316.c | 6 +- > sound/soc/intel/boards/bytcht_nocodec.c | 6 +- > sound/soc/intel/boards/bytcr_rt5640.c | 6 +- > sound/soc/intel/boards/bytcr_rt5651.c | 6 +- > sound/soc/intel/boards/bytcr_wm5102.c | 6 +- > sound/soc/intel/boards/cht_bsw_max98090_ti.c | 6 +- > sound/soc/intel/boards/cht_bsw_nau8824.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +- > sound/soc/intel/boards/cml_rt1011_rt5682.c | 15 ++-- > sound/soc/intel/boards/ehl_rt5660.c | 14 ++- > sound/soc/intel/boards/glk_rt5682_max98357a.c | 30 +++---- > sound/soc/intel/boards/hsw_rt5640.c | 10 +-- > sound/soc/intel/boards/kbl_da7219_max98357a.c | 26 +++--- > sound/soc/intel/boards/kbl_da7219_max98927.c | 54 +++++------- > sound/soc/intel/boards/kbl_rt5660.c | 18 ++-- > sound/soc/intel/boards/kbl_rt5663_max98927.c | 44 +++++----- > .../intel/boards/kbl_rt5663_rt5514_max98927.c | 22 ++--- > sound/soc/intel/boards/skl_hda_dsp_common.c | 14 ++- > .../soc/intel/boards/skl_nau88l25_max98357a.c | 26 +++--- > sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 26 +++--- > sound/soc/intel/boards/skl_rt286.c | 26 +++--- > sound/soc/intel/boards/sof_board_helpers.c | 13 +-- > sound/soc/intel/boards/sof_es8336.c | 8 +- > sound/soc/intel/boards/sof_pcm512x.c | 8 +- > sound/soc/intel/boards/sof_sdw.c | 4 +- > sound/soc/intel/boards/sof_wm8804.c | 2 - > sound/soc/mediatek/mt2701/mt2701-cs42448.c | 20 ++--- > sound/soc/mediatek/mt2701/mt2701-wm8960.c | 6 +- > sound/soc/mediatek/mt6797/mt6797-mt6351.c | 24 ++--- > sound/soc/mediatek/mt7986/mt7986-wm8960.c | 6 +- > sound/soc/mediatek/mt8173/mt8173-max98090.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5514.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5676.c | 10 +-- > sound/soc/mediatek/mt8173/mt8173-rt5650.c | 10 +-- > .../mediatek/mt8183/mt8183-da7219-max98357.c | 34 +++----- > .../mt8183/mt8183-mt6358-ts3a227-max98357.c | 34 +++----- > .../mt8186/mt8186-mt6366-da7219-max98357.c | 86 +++++++----------- > .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 86 +++++++----------- > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 58 ++++++------- > .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 78 ++++++++--------- > sound/soc/mediatek/mt8195/mt8195-mt6359.c | 60 +++++++------ > sound/soc/meson/axg-card.c | 9 +- > sound/soc/meson/gx-card.c | 1 - > sound/soc/meson/meson-card-utils.c | 4 +- > sound/soc/qcom/common.c | 1 - > sound/soc/samsung/odroid.c | 11 ++- > sound/soc/soc-compress.c | 10 ++- > sound/soc/soc-core.c | 20 +---- > sound/soc/soc-dai.c | 38 -------- > sound/soc/soc-pcm.c | 87 ++++++++----------- > sound/soc/soc-topology-test.c | 2 - > sound/soc/soc-topology.c | 4 +- > sound/soc/sof/nocodec.c | 4 - > 91 files changed, 502 insertions(+), 863 deletions(-)
On Mon 01 Apr 2024 at 00:31, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > soc_get_playback_capture() is now handling DPCM and normal comprehensively > for playback/capture stream. We can use playback/capture_only flag > instead of using dpcm_playback/capture. This patch replace these. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Looks OK Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> > --- > sound/soc/meson/axg-card.c | 8 ++++---- > sound/soc/meson/meson-card-utils.c | 4 ++-- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c > index 3180aa4d3a15..21bf1453af43 100644 > --- a/sound/soc/meson/axg-card.c > +++ b/sound/soc/meson/axg-card.c > @@ -132,7 +132,7 @@ static int axg_card_add_tdm_loopback(struct snd_soc_card *card, > lb->stream_name = lb->name; > lb->cpus->of_node = pad->cpus->of_node; > lb->cpus->dai_name = "TDM Loopback"; > - lb->dpcm_capture = 1; > + lb->capture_only = 1; > lb->no_pcm = 1; > lb->ops = &axg_card_tdm_be_ops; > lb->init = axg_card_tdm_dai_lb_init; > @@ -176,7 +176,7 @@ static int axg_card_parse_cpu_tdm_slots(struct snd_soc_card *card, > > /* Disable playback is the interface has no tx slots */ > if (!tx) > - link->dpcm_playback = 0; > + link->capture_only = 1; > > for (i = 0, rx = 0; i < AXG_TDM_NUM_LANES; i++) { > snprintf(propname, 32, "dai-tdm-slot-rx-mask-%d", i); > @@ -186,7 +186,7 @@ static int axg_card_parse_cpu_tdm_slots(struct snd_soc_card *card, > > /* Disable capture is the interface has no rx slots */ > if (!rx) > - link->dpcm_capture = 0; > + link->playback_only = 1; > > /* ... but the interface should at least have one of them */ > if (!tx && !rx) { > @@ -275,7 +275,7 @@ static int axg_card_parse_tdm(struct snd_soc_card *card, > return ret; > > /* Add loopback if the pad dai has playback */ > - if (link->dpcm_playback) { > + if (!link->capture_only) { > ret = axg_card_add_tdm_loopback(card, index); > if (ret) > return ret; > diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c > index ed6c7e2f609c..1a4ef124e4e2 100644 > --- a/sound/soc/meson/meson-card-utils.c > +++ b/sound/soc/meson/meson-card-utils.c > @@ -186,9 +186,9 @@ int meson_card_set_fe_link(struct snd_soc_card *card, > link->dpcm_merged_rate = 1; > > if (is_playback) > - link->dpcm_playback = 1; > + link->playback_only = 1; > else > - link->dpcm_capture = 1; > + link->capture_only = 1; > > return meson_card_set_link_name(card, link, node, "fe"); > }
On 4/3/24 20:53, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your feedback. > I could understand your comment 80%, but not yet 100% > >> With the older code, if the dpcm_playback was set for the dailink but >> there isn't any dai connected to support playback, an error was thrown. >> >> With the new code, if playback_only is set but there isn't any dai >> connected, there is no error thrown, is there? > (snip) >> Again we had a verification that if the dpcm_playback was set at the >> dailink level, it was actually supported by the dais. >> >> We seem to have lost this verification. We only have an error when there >> are no settings at all. > > Pseudo code of new soc_get_playback_capture() is like this > > soc_get_playback_capture(...) > { > ... > ^ for_each_rtd_ch_maps(...) { > | ... > (A) has_playback = xxx; > | has_capture = xxx; > v } > > ^ if (dai_link->playback_only) > | has_capture = 0; > (B) > | if (dai_link->capture_only) > v has_playback = 0; > > ^ if (!has_playback && !has_capture) { > (C) dev_err(...); > v return -EINVAL; > } > ... > } > > In old/new soc_get_playback_capture(), has_xxx will be set at least > if one of rtd connected DAI can handle playback/capture. > In new code, it will be handled at (A). > > And unneeded has_xxx will be removed if xxx_only was set (B) The problem is that we have two sources of information 1) the dais included in the dailink (the (A) part above) 2) the dailink itself (the (B) part above) the code in A) constructs the information from the ground-up, but it's overridden by B). You can view it as 'removing unneeded has_xxx' flags, but it's also a problem is the dailink information is incorrect... In the past we would report an error if the dailink was not aligned with the dais. Now we just ignore the dai information. That's the concern, we're changing the behavior. > Then, if neither has_xxx was set, it will be error (C) That's not the concern. The concern is a discrepancy between A) and B). > > In new code, if playback_only is set but there isn't any dai > connected, there is no error thrown, is there? > > If playback_only was set, has_capture will be removed at (B). > And if DAI was not playback-able, this means has_playback was not set at (A). > In such case, (C) will indicate error. Same things happen if capture_only too. > > So, old functions validation still exist in my opinion, but am I > misunderstanding ? > > One note here is that in DPCM case, old function checks CPU only, > but new function checks both CPU and Codec. > > 2nd note is that in current version of patch-set, if dai_link doesn't > have xxx_only settings (= it should have both playback/capture), but if > DAI has has_playback or has_capture only, it can't detect about it. > I suggested it in previous mail, and will fix in v3 > >> The point is that these flags are sometimes set in the machine driver, >> sometimes set in the framework, and the open is which one has the priority. > > I couldn't understand this. > > I think "machine driver" = CPU/Codec driver, but what is "these flags" > which is sometimes set in machine driver, and sometimes set in framework ?? > dpcm_xxx ? xxx_only ?? I don't think framework set these... The has_xxx flag is set based on dai capabilities in (A) - which I call "the framework" OR by the machine driver setting the playback_only/capture_only flags, then used in (B) to override (A). When you have two sources of information competing to set a state, we have to be really careful on which one has priority/precedence.
Hi Jerome Thank you for your feedback > playback_only and capture_only have implication on each other. If one is > set, the other can/must not be set. This leads to conditions which can > be fairly hard to read and possibly bugs. (snip) > Wouldn't it be better to replace those 2 flags with a single bitfield ? > > something like: > > unsigned int directions; > #define PLAYBACK_VALID BIT(0) > #define CAPTURE_VALID BIT(1) I think Amadeusz indicated similar idea, and basically I can agree about it. But in this patch-set, I want focus to removing dpcm_xxx flag as 1st step. So I'm happy to create such patch-set, but I want to handle it as another patch-set. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis Thank you for clarifying the point > > And unneeded has_xxx will be removed if xxx_only was set (B) > > The problem is that we have two sources of information > > 1) the dais included in the dailink (the (A) part above) > 2) the dailink itself (the (B) part above) > > the code in A) constructs the information from the ground-up, but it's > overridden by B). > > You can view it as 'removing unneeded has_xxx' flags, but it's also a > problem is the dailink information is incorrect... > > In the past we would report an error if the dailink was not aligned with > the dais. Now we just ignore the dai information. Ah, OK now I could understand. Hmm... is below what you mean in summary? dpcm_xxx is used to declare that the DAI/dailink is possible to use playback/capture. For example dpcm_playback means the DAI / dailink should playback-able, if not it is error. xxx_only is used to limit the playback/capture. For example the DAI / dailink can use both playback and capture, but want to use playback only for some reasons, we can use playback_only. So these are used for different purpose. Hmm... I re-consider about it for many cases, and indeed these can't merge. But in such case, this feature is needed not only for DPCM ? Now Jerome / Amadeusz are suggesting new idea to use bitfield idea. We can use it ? #define PLAYBACK_VALID BIT(0) #define CAPTURE_VALID BIT(1) #define PLAYBACK_LIMIT BIT(2) #define CAPTURE_LIMIT BIT(3) I need to think about keeping compatibility, but maybe OK. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Thu 04 Apr 2024 at 23:13, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Jerome > > Thank you for your feedback > >> playback_only and capture_only have implication on each other. If one is >> set, the other can/must not be set. This leads to conditions which can >> be fairly hard to read and possibly bugs. > (snip) >> Wouldn't it be better to replace those 2 flags with a single bitfield ? >> >> something like: >> >> unsigned int directions; >> #define PLAYBACK_VALID BIT(0) >> #define CAPTURE_VALID BIT(1) > > I think Amadeusz indicated similar idea, and basically I can agree about > it. I've seen it afterward. It is similar indeed but I don't think 'None' or 'Both' should have a dedicated bit. That would be yet another redundance/implication between flags/bits ... so another source of bugs/complexity IMO. > But in this patch-set, I want focus to removing dpcm_xxx flag as 1st > step. So I'm happy to create such patch-set, but I want to handle it as > another patch-set. Fine by me ... at least for the Amlogic part. > > Thank you for your help !! > Thanks for your work ! > Best regards > --- > Renesas Electronics > Ph.D. Kuninori Morimoto
Hi Jerome > >> unsigned int directions; > >> #define PLAYBACK_VALID BIT(0) > >> #define CAPTURE_VALID BIT(1) > > > > I think Amadeusz indicated similar idea, and basically I can agree about > > it. > > I've seen it afterward. It is similar indeed but I don't think 'None' or > 'Both' should have a dedicated bit. That would be yet another > redundance/implication between flags/bits ... so another source of > bugs/complexity IMO. I noticed that it is alreay using bitfield. The diff is it is possible to use "XXX | YYY" style or not. /* For unidirectional dai links */ unsigned int playback_only:1; unsigned int capture_only:1; ... /* DPCM capture and Playback support */ unsigned int dpcm_capture:1; unsigned int dpcm_playback:1; Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis Thank you for your feedback > The code looks fine, but what are we trying to achieve? > I thought the idea was to have a single field at the dailink, and with > the example above we would still have two - just like today. > This looks like a lot of code churn in many drivers for limited > benefits. Or I am missing something? Yeah. Main purpose of this patch-set is cleanup soc-pcm code which is very complex today. After sending mail, I noticed that xxx_only flag can be merged into new xxx_assertion flag. For example "playback_only" means it must playback available. One note here is that xxx_assertion flag is not mandatory dpcm_playback -> playabck_assertion = 1 dpcm_capture -> capture_assertion = 1 playback_only -> playback_assertion = 1 capture_assertion = 0 capture_only -> playback_assertion = 0 capture_assertion = 1 /* * Assertion check * * xxx_assertion flag is not mandatory */ if (dai_link->playback_assertion) { if (!has_playback) { dev_err(rtd->dev, ...); return -EINVAL; } /* makes it plyaback only */ if (!dai_link->capture_assertion) has_capture = 0; } if (dai_link->capture_assertion) { if (!has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } /* makes it capture only */ if (!dai_link->playback_assertion) has_playback = 0; } /* * Detect Mismatch */ if (!has_playback && !has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Mark This is v2 patch-set When we use DPCM, we need to set dpcm_playback/capture flag. If these flag are set, soc_get_playback_capture() will check its availability, but non DPCM doesn't need such special flags. OTOH, it cares playback/capture_only flag. It is needed. This patch remove DPCM special flag, and replace it playback/capture_only flag if needed. v1 -> v2 - based on latest ASoC branch - keep comment on Intel - tidyup patch title - tidyup DPCM BE warning output condition - Add new patch for Document Link: https://lore.kernel.org/r/87o7b353of.wl-kuninori.morimoto.gx@renesas.com Kuninori Morimoto (16): ASoC: soc-pcm.c: cleanup soc_get_playback_capture() ASoC: amd: Replace dpcm_playback/capture to playback/capture_only ASoC: fsl: Replace dpcm_playback/capture to playback/capture_only ASoC: sof: Replace dpcm_playback/capture to playback/capture_only ASoC: meson: Replace dpcm_playback/capture to playback/capture_only ASoC: Intel: Replace dpcm_playback/capture to playback/capture_only ASoC: samsung: Replace dpcm_playback/capture to playback/capture_only ASoC: mediatek: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-core: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-topology: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-compress: Replace dpcm_playback/capture to playback/capture_only ASoC: Intel: avs: boards: Replace dpcm_playback/capture to playback/capture_only ASoC: remove snd_soc_dai_link_set_capabilities() ASoC: soc-pcm: remove dpcm_playback/capture ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings ASoC: doc: remove .dpcm_playback/capture flags Documentation/sound/soc/dpcm.rst | 14 ++- include/sound/soc-dai.h | 1 - include/sound/soc.h | 4 - sound/soc/amd/acp-da7219-max98357a.c | 20 ++--- sound/soc/amd/acp-es8336.c | 2 - sound/soc/amd/acp/acp-mach-common.c | 24 ++--- sound/soc/amd/acp3x-rt5682-max9836.c | 6 +- sound/soc/amd/vangogh/acp5x-mach.c | 6 -- sound/soc/fsl/fsl-asoc-card.c | 16 ++-- sound/soc/fsl/imx-audmix.c | 6 +- sound/soc/fsl/imx-card.c | 7 +- sound/soc/generic/audio-graph-card.c | 2 - sound/soc/generic/audio-graph-card2.c | 2 - sound/soc/generic/simple-card.c | 2 - sound/soc/intel/avs/boards/da7219.c | 2 - sound/soc/intel/avs/boards/dmic.c | 4 +- sound/soc/intel/avs/boards/es8336.c | 2 - sound/soc/intel/avs/boards/hdaudio.c | 4 - sound/soc/intel/avs/boards/i2s_test.c | 2 - sound/soc/intel/avs/boards/max98357a.c | 2 +- sound/soc/intel/avs/boards/max98373.c | 2 - sound/soc/intel/avs/boards/max98927.c | 2 - sound/soc/intel/avs/boards/nau8825.c | 2 - sound/soc/intel/avs/boards/rt274.c | 2 - sound/soc/intel/avs/boards/rt286.c | 2 - sound/soc/intel/avs/boards/rt298.c | 2 - sound/soc/intel/avs/boards/rt5514.c | 2 +- sound/soc/intel/avs/boards/rt5663.c | 2 - sound/soc/intel/avs/boards/rt5682.c | 2 - sound/soc/intel/avs/boards/ssm4567.c | 2 - sound/soc/intel/boards/bdw-rt5650.c | 4 - sound/soc/intel/boards/bdw-rt5677.c | 4 - sound/soc/intel/boards/bdw_rt286.c | 10 +-- sound/soc/intel/boards/bxt_da7219_max98357a.c | 32 ++++--- sound/soc/intel/boards/bxt_rt298.c | 26 +++--- sound/soc/intel/boards/bytcht_cx2072x.c | 6 +- sound/soc/intel/boards/bytcht_da7213.c | 6 +- sound/soc/intel/boards/bytcht_es8316.c | 6 +- sound/soc/intel/boards/bytcht_nocodec.c | 6 +- sound/soc/intel/boards/bytcr_rt5640.c | 6 +- sound/soc/intel/boards/bytcr_rt5651.c | 6 +- sound/soc/intel/boards/bytcr_wm5102.c | 6 +- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 6 +- sound/soc/intel/boards/cht_bsw_nau8824.c | 6 +- sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +- sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +- sound/soc/intel/boards/cml_rt1011_rt5682.c | 15 ++-- sound/soc/intel/boards/ehl_rt5660.c | 14 ++- sound/soc/intel/boards/glk_rt5682_max98357a.c | 30 +++---- sound/soc/intel/boards/hsw_rt5640.c | 10 +-- sound/soc/intel/boards/kbl_da7219_max98357a.c | 26 +++--- sound/soc/intel/boards/kbl_da7219_max98927.c | 54 +++++------- sound/soc/intel/boards/kbl_rt5660.c | 18 ++-- sound/soc/intel/boards/kbl_rt5663_max98927.c | 44 +++++----- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 22 ++--- sound/soc/intel/boards/skl_hda_dsp_common.c | 14 ++- .../soc/intel/boards/skl_nau88l25_max98357a.c | 26 +++--- sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 26 +++--- sound/soc/intel/boards/skl_rt286.c | 26 +++--- sound/soc/intel/boards/sof_board_helpers.c | 13 +-- sound/soc/intel/boards/sof_es8336.c | 8 +- sound/soc/intel/boards/sof_pcm512x.c | 8 +- sound/soc/intel/boards/sof_sdw.c | 4 +- sound/soc/intel/boards/sof_wm8804.c | 2 - sound/soc/mediatek/mt2701/mt2701-cs42448.c | 20 ++--- sound/soc/mediatek/mt2701/mt2701-wm8960.c | 6 +- sound/soc/mediatek/mt6797/mt6797-mt6351.c | 24 ++--- sound/soc/mediatek/mt7986/mt7986-wm8960.c | 6 +- sound/soc/mediatek/mt8173/mt8173-max98090.c | 6 +- .../mediatek/mt8173/mt8173-rt5650-rt5514.c | 6 +- .../mediatek/mt8173/mt8173-rt5650-rt5676.c | 10 +-- sound/soc/mediatek/mt8173/mt8173-rt5650.c | 10 +-- .../mediatek/mt8183/mt8183-da7219-max98357.c | 34 +++----- .../mt8183/mt8183-mt6358-ts3a227-max98357.c | 34 +++----- .../mt8186/mt8186-mt6366-da7219-max98357.c | 86 +++++++----------- .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 86 +++++++----------- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 58 ++++++------- .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 78 ++++++++--------- sound/soc/mediatek/mt8195/mt8195-mt6359.c | 60 +++++++------ sound/soc/meson/axg-card.c | 9 +- sound/soc/meson/gx-card.c | 1 - sound/soc/meson/meson-card-utils.c | 4 +- sound/soc/qcom/common.c | 1 - sound/soc/samsung/odroid.c | 11 ++- sound/soc/soc-compress.c | 10 ++- sound/soc/soc-core.c | 20 +---- sound/soc/soc-dai.c | 38 -------- sound/soc/soc-pcm.c | 87 ++++++++----------- sound/soc/soc-topology-test.c | 2 - sound/soc/soc-topology.c | 4 +- sound/soc/sof/nocodec.c | 4 - 91 files changed, 502 insertions(+), 863 deletions(-)