Re: [PATCH 10/10] ALSA: usb-audio: UAC2: support read-only freq control

[Torstein Hegge <hegge@resisty.net>]

On Sun, Mar 31, 2013 at 17:52:32 +0200, Eldad Zack wrote:
> Some clocks might be read-only, e.g., external clocks (see also
> UAC2 4.7.2.1).
> 
> In this case, setting the sample frequency will always fail
> (even if the rate is equal to the current clock rate),
> therefore do not write, but read the value and compare to the
> requested rate.
> 
> If it doesn't match, return -ENXIO since the clock is invalid for
> this configuration.

I think could be more readable if it was built on top of [1]. Then it
could check the target rate against the prev_rate reported by the device
and return before the sample rate set, something like:

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 9e2703a..395327c 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -254,18 +254,14 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 	struct usb_device *dev = chip->dev;
 	unsigned char data[4];
 	int err, cur_rate, prev_rate;
-	int clock = snd_usb_clock_find_source(chip, fmt->clock);
+	int clock;
+	bool writeable;
+	struct uac_clock_source_descriptor *cs_desc;
 
+	clock = snd_usb_clock_find_source(chip, fmt->clock, true);
 	if (clock < 0)
 		return clock;
 
-	if (!uac_clock_source_is_valid(chip, clock)) {
-		/* TODO: should we try to find valid clock setups by ourself? */
-		snd_printk(KERN_ERR "%d:%d:%d: clock source %d is not valid, cannot use\n",
-			   dev->devnum, iface, fmt->altsetting, clock);
-		return -ENXIO;
-	}
-
 	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
 			      UAC2_CS_CONTROL_SAM_FREQ << 8,
@@ -279,6 +275,20 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 		prev_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24);
 	}
 
+	cs_desc = snd_usb_find_clock_source(chip->ctrl_intf, clock);
+	writeable = uac2_control_is_writeable(cs_desc->bmControls, UAC2_CS_CONTROL_SAM_FREQ - 1);
+	if (!writeable) {
+		if (prev_rate != rate) {
+			snd_printk(KERN_WARNING
+				"%d:%d:%d: freq mismatch (RO clock): req %d, clock runs @%d\n",
+				dev->devnum, iface, fmt->altsetting, rate, crate);
+			return -ENXIO;
+		}
+		else {
+			return 0;
+		}
+	}
+
 	data[0] = rate;
 	data[1] = rate >> 8;
 	data[2] = rate >> 16;

+ the other changes from the rest of your patch series.

[1] http://thread.gmane.org/gmane.linux.alsa.devel/106773

> +		err = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC2_CS_CUR,
> +					   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
> +					   UAC2_CS_CONTROL_SAM_FREQ << 8,
> +					   snd_usb_ctrl_intf(chip) | (clock << 8),
> +					   &data, sizeof(data));

Is the indentation here intentional?

> -	if ((err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
> +	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
>  				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
>  				   UAC2_CS_CONTROL_SAM_FREQ << 8,
>  				   snd_usb_ctrl_intf(chip) | (clock << 8),
> -				   &data, sizeof(rate))) < 0) {
> +				   &data, sizeof(rate));

Same formatting comment as above.

> +	if (crate != rate) {
> +		if (!writeable) {
> +			snd_printk(KERN_WARNING "%d:%d:%d: freq mismatch (RO clock): req %d, clock runs @%d\n",
> +				dev->devnum, iface, fmt->altsetting, rate, crate);
> +			return -ENXIO;
> +		}
>  		snd_printd(KERN_WARNING "current rate %d is different from the runtime rate %d\n", crate, rate);
> +	}

This bit didn't read very well on a narrow terminal.


Torstein