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

[Wesley Cheng]

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