Re: [RFC PATCH 04/14] sound: usb: card: Introduce USB SND vendor op callbacks

Hi,

On Sat, 24 Dec 2022 at 01:33, Wesley Cheng quic_wcheng@quicinc.com wrote:

Allow for different vendors to be notified on USB SND connect/disconnect seqeunces. This allows for vendor USB SND modules to properly initialize and populate internal structures with references to the USB SND chip device.

The commit message definitely needs some improvement. We do not notify vendors on SND connect/disconnect events.

Signed-off-by: Wesley Cheng quic_wcheng@quicinc.com

sound/usb/card.c | 22 ++++++++++++++++++++++ sound/usb/card.h | 7 +++++++ 2 files changed, 29 insertions(+)

diff --git a/sound/usb/card.c b/sound/usb/card.c index 26268ffb8274..212f55a7683c 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -117,6 +117,21 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no) static DEFINE_MUTEX(register_mutex); static struct snd_usb_audio *usb_chip[SNDRV_CARDS]; static struct usb_driver usb_audio_driver; +static struct snd_usb_vendor_ops *vendor_ops;

+int snd_usb_register_vendor_ops(struct snd_usb_vendor_ops *ops)

platform ops?

+{

•   vendor_ops = ops;
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(snd_usb_register_vendor_ops);

What happens if several platforms try to register different ops? I saw from the patch 09/14 that you register these ops unconditionally. If other devices follow your approach there is an obvious conflict.

+int snd_usb_unregister_vendor_ops(void) +{

•   vendor_ops = NULL;
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(snd_usb_unregister_vendor_ops);

/*

• disconnect streams

@@ -910,6 +925,10 @@ static int usb_audio_probe(struct usb_interface *intf, usb_set_intfdata(intf, chip); atomic_dec(&chip->active); mutex_unlock(&register_mutex);

•   if (vendor_ops->connect_cb)
  

•           vendor_ops->connect_cb(intf, chip);
  

•   return 0;
  
  

  __error:

@@ -943,6 +962,9 @@ static void usb_audio_disconnect(struct usb_interface *intf) if (chip == USB_AUDIO_IFACE_UNUSED) return;

•   if (vendor_ops->disconnect_cb)
  

•           vendor_ops->disconnect_cb(intf);
  

•   card = chip->card;
  
    mutex_lock(&register_mutex);
  

diff --git a/sound/usb/card.h b/sound/usb/card.h index 40061550105a..a785bb256b0d 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -206,4 +206,11 @@ struct snd_usb_stream { struct list_head list; };

+struct snd_usb_vendor_ops {

•   void (*connect_cb)(struct usb_interface *intf, struct snd_usb_audio *chip);
  

•   void (*disconnect_cb)(struct usb_interface *intf);
  

+};

+int snd_usb_register_vendor_ops(struct snd_usb_vendor_ops *ops); +int snd_usb_unregister_vendor_ops(void); #endif /* __USBAUDIO_CARD_H */

-- With best wishes

Dmitry