Re: [alsa-devel] [PATCH] ALSA: usb-audio: Disable quirks for BOSS Katana amplifiers

From: Takashi Iwai <tiwai@suse.de>
To: Mailing Lists <maillist@superlative.org>
Cc: alsa-devel@alsa-project.org, Mike Oliphant <oliphant@nostatic.org>
Subject: Re: [alsa-devel] [PATCH] ALSA: usb-audio: Disable quirks for BOSS Katana amplifiers
Date: Wed, 14 Oct 2020 16:55:02 +0200	[thread overview]
Message-ID: <s5hk0vskgc9.wl-tiwai@suse.de> (raw)
In-Reply-To: <CAPMdQDk0c5NcX1brErdaDroseLgC0oCocTnH9xKFHz-44Zjn=w@mail.gmail.com>

On Wed, 14 Oct 2020 16:31:02 +0200,
Mailing Lists wrote:
> 
> OK, I will do that.
> 
> Quick question: what is the best git I should clone to create patches against
> these days? I've been out of the loop for a few years now.

At best, for-linus branch of my sound.git tree
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git

There is a copy in github, too.

Takashi

> 
> Cheers,
> 
> Keith
> 
> On Wed, 14 Oct 2020 at 15:00, Takashi Iwai <tiwai@suse.de> wrote:
> 
>     On Wed, 14 Oct 2020 15:56:32 +0200,
>     Mailing Lists wrote:
>     >
>     > Yes, and I don't think this is practical (at least, not obviously).
>     >
>     > The "Roland" vs "BOSS" thing is pure branding. I have BOSS branded
>     devices,
>     > like the JS-8, which work perfectly without the patch as an example. It
>     > wouldn't surprise me if there are recent Roland branded devices which
>     require
>     > the patch.
>     >
>     > So I, personally, think it is beyond brand and is device range or
>     generation
>     > specific. I guess it is possible there may be some technical
>     parameter within
>     > the device descriptor which could indicate which variant the device is,
>     but I
>     > don't know what that might be (if at all). At this point I think we
>     probably
>     > have to apply a conditional setting based on the Product ID.
>     >
>     > Given that, is it best to continue hacking these into pcm.c, or should
>     we be
>     > looking at a quirks-table way to describe these?
>    
>     Currently it's better to grow the explicit allow-list, I suppose.
>     Those are still handful, hence manageable enough.
>    
>     But, we should consider improving search_roland_implicit_fb(), too.
>     Both actions don't conflict, and once after we establish the better
>     implicit-fb check, the allow-list can be dropped.
> 
>     thanks,
>    
>     Takashi
>    
>     >
>     > Cheers,
>     >
>     > Keith
>     >
>     > On Wed, 14 Oct 2020 at 14:47, Takashi Iwai <tiwai@suse.de> wrote:
>     >
>     >     On Wed, 14 Oct 2020 15:33:03 +0200,
>     >     Mailing Lists wrote:
>     >     >
>     >     > Thanks for the response Takashi,
>     >     >
>     >     > How should this be distinguishing between Roland and BOSS? They
>     both
>     >     have the
>     >     > vendor ID 0x0582.
>     >   
>     >     Ah, right, I missed that point :-<
>     >   
>     >     So the question would be rather how to detect BOSS devices
>     >     effectively...
>     >
>     >     thanks,
>     >   
>     >     Takashi
>     >   
>     >     >
>     >     > Cheers,
>     >     >
>     >     > Keith
>     >     >
>     >     > On Wed, 14 Oct 2020 at 14:09, Takashi Iwai <tiwai@suse.de> wrote:
>     >     >
>     >     >     On Wed, 14 Oct 2020 14:17:35 +0200,
>     >     >     Mailing Lists wrote:
>     >     >     >
>     >     >     > Following up on this, it appears there are a bunch of the
>     >     >     newer-generation
>     >     >     > Roland/Boss devices which need similar treatment.
>     >     >     >
>     >     >     > So far I have tested the GT-1, the GT-001, and the BR-80,
>     and
>     >     others
>     >     >     have
>     >     >     > reported the RC-300 as working with similar modifications. I
>     have
>     >     been
>     >     >     using
>     >     >     > the following change to the code in pcm.c
>     >     set_sync_ep_implicit_fb_quirk:
>     >     >     >
>     >     >     >     case USB_ID(0x0582, 0x01d8): /* BOSS Katana */
>     >     >     >     case USB_ID(0x0582, 0x0130): /* BOSS Micro BR-80 */
>     >     >     >     case USB_ID(0x0582, 0x0138): /* BOSS RC-300 */
>     >     >     >     case USB_ID(0x0582, 0x01d6): /* BOSS GT-1 */
>     >     >     >     case USB_ID(0x0582, 0x01e5): /* BOSS GT-001 */
>     >     >     > /* BOSS Katana amplifiers and many other newer BOSS devices
>     do not
>     >     need
>     >     >     quirks
>     >     >     > */
>     >     >     >
>     >     >     > There's probably others too, such as the GT-100 (I believe
>     the
>     >     GT-001
>     >     >     and
>     >     >     > GT-100 have similar hardware).
>     >     >     >
>     >     >     > My question is, should this just be submitted as a patch to
>     pcm.c
>     >     or
>     >     >     would it
>     >     >     > be better handled in quirks and, if so, how?
>     >     >     >
>     >     >     > Or something else?
>     >     >   
>     >     >     Do we really need this change at all?  I looked at the code
>     again,
>     >     and
>     >     >     I noticed that basically the function should return 0 without
>     >     setting
>     >     >     anything else even if you don't have the explicit ID checks
>     there.
>     >     >   
>     >     >     The function looks like:
>     >     >   
>     >     >     static int set_sync_ep_implicit_fb_quirk(struct
>     snd_usb_substream
>     >     *subs,
>     >     >                                              struct usb_device
>     *dev,
>     >     >                                              struct
>     >     usb_interface_descriptor
>     >     >     *altsd,
>     >     >                                              unsigned int attr)
>     >     >     {
>     >     >             ....
>     >     >             switch (subs->stream->chip->usb_id) {
>     >     >             ....
>     >     >             case USB_ID(0x0582, 0x01d8): /* BOSS Katana */
>     >     >                     /* BOSS Katana amplifiers do not need quirks *
>     /
>     >     >                     return 0;
>     >     >             }
>     >     >   
>     >     >             if (attr == USB_ENDPOINT_SYNC_ASYNC &&
>     >     >                 altsd->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
>     >     >                 altsd->bInterfaceProtocol == 2 &&
>     >     >                 altsd->bNumEndpoints == 1 &&
>     >     >                 USB_ID_VENDOR(subs->stream->chip->usb_id) ==
>     0x0582 /*
>     >     Roland
>     >     >     */ &&
>     >     >                 search_roland_implicit_fb(dev, altsd->
>     bInterfaceNumber +
>     >     1,
>     >     >                                           altsd->
>     bAlternateSetting,
>     >     >                                           &alts, &ep) >= 0) {
>     >     >                     goto add_sync_ep;
>     >     >             }
>     >     >   
>     >     >             /* No quirk */
>     >     >             return 0;
>     >     >   
>     >     >     ... and the lengthy if-conditions after the switch/case is
>     applied
>     >     >     only for Roland devices, hence it shouldn't influence on BOSS
>     >     >     devices.  After that point, the immediate return with 0, which
>     is
>     >     the
>     >     >     same as we do in switch/case.  So the explicit check of BOSS
>     devices
>     >     >     there looks superfluous.
>     >     >
>     >     >     thanks,
>     >     >   
>     >     >     Takashi
>     >     >
>     >     > --
>     >     > --
>     >     > Keith A Milner
>     >     >
>     >     >
>     >
>     > --
>     > --
>     > Keith A Milner
>     >
>     >
> 
> --
> --
> Keith A Milner
> 
> 