From: Kirill A. Korinsky Subject: Re: Support Jabra PanaCast 20 webcam To: me@xha.li, tech@openbsd.org, mglocker@openbsd.org Date: Wed, 27 Nov 2024 20:12:51 +0100 On Wed, 27 Nov 2024 19:09:47 +0100, Stuart Henderson wrote: > > On 2024/11/27 17:54, Kirill A. Korinsky wrote: > > > > + /* do device negotiation with commit after set alternate interface */ > > would "renegotiate after setting alternate interface" make sense? > Thanks, I think renegotiate is better. > > + error = uvideo_vs_negotiation(sc, 1); > > + if (error != USBD_NORMAL_COMPLETION) > > + printf("%s: could not negotiate after setting alternate interface!\n", > > + DEVNAME(sc)); > > + > > DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n", > > DEVNAME(sc), sc->sc_vs_cur->endpoint); > > please stick to <80 columns (either shorter text, or split the > string across two lines) > > should that return (error) too? > This is a point that I'm thinking a lot. From one hand it makes perfect sense to return error here, from another hand it may break some webcam which doesn't support it, but may continue to work, or may not. Thus, Linux uses 0 as a hardcoded alternate interface, has comments that soem device crashes without it, and returns error: https://github.com/torvalds/linux/blob/v6.12/drivers/media/usb/uvc/uvc_video.c#L2180-L2220 Base on all this return error is right move, I guess. Index: sys/dev/usb/uvideo.c =================================================================== RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v diff -u -p -r1.223 uvideo.c --- sys/dev/usb/uvideo.c 27 Nov 2024 11:37:23 -0000 1.223 +++ sys/dev/usb/uvideo.c 27 Nov 2024 18:45:42 -0000 @@ -1899,6 +1899,14 @@ uvideo_vs_open(struct uvideo_softc *sc) return (USBD_INVAL); } + /* renegotiate with commit after setting alternate interface */ + error = uvideo_vs_negotiation(sc, 1); + if (error != USBD_NORMAL_COMPLETION) { + printf("%s: could not renegotiate after setting " + "alternate interface!\n", DEVNAME(sc)); + return (error); + } + DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n", DEVNAME(sc), sc->sc_vs_cur->endpoint); error = usbd_open_pipe( -- wbr, Kirill