[alsa-devel] [PATCH] teac ud-501/ud-503 usb delay

[Takashi Iwai]

On Sun, 24 Jan 2016 17:36:17 +0100,
Guillaume Fougnies wrote:
> 
> 
> Hello,
> 
> This is a small patch against sound/usb/quirks.c to correct "Teac
> Corp." products delay.
> Tested on Teac UD-501 and UD-503 products, could not harm other
> Teac products.
> It corrects the "clock source 41 is not valid, cannot use" error
> for both devices.
> And more critically on the new UD-503, a complete freeze of the
> teac usb interface when switching between different rate/format.
> (Freeze only solved by a power switch of the device...)
> 
> The patch matches the problem of the "Playback Design" products. I just
> refactored a bit the code to include Teac products properly.

The changes look almost good to me.  Could you fix small nitpick below
and resubmit with a proper changelog, and most importantly, with your
sign-off, in order to merge to upstream?

> 
> Regards,
> Guillaume
> 
> 
> ---
>  sound/usb/quirks.c | 58 +++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index 23ea6d8..f3faaef 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -1202,40 +1202,48 @@ void snd_usb_endpoint_start_quirk(struct snd_usb_endpoint *ep)
>  void snd_usb_set_interface_quirk(struct usb_device *dev)
>  {
>  	/*
> -	 * "Playback Design" products need a 50ms delay after setting the
> +	 * Products needing a 50ms delay after setting the
>  	 * USB interface.
>  	 */
> -	if (le16_to_cpu(dev->descriptor.idVendor) == 0x23ba)
> -		mdelay(50);
> +	switch (le16_to_cpu(dev->descriptor.idVendor)) {
> +		case 0x23ba: /* Playback Design */
> +		case 0x0644: /* TEAC Corp. */

In the kernel code, case is has the same indent level as switch.

> +			mdelay(50);
> +	}

Put break after mdelay() call as the common practice.


>  }
>  
>  void snd_usb_ctl_msg_quirk(struct usb_device *dev, unsigned int pipe,
>  			   __u8 request, __u8 requesttype, __u16 value,
>  			   __u16 index, void *data, __u16 size)
>  {
> -	/*
> -	 * "Playback Design" products need a 20ms delay after each
> -	 * class compliant request
> -	 */
> -	if ((le16_to_cpu(dev->descriptor.idVendor) == 0x23ba) &&
> -	    (requesttype & USB_TYPE_MASK) == USB_TYPE_CLASS)
> -		mdelay(20);
> -
> -	/* Marantz/Denon devices with USB DAC functionality need a delay
> -	 * after each class compliant request
> -	 */
> -	if (is_marantz_denon_dac(USB_ID(le16_to_cpu(dev->descriptor.idVendor),
> -					le16_to_cpu(dev->descriptor.idProduct)))
> -	    && (requesttype & USB_TYPE_MASK) == USB_TYPE_CLASS)
> -		mdelay(20);
> +	if ((requesttype & USB_TYPE_MASK) == USB_TYPE_CLASS) {
> +		switch (le16_to_cpu(dev->descriptor.idVendor)) {
> +			/*
> +			 * "Playback Design"/"TEAC Corp." products need a 20ms delay after each
> +			 * class compliant request
> +			 */
> +			case 0x23ba: /* "Playback Design" */
> +			case 0x0644: /* TEAC Corp. */
> +				mdelay(20);
> +				return;
> +
> +				/* Zoom R16/24 needs a tiny delay here, otherwise requests like
> +				 * get/set frequency return as failed despite actually succeeding.
> +				 */
> +			case 0x1686:
> +				/* restrict to 0x00dd */
> +				if (le16_to_cpu(dev->descriptor.idProduct) == 0x00dd)
> +					mdelay(1);
> +				return;
> +		}
>  
> -	/* Zoom R16/24 needs a tiny delay here, otherwise requests like
> -	 * get/set frequency return as failed despite actually succeeding.
> -	 */
> -	if ((le16_to_cpu(dev->descriptor.idVendor) == 0x1686) &&
> -	    (le16_to_cpu(dev->descriptor.idProduct) == 0x00dd) &&
> -	    (requesttype & USB_TYPE_MASK) == USB_TYPE_CLASS)
> -		mdelay(1);
> +		/* Marantz/Denon devices with USB DAC functionality need a delay
> +		 * after each class compliant request
> +		 */
> +		if (is_marantz_denon_dac(USB_ID(le16_to_cpu(dev->descriptor.idVendor),
> +						le16_to_cpu(dev->descriptor.idProduct))))
> +			mdelay(20);
> +	}

In this case, I prefer keeping the old code as is.
Just add the check for your device there in a new if block.


thanks,

Takashi