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,
