Re: [PATCH v4 06/32] ASoC: Add SOC USB APIs for adding an USB backend

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Pierre,

On 7/25/2023 1:10 AM, Pierre-Louis Bossart wrote:
> 
>> +/**
>> + * struct snd_soc_usb
>> + * @list - list head for SND SOC struct list
>> + * @dev - USB backend device reference
>> + * @component - reference to DAPM component
> 
> ASoC component, not DAPM.
> 

Thanks for the detailed review. Will fix this.

>> + * @connection_status_cb - callback to notify connection events
>> + * @priv_data - driver data
>> + **/
>> +struct snd_soc_usb {
>> +	struct list_head list;
>> +	struct device *dev;
>> +	struct snd_soc_component *component;
>> +	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
>> +				int connected);
> 
> It's not clear what 'connected' really refers to, is this a boolean
> really or is this a "connection_event?
> 
> 

I'll change it to boolean for now, since its currently only 
connected/disconnected.  If more states are required, then we can add 
that in the future.

>> +	void *priv_data;
>> +};
>> +
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx);
>> +int snd_soc_usb_disconnect(struct device *usbdev);
>> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv);
> 
> this function is not part of this patch, is this intentional to have a
> get but not a set?
> 
>> +void *snd_soc_usb_get_priv_data(struct device *usbdev);
> 
> you are using 'usbdev' and 'dev' for the same type of parameters, why
> not align on one set of definition with a consistent naming.
> 
> 

I'll take a look at this and see what happened.  I think Greg mentioned 
something similar and I made the change to remove the set priv, and 
moved it elsewhere.

>> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +	struct snd_soc_usb *ctx = NULL;
> 
> this init doesn't seem required?
> 

Yes, not needed.

>> +
>> +	node = snd_soc_find_phandle(dev);
>> +	if (IS_ERR(node))
>> +		return NULL;
>> +
>> +	mutex_lock(&ctx_mutex);
>> +	list_for_each_entry(ctx, &usb_ctx_list, list) {
>> +		if (ctx->dev->of_node == node) {
>> +			of_node_put(node);
>> +			mutex_unlock(&ctx_mutex);
>> +			return ctx;
>> +		}
>> +	}
>> +	of_node_put(node);
>> +	mutex_unlock(&ctx_mutex);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
>> + * @dev: device reference
>> + *
>> + * Fetch the private data stored in the USB SND SOC structure.
>> + *
>> + */
>> +void *snd_soc_usb_get_priv_data(struct device *dev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	ctx = snd_soc_find_usb_ctx(dev);
> 
> so in this function you walk through the usb_ctx_list with locking...
> 
>> +	if (!ctx) {
>> +		/* Check if backend device */
>> +		list_for_each_entry(ctx, &usb_ctx_list, list) {
> 
> ... and here you walk again through the list without locking.
> 
> Something's weird here, if this was intentional you need to add comments.
> 

Yes, needs a lock here.

>> +			if (dev->of_node == ctx->dev->of_node)
>> +				goto out;
>> +		}
>> +		ctx = NULL;
>> +	}
>> +out:
>> +	return ctx ? ctx->priv_data : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
>> +
>> +/**
>> + * snd_soc_usb_add_port() - Add a USB backend port
>> + * @dev: USB backend device
>> + * @priv: private data
>> + * @connection_cb: connection status callback
>> + *
>> + * Register a USB backend device to the SND USB SOC framework.  Memory is
>> + * allocated as part of the USB backend device.
>> + *
>> + */
>> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
>> +			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
>> +			int connected))
>> +{
>> +	struct snd_soc_usb *usb;
>> +
>> +	usb = devm_kzalloc(dev, sizeof(*usb), GFP_KERNEL);
>> +	if (!usb)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	usb->connection_status_cb = connection_cb;
>> +	usb->dev = dev;
>> +	usb->priv_data = priv;
> 
> back to my comment above, you don't seem to need the set_priv_data() ?
> 

And yes, this is where I moved the priv data setting.

>> +
>> +	mutex_lock(&ctx_mutex);
>> +	list_add_tail(&usb->list, &usb_ctx_list);
>> +	mutex_unlock(&ctx_mutex);
>> +
>> +	return usb;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port);
> 
>> +/**
>> + * snd_soc_usb_connect() - Notification of USB device connection
>> + * @usbdev: USB bus device
>> + * @card_idx: USB SND card instance
>> + *
>> + * Notify of a new USB SND device connection.  The card_idx can be used to
>> + * handle how the USB backend selects, which device to enable offloading on.
> 
> "USB backend" is confusing, not sure if this is the same concept as DPCM
> "backend" or something else. Please try to avoid overloaded terms.
> 

Sure, I meant DPCM backend.

>> + *
>> + */
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, card_idx, 1);
> 
> so either the 'connected' value is 1...
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
>> +
>> +/**
>> + * snd_soc_usb_disconnect() - Notification of USB device disconnection
>> + * @usbdev: USB bus device
>> + *
>> + * Notify of a new USB SND device disconnection to the USB backend.
>> + *
>> + */
>> +int snd_soc_usb_disconnect(struct device *usbdev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, -1, 0);
> 
> ...and here it's zero.
> 
> should this 'connected' parameter be a boolean then with true/false
> value, or do you want to add enums/defines for more flexibility down the
> road?
> 

As mentioned above, will switch to boolean for now.  I don't see a need 
to have other enums.

Thanks
Wesley Cheng