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

[Daniel Mack <daniel@zonque.org>]

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