On 31/01/17(Tue) 14:05, Christopher Zimmermann wrote:
> On 2017-01-29 Martin Pieuchot <m...@openbsd.org> wrote:
> > On 29/01/17(Sun) 19:33, Christopher Zimmermann wrote:
> > > [...]
> > > @@ -444,6 +447,11 @@ uaudio_match(struct device *parent, void
> > > if (uaa->iface == NULL || uaa->device == NULL)
> > > return (UMATCH_NONE);
> > >
> > > + if (uaa->vendor == USB_VENDOR_MAUDIO &&
> > > + uaa->product == USB_PRODUCT_MAUDIO_FASTTRACKPRO &&
> > > + uaa->configno != 2)
> > > + return UMATCH_NONE;
> >
> > Why to you need this?
>
> This device exposes only a very limited set of features in
> configuration 1.
But configuration 1 is midi not audio right? So it won't match, so why
do you need that?
> > > @@ -3312,6 +3340,9 @@ uaudio_set_params(void *addr, int setmod
> > > }
> > > break;
> > > }
> > > +
> > > + if (sc->sc_quirks & UAUDIO_FLAG_BE)
> > > + p->encoding = AUDIO_ENCODING_SLINEAR_BE;
> >
> > Why do you need this chunk and we don't need to set
> > AUDIO_ENCODING_SLINEAR_LE in the other case?
>
> We probably should set it to _LE in the other case. I moved this piece
> of code into uaudio_match_alt() and made it agnostic about the specific
> endianness.
>
> > > @@ -152,6 +153,11 @@ umidi_match(struct device *parent, void
> > > DPRINTFN(1,("umidi_match\n"));
> > >
> > > if (uaa->iface == NULL)
> > > + return UMATCH_NONE;
> > > +
> > > + if (uaa->vendor == USB_VENDOR_MAUDIO &&
> > > + uaa->product == USB_PRODUCT_MAUDIO_FASTTRACKPRO &&
> > > + uaa->configno != 2)
> > > return UMATCH_NONE;
> >
> > I'd leave the configno check out and add a comment explaining why we
> > want to force this driver to attach to uaudio(4).
>
> See my comment above about the same piece of code in uaudio.c.
> If I don't add this condition umidi will attach to configuration 1 and
> configuration 2 wouldn't be tried, uaudio wouldn't attach.
Please re-read my comment. I'm talking about the "configno" check.
Think about somebody reading your code in 5 years, it will just looks
like black magic.
What you're doing is working around our stupid USB detection mechanism
to use a specific configuration. So you don't want your device to
attach to umidi(4) no matter with which configuration.
> + if (p->precision > 8)
> + /*
> + * Don't search for matching encoding.
> + * Just tell the upper layers which one we support.
> + */
> + p->encoding = sc->sc_alts[i].encoding;
Why do you need that?
> + else if (p->encoding != sc->sc_alts[i].encoding)
> + continue;
> +
> alts_eh |= 1 << i;
> DPRINTFN(6,("%s: matched %s alt %d for enc/pre\n", __func__,
> mode == AUMODE_RECORD ? "rec" : "play", i));
