Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>,
	alsa-devel@alsa-project.org, tiwai@suse.de,
	Daniel Baluta <daniel.baluta@gmail.com>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Mark Brown <broonie@kernel.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Bard Liao <yung-chuan.liao@linux.intel.com>
Subject: Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks
Date: Tue, 16 Jun 2020 11:32:22 -0500	[thread overview]
Message-ID: <7cbc9233-e5f2-03e0-5659-cf22dea75e53@linux.intel.com> (raw)
In-Reply-To: <20200616155544.GA8891@gerhold.net>

On 6/16/20 10:55 AM, Stephan Gerhold wrote:
> On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> On 6/16/20 9:52 AM, Mark Brown wrote:
>>> On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
>>>> On 6/16/20 4:02 AM, Stephan Gerhold wrote:
>>>>> On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
>>>
>>>>>> For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture
>>>>>> appropriately because it is basically only used with one particular
>>>>>> DAI driver. But simple-audio-card is generic and used with many
>>>>>> different drivers so hard-coding a call into some other driver like
>>>>>> Srinivas did above won't work in that case.
>>>
>>>> Doesn't simple-card rely on DT blobs that can also be updated?
>>>
>>> DT is an ABI just like ACPI - it's just more featureful.  Many systems
>>> can easily update their DTs but not all of them and users don't always
>>> want to try to keep it in lock step with the kernel.  Stuff like this is
>>> why I've been dubious about putting DPCM things in there, it's too much
>>> of a hard coding of internal APIs.
>>
>> ok, but is there any actual use of dpcm_playback/capture outside of C code?
>>
>> simple-card.c and audio-graph-card do hard-code but that's done with C in
>> the driver:
>>
>> 	ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep,
>> 				       NULL, &dai_link->dai_fmt);
>> 	if (ret < 0)
>> 		goto out_put_node;
>>
>> 	dai_link->dpcm_playback		= 1;
>> 	dai_link->dpcm_capture		= 1;
>>
>>
>> that that should be fixed based on the DAI format used in that dai_link - in
>> other words we can make sure the capabilities of the dailink are aligned
>> with the dais while parsing the DT blobs.
> 
> But how do you know which capabilities to set? The device tree doesn't
> tells us that. We could add some code to look up the snd_soc_dai_driver
> early, based on the references in the device tree (basically something
> like snd_soc_of_get_dai_name(), see
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-core.c#n2988)
> 
> At least to me that function doesn't exactly look trivial though,
> and that's just to properly fill in the dpcm_playback/capture
> parameters. Essentially those parameters only complicate the current
> device tree use case,  where you want the DAI link to be for both
> playback/capture, but restricted to the capabilities of the DAI.
> 
> Just wondering if setting up dpcm_playback/capture properly is worth it
> at all in this case. This isn't necessary for the non-DPCM case either,
> there we automatically set it based on the DAI capabilities.

We can add a simple loop for each direction that relies on
snd_soc_dai_stream_valid() to identify if each DAI is capable of doing 
playback/capture.