Re: [alsa-devel] [PATCH] ALSA: usb-audio: Add quirk for MOTU MicroBook II

On Wed, 27 Feb 2019 15:26:01 +0100, Manuel Reinhardt wrote:

Add an entry to the quirks-table to for usb-audio to recognize the Microbook II (although it only exposes vendor interfaces). A simple boot quirk is also implemented to set up the sample rate and make sure that no audio urbs are sent before the device is ready.

This patch only provides audio playback and capture at 96kHz sample rate. Notice the following shortcomings:

• The sample rate is currently hardcoded to 96k although the device also supports 48k and 44.1k.

• The various mixer controls of the MicroBook are not made available.

• The keep-iface control should be on by default because the device shuts down whenever the altsetting is reset which is usually unwanted. (I don't know the best way to do this)

Can be set either in the quirk callback you write. Or it'd be not bad to create a white list, too.

+static int snd_usb_motu_microbookii_boot_quirk(struct usb_device *dev) +{

• int err, actual_length, poll_attempts = 0;
• bool setup_finished = false;
• static const u8 set_samplerate_seq[] = { 0x00, 0x00, 0x00, 0x00,

• 				 0x00, 0x00, 0x0b, 0x14,
  

• 				 0x00, 0x00, 0x00, 0x01 };
  

• static const u8 poll_ready_seq[] = { 0x00, 0x04, 0x00, 0x00,

• 			     0x00, 0x00, 0x0b, 0x18 };
  

• u8 *buf = kzalloc(MICROBOOK_BUF_SIZE, GFP_KERNEL);
• if (!buf)

• return -ENOMEM;
  

• dev_info(&dev->dev, "Waiting for MOTU Microbook II to boot up...\n");
• /* First we tell the device which sample rate to use. */
• memcpy(buf, set_samplerate_seq, ARRAY_SIZE(set_samplerate_seq));

This is about byte-size, so better to use sizeof(). ARRAY_SIZE() confuses readers as if he needs to care about the type.

• actual_length = ARRAY_SIZE(set_samplerate_seq);

Ditto here. Also some other codes below, too.

• err = snd_usb_motu_microbookii_communicate(dev, buf, MICROBOOK_BUF_SIZE,

• 				   &actual_length);
  

• if (err < 0) {

• kfree(buf);
  

• dev_err(&dev->dev,
  

• 	"failed setting the sample rate for Motu MicroBook II: %d\n",
  

• 	err);
  

• return err;
  

Since kfree() is mandatory in the error path, it's better to use goto and kfree() there, instead of spreading over all places.

• }
• /* Then we poll every 100 ms until the device informs of its readiness. */
• while (!setup_finished) {

• if (++poll_attempts > 100) {
  

• 	kfree(buf);
  

• 	dev_err(&dev->dev,
  

• 		"failed booting Motu MicroBook II: timeout\n");
  

• 	return -ENODEV;
  

• }
  

• memset(buf, 0, MICROBOOK_BUF_SIZE);
  

• memcpy(buf, poll_ready_seq, ARRAY_SIZE(poll_ready_seq));
  

• actual_length = ARRAY_SIZE(poll_ready_seq);
  

• err = snd_usb_motu_microbookii_communicate(
  

• 	dev, buf, MICROBOOK_BUF_SIZE, &actual_length);
  

• if (err < 0) {
  

• 	kfree(buf);
  

• 	dev_err(&dev->dev,
  

• 		"failed booting Motu MicroBook II: communication error\n");
  

• 	return err;
  

• }
  

• if (buf[actual_length - 1] == 1)
  

• 	setup_finished = true;
  

I wouldn't trust the returned value. Let's be paranoid and check whether the actual_length is within the expected range.

• msleep(100);
  

BTW, setup_finished is superfluous. You can just write like

msleep(100); if (buf[actual_lenght - 1] == 1) break;

Or if 100ms sleep isn't needed in the successful case, it can be

if (buf[actual_lenght - 1] == 1) break; msleep(100);

thanks,

Takashi