diff mbox series

[v2,09/16] ASoC: soc-core: Replace dpcm_playback/capture to playback/capture_only

Message ID 87msqdudn7.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series ASoC: Replace dpcm_playback/capture to playback/capture_only | expand

Commit Message

Kuninori Morimoto April 1, 2024, 12:31 a.m. UTC 
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(-)

Comments

Pierre-Louis Bossart April 1, 2024, 4:22 p.m. UTC | #1 
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.
Kuninori Morimoto April 1, 2024, 11:27 p.m. UTC | #2 
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
Pierre-Louis Bossart April 2, 2024, 2:09 p.m. UTC | #3 
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.
Kuninori Morimoto April 4, 2024, 2:04 a.m. UTC | #4 
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 mbox series

Patch

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