Message-ID: <6855763c-0230-4535-a603-343059de5202@quicinc.com>
Date: Tue, 13 Aug 2024 15:57:44 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <corbet@....net>,
        <broonie@...nel.org>, <lgirdwood@...il.com>, <krzk+dt@...nel.org>,
        <Thinh.Nguyen@...opsys.com>, <bgoswami@...cinc.com>, <tiwai@...e.com>,
        <gregkh@...uxfoundation.org>, <robh@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <alsa-devel@...a-project.org>
Subject: Re: [PATCH v24 09/34] ASoC: Add SOC USB APIs for adding an USB
 backend

Hi Pierre,

On 8/1/2024 11:26 PM, Pierre-Louis Bossart wrote:
>
> On 8/1/24 23:43, Wesley Cheng wrote:
>> Hi Pierre,
>>
>> On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote:
>>>
>>>> +/**
>>>> + * struct snd_soc_usb_device
>>>> + * @card_idx - sound card index associated with USB device
>>>> + * @pcm_idx - PCM device index associated with USB device
>>>> + * @chip_idx - USB sound chip array index
>>>> + * @num_playback - number of playback streams
>>>> + * @num_capture - number of capture streams
>>> so here we have a clear separation between playback and capture...
>> Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated.
>>
>> I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path.  I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support.  Is it safe to assume that this is the right thinking?  If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below)  Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented.
> I don't think it's as simple as playback only or playback+capture. If
> there is no support for capture, then there is also no support for
> devices with implicit feedback - which uses the capture path. So you
> gradually start drawing a jagged boundary of what is supported and what
> isn't.
>
> My preference would be to add capture in APIs where we can, with TODOs
> added to make sure no one us under any illusion that the code is fully
> tested. But at least some of the basic plumbing will be in place.
>
> Takashi should chime in on this...
>
>>>> + * @list - list head for SoC USB devices
>>>> + **/
>>>> +struct snd_soc_usb_device {
>>>> +	int card_idx;
>>>> +	int pcm_idx;
>>>> +	int chip_idx;
>>>> +	int num_playback;
>>>> +	int num_capture;
>>>> +	struct list_head list;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct snd_soc_usb
>>>> + * @list - list head for SND SOC struct list
>>>> + * @component - reference to ASoC component
>>>> + * @num_supported_streams - number of supported concurrent sessions
>>> ... but here we don't. And it's not clear what the working 'sessions'
>>> means in the comment.

After taking a look at this "num_supported_streams" naming a bit more, I wanted to check with you to see adds to the complexity of the terminology being used across soc-usb.

The intention of this is to define how many concurrent USB devices the USB backend can support.  So for example, if the audio DSP did support multiple USB devices at the same time, this would denote that.  This is where I wanted to make sure the terminology was right....  So in this case, to me, it makes more sense if num_supported_streams --> num_supported_devices, because it determines how many USB devices the ASoC USB backend DAI can manage/support.  This adds a bit to the reason why I think using the term "port" for explaining the SOC USB context is reasonable.

Thanks

Wesley Cheng

>>>> + * @connection_status_cb - callback to notify connection events
>>>> + * @priv_data - driver data
>>>> + **/
>>>> +struct snd_soc_usb {
>>>> +	struct list_head list;
>>>> +	struct snd_soc_component *component;
>>>> +	unsigned int num_supported_streams;
>>>> +	int (*connection_status_cb)(struct snd_soc_usb *usb,
>>>> +			struct snd_soc_usb_device *sdev, bool connected);
>>>> +	void *priv_data;
>>>> +};
>>>> +/**
>>>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device
>>> USB port?
>> Noted, refer to the last comment.
>>>> + * @component: USB DPCM backend DAI component
>>>> + * @num_streams: number of offloading sessions supported
>>> same comment, is this direction-specific or not?
>> Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely.
>>>> + * @data: private data
>>>> + *
>>>> + * Allocate and initialize a SOC USB device.  This will populate parameters that
>>>> + * are used in subsequent sequences.
>>>> + *
>>>> + */
>>>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component,
>>>> +					      int num_streams, void *data)
>>>> +{
>>>> +	struct snd_soc_usb *usb;
>>>> +
>>>> +	usb = kzalloc(sizeof(*usb), GFP_KERNEL);
>>>> +	if (!usb)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	usb->component = component;
>>>> +	usb->priv_data = data;
>>>> +	usb->num_supported_streams = num_streams;
>>>> +
>>>> +	return usb;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port);
>>>> +
>>>> +/**
>>>> + * snd_soc_usb_free_port() - free a SOC USB device
>>>> + * @usb: allocated SOC USB device
>>>> +
>>>> + * Free and remove the SOC USB device from the available list of devices.
>>> Now I am lost again on the device:port relationship. I am sure you've
>>> explained this before but I forget things and the code isn't
>>> self-explanatory.
>>>
>> Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation.  Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port.  If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC?  I will use consistent wording throughout SOC USB if so.
> I am not sure USB uses 'port' at all. If by 'port' you meant 'connector'
> it's not quite right, USB audio works across hubs.
>
>

Powered by blists - more mailing lists