On Fri, May 27, 2016 at 08:36:54AM +0200, Marcus Glocker wrote:
> On Thu, May 26, 2016 at 04:18:58PM -0700, patrick keshishian wrote:
>
> > On Thu, May 26, 2016 at 10:55:47PM +0200, Marcus Glocker wrote:
> > > On Tue, May 17, 2016 at 05:15:51PM -0700, patrick keshishian wrote:
> > >
> > > > Greetings,
> > > >
> > > > I have been looking at uvideo trying to model a new driver I'm
> > > > attempting to port over and found a few issues (or what I precive
> > > > as issues).
> > > >
> > > > Since the list likes separate diffs for easier discussion, Here
> > > > is my attempt to break them up in four emails. I think, with
> > > > exception of one, all should apply and compile individually.
> > > >
> > > > Here are description of patches in decreasing order of my
> > > > confidence in proposing them:
> > > >
> > > > 1/4: Incorrect enum used for v4l2_buf.flags.
> > > > This is a paste error I believe. Very simple diff
> > > >
> > > > 2/4: Assumption on endpoint index to use in uvideo_vs_open() vs
> > > > actual saved endpoint address.
> > >
> > > Makes sense. I also would change the following DPRINTF text to
> > > remove 'sc->sc_vs_cur->endpoint' since this will be same as
> > > 'ed->bEndpointAddress' and also remove 'sc->sc_vs_cur->psize'
> > > since this information is already printed in uvideo_vs_set_alt().
> >
> > As long as it is understood that the actually packet size is
> > not the literal value of wMaxPacketSize, rather:
> >
> > psize = UGETW(ed->wMaxPacketSize);
> > psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));
>
> Right, which is also the information printed in uvideo_vs_set_alt()
> already. When we think a minute about what is done here with
> the retained endpoint descriptor in uvideo_vs_open(); mainly nothing.
> We just check if we can access the endpoint selected before by
> uvideo_vs_set_alt(). Therefore it's a kind of double check since
> latest at the next step when we try to open the pipe we would fail if
> the endpoint wouldn't be valid.
>
> My proposal is, leave it in just as a double check with your
> fix to do it for the proper endpoint and print a short debug
> info including the endpoint number before doing the pipe
> opening.
>
> > Fancy stuff this USB.
>
> Sigh ...
>
>
> Index: uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> retrieving revision 1.187
> diff -u -p -u -p -r1.187 uvideo.c
> --- uvideo.c 26 May 2016 04:47:08 -0000 1.187
> +++ uvideo.c 27 May 2016 06:07:22 -0000
> @@ -1817,19 +1817,17 @@ uvideo_vs_open(struct uvideo_softc *sc)
> return (error);
> }
>
> - ed = usbd_interface2endpoint_descriptor(sc->sc_vs_cur->ifaceh, 0);
> + /* double check if we can access the selected endpoint descriptor */
> + ed = usbd_get_endpoint_descriptor(sc->sc_vs_cur->ifaceh,
> + sc->sc_vs_cur->endpoint);
> if (ed == NULL) {
> printf("%s: no endpoint descriptor for VS iface\n",
> DEVNAME(sc));
> return (USBD_INVAL);
> }
> - DPRINTF(1, "%s: open pipe for ", DEVNAME(sc));
> - DPRINTF(1, "bEndpointAddress=0x%02x (0x%02x), wMaxPacketSize=%d (%d)\n",
> - ed->bEndpointAddress,
> - sc->sc_vs_cur->endpoint,
> - UGETW(ed->wMaxPacketSize),
> - sc->sc_vs_cur->psize);
>
> + DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x",
> + DEVNAME(sc), sc->sc_vs_cur->endpoint);
> error = usbd_open_pipe(
> sc->sc_vs_cur->ifaceh,
> sc->sc_vs_cur->endpoint,
>
This one has been committed as well now.
