| Message ID | 87msqdudn7.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: Replace dpcm_playback/capture to playback/capture_only | expand |
On 3/31/24 19:31, 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> > Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/soc-core.c | 20 +------------------- > 1 file changed, 1 insertion(+), 19 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 3ab6626ad680..b168cf642092 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -2000,25 +2000,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) > dai_link->platforms->name = component->name; > > /* convert non BE into BE */ > - if (!dai_link->no_pcm) { > - dai_link->no_pcm = 1; > - > - if (dai_link->dpcm_playback) > - dev_warn(card->dev, > - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", > - dai_link->name); > - if (dai_link->dpcm_capture) > - dev_warn(card->dev, > - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", > - dai_link->name); > - > - /* convert normal link into DPCM one */ > - if (!(dai_link->dpcm_playback || > - dai_link->dpcm_capture)) { > - dai_link->dpcm_playback = !dai_link->capture_only; > - dai_link->dpcm_capture = !dai_link->playback_only; > - } > - } > + dai_link->no_pcm = 1; > > /* > * override any BE fixups Not following this last change either, the code used to be conditional /* convert non BE into BE */ if (!dai_link->no_pcm) { dai_link->no_pcm = 1; and not it's unconditional dai_link->no_pcm = 1; It's not clear to me how this is related to the dpcm_playback/dpcm_capture removal.
Hi Pierre-Louis Thank you for the feedback > > /* convert non BE into BE */ > > - if (!dai_link->no_pcm) { > > - dai_link->no_pcm = 1; > > - > > - if (dai_link->dpcm_playback) > > - dev_warn(card->dev, > > - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", > > - dai_link->name); > > - if (dai_link->dpcm_capture) > > - dev_warn(card->dev, > > - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", > > - dai_link->name); > > - > > - /* convert normal link into DPCM one */ > > - if (!(dai_link->dpcm_playback || > > - dai_link->dpcm_capture)) { > > - dai_link->dpcm_playback = !dai_link->capture_only; > > - dai_link->dpcm_capture = !dai_link->playback_only; > > - } > > - } > > + dai_link->no_pcm = 1; (snip) > It's not clear to me how this is related to the > dpcm_playback/dpcm_capture removal. In my understanding, if "dai_link->no_pcm" was 0, it sets no_pcm and convert setting to BE. If no_pcm was 1, it is BE anyway. So no_pcm will be 1 anyway after this code. And then, dpcm_playback/capture is no longer needed. So it just set no_pcm = 1 here. But am I wrong ?? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/1/24 18:27, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for the feedback > >>> /* convert non BE into BE */ >>> - if (!dai_link->no_pcm) { >>> - dai_link->no_pcm = 1; >>> - >>> - if (dai_link->dpcm_playback) >>> - dev_warn(card->dev, >>> - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", >>> - dai_link->name); >>> - if (dai_link->dpcm_capture) >>> - dev_warn(card->dev, >>> - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", >>> - dai_link->name); >>> - >>> - /* convert normal link into DPCM one */ >>> - if (!(dai_link->dpcm_playback || >>> - dai_link->dpcm_capture)) { >>> - dai_link->dpcm_playback = !dai_link->capture_only; >>> - dai_link->dpcm_capture = !dai_link->playback_only; >>> - } >>> - } >>> + dai_link->no_pcm = 1; > (snip) >> It's not clear to me how this is related to the >> dpcm_playback/dpcm_capture removal. > > In my understanding, if "dai_link->no_pcm" was 0, it sets no_pcm and > convert setting to BE. If no_pcm was 1, it is BE anyway. So no_pcm will > be 1 anyway after this code. > And then, dpcm_playback/capture is no longer needed. > So it just set no_pcm = 1 here. But am I wrong ?? The problem is that the patchset is supposed to be only about removal of flags to align on one set, but then we also have "simplifications" or removal of checks without explanations. It would be far less invasive if we only replaced flags and had iso-functionality. Then we can discuss the merits of simplifications.
Hi Pierre-Louis Thank you for your feedback, but this is also my understanding is not yet 100% > >>> /* convert non BE into BE */ > >>> - if (!dai_link->no_pcm) { > >>> - dai_link->no_pcm = 1; > >>> - > >>> - if (dai_link->dpcm_playback) > >>> - dev_warn(card->dev, > >>> - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", > >>> - dai_link->name); > >>> - if (dai_link->dpcm_capture) > >>> - dev_warn(card->dev, > >>> - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", > >>> - dai_link->name); > >>> - > >>> - /* convert normal link into DPCM one */ > >>> - if (!(dai_link->dpcm_playback || > >>> - dai_link->dpcm_capture)) { > >>> - dai_link->dpcm_playback = !dai_link->capture_only; > >>> - dai_link->dpcm_capture = !dai_link->playback_only; > >>> - } > >>> - } > >>> + dai_link->no_pcm = 1; > > (snip) > >> It's not clear to me how this is related to the > >> dpcm_playback/dpcm_capture removal. > > > > In my understanding, if "dai_link->no_pcm" was 0, it sets no_pcm and > > convert setting to BE. If no_pcm was 1, it is BE anyway. So no_pcm will > > be 1 anyway after this code. > > And then, dpcm_playback/capture is no longer needed. > > So it just set no_pcm = 1 here. But am I wrong ?? > > The problem is that the patchset is supposed to be only about removal of > flags to align on one set, but then we also have "simplifications" or > removal of checks without explanations. Do you mean it need to have/keep the comment on the code ?? And/or what does your "removal of checks" mean ? I understand that patch should have enough explanation, and indeed above code has if (dai_link->dpcm_xxx) checks, but dpcm_xxx are no longer needed in new code. What kind of comment are you requesting to me ? > It would be far less invasive if we only replaced flags and had > iso-functionality. Then we can discuss the merits of simplifications. This was the most difficult comment to understand for me... Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 3ab6626ad680..b168cf642092 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2000,25 +2000,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dai_link->platforms->name = component->name; /* convert non BE into BE */ - if (!dai_link->no_pcm) { - dai_link->no_pcm = 1; - - if (dai_link->dpcm_playback) - dev_warn(card->dev, - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", - dai_link->name); - if (dai_link->dpcm_capture) - dev_warn(card->dev, - "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", - dai_link->name); - - /* convert normal link into DPCM one */ - if (!(dai_link->dpcm_playback || - dai_link->dpcm_capture)) { - dai_link->dpcm_playback = !dai_link->capture_only; - dai_link->dpcm_capture = !dai_link->playback_only; - } - } + dai_link->no_pcm = 1; /* * override any BE fixups