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

[Pierre-Louis Bossart]

>>> +
>>> +	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.
well, I find it interesting that you are suggesting changes that impact
existing to SoundWire *products* for what appears to be an experimental
setup that will have to be changed *again* if and when there's a user.

It's ok to push background changes ahead of time to prepare the future.
I am not sure what to think of your approach.

a) Mirroring HDaudio is not the goal, if that comes with a substandard
way of using the SoundWire subsystem and capabilities does all this
belong in the mainline? My take is that support for two amps is a
standard feature, be they on the same link or not.
b) At the very least you should have disclosed your goals, limitations
of your approach, and be mindful of reviewer time.