usb-audio: Correct way to do a mixer quirk?

usb-audio: Correct way to do a mixer quirk?

[Mark Hills]

The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor 
is broken and refers to a non-existing unit_id.

I worked around it with the patch below to remove the additional entry, 
and the device seems to work as expected.

But inserting code here is probably not desirable -- I imagine I need to 
turn this into a quirk of some sort. Am I correct?

And what is the best way to do this?

* It doesn't appear I can use quirks-table.h to adjust the content
  of a mixer unit (the bNrInPins attribute)

* It seems that to use mixer_quirks.c would require a function to
  create the whole mixer

It's possible I'm incorrect on the above, though. Can anyone advise on how 
to make this fix in the correct way?

Many thanks

-- 
Mark


--- 
 AudioControl Interface Descriptor:
    bLength                15
    bDescriptorType        36
    bDescriptorSubtype      4 (MIXER_UNIT)
    bUnitID                 6
    bNrInPins               3         <-- should be 2
    baSourceID( 0)          5
    baSourceID( 1)          4
    baSourceID( 2)          2         <-- bogus entry

---
 sound/usb/mixer.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 8147ffc..ab3ccca 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1387,6 +1387,9 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid, void *r
 		return 0;
 	}
 
+	if (state->chip->usb_id == USB_ID(0x200c, 0x1018) && unitid == 6)
+		input_pins = 2;
+
 	num_ins = 0;
 	ich = 0;
 	for (pin = 0; pin < input_pins; pin++) {
-- 
1.7.4.4

Re: usb-audio: Correct way to do a mixer quirk?

[Clemens Ladisch]

Mark Hills wrote:
> The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor
> is broken and refers to a non-existing unit_id.
>
> I worked around it with the patch below to remove the additional entry,
> and the device seems to work as expected.
>
> But inserting code here is probably not desirable -- I imagine I need to
> turn this into a quirk of some sort. Am I correct?
>
> And what is the best way to do this?
>
> * It doesn't appear I can use quirks-table.h to adjust the content
>   of a mixer unit (the bNrInPins attribute)
>
> * It seems that to use mixer_quirks.c would require a function to
>   create the whole mixer
>
> It's possible I'm incorrect on the above, though. Can anyone advise on how
> to make this fix in the correct way?

I'd suggest to change the code to not abort if a unit doesn't exist.


Regards,
Clemens


> ---
>  AudioControl Interface Descriptor:
>     bLength                15
>     bDescriptorType        36
>     bDescriptorSubtype      4 (MIXER_UNIT)
>     bUnitID                 6
>     bNrInPins               3         <-- should be 2
>     baSourceID( 0)          5
>     baSourceID( 1)          4
>     baSourceID( 2)          2         <-- bogus entry
>
> ---
>  sound/usb/mixer.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 8147ffc..ab3ccca 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -1387,6 +1387,9 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid, void *r
>  		return 0;
>  	}
>
> +	if (state->chip->usb_id == USB_ID(0x200c, 0x1018) && unitid == 6)
> +		input_pins = 2;
> +
>  	num_ins = 0;
>  	ich = 0;
>  	for (pin = 0; pin < input_pins; pin++) {

Re: usb-audio: Correct way to do a mixer quirk?

[Mark Hills]

On Mon, 20 Feb 2012, Clemens Ladisch wrote:

> Mark Hills wrote:
> > The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor
> > is broken and refers to a non-existing unit_id.
> >
> > I worked around it with the patch below to remove the additional entry,
> > and the device seems to work as expected.
> >
> > But inserting code here is probably not desirable -- I imagine I need to
> > turn this into a quirk of some sort. Am I correct?
> >
> > And what is the best way to do this?
> >
> > * It doesn't appear I can use quirks-table.h to adjust the content
> >   of a mixer unit (the bNrInPins attribute)
> >
> > * It seems that to use mixer_quirks.c would require a function to
> >   create the whole mixer
> >
> > It's possible I'm incorrect on the above, though. Can anyone advise on how
> > to make this fix in the correct way?
> 
> I'd suggest to change the code to not abort if a unit doesn't exist.

It would seem a little strange to skip over the error for one specific 
condition to accomodate a specific device -- when the rest of the code 
does not generally do this?

I'd be interested in Daniel's take on this too.

If I get time I'll take a closer look at the code and see about making a 
patch.

Thanks

-- 
Mark

Re: usb-audio: Correct way to do a mixer quirk?

[Clemens Ladisch]

Mark Hills wrote:
> On Mon, 20 Feb 2012, Clemens Ladisch wrote:
>> Mark Hills wrote:
>>> The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor
>>> is broken and refers to a non-existing unit_id.
>>
>> I'd suggest to change the code to not abort if a unit doesn't exist.
>
> It would seem a little strange to skip over the error for one specific
> condition to accomodate a specific device -- when the rest of the code
> does not generally do this?

It's still possible to complain about the broken firmware, but there is
no reason to prevent the user from accessing the PCM streams and all
other mixer controls just because one descriptors happens to be wrong.


Regards,
Clemens

Re: usb-audio: Correct way to do a mixer quirk?

[Mark Hills]

On Mon, 20 Feb 2012, Clemens Ladisch wrote:

> Mark Hills wrote:
> > On Mon, 20 Feb 2012, Clemens Ladisch wrote:
> >> Mark Hills wrote:
> >>> The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor
> >>> is broken and refers to a non-existing unit_id.
> >>
> >> I'd suggest to change the code to not abort if a unit doesn't exist.
> >
> > It would seem a little strange to skip over the error for one specific
> > condition to accomodate a specific device -- when the rest of the code
> > does not generally do this?
> 
> It's still possible to complain about the broken firmware, but there is
> no reason to prevent the user from accessing the PCM streams and all
> other mixer controls just because one descriptors happens to be wrong.

The patch for this is relatively simple, I put it below for reference. The 
device can then be used; audio streams and a mixer are available.

But I'm not sure the resulting mixer is correct or complete so I'm in the 
process of getting access to one of these devices -- will allow me to 
check everything. I'll post more information when I have it.

Thanks

-- 
Mark


---
 sound/usb/mixer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index ab23869..c374c72 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1388,7 +1388,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid, void *r
 	for (pin = 0; pin < input_pins; pin++) {
 		err = parse_audio_unit(state, desc->baSourceID[pin]);
 		if (err < 0)
-			return err;
+			continue;
 		err = check_input_term(state, desc->baSourceID[pin], &iterm);
 		if (err < 0)
 			return err;
-- 
1.7.4.4

Re: usb-audio: Correct way to do a mixer quirk?

[Mark Hills]

On Mon, 20 Feb 2012, Clemens Ladisch wrote:

> Mark Hills wrote:
> > The Electrix Ebox-44 is a new USB interface, but the MIXER_UNIT descriptor
> > is broken and refers to a non-existing unit_id.
> >
> > I worked around it with the patch below to remove the additional entry,
> > and the device seems to work as expected.
> >
> > But inserting code here is probably not desirable -- I imagine I need to
> > turn this into a quirk of some sort. Am I correct?
> >
> > And what is the best way to do this?
> >
> > * It doesn't appear I can use quirks-table.h to adjust the content
> >   of a mixer unit (the bNrInPins attribute)
> >
> > * It seems that to use mixer_quirks.c would require a function to
> >   create the whole mixer
> >
> > It's possible I'm incorrect on the above, though. Can anyone advise on how
> > to make this fix in the correct way?
> 
> I'd suggest to change the code to not abort if a unit doesn't exist.

After changing this code I find myself up against other bad things about 
the desciptor.

The input mixer is not visible. It has a FEATURE_UNIT (10) but referred to 
by a broken MIXER_UNIT (11). There is Maya44 code which stops it being 
found (see below). Removing it and the control is visible. But that does 
not excuse the bad MIXER_UNIT.

Even with the hack, alsamixer presents the 2 volumes as (left, right) 
pairs, when they are actually 2 volumes controlling input pair 1, and 
input pair 2. It would be sensible to present this as 2x mono controls.

I mapped out the description coming from the device. I'm new here, but it 
seems odd that 4[FU] comes under 7[FU], and that outputs seem to be 
inputs. Is this description just badly broken, and is this a problem?

  7[FU] --> 6[MU] --> 5[IT] ("USB streaming")
                  --> 4[FU] --> 3[IT] ("Headphones")
                  --> 2[?]

  11[MU] --> 10[FU] --> 9[IT] ("Line connector")

  8[OT] ("Speaker")

  12[OT] ("USB Streaming")

Where am I best to go next?

eg. should I be trying to write code in mixer_quirks.c to build the 
desired mixer from scratch. And is this even possible to turn the 
left/right controls into mono?

Or is there some better way?

Thanks

-- 
Mark


diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index c374c72..42df7b5 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1377,11 +1379,6 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid, void *r
                snd_printk(KERN_ERR "invalid MIXER UNIT descriptor %d\n", unitid);
                return -EINVAL;
        }
-       /* no bmControls field (e.g. Maya44) -> ignore */
-       if (desc->bLength <= 10 + input_pins) {
-               snd_printdd(KERN_INFO "MU %d has no bmControls field\n", unitid);
-               return 0;
-       }
 
        num_ins = 0;
        ich = 0;