Re: [PATCH v3 7/7] ASoC: Intel: avs: SoundWire PCM BE operations

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 11/6/2026 1:55 PM, Pierre-Louis Bossart wrote:
> I am afraid this patch needs to be completely re-visited. You mixed dai
> and dailink layers, and that explains most of the misunderstandings in
> the review of the PDI stuff where I mentioned that what matters is how
> the dailink is created.
>> +static int avs_sdw_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>> +	struct sdw_stream_runtime *sdw_stream;
>> +	struct snd_soc_acpi_mach *mach;
>> +	struct snd_soc_dai *codec_dai;
>> +	struct avs_dma_data *data;
>> +	struct sdw_cdns_pdi *pdi;
>> +	struct sdw_slave *codec;
>> +	int dir, ret;
>> +
>> +	mach = dev_get_platdata(dai->component->card->dev);
>> +	codec = mach->pdata;
>> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>> +		dir = SDW_DATA_DIR_RX;
>> +	else
>> +		dir = SDW_DATA_DIR_TX;
>> +
>> +	ret = avs_dai_startup(substream, dai);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sdw_stream = sdw_alloc_stream(substream->name, SDW_STREAM_PCM);
>> +	if (!sdw_stream) {
>> +		ret = -ENOMEM;
>> +		goto err_alloc_stream;
>> +	}
> 
> This isn't quite the intented use of the SoundWire stream API, which
> provides a sdw_startup_stream() that's supposed to be invoked during the
> dailink creation in the machine driver that binds cpu and codec dais
> 
> "
>  sdw_alloc_stream should be called only once per stream. Typically
>  invoked from ALSA/ASoC machine/platform driver.
> "
> 
> 
> The dailink maps exactly 1:1 to the SoundWire stream concept, and that's
> what allows us to deal with aggregation - when multiple cpu_dais on
> multiple links are connected to multiple amps. That's the #1 reason why
> SoundWire exists and the #1 difference to other audio interfaces.


dai_link->startup() occurs at pcm->open(), directly before
dai->startup().  The binding process relates to card-binding and
FrontEnd exposition, at least that's my understanding of the subject.

Now, thank you for mentioning the intention.  Did not understand the
point before that.  However, given the above, plus the fact
dai->shutdown() frees the stream object, I see no difference from
functional perspective - allocate a stream object once per runtime, free
it when runtime is about to be destroyed.  The exact same thing happens
here.  For avs-driver, the machine boards are minimal.  Dai-link
operations tend to be used to patch up stuff between codec <> platform
expectations.  The intention is to make populating the configurations
easier _and_ have one location for the PCM operations - everything
happens in pcm.c file -or- in case of SDW, pcm_sdw.c

> BPT/BRA support is the only case where sdw_alloc_stream() is used in the
> Intel platform driver, since there's clearly no concept of cpu_ or codec
> DAI.

No intention to support that (BRA) for automotive right now.

>> +
>> +	pdi = sdw_cdns_assign_pdi(codec->bus, dir);
>> +	if (!pdi) {
>> +		ret = -EBUSY;
>> +		goto err_assign_pdi;
>> +	}
>> +
>> +	/* Fulfill the codec needs by setting the sdw_stream. */
>> +	codec_dai = snd_soc_rtd_to_codec(rtd, 0);
>> +	snd_soc_dai_set_stream(codec_dai, sdw_stream, substream->stream);
> 
> That absolutely does not scale to the case where you have multiple
> cpu_dais and multiple codec_dais.
> 
> Please take a look at the sof_sdw machine driver (same solution used by
> AMD), and please use the sdw_utils library to deal with all the stream
> management.


I believe you are talking about aggregation.  No intention to support
that with this code.

After reading the read I have an impression that the expectation of this
patchset is: full brown SoundWire support.  I wish I could agree with
you.  The reality is: standard PCM over codecs with and without SDCA.
If I get a user and an external release on top of that, I'll come back
and revisit.  The code present here provides simple implementation of
the goal, basically mirroring what's done for HDA in pcm.c nicely making
it easy to follow.

>> +	data = snd_soc_dai_get_dma_data(dai, substream);
>> +	data->sdw_stream = sdw_stream;
>> +	data->pdi = pdi;
>> +	return 0;
>> +
>> +err_assign_pdi:
>> +	sdw_release_stream(sdw_stream);
>> +err_alloc_stream:
>> +	avs_dai_shutdown(substream, dai);
>> +	return ret;
>> +}
>