[PATCH 00/10] UAC2: Automatic clock switching

[PATCH 00/10] UAC2: Automatic clock switching

[Eldad Zack]

Hi,

This patch series is intended for-next.

It provides automatic clock switching for UAC2 devices (patches #6-#8).
If the current selected clock is not valid, other clocks will be checked
and the first (indexwise) valid clock will be used.

Patch #8 provides a module flag to turn this logic off, since
in some use cases (e.g., studio work) it might not be desired.
But I think most users want their audio devices to "just work",
so the default is on.

To make the change more obvious, I first moved the validity check
around without changing any logic (#6).

Patch #10 adds support for clocks with read-only sample frequency
control.

All other patches included are trivial clean up patches.

I'd appreciate a review and/or testing this patch series.
This applies to all UAC2 devices, so please let me know if anyone
sees any issue.

Tested with mainline.
Applies against mainline (3.9-rc4), HEAD at 46a1f21a679abaaeae6db9969963dc998c9f1c1c
Applies against Takashi's for-next, HEAD at 4abdbd1c2c1832e7270e546307ffb3e56b286db2

Cheers,

Eldad Zack (10):
  ALSA: usb-audio: convert list_for_each to entry variant
  ALSA: usb-audio: neaten MODULE_DEVICE_TABLE placement
  ALSA: usb-audio: neaten EXPORT_SYMBOLS placement
  ALSA: usb-audio: spelling correction
  ALSA: usb-audio: use endianness macros
  ALSA: usb-audio: UAC2: do clock validity check earlier
  ALSA: usb-audio: UAC2: try to find and switch to valid clock
  ALSA: usb-audio: UAC2: auto clock selection module param
  ALSA: usb-audio: show err in set_sample_rate_v2 debug
  ALSA: usb-audio: UAC2: support read-only freq control

 sound/usb/card.c     |  11 ++--
 sound/usb/clock.c    | 150 ++++++++++++++++++++++++++++++++++++++-------------
 sound/usb/clock.h    |   3 +-
 sound/usb/endpoint.c |  18 +++----
 sound/usb/endpoint.h |   2 +-
 sound/usb/format.c   |   2 +-
 sound/usb/midi.c     |  12 ++---
 sound/usb/pcm.c      |  32 ++++-------
 sound/usb/proc.c     |   7 ++-
 sound/usb/stream.c   |  12 ++---
 sound/usb/usbaudio.h |   1 +
 11 files changed, 155 insertions(+), 95 deletions(-)

-- 
1.8.1.5

[PATCH 01/10] ALSA: usb-audio: convert list_for_each to entry variant

[Eldad Zack]

Change occurances of list_for_each into list_for_each_entry where
applicable.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/card.c     |  4 +---
 sound/usb/endpoint.c |  4 +---
 sound/usb/midi.c     |  5 ++---
 sound/usb/pcm.c      | 30 ++++++++++--------------------
 sound/usb/proc.c     |  7 +++----
 sound/usb/stream.c   | 12 ++++--------
 6 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 2da8ad7..8bab36c 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -645,7 +645,6 @@ void snd_usb_autosuspend(struct snd_usb_audio *chip)
 static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct snd_usb_audio *chip = usb_get_intfdata(intf);
-	struct list_head *p;
 	struct snd_usb_stream *as;
 	struct usb_mixer_interface *mixer;
 
@@ -655,8 +654,7 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
 	if (!PMSG_IS_AUTO(message)) {
 		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
 		if (!chip->num_suspended_intf++) {
-			list_for_each(p, &chip->pcm_list) {
-				as = list_entry(p, struct snd_usb_stream, list);
+			list_for_each_entry(as, &chip->pcm_list, list) {
 				snd_pcm_suspend_all(as->pcm);
 				as->substream[0].need_setup_ep =
 					as->substream[1].need_setup_ep = true;
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 21049b8..b1f687f 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -415,14 +415,12 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 					      struct usb_host_interface *alts,
 					      int ep_num, int direction, int type)
 {
-	struct list_head *p;
 	struct snd_usb_endpoint *ep;
 	int is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
 
 	mutex_lock(&chip->mutex);
 
-	list_for_each(p, &chip->ep_list) {
-		ep = list_entry(p, struct snd_usb_endpoint, list);
+	list_for_each_entry(ep, &chip->ep_list, list) {
 		if (ep->ep_num == ep_num &&
 		    ep->iface == alts->desc.bInterfaceNumber &&
 		    ep->alt_idx == alts->desc.bAlternateSetting) {
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 34b9bb7..1cf943d 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1465,10 +1465,9 @@ static void snd_usbmidi_rawmidi_free(struct snd_rawmidi *rmidi)
 static struct snd_rawmidi_substream *snd_usbmidi_find_substream(struct snd_usb_midi* umidi,
 								int stream, int number)
 {
-	struct list_head* list;
+	struct snd_rawmidi_substream *substream;
 
-	list_for_each(list, &umidi->rmidi->streams[stream].substreams) {
-		struct snd_rawmidi_substream *substream = list_entry(list, struct snd_rawmidi_substream, list);
+	list_for_each_entry(substream, &umidi->rmidi->streams[stream].substreams, list) {
 		if (substream->number == number)
 			return substream;
 	}
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index f94397b..e42fc19 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -94,13 +94,11 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
  */
 static struct audioformat *find_format(struct snd_usb_substream *subs)
 {
-	struct list_head *p;
+	struct audioformat *fp;
 	struct audioformat *found = NULL;
 	int cur_attr = 0, attr;
 
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
-		fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!(fp->formats & (1uLL << subs->pcm_format)))
 			continue;
 		if (fp->channels != subs->channels)
@@ -802,7 +800,7 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params,
 			struct snd_pcm_hw_rule *rule)
 {
 	struct snd_usb_substream *subs = rule->private;
-	struct list_head *p;
+	struct audioformat *fp;
 	struct snd_interval *it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
 	unsigned int rmin, rmax;
 	int changed;
@@ -810,9 +808,7 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params,
 	hwc_debug("hw_rule_rate: (%d,%d)\n", it->min, it->max);
 	changed = 0;
 	rmin = rmax = 0;
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
-		fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!hw_check_valid_format(subs, params, fp))
 			continue;
 		if (changed++) {
@@ -856,7 +852,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params,
 			    struct snd_pcm_hw_rule *rule)
 {
 	struct snd_usb_substream *subs = rule->private;
-	struct list_head *p;
+	struct audioformat *fp;
 	struct snd_interval *it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
 	unsigned int rmin, rmax;
 	int changed;
@@ -864,9 +860,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params,
 	hwc_debug("hw_rule_channels: (%d,%d)\n", it->min, it->max);
 	changed = 0;
 	rmin = rmax = 0;
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
-		fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!hw_check_valid_format(subs, params, fp))
 			continue;
 		if (changed++) {
@@ -909,7 +903,7 @@ static int hw_rule_format(struct snd_pcm_hw_params *params,
 			  struct snd_pcm_hw_rule *rule)
 {
 	struct snd_usb_substream *subs = rule->private;
-	struct list_head *p;
+	struct audioformat *fp;
 	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
 	u64 fbits;
 	u32 oldbits[2];
@@ -917,9 +911,7 @@ static int hw_rule_format(struct snd_pcm_hw_params *params,
 
 	hwc_debug("hw_rule_format: %x:%x\n", fmt->bits[0], fmt->bits[1]);
 	fbits = 0;
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
-		fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!hw_check_valid_format(subs, params, fp))
 			continue;
 		fbits |= fp->formats;
@@ -1027,7 +1019,7 @@ static int snd_usb_pcm_check_knot(struct snd_pcm_runtime *runtime,
 
 static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substream *subs)
 {
-	struct list_head *p;
+	struct audioformat *fp;
 	unsigned int pt, ptmin;
 	int param_period_time_if_needed;
 	int err;
@@ -1041,9 +1033,7 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre
 	runtime->hw.rates = 0;
 	ptmin = UINT_MAX;
 	/* check min/max rates and channels */
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
-		fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		runtime->hw.rates |= fp->rates;
 		if (runtime->hw.rate_min > fp->rate_min)
 			runtime->hw.rate_min = fp->rate_min;
diff --git a/sound/usb/proc.c b/sound/usb/proc.c
index d218f76..0182ef6 100644
--- a/sound/usb/proc.c
+++ b/sound/usb/proc.c
@@ -73,15 +73,14 @@ void snd_usb_audio_create_proc(struct snd_usb_audio *chip)
  */
 static void proc_dump_substream_formats(struct snd_usb_substream *subs, struct snd_info_buffer *buffer)
 {
-	struct list_head *p;
+	struct audioformat *fp;
 	static char *sync_types[4] = {
 		"NONE", "ASYNC", "ADAPTIVE", "SYNC"
 	};
 
-	list_for_each(p, &subs->fmt_list) {
-		struct audioformat *fp;
+	list_for_each_entry(fp, &subs->fmt_list, list) {
 		snd_pcm_format_t fmt;
-		fp = list_entry(p, struct audioformat, list);
+
 		snd_iprintf(buffer, "  Interface %d\n", fp->iface);
 		snd_iprintf(buffer, "    Altset %d\n", fp->altsetting);
 		snd_iprintf(buffer, "    Format:");
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index ad181d5..42b7e0a 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -42,12 +42,11 @@
  */
 static void free_substream(struct snd_usb_substream *subs)
 {
-	struct list_head *p, *n;
+	struct audioformat *fp, *n;
 
 	if (!subs->num_formats)
 		return; /* not initialized */
-	list_for_each_safe(p, n, &subs->fmt_list) {
-		struct audioformat *fp = list_entry(p, struct audioformat, list);
+	list_for_each_entry_safe(fp, n, &subs->fmt_list, list) {
 		kfree(fp->rate_table);
 		kfree(fp->chmap);
 		kfree(fp);
@@ -313,14 +312,12 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 			     int stream,
 			     struct audioformat *fp)
 {
-	struct list_head *p;
 	struct snd_usb_stream *as;
 	struct snd_usb_substream *subs;
 	struct snd_pcm *pcm;
 	int err;
 
-	list_for_each(p, &chip->pcm_list) {
-		as = list_entry(p, struct snd_usb_stream, list);
+	list_for_each_entry(as, &chip->pcm_list, list) {
 		if (as->fmt_type != fp->fmt_type)
 			continue;
 		subs = &as->substream[stream];
@@ -332,8 +329,7 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 		}
 	}
 	/* look for an empty stream */
-	list_for_each(p, &chip->pcm_list) {
-		as = list_entry(p, struct snd_usb_stream, list);
+	list_for_each_entry(as, &chip->pcm_list, list) {
 		if (as->fmt_type != fp->fmt_type)
 			continue;
 		subs = &as->substream[stream];
-- 
1.8.1.5

[PATCH 02/10] ALSA: usb-audio: neaten MODULE_DEVICE_TABLE placement

[Eldad Zack]

Minor style fix, following a general code style in the kernel.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/card.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 8bab36c..314e8a2 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -714,8 +714,7 @@ static struct usb_device_id usb_audio_ids [] = {
       .bInterfaceSubClass = USB_SUBCLASS_AUDIOCONTROL },
     { }						/* Terminating entry */
 };
-
-MODULE_DEVICE_TABLE (usb, usb_audio_ids);
+MODULE_DEVICE_TABLE(usb, usb_audio_ids);
 
 /*
  * entry point for linux usb interface
-- 
1.8.1.5

[PATCH 03/10] ALSA: usb-audio: neaten EXPORT_SYMBOLS placement

[Eldad Zack]

Put EXPORT_SYMBOLS directly under the exported function.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/midi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 1cf943d..60c7aa7 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1455,6 +1455,7 @@ void snd_usbmidi_disconnect(struct list_head* p)
 	}
 	del_timer_sync(&umidi->error_timer);
 }
+EXPORT_SYMBOL(snd_usbmidi_disconnect);
 
 static void snd_usbmidi_rawmidi_free(struct snd_rawmidi *rmidi)
 {
@@ -2090,6 +2091,7 @@ void snd_usbmidi_input_stop(struct list_head* p)
 	}
 	umidi->input_running = 0;
 }
+EXPORT_SYMBOL(snd_usbmidi_input_stop);
 
 static void snd_usbmidi_input_start_ep(struct snd_usb_midi_in_endpoint* ep)
 {
@@ -2119,6 +2121,7 @@ void snd_usbmidi_input_start(struct list_head* p)
 		snd_usbmidi_input_start_ep(umidi->endpoints[i].in);
 	umidi->input_running = 1;
 }
+EXPORT_SYMBOL(snd_usbmidi_input_start);
 
 /*
  * Creates and registers everything needed for a MIDI streaming interface.
@@ -2258,8 +2261,4 @@ int snd_usbmidi_create(struct snd_card *card,
 	list_add_tail(&umidi->list, midi_list);
 	return 0;
 }
-
 EXPORT_SYMBOL(snd_usbmidi_create);
-EXPORT_SYMBOL(snd_usbmidi_input_stop);
-EXPORT_SYMBOL(snd_usbmidi_input_start);
-EXPORT_SYMBOL(snd_usbmidi_disconnect);
-- 
1.8.1.5

[PATCH 04/10] ALSA: usb-audio: spelling correction

[Eldad Zack]

Correct spelling of snd_usb_endpoint_implict_feedback_sink in all
occurances.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c | 14 +++++++-------
 sound/usb/endpoint.h |  2 +-
 sound/usb/pcm.c      |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index b1f687f..7e9c55a 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -128,7 +128,7 @@ static const char *usb_error_string(int err)
  * Determine whether an endpoint is driven by an implicit feedback
  * data endpoint source.
  */
-int snd_usb_endpoint_implict_feedback_sink(struct snd_usb_endpoint *ep)
+int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep)
 {
 	return  ep->sync_master &&
 		ep->sync_master->type == SND_USB_ENDPOINT_TYPE_DATA &&
@@ -363,7 +363,7 @@ static void snd_complete_urb(struct urb *urb)
 		if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
 			goto exit_clear;
 
-		if (snd_usb_endpoint_implict_feedback_sink(ep)) {
+		if (snd_usb_endpoint_implicit_feedback_sink(ep)) {
 			unsigned long flags;
 
 			spin_lock_irqsave(&ep->lock, flags);
@@ -605,7 +605,7 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
 	else
 		packs_per_ms = 1;
 
-	if (is_playback && !snd_usb_endpoint_implict_feedback_sink(ep)) {
+	if (is_playback && !snd_usb_endpoint_implicit_feedback_sink(ep)) {
 		urb_packs = max(ep->chip->nrpacks, 1);
 		urb_packs = min(urb_packs, (unsigned int) MAX_PACKS);
 	} else {
@@ -614,11 +614,11 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
 
 	urb_packs *= packs_per_ms;
 
-	if (sync_ep && !snd_usb_endpoint_implict_feedback_sink(ep))
+	if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep))
 		urb_packs = min(urb_packs, 1U << sync_ep->syncinterval);
 
 	/* decide how many packets to be used */
-	if (is_playback && !snd_usb_endpoint_implict_feedback_sink(ep)) {
+	if (is_playback && !snd_usb_endpoint_implicit_feedback_sink(ep)) {
 		unsigned int minsize, maxpacks;
 		/* determine how small a packet can be */
 		minsize = (ep->freqn >> (16 - ep->datainterval))
@@ -845,7 +845,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep)
 
 	set_bit(EP_FLAG_RUNNING, &ep->flags);
 
-	if (snd_usb_endpoint_implict_feedback_sink(ep)) {
+	if (snd_usb_endpoint_implicit_feedback_sink(ep)) {
 		for (i = 0; i < ep->nurbs; i++) {
 			struct snd_urb_ctx *ctx = ep->urb + i;
 			list_add_tail(&ctx->ready_list, &ep->ready_playback_urbs);
@@ -988,7 +988,7 @@ void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
 	 * and add it to the list of pending urbs. queue_pending_output_urbs()
 	 * will take care of them later.
 	 */
-	if (snd_usb_endpoint_implict_feedback_sink(ep) &&
+	if (snd_usb_endpoint_implicit_feedback_sink(ep) &&
 	    ep->use_count != 0) {
 
 		/* implicit feedback case */
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 447902d..2287adf 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -23,7 +23,7 @@ int  snd_usb_endpoint_activate(struct snd_usb_endpoint *ep);
 int  snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_free(struct list_head *head);
 
-int snd_usb_endpoint_implict_feedback_sink(struct snd_usb_endpoint *ep);
+int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
 int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep);
 
 void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index e42fc19..9531355 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1264,7 +1264,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 			}
 		}
 		if (period_elapsed &&
-		    !snd_usb_endpoint_implict_feedback_sink(subs->data_endpoint)) /* finish at the period boundary */
+		    !snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint)) /* finish at the period boundary */
 			break;
 	}
 	bytes = frames * stride;
-- 
1.8.1.5

[PATCH 05/10] ALSA: usb-audio: use endianness macros

[Eldad Zack]

Replace the endianness conversions with the kernel-wide swabbing macros.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/clock.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 5e634a2..47b601d 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -252,7 +252,7 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 			      struct audioformat *fmt, int rate)
 {
 	struct usb_device *dev = chip->dev;
-	unsigned char data[4];
+	u32 data;
 	int err, crate;
 	int clock = snd_usb_clock_find_source(chip, fmt->clock);
 
@@ -266,15 +266,12 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 		return -ENXIO;
 	}
 
-	data[0] = rate;
-	data[1] = rate >> 8;
-	data[2] = rate >> 16;
-	data[3] = rate >> 24;
+	data = cpu_to_le32(rate);
 	if ((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))) < 0) {
+				   &data, sizeof(data))) < 0) {
 		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2)\n",
 			   dev->devnum, iface, fmt->altsetting, rate);
 		return err;
@@ -284,13 +281,13 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
 				   UAC2_CS_CONTROL_SAM_FREQ << 8,
 				   snd_usb_ctrl_intf(chip) | (clock << 8),
-				   data, sizeof(data))) < 0) {
+				   &data, sizeof(rate))) < 0) {
 		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n",
 			   dev->devnum, iface, fmt->altsetting);
 		return err;
 	}
 
-	crate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24);
+	crate = le32_to_cpu(data);
 	if (crate != rate)
 		snd_printd(KERN_WARNING "current rate %d is different from the runtime rate %d\n", crate, rate);
 
-- 
1.8.1.5

[PATCH 06/10] ALSA: usb-audio: UAC2: do clock validity check earlier

[Eldad Zack]

Move the check that parse_audio_format_rates_v2() do after
receiving the clock source entity ID directly into the find
function and add a validation flag to the function.

This patch does not introduce any logic flow change.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/clock.c  | 35 +++++++++++++++++++----------------
 sound/usb/clock.h  |  3 ++-
 sound/usb/format.c |  2 +-
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 47b601d..08fa345 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -131,7 +131,8 @@ static bool uac_clock_source_is_valid(struct snd_usb_audio *chip, int source_id)
 }
 
 static int __uac_clock_find_source(struct snd_usb_audio *chip,
-				   int entity_id, unsigned long *visited)
+				   int entity_id, unsigned long *visited,
+				   bool validate)
 {
 	struct uac_clock_source_descriptor *source;
 	struct uac_clock_selector_descriptor *selector;
@@ -148,8 +149,15 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 
 	/* first, see if the ID we're looking for is a clock source already */
 	source = snd_usb_find_clock_source(chip->ctrl_intf, entity_id);
-	if (source)
-		return source->bClockID;
+	if (source) {
+		entity_id = source->bClockID;
+		if (validate && !uac_clock_source_is_valid(chip, entity_id)) {
+			snd_printk(KERN_ERR "usb-audio:%d: clock source %d is not valid, cannot use\n",
+				   chip->dev->devnum, entity_id);
+			return -ENXIO;
+		}
+		return entity_id;
+	}
 
 	selector = snd_usb_find_clock_selector(chip->ctrl_intf, entity_id);
 	if (selector) {
@@ -164,7 +172,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 		/* Selector values are one-based */
 
 		if (ret > selector->bNrInPins || ret < 1) {
-			printk(KERN_ERR
+			snd_printk(KERN_ERR
 				"%s(): selector reported illegal value, id %d, ret %d\n",
 				__func__, selector->bClockID, ret);
 
@@ -172,14 +180,14 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 		}
 
 		return __uac_clock_find_source(chip, selector->baCSourceID[ret-1],
-					       visited);
+					       visited, validate);
 	}
 
 	/* FIXME: multipliers only act as pass-thru element for now */
 	multiplier = snd_usb_find_clock_multiplier(chip->ctrl_intf, entity_id);
 	if (multiplier)
 		return __uac_clock_find_source(chip, multiplier->bCSourceID,
-						visited);
+						visited, validate);
 
 	return -EINVAL;
 }
@@ -195,11 +203,12 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
  *
  * Returns the clock source UnitID (>=0) on success, or an error.
  */
-int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id)
+int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id,
+			      bool validate)
 {
 	DECLARE_BITMAP(visited, 256);
 	memset(visited, 0, sizeof(visited));
-	return __uac_clock_find_source(chip, entity_id, visited);
+	return __uac_clock_find_source(chip, entity_id, visited, validate);
 }
 
 static int set_sample_rate_v1(struct snd_usb_audio *chip, int iface,
@@ -254,18 +263,12 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 	struct usb_device *dev = chip->dev;
 	u32 data;
 	int err, crate;
-	int clock = snd_usb_clock_find_source(chip, fmt->clock);
+	int clock;
 
+	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;
-	}
-
 	data = cpu_to_le32(rate);
 	if ((err = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC2_CS_CUR,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
diff --git a/sound/usb/clock.h b/sound/usb/clock.h
index 4663093..d592e4a 100644
--- a/sound/usb/clock.h
+++ b/sound/usb/clock.h
@@ -5,6 +5,7 @@ int snd_usb_init_sample_rate(struct snd_usb_audio *chip, int iface,
 			     struct usb_host_interface *alts,
 			     struct audioformat *fmt, int rate);
 
-int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id);
+int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id,
+			     bool validate);
 
 #endif /* __USBAUDIO_CLOCK_H */
diff --git a/sound/usb/format.c b/sound/usb/format.c
index e831ee4..1086b87 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -277,7 +277,7 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	struct usb_device *dev = chip->dev;
 	unsigned char tmp[2], *data;
 	int nr_triplets, data_size, ret = 0;
-	int clock = snd_usb_clock_find_source(chip, fp->clock);
+	int clock = snd_usb_clock_find_source(chip, fp->clock, false);
 
 	if (clock < 0) {
 		snd_printk(KERN_ERR "%s(): unable to find clock source (clock %d)\n",
-- 
1.8.1.5

[PATCH 07/10] ALSA: usb-audio: UAC2: try to find and switch to valid clock

[Eldad Zack]

If a selector is available on a device, it may be pointing to a
clock source which is currently invalid.
If there is a valid clock source which can be selected, switch
to it.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/clock.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 08fa345..6b79e25 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -99,6 +99,40 @@ static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_i
 	return buf;
 }
 
+static int uac_clock_selector_set_val(struct snd_usb_audio *chip, int selector_id,
+					unsigned char pin)
+{
+	unsigned char buf;
+	int ret;
+
+	ret = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
+			      UAC2_CS_CUR,
+			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT,
+			      UAC2_CX_CLOCK_SELECTOR << 8,
+			      snd_usb_ctrl_intf(chip) | (selector_id << 8),
+			      &pin, sizeof(pin));
+
+	if (ret < 0)
+		return ret;
+
+	if (ret != sizeof(pin)) {
+		snd_printk(KERN_ERR
+			"usb-audio:%d: setting selector (id %d) unexpected length %d\n",
+			chip->dev->devnum, selector_id, ret);
+		return -EINVAL;
+	}
+
+	buf = uac_clock_selector_get_val(chip, selector_id);
+	if (buf != pin) {
+		snd_printk(KERN_ERR
+			"usb-audio:%d: setting selector (id %d) failed (wanted %x, got %x)\n",
+			chip->dev->devnum, selector_id, pin, buf);
+		return -EINVAL;
+	}
+
+	return buf;
+}
+
 static bool uac_clock_source_is_valid(struct snd_usb_audio *chip, int source_id)
 {
 	int err;
@@ -161,7 +195,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 
 	selector = snd_usb_find_clock_selector(chip->ctrl_intf, entity_id);
 	if (selector) {
-		int ret;
+		int ret, i, cur;
 
 		/* the entity ID we are looking for is a selector.
 		 * find out what it currently selects */
@@ -179,8 +213,35 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 			return -EINVAL;
 		}
 
-		return __uac_clock_find_source(chip, selector->baCSourceID[ret-1],
+		cur = ret;
+		ret = __uac_clock_find_source(chip, selector->baCSourceID[ret - 1],
 					       visited, validate);
+		if (!validate || ret > 0)
+			return ret;
+
+		/* The current clock source is invalid, try others. */
+		for (i = 1; i <= selector->bNrInPins; i++) {
+			int err;
+
+			if (i == cur)
+				continue;
+
+			ret = __uac_clock_find_source(chip, selector->baCSourceID[i - 1],
+				visited, true);
+			if (ret < 0)
+				continue;
+
+			err = uac_clock_selector_set_val(chip, entity_id, i);
+			if (err < 0)
+				continue;
+
+			snd_printk(KERN_INFO
+				"usb-audio:%d: found and selected valid clock source %d\n",
+				chip->dev->devnum, ret);
+			return ret;
+		}
+
+		return -ENXIO;
 	}
 
 	/* FIXME: multipliers only act as pass-thru element for now */
-- 
1.8.1.5

[PATCH 08/10] ALSA: usb-audio: UAC2: auto clock selection module param

[Eldad Zack]

Add a module param to disable auto clock selection.
This is provided for users that expect the audio stream to
fail when the clock source is invalid (e.g., the word clock
was unintentionally disconnected).

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/card.c     | 4 ++++
 sound/usb/clock.c    | 2 +-
 sound/usb/usbaudio.h | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 314e8a2..dfbf317 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -82,6 +82,7 @@ static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 };
 static int nrpacks = 8;		/* max. number of packets per urb */
 static int device_setup[SNDRV_CARDS]; /* device parameter for this card */
 static bool ignore_ctl_error;
+static bool noautoclk;
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for the USB audio adapter.");
@@ -100,6 +101,8 @@ MODULE_PARM_DESC(device_setup, "Specific device setup (if needed).");
 module_param(ignore_ctl_error, bool, 0444);
 MODULE_PARM_DESC(ignore_ctl_error,
 		 "Ignore errors from USB controller for mixer interfaces.");
+module_param(noautoclk, bool, 0444);
+MODULE_PARM_DESC(noautoclk, "Disables auto-clock selection for UAC2 devices.");
 
 /*
  * we keep the snd_usb_audio_t instances by ourselves for merging
@@ -354,6 +357,7 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx,
 	chip->card = card;
 	chip->setup = device_setup[idx];
 	chip->nrpacks = nrpacks;
+	chip->noautoclk = noautoclk;
 	chip->probing = 1;
 
 	chip->usb_id = USB_ID(le16_to_cpu(dev->descriptor.idVendor),
diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 6b79e25..e3075ca 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -216,7 +216,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 		cur = ret;
 		ret = __uac_clock_find_source(chip, selector->baCSourceID[ret - 1],
 					       visited, validate);
-		if (!validate || ret > 0)
+		if (!validate || ret > 0 || chip->noautoclk)
 			return ret;
 
 		/* The current clock source is invalid, try others. */
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 1ac3fd9..4852eb0 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -56,6 +56,7 @@ struct snd_usb_audio {
 
 	int setup;			/* from the 'device_setup' module param */
 	int nrpacks;			/* from the 'nrpacks' module param */
+	bool noautoclk;			/* from the 'noautoclk' module param */
 
 	struct usb_host_interface *ctrl_intf;	/* the audio control interface */
 };
-- 
1.8.1.5

[PATCH 09/10] ALSA: usb-audio: show err in set_sample_rate_v2 debug

[Eldad Zack]

Show the error code returned from the USB subsystem in
the debug messages.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/clock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index e3075ca..1c0ec82 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -336,8 +336,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 				   UAC2_CS_CONTROL_SAM_FREQ << 8,
 				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   &data, sizeof(data))) < 0) {
-		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2)\n",
-			   dev->devnum, iface, fmt->altsetting, rate);
+		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2): err %d\n",
+			   dev->devnum, iface, fmt->altsetting, rate, err);
 		return err;
 	}
 
@@ -346,8 +346,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 				   UAC2_CS_CONTROL_SAM_FREQ << 8,
 				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   &data, sizeof(rate))) < 0) {
-		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n",
-			   dev->devnum, iface, fmt->altsetting);
+		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2): err %d\n",
+			   dev->devnum, iface, fmt->altsetting, err);
 		return err;
 	}
 
-- 
1.8.1.5

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

[Eldad Zack]

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.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/clock.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 1c0ec82..6ad9951 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -325,35 +325,50 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 	u32 data;
 	int err, crate;
 	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;
 
-	data = cpu_to_le32(rate);
-	if ((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))) < 0) {
-		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2): err %d\n",
-			   dev->devnum, iface, fmt->altsetting, rate, err);
-		return err;
+	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) {
+		data = cpu_to_le32(rate);
+		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));
+		if (err < 0) {
+			snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2): err %d\n",
+				   dev->devnum, iface, fmt->altsetting, rate, err);
+			return err;
+		}
 	}
 
-	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));
+	if (err < 0) {
 		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2): err %d\n",
 			   dev->devnum, iface, fmt->altsetting, err);
 		return err;
 	}
 
 	crate = le32_to_cpu(data);
-	if (crate != rate)
+	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);
+	}
 
 	return 0;
 }
-- 
1.8.1.5

Re: [PATCH 05/10] ALSA: usb-audio: use endianness macros

[Clemens Ladisch]

Eldad Zack wrote:
> Replace the endianness conversions with the kernel-wide swabbing macros.

> +	u32 data;
> +	data = cpu_to_le32(rate);

cpu_to_le32() returns not u32 but __le32.
Please use that to make sparse happy.


Regards,
Clemens

Re: [PATCH 05/10] ALSA: usb-audio: use endianness macros

[Eldad Zack]

Hi Clemens,

On Sun, 31 Mar 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > Replace the endianness conversions with the kernel-wide swabbing macros.
> 
> > +	u32 data;
> > +	data = cpu_to_le32(rate);
> 
> cpu_to_le32() returns not u32 but __le32.
> Please use that to make sparse happy.

Thanks! Fixed locally. This propogates along the series, so I'll wait
for other comments to repost.

BTW, sparse (0.4.3) didn't complain on my box. Maybe I'm doing it wrong?
I'm just calling make C=1.

Cheers,
Eldad

Re: [PATCH 05/10] ALSA: usb-audio: use endianness macros

[Clemens Ladisch]

Eldad Zack wrote:
> On Sun, 31 Mar 2013, Clemens Ladisch wrote:
>> cpu_to_le32() returns not u32 but __le32.
>> Please use that to make sparse happy.
>
> BTW, sparse (0.4.3) didn't complain on my box.
> I'm just calling make C=1.

make C=1 CF=-D__CHECK_ENDIAN__


Regards,
Clemens

Re: [PATCH 06/10] ALSA: usb-audio: UAC2: do clock validity check earlier

[Torstein Hegge]

On Sun, Mar 31, 2013 at 17:52:28 +0200, Eldad Zack wrote:
> Move the check that parse_audio_format_rates_v2() do after
> receiving the clock source entity ID directly into the find
> function and add a validation flag to the function.
> 
> This patch does not introduce any logic flow change.

What would be lost by letting __uac_clock_find_source() always check
that the clock source is valid? It would avoid having to pass the
validate parameter, at the cost of having parse_audio_format_rates_v2()
validate clock source.

Torstein

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

[Torstein Hegge]

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

Re: [PATCH 06/10] ALSA: usb-audio: UAC2: do clock validity check earlier

[Eldad Zack]

Hi Torstein,

On Mon, 1 Apr 2013, Torstein Hegge wrote:

> On Sun, Mar 31, 2013 at 17:52:28 +0200, Eldad Zack wrote:
> > Move the check that parse_audio_format_rates_v2() do after
> > receiving the clock source entity ID directly into the find
> > function and add a validation flag to the function.
> > 
> > This patch does not introduce any logic flow change.
> 
> What would be lost by letting __uac_clock_find_source() always check
> that the clock source is valid? It would avoid having to pass the
> validate parameter, at the cost of having parse_audio_format_rates_v2()
> validate clock source.

I didn't want to change that logic since (1) I can't test it, my device
doesn't go through that code and (2) failing there on account of that
check would error out and not support the device (as far as I 
understand from the code). So if a (theoretical) device would only 
return valid one minute after it is connected, this will cause a
regression. I might be wrong, but I figured it's better to do one thing 
at a time.

Cheers,
Eldad

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

[Eldad Zack]

Hi Torstein,

On Mon, 1 Apr 2013, Torstein Hegge wrote:

> 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:

Thanks, I think it's a good idea. I'll wait with this patch until you 
get your change to Takashi's tree to save some work for everyone.

> @@ -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);

You might also want to convert this into le32_to_cpu, etc. like in 
patch #5 of this series -- note that as Clemens said, the type should 
should be __le32 (and not u32).

http://mailman.alsa-project.org/pipermail/alsa-devel/2013-March/060738.html

Thanks for pointing out the formatting issues, I'll fix these before 
reposting.

Cheers,
Eldad

Re: [PATCH 06/10] ALSA: usb-audio: UAC2: do clock validity check earlier

[Takashi Iwai]

At Sun, 31 Mar 2013 17:52:28 +0200,
Eldad Zack wrote:
> 
> Move the check that parse_audio_format_rates_v2() do after
> receiving the clock source entity ID directly into the find
> function and add a validation flag to the function.
> 
> This patch does not introduce any logic flow change.

Please give a few more words why this change is introduced.


thanks,

Takashi

> 
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/clock.c  | 35 +++++++++++++++++++----------------
>  sound/usb/clock.h  |  3 ++-
>  sound/usb/format.c |  2 +-
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 47b601d..08fa345 100644
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -131,7 +131,8 @@ static bool uac_clock_source_is_valid(struct snd_usb_audio *chip, int source_id)
>  }
>  
>  static int __uac_clock_find_source(struct snd_usb_audio *chip,
> -				   int entity_id, unsigned long *visited)
> +				   int entity_id, unsigned long *visited,
> +				   bool validate)
>  {
>  	struct uac_clock_source_descriptor *source;
>  	struct uac_clock_selector_descriptor *selector;
> @@ -148,8 +149,15 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  
>  	/* first, see if the ID we're looking for is a clock source already */
>  	source = snd_usb_find_clock_source(chip->ctrl_intf, entity_id);
> -	if (source)
> -		return source->bClockID;
> +	if (source) {
> +		entity_id = source->bClockID;
> +		if (validate && !uac_clock_source_is_valid(chip, entity_id)) {
> +			snd_printk(KERN_ERR "usb-audio:%d: clock source %d is not valid, cannot use\n",
> +				   chip->dev->devnum, entity_id);
> +			return -ENXIO;
> +		}
> +		return entity_id;
> +	}
>  
>  	selector = snd_usb_find_clock_selector(chip->ctrl_intf, entity_id);
>  	if (selector) {
> @@ -164,7 +172,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  		/* Selector values are one-based */
>  
>  		if (ret > selector->bNrInPins || ret < 1) {
> -			printk(KERN_ERR
> +			snd_printk(KERN_ERR
>  				"%s(): selector reported illegal value, id %d, ret %d\n",
>  				__func__, selector->bClockID, ret);
>  
> @@ -172,14 +180,14 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  		}
>  
>  		return __uac_clock_find_source(chip, selector->baCSourceID[ret-1],
> -					       visited);
> +					       visited, validate);
>  	}
>  
>  	/* FIXME: multipliers only act as pass-thru element for now */
>  	multiplier = snd_usb_find_clock_multiplier(chip->ctrl_intf, entity_id);
>  	if (multiplier)
>  		return __uac_clock_find_source(chip, multiplier->bCSourceID,
> -						visited);
> +						visited, validate);
>  
>  	return -EINVAL;
>  }
> @@ -195,11 +203,12 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>   *
>   * Returns the clock source UnitID (>=0) on success, or an error.
>   */
> -int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id)
> +int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id,
> +			      bool validate)
>  {
>  	DECLARE_BITMAP(visited, 256);
>  	memset(visited, 0, sizeof(visited));
> -	return __uac_clock_find_source(chip, entity_id, visited);
> +	return __uac_clock_find_source(chip, entity_id, visited, validate);
>  }
>  
>  static int set_sample_rate_v1(struct snd_usb_audio *chip, int iface,
> @@ -254,18 +263,12 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
>  	struct usb_device *dev = chip->dev;
>  	u32 data;
>  	int err, crate;
> -	int clock = snd_usb_clock_find_source(chip, fmt->clock);
> +	int clock;
>  
> +	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;
> -	}
> -
>  	data = cpu_to_le32(rate);
>  	if ((err = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC2_CS_CUR,
>  				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
> diff --git a/sound/usb/clock.h b/sound/usb/clock.h
> index 4663093..d592e4a 100644
> --- a/sound/usb/clock.h
> +++ b/sound/usb/clock.h
> @@ -5,6 +5,7 @@ int snd_usb_init_sample_rate(struct snd_usb_audio *chip, int iface,
>  			     struct usb_host_interface *alts,
>  			     struct audioformat *fmt, int rate);
>  
> -int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id);
> +int snd_usb_clock_find_source(struct snd_usb_audio *chip, int entity_id,
> +			     bool validate);
>  
>  #endif /* __USBAUDIO_CLOCK_H */
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index e831ee4..1086b87 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -277,7 +277,7 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
>  	struct usb_device *dev = chip->dev;
>  	unsigned char tmp[2], *data;
>  	int nr_triplets, data_size, ret = 0;
> -	int clock = snd_usb_clock_find_source(chip, fp->clock);
> +	int clock = snd_usb_clock_find_source(chip, fp->clock, false);
>  
>  	if (clock < 0) {
>  		snd_printk(KERN_ERR "%s(): unable to find clock source (clock %d)\n",
> -- 
> 1.8.1.5
> 

Re: [PATCH 07/10] ALSA: usb-audio: UAC2: try to find and switch to valid clock

[Takashi Iwai]

At Sun, 31 Mar 2013 17:52:29 +0200,
Eldad Zack wrote:
> 
> If a selector is available on a device, it may be pointing to a
> clock source which is currently invalid.
> If there is a valid clock source which can be selected, switch
> to it.
> 
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/clock.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 08fa345..6b79e25 100644
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -99,6 +99,40 @@ static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_i
>  	return buf;
>  }
>  
> +static int uac_clock_selector_set_val(struct snd_usb_audio *chip, int selector_id,
> +					unsigned char pin)
> +{
> +	unsigned char buf;
> +	int ret;
> +
> +	ret = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +			      UAC2_CS_CUR,
> +			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT,
> +			      UAC2_CX_CLOCK_SELECTOR << 8,
> +			      snd_usb_ctrl_intf(chip) | (selector_id << 8),
> +			      &pin, sizeof(pin));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != sizeof(pin)) {
> +		snd_printk(KERN_ERR
> +			"usb-audio:%d: setting selector (id %d) unexpected length %d\n",
> +			chip->dev->devnum, selector_id, ret);
> +		return -EINVAL;
> +	}
> +
> +	buf = uac_clock_selector_get_val(chip, selector_id);
> +	if (buf != pin) {


uac_clock_selector_get_val() returns an int with a negative value,
so it's safer to compare it as an int.



Takashi

Re: [PATCH 08/10] ALSA: usb-audio: UAC2: auto clock selection module param

[Takashi Iwai]

At Sun, 31 Mar 2013 17:52:30 +0200,
Eldad Zack wrote:
> 
> Add a module param to disable auto clock selection.
> This is provided for users that expect the audio stream to
> fail when the clock source is invalid (e.g., the word clock
> was unintentionally disconnected).
> 
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/card.c     | 4 ++++
>  sound/usb/clock.c    | 2 +-
>  sound/usb/usbaudio.h | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 314e8a2..dfbf317 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -82,6 +82,7 @@ static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 };
>  static int nrpacks = 8;		/* max. number of packets per urb */
>  static int device_setup[SNDRV_CARDS]; /* device parameter for this card */
>  static bool ignore_ctl_error;
> +static bool noautoclk;
>  
>  module_param_array(index, int, NULL, 0444);
>  MODULE_PARM_DESC(index, "Index value for the USB audio adapter.");
> @@ -100,6 +101,8 @@ MODULE_PARM_DESC(device_setup, "Specific device setup (if needed).");
>  module_param(ignore_ctl_error, bool, 0444);
>  MODULE_PARM_DESC(ignore_ctl_error,
>  		 "Ignore errors from USB controller for mixer interfaces.");
> +module_param(noautoclk, bool, 0444);
> +MODULE_PARM_DESC(noautoclk, "Disables auto-clock selection for UAC2 devices.");

Don't use "no" for a boolean option.  You can pass "yes" or "no" as an
option value, so it can confuse easily by passing noautoclk=yes.  See?

It's fine to use internally an inverted boolean, of course, though.
But the exposed option should be less confusing.

In addition, you don't have to use abbreviation.
The option autoclock or auto_clock should be better than autoclk.


Takashi

>  
>  /*
>   * we keep the snd_usb_audio_t instances by ourselves for merging
> @@ -354,6 +357,7 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx,
>  	chip->card = card;
>  	chip->setup = device_setup[idx];
>  	chip->nrpacks = nrpacks;
> +	chip->noautoclk = noautoclk;
>  	chip->probing = 1;
>  
>  	chip->usb_id = USB_ID(le16_to_cpu(dev->descriptor.idVendor),
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 6b79e25..e3075ca 100644
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -216,7 +216,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  		cur = ret;
>  		ret = __uac_clock_find_source(chip, selector->baCSourceID[ret - 1],
>  					       visited, validate);
> -		if (!validate || ret > 0)
> +		if (!validate || ret > 0 || chip->noautoclk)
>  			return ret;
>  
>  		/* The current clock source is invalid, try others. */
> diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
> index 1ac3fd9..4852eb0 100644
> --- a/sound/usb/usbaudio.h
> +++ b/sound/usb/usbaudio.h
> @@ -56,6 +56,7 @@ struct snd_usb_audio {
>  
>  	int setup;			/* from the 'device_setup' module param */
>  	int nrpacks;			/* from the 'nrpacks' module param */
> +	bool noautoclk;			/* from the 'noautoclk' module param */
>  
>  	struct usb_host_interface *ctrl_intf;	/* the audio control interface */
>  };
> -- 
> 1.8.1.5
> 

Re: [PATCH 07/10] ALSA: usb-audio: UAC2: try to find and switch to valid clock

[Eldad Zack]

On Tue, 2 Apr 2013, Takashi Iwai wrote:

> At Sun, 31 Mar 2013 17:52:29 +0200,
> Eldad Zack wrote:
> > 
> > If a selector is available on a device, it may be pointing to a
> > clock source which is currently invalid.
> > If there is a valid clock source which can be selected, switch
> > to it.
> > 
> > Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> > ---
> >  sound/usb/clock.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> > index 08fa345..6b79e25 100644
> > --- a/sound/usb/clock.c
> > +++ b/sound/usb/clock.c
> > @@ -99,6 +99,40 @@ static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_i

> > +	buf = uac_clock_selector_get_val(chip, selector_id);
> > +	if (buf != pin) {
> 
> uac_clock_selector_get_val() returns an int with a negative value,
> so it's safer to compare it as an int.

Right, and I need to return the error code, too. Thanks!

Cheers,
Eldad

Re: [PATCH 08/10] ALSA: usb-audio: UAC2: auto clock selection module param

[Eldad Zack]

On Tue, 2 Apr 2013, Takashi Iwai wrote:

> At Sun, 31 Mar 2013 17:52:30 +0200,
> Eldad Zack wrote:
> > 

> > +module_param(noautoclk, bool, 0444);
> > +MODULE_PARM_DESC(noautoclk, "Disables auto-clock selection for UAC2 devices.");
> 
> Don't use "no" for a boolean option.  You can pass "yes" or "no" as an
> option value, so it can confuse easily by passing noautoclk=yes.  See?

Ah, I understand. Good point. I'll change it to autoclock like you 
suggested (also internally).

Thanks!

I'll add some text to the other patch and repost.

Cheers,
Eldad

Re: [PATCH 05/10] ALSA: usb-audio: use endianness macros

[Eldad Zack]

On Sun, 31 Mar 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > On Sun, 31 Mar 2013, Clemens Ladisch wrote:
> >> cpu_to_le32() returns not u32 but __le32.
> >> Please use that to make sparse happy.
> >
> > BTW, sparse (0.4.3) didn't complain on my box.
> > I'm just calling make C=1.
> 
> make C=1 CF=-D__CHECK_ENDIAN__

Heh, I felt quite silly about this question now that I read
Documentation/sparse.txt to the end :)

I see why you care so much about it too. I just found 3 places 
with endian bugs thanks to sparse, where usb_control_msg() is
handed a LE instead of CPU native.

snd_nativeinstruments_control_get()/put() and 
snd_usb_nativeinstruments_boot_quirk do this (I'll post a patch to
fix this soon).

Cheers,
Eldad