Re: [PATCH v24 09/34] ASoC: Add SOC USB APIs for adding an USB backend

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

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.

>> + * @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.
>
>> + * @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.

Thanks

Wesley Cheng

>> + *
>> + */
>> +void snd_soc_usb_free_port(struct snd_soc_usb *usb)
>> +{
>> +	snd_soc_usb_remove_port(usb);
>> +	kfree(usb);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_free_port);
>> +
>> +/**
>> + * snd_soc_usb_add_port() - Add a USB backend port
>> + * @usb: soc usb device to add
>> + *
>> + * Register a USB backend device to the SND USB SOC framework.  Memory is
>> + * allocated as part of the USB backend device.
>> + *
>> + */
>> +void snd_soc_usb_add_port(struct snd_soc_usb *usb)
>> +{
>> +	mutex_lock(&ctx_mutex);
>> +	list_add_tail(&usb->list, &usb_ctx_list);
>> +	mutex_unlock(&ctx_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port);
>> +
>> +/**
>> + * snd_soc_usb_remove_port() - Remove a USB backend port
>> + * @usb: soc usb device to remove
>> + *
>> + * Remove a USB backend device from USB SND SOC.  Memory is freed when USB
>> + * backend is removed.
>> + *
>> + */
>> +void snd_soc_usb_remove_port(struct snd_soc_usb *usb)
>> +{
>> +	struct snd_soc_usb *ctx, *tmp;
>> +
>> +	mutex_lock(&ctx_mutex);
>> +	list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) {
>> +		if (ctx == usb) {
>> +			list_del(&ctx->list);
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&ctx_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);