[2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

Commit Message

Takashi Iwai Nov. 9, 2014, 9:18 a.m. UTC

At Sun, 09 Nov 2014 00:35:29 +1100,
Damien Zammit wrote:
> 
> Hi Takashi,
> 
> On 07/11/14 01:15, Takashi Iwai wrote:
> > Hmm, can we achieve this without introducing the new audioformats
> > thing, e.g. with COMPOSITE, instead?
> 
> I don't think COMPOSITE will work because the same interface has
> multiple endpoints.  My code provides a framework for other devices with
> the same issue.  I was told previously that this was the problem with
> getting my previous attempt into the kernel.  Or is this doable without
> adding a struct?

I think the only point is the check in create_composite_quirk(), where
it marks the iface as claimed and skips the next entry that has been
already claimed.  However, the current code looks inconsistent -- it
allows multiple entries only if the iface matches with the current
one.  Fixing it like below would make things working.

It's a quick idea, so a bit more reviews would be needed, though.
Clemens, what do you think?

BTW, I won't take a look at you v2 series.  If the change above really
works, I'll brush up the patch below properly, so that you can include
in your new patch series.

Last but not least, please add maintainers to Cc when you post patches
for reviewing.


thanks,

Takashi

---

Comments

Clemens Ladisch Nov. 9, 2014, 2:04 p.m. UTC | #1

Takashi Iwai wrote:
> Damien Zammit wrote:
>> On 07/11/14 01:15, Takashi Iwai wrote:
>>> Hmm, can we achieve this without introducing the new audioformats
>>> thing, e.g. with COMPOSITE, instead?
>>
>> I don't think COMPOSITE will work because the same interface has
>> multiple endpoints.  My code provides a framework for other devices with
>> the same issue.  I was told previously that this was the problem with
>> getting my previous attempt into the kernel.  Or is this doable without
>> adding a struct?
>
> I think the only point is the check in create_composite_quirk(), where
> it marks the iface as claimed and skips the next entry that has been
> already claimed.  However, the current code looks inconsistent -- it
> allows multiple entries only if the iface matches with the current
> one.  Fixing it like below would make things working.
>
> It's a quick idea, so a bit more reviews would be needed, though.
> Clemens, what do you think?

Reviewed-by: Clemens Ladisch <[e-mail]>

> ---
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
>  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
>  		if (err < 0)
>  			return err;
> -		if (quirk->ifnum != probed_ifnum)
> +	}
> +
> +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
> +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
> +		if (!iface)
> +			continue;
> +		if (quirk->ifnum != probed_ifnum &&
> +		    !usb_interface_claimed(iface))
>  			usb_driver_claim_interface(driver, iface, (void *)-1L);
>  	}
> +
>  	return 0;
>  }
>

Takashi Iwai Nov. 9, 2014, 5:27 p.m. UTC | #2

At Sun, 09 Nov 2014 15:04:42 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Damien Zammit wrote:
> >> On 07/11/14 01:15, Takashi Iwai wrote:
> >>> Hmm, can we achieve this without introducing the new audioformats
> >>> thing, e.g. with COMPOSITE, instead?
> >>
> >> I don't think COMPOSITE will work because the same interface has
> >> multiple endpoints.  My code provides a framework for other devices with
> >> the same issue.  I was told previously that this was the problem with
> >> getting my previous attempt into the kernel.  Or is this doable without
> >> adding a struct?
> >
> > I think the only point is the check in create_composite_quirk(), where
> > it marks the iface as claimed and skips the next entry that has been
> > already claimed.  However, the current code looks inconsistent -- it
> > allows multiple entries only if the iface matches with the current
> > one.  Fixing it like below would make things working.
> >
> > It's a quick idea, so a bit more reviews would be needed, though.
> > Clemens, what do you think?
> 
> Reviewed-by: Clemens Ladisch <[e-mail]>

Thanks.  I now queued the proper patch to for-next branch and pushed
out.

Damien, could you rebase your patches to for-next branch of sound git
tree, and use this new feature?


Takashi

> 
> > ---
> > --- a/sound/usb/quirks.c
> > +++ b/sound/usb/quirks.c
> > @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
> >  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
> >  		if (err < 0)
> >  			return err;
> > -		if (quirk->ifnum != probed_ifnum)
> > +	}
> > +
> > +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
> > +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
> > +		if (!iface)
> > +			continue;
> > +		if (quirk->ifnum != probed_ifnum &&
> > +		    !usb_interface_claimed(iface))
> >  			usb_driver_claim_interface(driver, iface, (void *)-1L);
> >  	}
> > +
> >  	return 0;
> >  }
> >
>

Patch

--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -58,9 +58,17 @@  static int create_composite_quirk(struct snd_usb_audio *chip,
 		err = snd_usb_create_quirk(chip, iface, driver, quirk);
 		if (err < 0)
 			return err;
-		if (quirk->ifnum != probed_ifnum)
+	}
+
+	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
+		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
+		if (!iface)
+			continue;
+		if (quirk->ifnum != probed_ifnum &&
+		    !usb_interface_claimed(iface))
 			usb_driver_claim_interface(driver, iface, (void *)-1L);
 	}
+
 	return 0;
 }