Subject: Re: uftdi.c code review request (simple)
To: None <tech-misc@netbsd.org>
From: Christos Zoulas <christos@astron.com>
List: tech-misc
Date: 09/24/2006 17:23:10
In article <20060924150227.W19200@freesbee.wheel.dk>,
Claus Andersen <clan@wheel.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