List Info

Thread: uftdi.c code review request (simple)




uftdi.c code review request (simple)
user name
2006-09-24 17:23:10
In article <20060924150227.W19200freesbee.wheel.dk>,
Claus Andersen  <clanwheel.dk> wrote:
>-=-=-=-=-=-
>
>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?

I don't know about 6, but I agree with 1-5.

I committed your changes as you posted them. Thanks. In the
future, please
send patches using send-pr so that they don't get lost!

Best,

christos

[1]

about | contact  Other archives ( Real Estate discussion Medical topics )