Subject: uftdi.c code review request (simple)
To: None <tech-misc@netbsd.org>
From: Claus Andersen <clan@wheel.dk>
List: tech-misc
Date: 09/24/2006 15:14:57
  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--0-1717552568-1159103697=:19200
Content-Type: TEXT/PLAIN; charset=iso-8859-1; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE

Hello!

I have made some changes to the uftdi driver on my 3.0.1 system and=20
thought it might be worth contributing them back. This is however a first=
=20
for me so I would highly appreciate any and all feedback. I have attached=
=20
my "diff -u".

1) I changed USB_MATCH to use usb_lookup like the rest of the u* drivers=20
do. I think it looks cleaner and is easier to maintain?

2) I added the following devices as they where known in Free/OpenBSD:
 =09B&B Electronics uLinks RS-422/485
 =09Falcom Twist GSM/GPRS modem
 =09Falcom Samba 55/56 GSM/GPRS modem
 =09Future Technology Devices KW
 =09Future Technology Devices YS
 =09Future Technology Devices Y6
 =09Future Technology Devices Y8
 =09Future Technology Devices IC
 =09Future Technology Devices DB9
 =09Future Technology Devices RS232
 =09Future Technology Devices Y9
 =09Future Technology Devices / Coastal ChipWorks TNC-X
 =09Future Technology Devices / Matrix Orbital MX200 Series LCD
 =09Future Technology Devices / Matrix Orbital LK202-24 LCD
 =09Future Technology Devices / Matrix Orbital LK204-24 LCD
 =09Future Technology Devices / Crystalfontz CFA-632 LCD
 =09Future Technology Devices / Crystalfontz CFA-634 LCD
 =09Future Technology Devices / Crystalfontz CFA-633 LCD
 =09Interpid Control Systems ValueCAN
 =09Interpid Control Systems NeoVI Blue
 =09SIIG 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=20
product id's across vendors. The switch was choosen rather than if to make=
=20
additions easier. Switch is faster than if (Suboptimizing is still=20
optimizing ;-)) OK?

4) The "switch (uaa->product)" in USB_ATTACH seemed bloated. Only one=20
device is UFTDI_TYPE_SIO - the rest are UFTDI_TYPE_8U232AM. The previous=20
"default:" had a /* Can't happen */ comment as we should only see devices=
=20
matched by USB_MATCH. So I thought it reasonable and less prone to error=20
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=20
explicitly in USB_ATTACH would it not be nicer with a static struct =E1 la=
=20
"usb_lookup"?

6) It says "The ucom layer needs to be extended first" to handle more=20
ports. Has this happened?

The changes compiles cleanly and worked with my Falcom Samba 55 GSM/GPRS=20
modem:
Attach:
 =09uftdi0 at uhub0 port 1
 =09uftdi0: FALCOM Falcom SAMBA, rev 1.10/2.00, addr 2
 =09ucom0 at uftdi0 portno 1: portno 1
Detach:
 =09uftdi0: at uhub0 port 1 (addr 2) disconnected
 =09ucom0 detached
 =09uftdi0 detached
(and I've been able to send/receive SMS)

Kind Regards,
Claus Andersen
--0-1717552568-1159103697=:19200
Content-Type: TEXT/plain; charset=US-ASCII; name=usbdevs.diff
Content-Transfer-Encoding: BASE64
Content-ID: <20060924151457.O19200@freesbee.wheel.dk>
Content-Description: diff -u of usbdevs
Content-Disposition: attachment; filename=usbdevs.diff

LS0tIHVzYmRldnMub3JnCTIwMDYtMDktMjMgMTg6NDQ6MzguMDAwMDAwMDAw
ICswMjAwDQorKysgdXNiZGV2cwkyMDA2LTA5LTI0IDEyOjI0OjQyLjAwMDAw
MDAwMCArMDIwMA0KQEAgLTMwMCw2ICszMDAsNyBAQA0KIHZlbmRvciBESUFN
T05ECQkweDA4NDEJRGlhbW9uZA0KIHZlbmRvciBORVRHRUFSCQkweDA4NDYJ
QmF5TkVUR0VBUg0KIHZlbmRvciBBQ1RJVkVXSVJFCTB4MDg1NAlBY3RpdmVX
aXJlDQordmVuZG9yIEJCRUxFQ1RST05JQ1MgICAgMHgwODU2ICBCJkIgRWxl
Y3Ryb25pY3MNCiB2ZW5kb3IgUE9SVEdFQVIJCTB4MDg1YQlQb3J0R2Vhcg0K
IHZlbmRvciBORVRHRUFSMgkJMHgwODY0CU5ldGdlYXINCiB2ZW5kb3IgU1lT
VEVNVEFMS1MJMHgwODZlCVN5c3RlbSBUYWxrcw0KQEAgLTM4OCw2ICszODks
NyBAQA0KIHZlbmRvciBQSUxPVEVDSAkJMHgwZWFmCVBpbG90ZWNoDQogdmVu
ZG9yIEVHQUxBWAkJMHgwZWVmCWVHYWxheA0KIHZlbmRvciBBSVJQUklNRQkJ
MHgwZjNkCUFpclByaW1lLCBJbmNvcnBvcmF0ZWQNCit2ZW5kb3IgRkFMQ09N
ICAgICAgICAgICAweDBmOTQgIEZhbGNvbSBXaXJlbGVzcyBDb21tdW5pY2F0
aW9ucyBHbWJIDQogdmVuZG9yIFFVQUxDT01NCQkweDEwMDQJUXVhbGNvbW0N
CiB2ZW5kb3IgTU9UT1JPTEEJCTB4MTA2MwlNb3Rvcm9sYQ0KIHZlbmRvciBD
Q1lVCQkweDEwNjUJQ0NZVSBUZWNobm9sb2d5DQpAQCAtNjEzLDYgKzYxNSw5
IEBADQogLyogQXZpc2lvbiBwcm9kdWN0cyAqLw0KIHByb2R1Y3QgQVZJU0lP
TiAxMjAwVQkJMHgwMjY4CTEyMDBVIHNjYW5uZXINCiANCisvKiBCJkIgRWxl
Y3Ryb25pY3MgcHJvZHVjdHMgKi8NCitwcm9kdWN0IEJCRUxFQ1RST05JQ1Mg
VVNPVEw0ICAgIDB4QUMwMSAgdUxpbmtzIFJTLTQyMi80ODUNCisNCiAvKiBC
ZWxraW4gcHJvZHVjdHMgKi8NCiAvKnByb2R1Y3QgQkVMS0lOIEY1VTExMQkJ
MHg/Pz8/CUY1VTExMSBFdGhlcm5ldCBhZGFwdGVyKi8NCiBwcm9kdWN0IEJF
TEtJTjIgRjVVMDAyCQkweDAwMDIJRjVVMDAyIFBhcmFsbGVsIHByaW50ZXIg
YWRhcHRlcg0KQEAgLTg1MCw2ICs4NTUsMTAgQEANCiAvKiBFeHRlbmRlZCBT
eXN0ZW1zIHByb2R1Y3RzICovDQogcHJvZHVjdCBFWFRFTkRFRCBYVE5EQUND
RVNTCTB4MDEwMAlYVE5EQWNjZXNzIElyREENCiANCisvKiBGYWxjb20gcHJv
ZHVjdHMgKi8NCitwcm9kdWN0IEZBTENPTSBUV0lTVCAgICAgICAgICAgIDB4
MDAwMSAgVHdpc3QgR1NNL0dQUlMgbW9kZW0NCitwcm9kdWN0IEZBTENPTSBT
QU1CQSAgICAgICAgICAgIDB4MDAwNSAgU2FtYmEgNTUvNTYgR1NNL0dQUlMg
bW9kZW0NCisNCiAvKiBGcmVlY29tIHByb2R1Y3RzICovDQogcHJvZHVjdCBG
UkVFQ09NIERWRAkJMHhmYzAxCUNvbm5lY3RvciBmb3IgRFZEIGRyaXZlDQog
DQpAQCAtODU3LDEzICs4NjYsMjIgQEANCiBwcm9kdWN0IEZUREkgU0VSSUFM
XzhVMjMyQU0JMHg2MDAxCThVMjMyQU0gU2VyaWFsIGNvbnZlcnRlcg0KIHBy
b2R1Y3QgRlRESSBQUzJLQkRNUwkJMHg4MzcxCVBTLzIgS2V5Ym9hcmQvTW91
c2UNCiBwcm9kdWN0IEZUREkgU0VSSUFMXzhVMTAwQVgJMHg4MzcyCThVMTAw
QVggU2VyaWFsIGNvbnZlcnRlcg0KK3Byb2R1Y3QgRlRESSBNSEFNX0tXICAg
ICAgICAgICAgMHhlZWU4ICBLVw0KK3Byb2R1Y3QgRlRESSBNSEFNX1lTICAg
ICAgICAgICAgMHhlZWU5ICBZUw0KK3Byb2R1Y3QgRlRESSBNSEFNX1k2ICAg
ICAgICAgICAgMHhlZWVhICBZNg0KK3Byb2R1Y3QgRlRESSBNSEFNX1k4ICAg
ICAgICAgICAgMHhlZWViICBZOA0KK3Byb2R1Y3QgRlRESSBNSEFNX0lDICAg
ICAgICAgICAgMHhlZWVjICBJQw0KK3Byb2R1Y3QgRlRESSBNSEFNX0RCOSAg
ICAgICAgICAgMHhlZWVkICBEQjkNCitwcm9kdWN0IEZUREkgTUhBTV9SUzIz
MiAgICAgICAgIDB4ZWVlZSAgUlMyMzINCitwcm9kdWN0IEZUREkgTUhBTV9Z
OSAgICAgICAgICAgIDB4ZWVlZiAgWTkNCitwcm9kdWN0IEZUREkgQ09BU1RB
TF9UTkNYICAgICAgIDB4ZjQ0OCAgQ29hc3RhbCBDaGlwV29ya3MgVE5DLVgN
CiBwcm9kdWN0IEZUREkgTENEX01YMjAwX1VTQgkweGZhMDEJTWF0cml4IE9y
Yml0YWwgTVgyMDAgU2VyaWVzIExDRA0KIHByb2R1Y3QgRlRESSBMQ0RfTEsy
MDJfMjRfVVNCCTB4ZmEwMwlNYXRyaXggT3JiaXRhbCBMSzIwMi0yNCBMQ0QN
Ci1wcm9kdWN0IEZUREkgTENEX0xLMjA0XzI0CTB4ZmEwNAlNYXRyaXggT3Ji
aXRhbCBMSzIwNC0yNCBMQ0QNCitwcm9kdWN0IEZUREkgTENEX0xLMjA0XzI0
X1VTQgkweGZhMDQJTWF0cml4IE9yYml0YWwgTEsyMDQtMjQgTENEDQogcHJv
ZHVjdCBGVERJIExDRF9DRkFfNjMyCTB4ZmMwOCAgQ3J5c3RhbGZvbnR6IENG
QS02MzIgTENEDQogcHJvZHVjdCBGVERJIExDRF9DRkFfNjM0CTB4ZmMwOSAg
Q3J5c3RhbGZvbnR6IENGQS02MzQgTENEDQogcHJvZHVjdCBGVERJIExDRF9D
RkFfNjMzCTB4ZmMwYiAgQ3J5c3RhbGZvbnR6IENGQS02MzMgTENEDQotcHJv
ZHVjdCBGVERJIENGQV82MzEJCTB4ZmMwYwlDcnlzdGFsZm9udHogQ0ZBLTYz
MSBMQ0QNCitwcm9kdWN0IEZUREkgTENEX0NGQV82MzEJMHhmYzBjCUNyeXN0
YWxmb250eiBDRkEtNjMxIExDRA0KIHByb2R1Y3QgRlRESSBTRU1DX0RTUzIw
CQkweGZjODIJU0VNQyBEU1MtMjAgU3luY1N0YXRpb24NCiANCiAvKiBGdWpp
IHBob3RvIHByb2R1Y3RzICovDQo=

--0-1717552568-1159103697=:19200
Content-Type: TEXT/plain; charset=US-ASCII; name=uftdi.c.diff
Content-Transfer-Encoding: BASE64
Content-ID: <20060924151457.S19200@freesbee.wheel.dk>
Content-Description: diff -u of /usr/src/sys/dev/usb/uftdi.c
Content-Disposition: attachment; filename=uftdi.c.diff

LS0tIHVmdGRpLmMub3JnCTIwMDYtMDktMjMgMTg6NTI6MjYuMDAwMDAwMDAw
ICswMjAwDQorKysgdWZ0ZGkuYwkyMDA2LTA5LTI0IDEzOjU5OjE3LjAwMDAw
MDAwMCArMDIwMA0KQEAgLTEyNCw2ICsxMjQsNDAgQEANCiAJdWZ0ZGlfd3Jp
dGUsDQogfTsNCiANCisvKiANCisgKiBUaGUgZGV2aWNlcyBkZWZhdWx0IHRv
IFVGVERJX1RZUEVfOFUyMzJBTS4NCisgKiBSZW1lbWJlciB0byB1cGRhdGUg
VVNCX0FUVEFDSCBpZiBpdCBzaG91bGQgYmUgVUZURElfVFlQRV9TSU8gaW5z
dGVhZA0KKyAqLw0KK3N0YXRpYyBjb25zdCBzdHJ1Y3QgdXNiX2Rldm5vIHVm
dGRpX2RldnNbXSA9IHsNCisJeyBVU0JfVkVORE9SX0JCRUxFQ1RST05JQ1Ms
IFVTQl9QUk9EVUNUX0JCRUxFQ1RST05JQ1NfVVNPVEw0IH0sDQorCXsgVVNC
X1ZFTkRPUl9GQUxDT00sIFVTQl9QUk9EVUNUX0ZBTENPTV9UV0lTVCB9LA0K
Kwl7IFVTQl9WRU5ET1JfRkFMQ09NLCBVU0JfUFJPRFVDVF9GQUxDT01fU0FN
QkEgfSwNCisJeyBVU0JfVkVORE9SX0ZUREksIFVTQl9QUk9EVUNUX0ZURElf
U0VSSUFMXzhVMTAwQVggfSwNCisJeyBVU0JfVkVORE9SX0ZUREksIFVTQl9Q
Uk9EVUNUX0ZURElfU0VSSUFMXzhVMjMyQU0gfSwNCisJeyBVU0JfVkVORE9S
X0ZUREksIFVTQl9QUk9EVUNUX0ZURElfTUhBTV9LVyB9LA0KKwl7IFVTQl9W
RU5ET1JfRlRESSwgVVNCX1BST0RVQ1RfRlRESV9NSEFNX1lTIH0sDQorCXsg
VVNCX1ZFTkRPUl9GVERJLCBVU0JfUFJPRFVDVF9GVERJX01IQU1fWTYgfSwN
CisJeyBVU0JfVkVORE9SX0ZUREksIFVTQl9QUk9EVUNUX0ZURElfTUhBTV9Z
OCB9LA0KKwl7IFVTQl9WRU5ET1JfRlRESSwgVVNCX1BST0RVQ1RfRlRESV9N
SEFNX0lDIH0sDQorCXsgVVNCX1ZFTkRPUl9GVERJLCBVU0JfUFJPRFVDVF9G
VERJX01IQU1fREI5IH0sDQorCXsgVVNCX1ZFTkRPUl9GVERJLCBVU0JfUFJP
RFVDVF9GVERJX01IQU1fUlMyMzIgfSwNCisJeyBVU0JfVkVORE9SX0ZUREks
IFVTQl9QUk9EVUNUX0ZURElfTUhBTV9ZOSB9LA0KKwl7IFVTQl9WRU5ET1Jf
RlRESSwgVVNCX1BST0RVQ1RfRlRESV9DT0FTVEFMX1ROQ1ggfSwNCisJeyBV
U0JfVkVORE9SX0ZUREksIFVTQl9QUk9EVUNUX0ZURElfU0VNQ19EU1MyMCB9
LA0KKwl7IFVTQl9WRU5ET1JfRlRESSwgVVNCX1BST0RVQ1RfRlRESV9MQ0Rf
TEsyMDJfMjRfVVNCIH0sDQorCXsgVVNCX1ZFTkRPUl9GVERJLCBVU0JfUFJP
RFVDVF9GVERJX0xDRF9MSzIwNF8yNF9VU0IgfSwNCisJeyBVU0JfVkVORE9S
X0ZUREksIFVTQl9QUk9EVUNUX0ZURElfTENEX01YMjAwX1VTQiB9LA0KKwl7
IFVTQl9WRU5ET1JfRlRESSwgVVNCX1BST0RVQ1RfRlRESV9MQ0RfQ0ZBXzYz
MSB9LA0KKwl7IFVTQl9WRU5ET1JfRlRESSwgVVNCX1BST0RVQ1RfRlRESV9M
Q0RfQ0ZBXzYzMiB9LA0KKwl7IFVTQl9WRU5ET1JfRlRESSwgVVNCX1BST0RV
Q1RfRlRESV9MQ0RfQ0ZBXzYzMyB9LA0KKwl7IFVTQl9WRU5ET1JfRlRESSwg
VVNCX1BST0RVQ1RfRlRESV9MQ0RfQ0ZBXzYzNCB9LA0KKwl7IFVTQl9WRU5E
T1JfSU5UUkVQSURDUywgVVNCX1BST0RVQ1RfSU5UUkVQSURDU19WQUxVRUNB
TiB9LA0KKwl7IFVTQl9WRU5ET1JfSU5UUkVQSURDUywgVVNCX1BST0RVQ1Rf
SU5UUkVQSURDU19ORU9WSSB9LA0KKwl7IFVTQl9WRU5ET1JfU0VBTEVWRUws
IFVTQl9QUk9EVUNUX1NFQUxFVkVMX1VTQlNFUklBTCB9LA0KKwl7IFVTQl9W
RU5ET1JfU0lJRzIsIFVTQl9QUk9EVUNUX1NJSUcyX1VTMjMwOCB9LA0KK307
DQorI2RlZmluZSB1ZnRkaV9sb29rdXAodiwgcCkgdXNiX2xvb2t1cCh1ZnRk
aV9kZXZzLCB2LCBwKQ0KKw0KIFVTQl9ERUNMQVJFX0RSSVZFUih1ZnRkaSk7
DQogDQogVVNCX01BVENIKHVmdGRpKQ0KQEAgLTEzNiwyMCArMTcwLDggQEAN
CiAJRFBSSU5URk4oMjAsKCJ1ZnRkaTogdmVuZG9yPTB4JXgsIHByb2R1Y3Q9
MHgleFxuIiwNCiAJCSAgICAgdWFhLT52ZW5kb3IsIHVhYS0+cHJvZHVjdCkp
Ow0KIA0KLQlpZiAodWFhLT52ZW5kb3IgPT0gVVNCX1ZFTkRPUl9GVERJICYm
DQotCSAgICAodWFhLT5wcm9kdWN0ID09IFVTQl9QUk9EVUNUX0ZURElfU0VS
SUFMXzhVMTAwQVggfHwNCi0JICAgICB1YWEtPnByb2R1Y3QgPT0gVVNCX1BS
T0RVQ1RfRlRESV9TRVJJQUxfOFUyMzJBTSB8fA0KLQkgICAgIHVhYS0+cHJv
ZHVjdCA9PSBVU0JfUFJPRFVDVF9GVERJX1NFTUNfRFNTMjAgfHwNCi0JICAg
ICB1YWEtPnByb2R1Y3QgPT0gVVNCX1BST0RVQ1RfRlRESV9MQ0RfTEsyMDJf
MjRfVVNCIHx8DQotCSAgICAgdWFhLT5wcm9kdWN0ID09IFVTQl9QUk9EVUNU
X0ZURElfTENEX01YMjAwX1VTQiB8fA0KLQkgICAgIHVhYS0+cHJvZHVjdCA9
PSBVU0JfUFJPRFVDVF9GVERJX0NGQV82MzEpKQ0KLQkJcmV0dXJuIChVTUFU
Q0hfVkVORE9SX1BST0RVQ1QpOw0KLQ0KLQlpZiAodWFhLT52ZW5kb3IgPT0g
VVNCX1ZFTkRPUl9TRUFMRVZFTCAmJg0KLQkgICAgdWFhLT5wcm9kdWN0ID09
IFVTQl9QUk9EVUNUX1NFQUxFVkVMX1VTQlNFUklBTCkNCi0JCXJldHVybiAo
VU1BVENIX1ZFTkRPUl9QUk9EVUNUKTsNCi0NCi0JcmV0dXJuIChVTUFUQ0hf
Tk9ORSk7DQorICAgICAgICByZXR1cm4gKHVmdGRpX2xvb2t1cCh1YWEtPnZl
bmRvciwgdWFhLT5wcm9kdWN0KSAhPSBOVUxMID8NCisgICAgICAgICAgICAg
ICAgVU1BVENIX1ZFTkRPUl9QUk9EVUNUIDogVU1BVENIX05PTkUpOw0KIH0N
CiANCiBVU0JfQVRUQUNIKHVmdGRpKQ0KQEAgLTE5MSwyNCArMjEzLDIxIEBA
DQogCXNjLT5zY191ZGV2ID0gZGV2Ow0KIAlzYy0+c2NfaWZhY2UgPSBpZmFj
ZTsNCiANCi0Jc3dpdGNoICh1YWEtPnByb2R1Y3QpIHsNCi0JY2FzZSBVU0Jf
UFJPRFVDVF9GVERJX1NFUklBTF84VTEwMEFYOg0KLQkJc2MtPnNjX3R5cGUg
PSBVRlRESV9UWVBFX1NJTzsNCi0JCXNjLT5zY19oZHJsZW4gPSAxOw0KKwlz
d2l0Y2goIHVhYS0+dmVuZG9yICkgew0KKwljYXNlIFVTQl9WRU5ET1JfRlRE
SToNCisJCXN3aXRjaCAodWFhLT5wcm9kdWN0KSB7DQorCQljYXNlIFVTQl9Q
Uk9EVUNUX0ZURElfU0VSSUFMXzhVMTAwQVg6DQorCQkJc2MtPnNjX3R5cGUg
PSBVRlRESV9UWVBFX1NJTzsNCisJCQlzYy0+c2NfaGRybGVuID0gMTsNCisJ
CQlicmVhazsNCisJCWRlZmF1bHQ6CQkvKiBNb3N0IHVmdGRpIGRldmljZXMg
YXJlIDhVMjMyQU0gKi8NCisJCQlzYy0+c2NfdHlwZSA9IFVGVERJX1RZUEVf
OFUyMzJBTTsNCisJCQlzYy0+c2NfaGRybGVuID0gMDsNCisJCX0NCiAJCWJy
ZWFrOw0KLQ0KLQljYXNlIFVTQl9QUk9EVUNUX0ZURElfU0VNQ19EU1MyMDoN
Ci0JY2FzZSBVU0JfUFJPRFVDVF9GVERJX1NFUklBTF84VTIzMkFNOg0KLQlj
YXNlIFVTQl9QUk9EVUNUX0ZURElfTENEX0xLMjAyXzI0X1VTQjoNCi0JY2Fz
ZSBVU0JfUFJPRFVDVF9GVERJX0xDRF9NWDIwMF9VU0I6DQotCWNhc2UgVVNC
X1BST0RVQ1RfRlRESV9DRkFfNjMxOg0KLQljYXNlIFVTQl9QUk9EVUNUX1NF
QUxFVkVMX1VTQlNFUklBTDoNCisJZGVmYXVsdDoJCS8qIE1vc3QgdWZ0ZGkg
ZGV2aWNlcyBhcmUgOFUyMzJBTSAqLw0KIAkJc2MtPnNjX3R5cGUgPSBVRlRE
SV9UWVBFXzhVMjMyQU07DQogCQlzYy0+c2NfaGRybGVuID0gMDsNCi0JCWJy
ZWFrOw0KLQ0KLQlkZWZhdWx0OgkJLyogQ2FuJ3QgaGFwcGVuICovDQotCQln
b3RvIGJhZDsNCiAJfQ0KIA0KIAl1Y2EuYnVsa2luID0gdWNhLmJ1bGtvdXQg
PSAtMTsNCg==

--0-1717552568-1159103697=:19200--