[PATCH 1/2] sound: usb: add UAC2 clock sources as mixer controls

[PATCH 1/2] sound: usb: add UAC2 clock sources as mixer controls

[Daniel Mack]

UAC2 specifies clock sources that optionally have validity controls.
This patch exposes them as mixer controls, so they can be read (and
at least in theory even be written) by userspace applications in order
to make clock selection policy decisions.

This implementation does nothing if the device is not UAC2 compliant,
or if the clock source does not define said validity control bits.

Tested with a miniDSP USBStreamer (0x2752/0x0016),

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 sound/usb/mixer.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 4f85757..973274b 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -45,6 +45,7 @@
 #include <linux/bitops.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/log2.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/usb.h>
@@ -1378,6 +1379,68 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
 	snd_usb_mixer_add_control(&cval->head, kctl);
 }
 
+static int parse_clock_source_unit(struct mixer_build *state, int unitid,
+				   void *_ftr)
+
+{
+	struct uac_clock_source_descriptor *hdr = _ftr;
+	struct usb_mixer_elem_info *cval;
+	struct snd_kcontrol *kctl;
+	char name[100];
+	int ret;
+
+	if (state->mixer->protocol != UAC_VERSION_2)
+		return -EINVAL;
+
+	if (hdr->bLength != sizeof(*hdr))
+		return -EINVAL;
+
+	/*
+	 * The only property of this unit we are interested in is the
+	 * clock source validity. If that isn't readable, just bail out.
+	 */
+	if (!uac2_control_is_readable(hdr->bmControls,
+				      ilog2(UAC2_CS_CONTROL_CLOCK_VALID)))
+		return 0;
+
+	cval = kzalloc(sizeof(*cval), GFP_KERNEL);
+	if (!cval)
+		return -ENOMEM;
+
+	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, hdr->bClockID);
+
+	cval->min = 0;
+	cval->max = 1;
+	cval->channels = 1;
+	cval->val_type = USB_MIXER_BOOLEAN;
+	cval->control = UAC2_CS_CONTROL_CLOCK_VALID;
+
+	if (uac2_control_is_writeable(hdr->bmControls,
+				      ilog2(UAC2_CS_CONTROL_CLOCK_VALID)))
+		kctl = snd_ctl_new1(&usb_feature_unit_ctl, cval);
+	else {
+		cval->master_readonly = 1;
+		kctl = snd_ctl_new1(&usb_feature_unit_ctl_ro, cval);
+	}
+
+	if (!kctl) {
+		kfree(cval);
+		return -ENOMEM;
+	}
+
+	kctl->private_free = snd_usb_mixer_elem_free;
+	ret = snd_usb_copy_string_desc(state, hdr->iClockSource,
+				       name, sizeof(name));
+	if (ret > 0)
+		snprintf(kctl->id.name, sizeof(kctl->id.name),
+			 "%s validity", name);
+	else
+		snprintf(kctl->id.name, sizeof(kctl->id.name),
+			 "Clock source %d validity", hdr->bClockID);
+
+	return snd_usb_mixer_add_control(&cval->head, kctl);
+}
+
 /*
  * parse a feature unit
  *
@@ -2126,10 +2189,11 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
 
 	switch (p1[2]) {
 	case UAC_INPUT_TERMINAL:
-	case UAC2_CLOCK_SOURCE:
 		return 0; /* NOP */
 	case UAC_MIXER_UNIT:
 		return parse_audio_mixer_unit(state, unitid, p1);
+	case UAC2_CLOCK_SOURCE:
+		return parse_clock_source_unit(state, unitid, p1);
 	case UAC_SELECTOR_UNIT:
 	case UAC2_CLOCK_SELECTOR:
 		return parse_audio_selector_unit(state, unitid, p1);
-- 
2.4.3

[PATCH 2/2] sound: usb: allow clock source validity interrupts

[Daniel Mack]

miniDSP USBStreamer UAC2 devices send clock validity changes with the
control field set to zero. The current interrupt handler ignores all
packets if the control field does not match the mixer element's, but
it really should only do that in case that field is needed to
distinguish multiple elements with the same ID.

This patch implements a logic that lets notifications packets pass
if the element ID is unique for a given device.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 sound/usb/mixer.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 973274b..aa6f16e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2371,6 +2371,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
 	__u8 unitid = (index >> 8) & 0xff;
 	__u8 control = (value >> 8) & 0xff;
 	__u8 channel = value & 0xff;
+	unsigned int count = 0;
 
 	if (channel >= MAX_CHANNELS) {
 		usb_audio_dbg(mixer->chip,
@@ -2379,6 +2380,12 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
 		return;
 	}
 
+	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem)
+		count++;
+
+	if (count == 0)
+		return;
+
 	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem) {
 		struct usb_mixer_elem_info *info;
 
@@ -2386,7 +2393,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
 			continue;
 
 		info = (struct usb_mixer_elem_info *)list;
-		if (info->control != control)
+		if (count > 1 && info->control != control)
 			continue;
 
 		switch (attribute) {
-- 
2.4.3

Re: [PATCH 1/2] sound: usb: add UAC2 clock sources as mixer controls

[Takashi Iwai]

On Fri, 08 Apr 2016 19:52:01 +0200,
Daniel Mack wrote:
> 
> UAC2 specifies clock sources that optionally have validity controls.
> This patch exposes them as mixer controls, so they can be read (and
> at least in theory even be written) by userspace applications in order
> to make clock selection policy decisions.
> 
> This implementation does nothing if the device is not UAC2 compliant,
> or if the clock source does not define said validity control bits.
> 
> Tested with a miniDSP USBStreamer (0x2752/0x0016),
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  sound/usb/mixer.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 4f85757..973274b 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -45,6 +45,7 @@
>  #include <linux/bitops.h>
>  #include <linux/init.h>
>  #include <linux/list.h>
> +#include <linux/log2.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/usb.h>
> @@ -1378,6 +1379,68 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
>  	snd_usb_mixer_add_control(&cval->head, kctl);
>  }
>  
> +static int parse_clock_source_unit(struct mixer_build *state, int unitid,
> +				   void *_ftr)
> +
> +{
> +	struct uac_clock_source_descriptor *hdr = _ftr;
> +	struct usb_mixer_elem_info *cval;
> +	struct snd_kcontrol *kctl;
> +	char name[100];

No need for 100.  SNDRV_CTL_ELEM_ID_NAME_MAXLEN should suffice.

> +	int ret;
> +
> +	if (state->mixer->protocol != UAC_VERSION_2)
> +		return -EINVAL;
> +
> +	if (hdr->bLength != sizeof(*hdr))
> +		return -EINVAL;

I wonder whether we should abort here and return an error.
It's a wacky firmware, and no kernel error, after all.

> +	/*
> +	 * The only property of this unit we are interested in is the
> +	 * clock source validity. If that isn't readable, just bail out.
> +	 */
> +	if (!uac2_control_is_readable(hdr->bmControls,
> +				      ilog2(UAC2_CS_CONTROL_CLOCK_VALID)))
> +		return 0;
> +
> +	cval = kzalloc(sizeof(*cval), GFP_KERNEL);
> +	if (!cval)
> +		return -ENOMEM;
> +
> +	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, hdr->bClockID);
> +
> +	cval->min = 0;
> +	cval->max = 1;
> +	cval->channels = 1;
> +	cval->val_type = USB_MIXER_BOOLEAN;
> +	cval->control = UAC2_CS_CONTROL_CLOCK_VALID;
> +
> +	if (uac2_control_is_writeable(hdr->bmControls,
> +				      ilog2(UAC2_CS_CONTROL_CLOCK_VALID)))
> +		kctl = snd_ctl_new1(&usb_feature_unit_ctl, cval);
> +	else {
> +		cval->master_readonly = 1;
> +		kctl = snd_ctl_new1(&usb_feature_unit_ctl_ro, cval);
> +	}
> +
> +	if (!kctl) {
> +		kfree(cval);
> +		return -ENOMEM;
> +	}
> +
> +	kctl->private_free = snd_usb_mixer_elem_free;
> +	ret = snd_usb_copy_string_desc(state, hdr->iClockSource,
> +				       name, sizeof(name));
> +	if (ret > 0)
> +		snprintf(kctl->id.name, sizeof(kctl->id.name),
> +			 "%s validity", name);
> +	else
> +		snprintf(kctl->id.name, sizeof(kctl->id.name),
> +			 "Clock source %d validity", hdr->bClockID);

Each word in control name is usually starting with a capital letter.


thanks,

Takashi

Re: [PATCH 2/2] sound: usb: allow clock source validity interrupts

[Takashi Iwai]

On Fri, 08 Apr 2016 19:52:02 +0200,
Daniel Mack wrote:
> 
> miniDSP USBStreamer UAC2 devices send clock validity changes with the
> control field set to zero. The current interrupt handler ignores all
> packets if the control field does not match the mixer element's, but
> it really should only do that in case that field is needed to
> distinguish multiple elements with the same ID.
> 
> This patch implements a logic that lets notifications packets pass
> if the element ID is unique for a given device.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  sound/usb/mixer.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 973274b..aa6f16e 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2371,6 +2371,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>  	__u8 unitid = (index >> 8) & 0xff;
>  	__u8 control = (value >> 8) & 0xff;
>  	__u8 channel = value & 0xff;
> +	unsigned int count = 0;
>  
>  	if (channel >= MAX_CHANNELS) {
>  		usb_audio_dbg(mixer->chip,
> @@ -2379,6 +2380,12 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>  		return;
>  	}
>  
> +	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem)
> +		count++;
> +
> +	if (count == 0)
> +		return;
> +
>  	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem) {
>  		struct usb_mixer_elem_info *info;
>  
> @@ -2386,7 +2393,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>  			continue;
>  
>  		info = (struct usb_mixer_elem_info *)list;
> -		if (info->control != control)
> +		if (count > 1 && info->control != control)
>  			continue;

Just for checking count=0 and count=1, we need no loop to count
beforehand.
		if (info->control != control &&
		    (list != mixer->id_elems[unit] ||
		     list->list_next_id_elem))
			continue;

But, this doesn't look better and is more harder to understand, so I'm
not willing to sell it :)


thanks,

Takashi

Re: [PATCH 1/2] sound: usb: add UAC2 clock sources as mixer controls

[Daniel Mack]

Hi Takashi,

On 04/09/2016 10:43 AM, Takashi Iwai wrote:
> On Fri, 08 Apr 2016 19:52:01 +0200,
> Daniel Mack wrote:
>>
>> UAC2 specifies clock sources that optionally have validity controls.
>> This patch exposes them as mixer controls, so they can be read (and
>> at least in theory even be written) by userspace applications in order
>> to make clock selection policy decisions.
>>
>> This implementation does nothing if the device is not UAC2 compliant,
>> or if the clock source does not define said validity control bits.
>>
>> Tested with a miniDSP USBStreamer (0x2752/0x0016),
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>  sound/usb/mixer.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 4f85757..973274b 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -45,6 +45,7 @@
>>  #include <linux/bitops.h>
>>  #include <linux/init.h>
>>  #include <linux/list.h>
>> +#include <linux/log2.h>
>>  #include <linux/slab.h>
>>  #include <linux/string.h>
>>  #include <linux/usb.h>
>> @@ -1378,6 +1379,68 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
>>  	snd_usb_mixer_add_control(&cval->head, kctl);
>>  }
>>  
>> +static int parse_clock_source_unit(struct mixer_build *state, int unitid,
>> +				   void *_ftr)
>> +
>> +{
>> +	struct uac_clock_source_descriptor *hdr = _ftr;
>> +	struct usb_mixer_elem_info *cval;
>> +	struct snd_kcontrol *kctl;
>> +	char name[100];
> 
> No need for 100.  SNDRV_CTL_ELEM_ID_NAME_MAXLEN should suffice.
> 
>> +	int ret;
>> +
>> +	if (state->mixer->protocol != UAC_VERSION_2)
>> +		return -EINVAL;
>> +
>> +	if (hdr->bLength != sizeof(*hdr))
>> +		return -EINVAL;
> 
> I wonder whether we should abort here and return an error.
> It's a wacky firmware, and no kernel error, after all.

This is what we're doing in other parsers as well, but you're right,
it's better to just add a debug print here and handle this gracefully.

Will send a new patch soon, also addressing the other details.


Thanks,
Daniel

Re: [PATCH 2/2] sound: usb: allow clock source validity interrupts

[Daniel Mack]

On 04/09/2016 10:52 AM, Takashi Iwai wrote:
> On Fri, 08 Apr 2016 19:52:02 +0200,
> Daniel Mack wrote:
>>
>> miniDSP USBStreamer UAC2 devices send clock validity changes with the
>> control field set to zero. The current interrupt handler ignores all
>> packets if the control field does not match the mixer element's, but
>> it really should only do that in case that field is needed to
>> distinguish multiple elements with the same ID.
>>
>> This patch implements a logic that lets notifications packets pass
>> if the element ID is unique for a given device.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>  sound/usb/mixer.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 973274b..aa6f16e 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -2371,6 +2371,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>>  	__u8 unitid = (index >> 8) & 0xff;
>>  	__u8 control = (value >> 8) & 0xff;
>>  	__u8 channel = value & 0xff;
>> +	unsigned int count = 0;
>>  
>>  	if (channel >= MAX_CHANNELS) {
>>  		usb_audio_dbg(mixer->chip,
>> @@ -2379,6 +2380,12 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>>  		return;
>>  	}
>>  
>> +	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem)
>> +		count++;
>> +
>> +	if (count == 0)
>> +		return;
>> +
>>  	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem) {
>>  		struct usb_mixer_elem_info *info;
>>  
>> @@ -2386,7 +2393,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
>>  			continue;
>>  
>>  		info = (struct usb_mixer_elem_info *)list;
>> -		if (info->control != control)
>> +		if (count > 1 && info->control != control)
>>  			continue;
> 
> Just for checking count=0 and count=1, we need no loop to count
> beforehand.
> 		if (info->control != control &&
> 		    (list != mixer->id_elems[unit] ||
> 		     list->list_next_id_elem))
> 			continue;
> 
> But, this doesn't look better and is more harder to understand, so I'm
> not willing to sell it :)

I had something like that before but opted for the more readable
version. But you're right. I'll add a comment and do it your way.


Thanks!
Daniel

Re: [PATCH 2/2] sound: usb: allow clock source validity interrupts

[Takashi Iwai]

On Sat, 09 Apr 2016 11:16:59 +0200,
Daniel Mack wrote:
> 
> On 04/09/2016 10:52 AM, Takashi Iwai wrote:
> > On Fri, 08 Apr 2016 19:52:02 +0200,
> > Daniel Mack wrote:
> >>
> >> miniDSP USBStreamer UAC2 devices send clock validity changes with the
> >> control field set to zero. The current interrupt handler ignores all
> >> packets if the control field does not match the mixer element's, but
> >> it really should only do that in case that field is needed to
> >> distinguish multiple elements with the same ID.
> >>
> >> This patch implements a logic that lets notifications packets pass
> >> if the element ID is unique for a given device.
> >>
> >> Signed-off-by: Daniel Mack <daniel@zonque.org>
> >> ---
> >>  sound/usb/mixer.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> >> index 973274b..aa6f16e 100644
> >> --- a/sound/usb/mixer.c
> >> +++ b/sound/usb/mixer.c
> >> @@ -2371,6 +2371,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
> >>  	__u8 unitid = (index >> 8) & 0xff;
> >>  	__u8 control = (value >> 8) & 0xff;
> >>  	__u8 channel = value & 0xff;
> >> +	unsigned int count = 0;
> >>  
> >>  	if (channel >= MAX_CHANNELS) {
> >>  		usb_audio_dbg(mixer->chip,
> >> @@ -2379,6 +2380,12 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
> >>  		return;
> >>  	}
> >>  
> >> +	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem)
> >> +		count++;
> >> +
> >> +	if (count == 0)
> >> +		return;
> >> +
> >>  	for (list = mixer->id_elems[unitid]; list; list = list->next_id_elem) {
> >>  		struct usb_mixer_elem_info *info;
> >>  
> >> @@ -2386,7 +2393,7 @@ static void snd_usb_mixer_interrupt_v2(struct usb_mixer_interface *mixer,
> >>  			continue;
> >>  
> >>  		info = (struct usb_mixer_elem_info *)list;
> >> -		if (info->control != control)
> >> +		if (count > 1 && info->control != control)
> >>  			continue;
> > 
> > Just for checking count=0 and count=1, we need no loop to count
> > beforehand.
> > 		if (info->control != control &&
> > 		    (list != mixer->id_elems[unit] ||
> > 		     list->list_next_id_elem))
> > 			continue;
> > 
> > But, this doesn't look better and is more harder to understand, so I'm
> > not willing to sell it :)
> 
> I had something like that before but opted for the more readable
> version. But you're right. I'll add a comment and do it your way.

Oh no, sorry, I wasn't clear: I meant that my version is worse in the
end, and I prefer your first version, just for simplicity.


Takashi

Re: [PATCH 2/2] sound: usb: allow clock source validity interrupts

[Daniel Mack]

On 04/09/2016 11:51 AM, Takashi Iwai wrote:
> On Sat, 09 Apr 2016 11:16:59 +0200, Daniel Mack wrote:
>> On 04/09/2016 10:52 AM, Takashi Iwai wrote:

>>> Just for checking count=0 and count=1, we need no loop to count
>>> beforehand.
>>> 		if (info->control != control &&
>>> 		    (list != mixer->id_elems[unit] ||
>>> 		     list->list_next_id_elem))
>>> 			continue;
>>>
>>> But, this doesn't look better and is more harder to understand, so I'm
>>> not willing to sell it :)
>>
>> I had something like that before but opted for the more readable
>> version. But you're right. I'll add a comment and do it your way.
> 
> Oh no, sorry, I wasn't clear: I meant that my version is worse in the
> end, and I prefer your first version, just for simplicity.

Ah ok :) Well, ultimately up to you; you have both versions, and the two
patches should apply independently anyway.

Or I can resend both if you prefer that.


Thanks,
Daniel

Re: [PATCH 2/2] sound: usb: allow clock source validity interrupts

[Takashi Iwai]

On Sat, 09 Apr 2016 16:37:28 +0200,
Daniel Mack wrote:
> 
> On 04/09/2016 11:51 AM, Takashi Iwai wrote:
> > On Sat, 09 Apr 2016 11:16:59 +0200, Daniel Mack wrote:
> >> On 04/09/2016 10:52 AM, Takashi Iwai wrote:
> 
> >>> Just for checking count=0 and count=1, we need no loop to count
> >>> beforehand.
> >>> 		if (info->control != control &&
> >>> 		    (list != mixer->id_elems[unit] ||
> >>> 		     list->list_next_id_elem))
> >>> 			continue;
> >>>
> >>> But, this doesn't look better and is more harder to understand, so I'm
> >>> not willing to sell it :)
> >>
> >> I had something like that before but opted for the more readable
> >> version. But you're right. I'll add a comment and do it your way.
> > 
> > Oh no, sorry, I wasn't clear: I meant that my version is worse in the
> > end, and I prefer your first version, just for simplicity.
> 
> Ah ok :) Well, ultimately up to you; you have both versions, and the two
> patches should apply independently anyway.
> 
> Or I can resend both if you prefer that.

Don't worry, I picked the preferred patches now (v2 for patch 1 and v1
for patch 2).


thanks,

Takashi