Hi, Some feedback inline:
On Tue, 28 May 2019 at 18:42:51 -0400, Cody Cutler wrote: > Hello tech, I'm submitting the following patch for inclusion. The patch > implements a driver for the Keyspan USA-19HS USB-to-serial dongle. > > I've used it for a few months now without any problems. Please let me know if > you spot any problems. > > Thanks! > > diff --git sys/arch/amd64/conf/GENERIC sys/arch/amd64/conf/GENERIC > index ad192f4ea1d..052915d10e0 100644 > --- sys/arch/amd64/conf/GENERIC > +++ sys/arch/amd64/conf/GENERIC > @@ -224,6 +224,8 @@ uvscom* at uhub? # SUNTAC Slipper U > VS-10U serial > ucom* at uvscom? > ubsa* at uhub? # Belkin serial adapter > ucom* at ubsa? > +ukspan* at uhub? # Keyspan USA19HS Nit: maybe add "serial adapter" at the end > +ucom* at ukspan? > uftdi* at uhub? # FTDI FT8U100AX serial adapter > ucom* at uftdi? > uplcom* at uhub? # I/O DATA USB-RSAQ2 serial adapter > diff --git sys/dev/usb/files.usb sys/dev/usb/files.usb > index 1036cf36232..29bc1205540 100644 > --- sys/dev/usb/files.usb > +++ sys/dev/usb/files.usb > @@ -317,6 +317,11 @@ device ubsa: ucombus > attach ubsa at uhub > file dev/usb/ubsa.c ubsa > > +# Keyspan USA19HS serial > +device ukspan: ucombus > +attach ukspan at uhub > +file dev/usb/ukspan.c ukspan > + > # Silicon Laboratories CP210x serial > device uslcom: ucombus > attach uslcom at uhub > diff --git sys/dev/usb/ukspan.c sys/dev/usb/ukspan.c > new file mode 100644 > index 00000000000..749144058a0 > --- /dev/null > +++ sys/dev/usb/ukspan.c > @@ -0,0 +1,567 @@ > +#include <sys/param.h> Please add a copyright and license at the top of the file. /usr/share/misc/license.template is a good one to use. Also, is there any documentation for the device that you used? A URL to it is always useful to include in the header. > +#include <sys/systm.h> > +#include <sys/tty.h> > + > +#include <dev/usb/usb.h> > +#include <dev/usb/usbdevs.h> > +#include <dev/usb/usbdi.h> > +#include <dev/usb/usbdi_util.h> > + > +#include <dev/usb/ucomvar.h> > + > +#if 0 > + #define DBG(...) do { printf("ukspan " __VA_ARGS__); } while (0) > +#else > + #define DBG(...) > +#endif Can you put that behind an ifdef UKSPAN_DEBUG and leave a commented out example? That is usually how drivers do it. /* #define UKSPAN_DEBUG */ #ifdef UKSPAN_DEBUG #define DPRINTF(x...) do { printf(x); } while (0); #else #define DPRINTF(x...) #endif > +#define UKSPAN_PARITY_NONE 0x0 > +#define UKSPAN_PARITY_ODD 0x08 > +#define UKSPAN_PARITY_EVEN 0x18 > + > +#define UKSPAN_DATA_5 0x0 > +#define UKSPAN_DATA_6 0x1 > +#define UKSPAN_DATA_7 0x2 > +#define UKSPAN_DATA_8 0x3 > + > +#define UKSPAN_STOP_1 0x0 > +#define UKSPAN_STOP_2 0x4 > + > +#define UKSPAN_MAGIC 0x2 > + > +#define UKSPAN_CLOCK 14769231 > + > +/* > + * The following USB endpoint addresses may be specific to the Keyspan > USA19HS > + * device > + */ > +#define UKSPAN_CONFIG_IDX 1 > +#define UKSPAN_IFACE_IDX 0 > + > +#define UKSPAN_EA_BULKIN (UE_DIR_IN | 1) > +#define UKSPAN_EA_BULKOUT (UE_DIR_OUT | 1) > +#define UKSPAN_EA_CONFIGIN (UE_DIR_IN | 2) > +#define UKSPAN_EA_CONFIGOUT (UE_DIR_OUT | 2) > + > +/* Sent to device on control out endpoint */ > +struct ukspan_cmsg { > + uint8_t setclock; > + uint8_t baudlo; > + uint8_t baudhi; > + uint8_t setlcr; > + uint8_t lcr; > + uint8_t setrxmode; > + uint8_t rxmode; > + uint8_t settxmode; > + uint8_t txmode; > + uint8_t settxflowcontrol; > + uint8_t txflowcontrol; > + uint8_t setrxflowcontrol; > + uint8_t rxflowcontrol; > + uint8_t sendxoff; > + uint8_t sendxon; > + uint8_t xonchar; > + uint8_t xoffchar; > + uint8_t sendchar; > + uint8_t txchar; > + uint8_t setrts; > + uint8_t rts; > + uint8_t setdtr; > + uint8_t dtr; > + > + uint8_t rxforwardingchars; > + uint8_t rxforwardingtimeoutms; > + uint8_t txacksetting; > + > + uint8_t portenabled; > + uint8_t txflush; > + uint8_t txbreak; > + uint8_t loopbackmode; > + > + uint8_t rxflush; > + uint8_t rxforward; > + uint8_t cancelrxoff; > + uint8_t returnstatus; > +}; > + > +/* Received from device on control in endpoint */ > +struct ukspan_smsg { > + uint8_t msr; > + uint8_t cts; > + uint8_t dcd; > + uint8_t dsr; > + uint8_t ri; > + uint8_t txxoff; > + uint8_t rxbreak; > + uint8_t rxoverrun; > + uint8_t rxparity; > + uint8_t rxframe; > + uint8_t portstate; > + uint8_t messageack; > + uint8_t charack; > + uint8_t controlresp; > +}; If these structures are communicated directly with the device, it's a good idea to make the structs have the __packed attribute. struct ukspan_smsg { ... } __packed; > + > +struct ukspan_softc { > + struct device sc_dev; > + struct usbd_device *udev; > + struct usbd_interface *iface; > + struct usbd_pipe *cout_pipe; > + struct usbd_pipe *cin_pipe; > + struct usbd_xfer *ixfer; > + struct usbd_xfer *oxfer; > + struct device *ucom_dev; > + struct ukspan_smsg smsg; > + struct ukspan_cmsg cmsg; > + u_char lsr; > + u_char msr; > +}; > + > +int ukspan_match(struct device *, void *, void *); > +void ukspan_attach(struct device *, struct device *, void *); > +int ukspan_detach(struct device *, int); > + > +void ukspan_close(void *, int); > +int ukspan_open(void *, int); > +int ukspan_param(void *, int, struct termios *); > +void ukspan_set(void *, int, int, int); > +void ukspan_get_status(void *, int, u_char *, u_char *); > + > +void kmsg_init(bool, struct ukspan_cmsg *); > +int send_cmsg(struct ukspan_softc *); Any particular reason those are not prefixed with ukspan_? > +void ukspan_incb(struct usbd_xfer *, void *, usbd_status); > +void ukspan_outcb(struct usbd_xfer *, void *, usbd_status); > +void ukspan_destroy(struct ukspan_softc *); > + > +struct cfdriver ukspan_cd = { > + NULL, "ukspan", DV_DULL > +}; > + > +const struct cfattach ukspan_ca = { > + sizeof(struct ukspan_softc), ukspan_match, ukspan_attach, > + ukspan_detach > +}; > + > +static const struct usb_devno ukspan_devs[] = { > + { USB_VENDOR_KEYSPAN, USB_PRODUCT_KEYSPAN_USA19HS}, Pedantic: add a space before the }, > +}; > + > +static struct ucom_methods ukspan_methods = { > + .ucom_get_status = ukspan_get_status, > + .ucom_set = ukspan_set, > + .ucom_param = ukspan_param, > + .ucom_ioctl = NULL, > + .ucom_open = ukspan_open, > + .ucom_close = ukspan_close, > + .ucom_read = NULL, > + .ucom_write = NULL, > +}; > + > +int > +ukspan_match(struct device *parent, void *match, void *aux) > +{ > + struct usb_attach_arg *uaa = aux; > + > + if (uaa->iface != NULL) > + return UMATCH_NONE; > + > + int found = usb_lookup(ukspan_devs, uaa->vendor, uaa->product) != NULL; > + return found ? UMATCH_VENDOR_PRODUCT : UMATCH_NONE; > +} > + > +void > +ukspan_attach(struct device *parent, struct device *self, void *aux) > +{ > + struct ukspan_softc *sc = (struct ukspan_softc *)self; > + struct usb_attach_arg *uaa = aux; > + struct usbd_device *dev = uaa->device; > + struct ucom_attach_args uca = {0}; > + usb_endpoint_descriptor_t *ed; > + const char *devname = sc->sc_dev.dv_xname; > + usbd_status err; > + int t1, t2, t3, t4; > + > + DBG("attach\n"); > + > + sc->udev = dev; > + sc->cin_pipe = sc->cout_pipe = NULL; > + sc->ixfer = sc->oxfer = NULL; > + sc->ucom_dev = NULL; > + > + /* > + * Switch to configuration 1 where the transfer mode of the input > + * endpoints is bulk instead of interrupt, as ucom expects > + */ > + err = usbd_set_config_index(sc->udev, UKSPAN_CONFIG_IDX, 1); > + if (err) { > + printf("%s: set config failed\n", devname); > + goto fail; > + } > + > + err = usbd_device2interface_handle(sc->udev, UKSPAN_IFACE_IDX, > + &sc->iface); > + if (err) { > + printf("%s: get interface failed\n", devname); > + goto fail; > + } > + > + ed = usbd_get_endpoint_descriptor(sc->iface, UKSPAN_EA_BULKIN); > + t1 = UE_GET_XFERTYPE(ed->bmAttributes); > + uca.ibufsize = UGETW(ed->wMaxPacketSize); > + uca.bulkin = UKSPAN_EA_BULKIN; > + > + ed = usbd_get_endpoint_descriptor(sc->iface, UKSPAN_EA_BULKOUT); > + t2 = UE_GET_XFERTYPE(ed->bmAttributes); > + uca.obufsize = UGETW(ed->wMaxPacketSize); > + uca.bulkout = UKSPAN_EA_BULKOUT; > + > + ed = usbd_get_endpoint_descriptor(sc->iface, UKSPAN_EA_CONFIGIN); > + t3 = UE_GET_XFERTYPE(ed->bmAttributes); > + if (UGETW(ed->wMaxPacketSize) < sizeof(struct ukspan_smsg)) { > + printf("%s: in config packet size too small\n", devname); > + goto fail; > + } > + > + ed = usbd_get_endpoint_descriptor(sc->iface, UKSPAN_EA_CONFIGOUT); > + t4 = UE_GET_XFERTYPE(ed->bmAttributes); > + if (UGETW(ed->wMaxPacketSize) < sizeof(struct ukspan_cmsg)) { > + printf("%s: out config packet size too small\n", devname); > + goto fail; > + } > + > + if (t1 != UE_BULK || t2 != UE_BULK || t3 != UE_BULK || t4 != UE_BULK) { > + printf("%s: unexpected xfertypes %x %x %x %x != %x\n", devname, > + t1, t2, t3, t4, UE_BULK); > + goto fail; > + } > + > + /* Resource acquisition starts here */ > + err = usbd_open_pipe(sc->iface, UKSPAN_EA_CONFIGOUT, USBD_EXCLUSIVE_USE, > + &sc->cout_pipe); > + if (err) { > + printf("%s: failed to create control out pipe\n", devname); > + goto fail; > + } > + > + err = usbd_open_pipe(sc->iface, UKSPAN_EA_CONFIGIN, USBD_EXCLUSIVE_USE, > + &sc->cin_pipe); > + if (err) { > + printf("%s: failed to create control out pipe\n", devname); > + goto fail; > + } > + > + sc->ixfer = usbd_alloc_xfer(sc->udev); > + sc->oxfer = usbd_alloc_xfer(sc->udev); > + if (!sc->ixfer || !sc->oxfer) { > + printf("%s: failed to allocate xfers\n", devname); > + goto fail; > + } > + > + usbd_setup_xfer(sc->ixfer, sc->cin_pipe, sc, &sc->smsg, > + sizeof(sc->smsg), 0, USBD_NO_TIMEOUT, ukspan_incb); > + err = usbd_transfer(sc->ixfer); > + if (err && err != USBD_IN_PROGRESS) { > + printf("%s: failed to start ixfer\n", devname); > + goto fail; > + } > + > + uca.portno = UCOM_UNK_PORTNO; > + uca.ibufsizepad = uca.ibufsize; > + uca.opkthdrlen = 0; > + uca.device = dev; > + uca.iface = sc->iface; > + uca.methods = &ukspan_methods; > + uca.arg = sc; > + uca.info = NULL; > + > + sc->ucom_dev = config_found_sm(self, &uca, ucomprint, ucomsubmatch); > + > + DBG("attach done\n"); > + > + return; > +fail: > + ukspan_destroy(sc); > + usbd_deactivate(sc->udev); > +} > + > +int > +ukspan_detach(struct device *self, int flags) > +{ > + struct ukspan_softc *sc = (struct ukspan_softc *)self; > + DBG("detach\n"); > + > + ukspan_destroy(sc); > + > + if (sc->ucom_dev) { > + config_detach(sc->ucom_dev, flags); > + sc->ucom_dev = NULL; > + } > + return 0; > +} > + > +void > +ukspan_outcb(struct usbd_xfer *xfer, void *priv, usbd_status status) > +{ > + struct ukspan_softc *sc = (struct ukspan_softc *)priv; > + const char *devname = sc->sc_dev.dv_xname; > + > + DBG("outcb\n"); > + > + if (usbd_is_dying(sc->udev)) { > + DBG("usb dying\n"); > + return; > + } > + if (status != USBD_NORMAL_COMPLETION) { > + printf("%s: oxfer failed\n", devname); > + return; > + } > +} > + > +void > +ukspan_incb(struct usbd_xfer *xfer, void *priv, usbd_status status) > +{ > + struct ukspan_softc *sc = (struct ukspan_softc *)priv; > + const char *devname = sc->sc_dev.dv_xname; > + const struct ukspan_smsg *smsg = &sc->smsg; > + usbd_status err; > + u_int32_t len; > + > + DBG("incb\n"); > + > + if (usbd_is_dying(sc->udev)) { > + printf("%s: usb dying\n", devname); > + return; > + } > + if (!sc->cin_pipe || !sc->ixfer) { > + printf("%s: no cin_pipe, but not dying?\n", devname); > + return; > + } > + if (status != USBD_NORMAL_COMPLETION) { > + if (status != USBD_NOT_STARTED && status != USBD_CANCELLED) > + printf("%s: ixfer failed\n", devname); > + return; > + } > + > + usbd_get_xfer_status(xfer, NULL, NULL, &len, NULL); > + if (len < sizeof(struct ukspan_smsg)) { > + printf("%s: short read\n", devname); > + return; > + } > + > + /* The device provides the actual MSR register */ > + sc->msr = smsg->msr; > + /* But not LSR... */ > + sc->lsr = (smsg->rxoverrun ? ULSR_OE : 0) | > + (smsg->rxparity ? ULSR_PE : 0) | > + (smsg->rxframe ? ULSR_FE : 0) | > + (smsg->rxbreak ? ULSR_BI : 0); > + ucom_status_change((struct ucom_softc *)sc->ucom_dev); > + > + usbd_setup_xfer(sc->ixfer, sc->cin_pipe, sc, &sc->smsg, > + sizeof(sc->smsg), USBD_SHORT_XFER_OK, USBD_NO_TIMEOUT, > + ukspan_incb); > + err = usbd_transfer(sc->ixfer); > + if (err && err != USBD_IN_PROGRESS) > + printf("%s: usbd transfer failed\n", devname); > +} > + > +void > +ukspan_get_status(void *addr, int portno, u_char *lsr, u_char *msr) > +{ > + struct ukspan_softc *sc = addr; > + DBG("get status\n"); > + if (lsr) > + *lsr = sc->lsr; > + if (msr) > + *msr = sc->msr; > +} > + > +void > +kmsg_init(bool opening, struct ukspan_cmsg *omsg) > +{ > + bzero(omsg, sizeof(*omsg)); > + > + omsg->xonchar = 17; > + omsg->xoffchar = 19; > + > + omsg->rxforwardingchars = 16; > + omsg->rxforwardingtimeoutms = 16; > + omsg->txacksetting = 0; > + omsg->txbreak = 0; > + if (opening) { > + omsg->portenabled = 1; > + omsg->rxflush = 1; > + } > +} > + > +int > +send_cmsg(struct ukspan_softc *sc) > +{ > + const char *devname = sc->sc_dev.dv_xname; > + usbd_status err; > + > + usbd_setup_xfer(sc->oxfer, sc->cout_pipe, sc, &sc->cmsg, > + sizeof(sc->cmsg), USBD_SYNCHRONOUS, USBD_NO_TIMEOUT, ukspan_outcb); > + err = usbd_transfer(sc->oxfer); > + if (err != USBD_NORMAL_COMPLETION) { > + printf("%s: control xfer failed\n", devname); > + return EIO; > + } > + return 0; > +} > + > +void > +ukspan_set(void *addr, int portno, int reg, int onoff) > +{ > + struct ukspan_softc *sc = addr; > + const char *devname = sc->sc_dev.dv_xname; > + DBG("set %#x = %#x\n", reg, onoff); > + int flag = !!onoff; > + switch (reg) { > + case UCOM_SET_DTR: > + sc->cmsg.setdtr = 1; > + sc->cmsg.dtr = flag; > + break; > + case UCOM_SET_RTS: > + sc->cmsg.setrts = 1; > + sc->cmsg.rts = flag; > + break; > + case UCOM_SET_BREAK: > + sc->cmsg.txbreak = flag; > + break; > + default: > + printf("%s: unhandled reg %#x\n", devname, reg); > + return; > + } > + send_cmsg(sc); > +} > + > +int > +ukspan_param(void *addr, int portno, struct termios *ti) > +{ > + struct ukspan_softc *sc = addr; > + const char *devname = sc->sc_dev.dv_xname; > + struct ukspan_cmsg *cmsg = &sc->cmsg; > + speed_t baud; > + tcflag_t cflag; > + u_int32_t div; > + u_int8_t lcr; > + > + DBG("param: %#x %#x %#x\n", ti->c_ospeed, ti->c_cflag, ti->c_iflag); > + > + /* Set baud */ > + div = 1; > + baud = ti->c_ospeed; > + switch (baud) { > + case B300: > + case B600: > + case B1200: > + case B2400: > + case B4800: > + case B9600: > + case B19200: > + case B38400: > + case B57600: > + case B115200: > + case B230400: > + div = UKSPAN_CLOCK / (baud * 16); > + break; > + default: > + printf("%s: unexpected baud: %d\n", devname, baud); > + return EINVAL; > + } > + > + cmsg->setclock = 1; > + cmsg->baudlo = div & 0xff; > + cmsg->baudhi = div >> 8; > + > + cmsg->setrxmode = 1; > + cmsg->settxmode = 1; > + if (baud > 57600) > + cmsg->rxmode = cmsg->txmode = UKSPAN_MAGIC; > + else > + cmsg->rxmode = cmsg->txmode = 0; > + > + /* Set parity, data, and stop bits */ > + cflag = ti->c_cflag; > + if (cflag & PARENB) > + lcr = (cflag & PARODD) ? UKSPAN_PARITY_ODD : UKSPAN_PARITY_EVEN; > + else > + lcr = UKSPAN_PARITY_NONE; > + switch (cflag & CSIZE) { > + case CS5: > + lcr |= UKSPAN_DATA_5; > + break; > + case CS6: > + lcr |= UKSPAN_DATA_6; > + break; > + case CS7: > + lcr |= UKSPAN_DATA_7; > + break; > + case CS8: > + lcr |= UKSPAN_DATA_8; > + break; > + } > + > + lcr |= (cflag & CSTOPB) ? UKSPAN_STOP_2 : UKSPAN_STOP_1; > + > + cmsg->setlcr = 1; > + cmsg->lcr = lcr; > + > + /* XXX flow control? */ > + > + send_cmsg(sc); > + return 0; > +} > + > +int > +ukspan_open(void *addr, int portno) > +{ > + struct ukspan_softc *sc = addr; > + int ret; > + > + DBG("open\n"); > + if (usbd_is_dying(sc->udev)) { > + DBG("usb dying\n"); > + return ENXIO; > + } > + > + kmsg_init(true, &sc->cmsg); > + ret = send_cmsg(sc); > + return ret; > +} > + > +void > +ukspan_close(void *addr, int portno) > +{ > + struct ukspan_softc *sc = addr; > + DBG("close\n"); > + if (usbd_is_dying(sc->udev)) { > + DBG("usb dying\n"); > + return; > + } > + kmsg_init(false, &sc->cmsg); > + send_cmsg(sc); > +} > + > +void > +ukspan_destroy(struct ukspan_softc *sc) > +{ > + DBG("destroy\n"); > + if (sc->cin_pipe) { > + usbd_close_pipe(sc->cin_pipe); > + sc->cin_pipe = NULL; > + } > + if (sc->cout_pipe) { > + usbd_close_pipe(sc->cout_pipe); > + sc->cout_pipe = NULL; > + } > + if (sc->oxfer) { > + usbd_free_xfer(sc->oxfer); > + sc->oxfer = NULL; > + } > + if (sc->ixfer) { > + usbd_free_xfer(sc->ixfer); > + sc->ixfer = NULL; > + } > +} > diff --git sys/dev/usb/usbdevs.h sys/dev/usb/usbdevs.h > index 84f91a79403..78dc057745b 100644 > --- sys/dev/usb/usbdevs.h > +++ sys/dev/usb/usbdevs.h > @@ -2461,6 +2461,7 @@ > #define USB_PRODUCT_KEYSPAN_USA18XA 0x0117 /* USA-18XA > serial */ > #define USB_PRODUCT_KEYSPAN_USA19QW_NF 0x0118 /* USA-19WQ > serial */ > #define USB_PRODUCT_KEYSPAN_USA19QW 0x0119 /* USA-19WQ > serial */ > +#define USB_PRODUCT_KEYSPAN_USA19HS 0x0121 /* USA-19HS > serial */ > #define USB_PRODUCT_KEYSPAN_UIA10 0x0201 /* UIA-10 > remote control */ > #define USB_PRODUCT_KEYSPAN_UIA11 0x0202 /* UIA-11 > remote control */ The routine for adding USB devices is to just add them to /usr/src/sys/dev/usb/usbdevs, then run 'make' in /usr/src/sys/dev/usb/. That will automatically regenerate usbdevs.h and usbdevs_data.h.