uftdi.c code review request (simple)
Claus Andersen <[email protected]>
| Newsgroups | gmane.os.netbsd.devel.general |
|---|---|
| Message-ID | <[email protected]> |
Hello! I have made some changes to the uftdi driver on my 3.0.1 system and thought it might be worth contributing them back. This is however a first for me so I would highly appreciate any and all feedback. I have attached my "diff -u". 1) I changed USB_MATCH to use usb_lookup like the rest of the u* drivers do. I think it looks cleaner and is easier to maintain? 2) I added the following devices as they where known in Free/OpenBSD: B&B Electronics uLinks RS-422/485 Falcom Twist GSM/GPRS modem Falcom Samba 55/56 GSM/GPRS modem Future Technology Devices KW Future Technology Devices YS Future Technology Devices Y6 Future Technology Devices Y8 Future Technology Devices IC Future Technology Devices DB9 Future Technology Devices RS232 Future Technology Devices Y9 Future Technology Devices / Coastal ChipWorks TNC-X Future Technology Devices / Matrix Orbital MX200 Series LCD Future Technology Devices / Matrix Orbital LK202-24 LCD Future Technology Devices / Matrix Orbital LK204-24 LCD Future Technology Devices / Crystalfontz CFA-632 LCD Future Technology Devices / Crystalfontz CFA-634 LCD Future Technology Devices / Crystalfontz CFA-633 LCD Interpid Control Systems ValueCAN Interpid Control Systems NeoVI Blue SIIG SIIG2 US2308 Serial (Some of these where already listed in usbdevs but not in uftdi) (I do only have a Falcom Samba to test against) 3) I added the "switch (uaa->vendor)" in USB_ATTACH to avoid clashing product id's across vendors. The switch was choosen rather than if to make additions easier. Switch is faster than if (Suboptimizing is still optimizing ;-)) OK? 4) The "switch (uaa->product)" in USB_ATTACH seemed bloated. Only one device is UFTDI_TYPE_SIO - the rest are UFTDI_TYPE_8U232AM. The previous "default:" had a /* Can't happen */ comment as we should only see devices matched by USB_MATCH. So I thought it reasonable and less prone to error when adding devices to make UFTDI_TYPE_8U232AM the default? 5) If I'm wrong regarding to 3 or 4 and the devices should be handled explicitly in USB_ATTACH would it not be nicer with a static struct á la "usb_lookup"? 6) It says "The ucom layer needs to be extended first" to handle more ports. Has this happened? The changes compiles cleanly and worked with my Falcom Samba 55 GSM/GPRS modem: Attach: uftdi0 at uhub0 port 1 uftdi0: FALCOM Falcom SAMBA, rev 1.10/2.00, addr 2 ucom0 at uftdi0 portno 1: portno 1 Detach: uftdi0: at uhub0 port 1 (addr 2) disconnected ucom0 detached uftdi0 detached (and I've been able to send/receive SMS) Kind Regards, Claus Andersen
usbdevs.diff
(text/plain, 2.6 KB)
--- usbdevs.org 2006-09-23 18:44:38.000000000 +0200 +++ usbdevs 2006-09-24 12:24:42.000000000 +0200 @@ -300,6 +300,7 @@ vendor DIAMOND 0x0841 Diamond vendor NETGEAR 0x0846 BayNETGEAR vendor ACTIVEWIRE 0x0854 ActiveWire +vendor BBELECTRONICS 0x0856 B&B Electronics vendor PORTGEAR 0x085a PortGear vendor NETGEAR2 0x0864 Netgear vendor SYSTEMTALKS 0x086e System Talks @@ -388,6 +389,7 @@ vendor PILOTECH 0x0eaf Pilotech vendor EGALAX 0x0eef eGalax vendor AIRPRIME 0x0f3d AirPrime, Incorporated +vendor FALCOM 0x0f94 Falcom Wireless Communications GmbH vendor QUALCOMM 0x1004 Qualcomm vendor MOTOROLA 0x1063 Motorola vendor CCYU 0x1065 CCYU Technology @@ -613,6 +615,9 @@ /* Avision products */ product AVISION 1200U 0x0268 1200U scanner +/* B&B Electronics products */ +product BBELECTRONICS USOTL4 0xAC01 uLinks RS-422/485 + /* Belkin products */ /*product BELKIN F5U111 0x???? F5U111 Ethernet adapter*/ product BELKIN2 F5U002 0x0002 F5U002 Parallel printer adapter @@ -850,6 +855,10 @@ /* Extended Systems products */ product EXTENDED XTNDACCESS 0x0100 XTNDAccess IrDA +/* Falcom products */ +product FALCOM TWIST 0x0001 Twist GSM/GPRS modem +product FALCOM SAMBA 0x0005 Samba 55/56 GSM/GPRS modem + /* Freecom products */ product FREECOM DVD 0xfc01 Connector for DVD drive @@ -857,13 +866,22 @@ product FTDI SERIAL_8U232AM 0x6001 8U232AM Serial converter product FTDI PS2KBDMS 0x8371 PS/2 Keyboard/Mouse product FTDI SERIAL_8U100AX 0x8372 8U100AX Serial converter +product FTDI MHAM_KW 0xeee8 KW +product FTDI MHAM_YS 0xeee9 YS +product FTDI MHAM_Y6 0xeeea Y6 +product FTDI MHAM_Y8 0xeeeb Y8 +product FTDI MHAM_IC 0xeeec IC +product FTDI MHAM_DB9 0xeeed DB9 +product FTDI MHAM_RS232 0xeeee RS232 +product FTDI MHAM_Y9 0xeeef Y9 +product FTDI COASTAL_TNCX 0xf448 Coastal ChipWorks TNC-X product FTDI LCD_MX200_USB 0xfa01 Matrix Orbital MX200 Series LCD product FTDI LCD_LK202_24_USB 0xfa03 Matrix Orbital LK202-24 LCD -product FTDI LCD_LK204_24 0xfa04 Matrix Orbital LK204-24 LCD +product FTDI LCD_LK204_24_USB 0xfa04 Matrix Orbital LK204-24 LCD product FTDI LCD_CFA_632 0xfc08 Crystalfontz CFA-632 LCD product FTDI LCD_CFA_634 0xfc09 Crystalfontz CFA-634 LCD product FTDI LCD_CFA_633 0xfc0b Crystalfontz CFA-633 LCD -product FTDI CFA_631 0xfc0c Crystalfontz CFA-631 LCD +product FTDI LCD_CFA_631 0xfc0c Crystalfontz CFA-631 LCD product FTDI SEMC_DSS20 0xfc82 SEMC DSS-20 SyncStation /* Fuji photo products */
uftdi.c.diff
(text/plain, 3.6 KB)
--- uftdi.c.org 2006-09-23 18:52:26.000000000 +0200
+++ uftdi.c 2006-09-24 13:59:17.000000000 +0200
@@ -124,6 +124,40 @@
uftdi_write,
};
+/*
+ * The devices default to UFTDI_TYPE_8U232AM.
+ * Remember to update USB_ATTACH if it should be UFTDI_TYPE_SIO instead
+ */
+static const struct usb_devno uftdi_devs[] = {
+ { USB_VENDOR_BBELECTRONICS, USB_PRODUCT_BBELECTRONICS_USOTL4 },
+ { USB_VENDOR_FALCOM, USB_PRODUCT_FALCOM_TWIST },
+ { USB_VENDOR_FALCOM, USB_PRODUCT_FALCOM_SAMBA },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_SERIAL_8U100AX },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_SERIAL_8U232AM },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_KW },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_YS },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_Y6 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_Y8 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_IC },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_DB9 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_RS232 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_MHAM_Y9 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_COASTAL_TNCX },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_SEMC_DSS20 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_LK202_24_USB },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_LK204_24_USB },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_MX200_USB },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_CFA_631 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_CFA_632 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_CFA_633 },
+ { USB_VENDOR_FTDI, USB_PRODUCT_FTDI_LCD_CFA_634 },
+ { USB_VENDOR_INTREPIDCS, USB_PRODUCT_INTREPIDCS_VALUECAN },
+ { USB_VENDOR_INTREPIDCS, USB_PRODUCT_INTREPIDCS_NEOVI },
+ { USB_VENDOR_SEALEVEL, USB_PRODUCT_SEALEVEL_USBSERIAL },
+ { USB_VENDOR_SIIG2, USB_PRODUCT_SIIG2_US2308 },
+};
+#define uftdi_lookup(v, p) usb_lookup(uftdi_devs, v, p)
+
USB_DECLARE_DRIVER(uftdi);
USB_MATCH(uftdi)
@@ -136,20 +170,8 @@
DPRINTFN(20,("uftdi: vendor=0x%x, product=0x%x\n",
uaa->vendor, uaa->product));
- if (uaa->vendor == USB_VENDOR_FTDI &&
- (uaa->product == USB_PRODUCT_FTDI_SERIAL_8U100AX ||
- uaa->product == USB_PRODUCT_FTDI_SERIAL_8U232AM ||
- uaa->product == USB_PRODUCT_FTDI_SEMC_DSS20 ||
- uaa->product == USB_PRODUCT_FTDI_LCD_LK202_24_USB ||
- uaa->product == USB_PRODUCT_FTDI_LCD_MX200_USB ||
- uaa->product == USB_PRODUCT_FTDI_CFA_631))
- return (UMATCH_VENDOR_PRODUCT);
-
- if (uaa->vendor == USB_VENDOR_SEALEVEL &&
- uaa->product == USB_PRODUCT_SEALEVEL_USBSERIAL)
- return (UMATCH_VENDOR_PRODUCT);
-
- return (UMATCH_NONE);
+ return (uftdi_lookup(uaa->vendor, uaa->product) != NULL ?
+ UMATCH_VENDOR_PRODUCT : UMATCH_NONE);
}
USB_ATTACH(uftdi)
@@ -191,24 +213,21 @@
sc->sc_udev = dev;
sc->sc_iface = iface;
- switch (uaa->product) {
- case USB_PRODUCT_FTDI_SERIAL_8U100AX:
- sc->sc_type = UFTDI_TYPE_SIO;
- sc->sc_hdrlen = 1;
+ switch( uaa->vendor ) {
+ case USB_VENDOR_FTDI:
+ switch (uaa->product) {
+ case USB_PRODUCT_FTDI_SERIAL_8U100AX:
+ sc->sc_type = UFTDI_TYPE_SIO;
+ sc->sc_hdrlen = 1;
+ break;
+ default: /* Most uftdi devices are 8U232AM */
+ sc->sc_type = UFTDI_TYPE_8U232AM;
+ sc->sc_hdrlen = 0;
+ }
break;
-
- case USB_PRODUCT_FTDI_SEMC_DSS20:
- case USB_PRODUCT_FTDI_SERIAL_8U232AM:
- case USB_PRODUCT_FTDI_LCD_LK202_24_USB:
- case USB_PRODUCT_FTDI_LCD_MX200_USB:
- case USB_PRODUCT_FTDI_CFA_631:
- case USB_PRODUCT_SEALEVEL_USBSERIAL:
+ default: /* Most uftdi devices are 8U232AM */
sc->sc_type = UFTDI_TYPE_8U232AM;
sc->sc_hdrlen = 0;
- break;
-
- default: /* Can't happen */
- goto bad;
}
uca.bulkin = uca.bulkout = -1;