Re: [PATCH v3 01/23] ASoC: soc-pcm: cleanup soc_get_playback_capture()

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

Hi Pierre-Louis

> > (X) part is for DPCM, and it checks playback/capture availability
> > if dai_link has dpcm_playback/capture flag (a)(b).
> > This availability check should be available not only for DPCM, but for
> > all connections. But it is not mandatory option. Let's name it as
> > assertion.
> 
> I don't follow the 'not mandatory option'. Why not make these
> 'assertions' mandatory? What happens in case the the option is not present?

The big reason why "assertion flag" is not mandatory is that non-DPCM doesn't
have such flag before. I can't add such flags to all of non-DPCM,
because I don't know which direction (playback/capture) is available on
each DAIs.

> > In case of having assertion flag, non specific side will be disabled.
> 
> Not following the wording, multiple negatives and not clear on what
> 'side' refers to (direction or DPCM/non-DPCM).

How about this ?

	If either playback or capture assertion flag was presented,
	not presented direction will be disabled by ASoC even if
	it was available.

> I see a contradiction between "expanding the assertion to all
> connections" and "not mandatory".
(snip)
> again the 'not mandatory' and 'non specific side will be disabled' are
> confusing.
(snip)
> > +	/*
> > +	 * Assertion check
> > +	 *
> > +	 * playback_assertion = 0	No assertion check.
> > +	 * capture_assertion  = 0	ASoC will use detected playback/capture as-is.
> > +	 *				No playback, No capture will be error.
> 
> did you mean "this combination will be handled as an error" ?

Hmm... is it word selection issue ?? is "must_support" better ?

(playback_xxx, capture_xxx)

(0, 0) : Both are not must item. available direction is used as-is.
         But it will be error if nothing was available.

(1, 0) : DAI must support selected direction.
(0, 1)   Not selected direction will be disabled even though it was available

(1, 1) : Both must be supported.

> It's probably best to have a different presentation, to avoid
> confusions. Using multiple lines without a separator isn't great.
> 
> Suggested example:
> 
> playback_assertion = 0 capture_assertion  = 0
> this combination will be handled as an error
> 
> playback_assertion = 1 capture_assertion  = 0
> the DAIs in this dailink must support playback.
> ASoC will disable capture.
> In other words "playback_only"

Yeah, I agree

> > +	 * playback_assertion = 1	DAI must playback available. ASoC will disable capture.
> > +	 * capture_assertion  = 0	In other words "playback_only"
> > +	 *
> > +	 * playback_assertion = 0	DAI must capture available. ASoC will disable playback.
> > +	 * capture_assertion  = 1	In other words "capture_only"
> > +	 *
> > +	 * playback_assertion = 1	DAI must both playback/capture available.
> > +	 * capture_assertion  = 1
> 
> nit-pick: the 'must X available' does not read well, 'must support X' is
> probably what you meant.

Thanks. will fix in v4

> > +	if (dai_link->capture_assertion) {
> > +		if (!has_capture) {
> > +			dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name);
> > +			return -EINVAL;
> > +		}
> > +		/* makes it capture only */
> > +		if (!dai_link->playback_assertion)
> > +			has_playback = 0;
> > +	}
> 
> we probably want a dev_ log when the has_playback/capture is overridden?

OK, will do in v4


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto