Re: [PATCH v2 06/16] ASoC: Intel: Replace dpcm_playback/capture to playback/capture_only

[Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>]

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