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

[Takashi Iwai]

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