Fintek F81232 USB to serial driver
ClosedPublic
Actions

Authored by db on Sun, Nov 23, 4:00 PM.

Details

Reviewers

adrian
thj

Group Reviewers

USB

Commits

rG791c531402bd: Fintek F81232 USB to serial driver

Summary

Fintek f81232 usb to serial driver

Test Plan

I have only tested it on a Protecli box which came with a Fintek
USB to serial cable.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

db created this revision.Sun, Nov 23, 4:00 PM

Herald added a subscriber: imp. · View Herald TranscriptSun, Nov 23, 4:00 PM

db requested review of this revision.Sun, Nov 23, 4:00 PM

Harbormaster completed remote builds in B68813: Diff 166989.Sun, Nov 23, 4:00 PM

looks good to me but the lack of mutexes anywhere is suspicious. ;-)

In D53893#1231126, @adrian wrote:

looks good to me but the lack of mutexes anywhere is suspicious. ;-)

There is an sc_mtx created and used in attach. ucom handles locking for serial devices with their mutex.

sys/dev/usb/serial/ufintek.c
2	I think we need an spdx identifier for new files. We don't need the full copyright text, but that call is up to you.
29	Do we need this comment?
289
368	Can you combine these lines, it makes grepping for the error message possible
653	Should this be a device_printf? I'm not sure what the ZZZ is for
sys/modules/ufintek/Makefile
2	Same spdx comment as above

This looks great to me, some small nits for which I have made comments.

Can you either remove the commented out code lines in the driver, or add additional context so we know why they are gone/what they are for when we eventually have to look in n years?

This revision now requires changes to proceed.Mon, Nov 24, 8:26 AM

db marked an inline comment as done.Mon, Nov 24, 1:28 PM

db added inline comments.

sys/dev/usb/serial/ufintek.c
2	I think we need an spdx identifier for new files. We don't need the full copyright text, but that call is up to you. Good idea. Done.
29	Agreed and removed.
289	moved not mored or moored.
368	Done
653	Eeek inadvertently left in a debug message. I often use ZZZ for these.
sys/modules/ufintek/Makefile
2	Fixed. Also fixed a C'ism that crept into the Makefile.

Updating D53893: Fintek F81232 USB to serial driver

Addressing reviewers changes

Harbormaster completed remote builds in B68823: Diff 167018.Mon, Nov 24, 1:35 PM

thj requested changes to this revision.Tue, Nov 25, 9:24 AM

thj added inline comments.

sys/dev/usb/serial/ufintek.c
742	These commented out lines are still here, can you add an explanation for why they are here, but commented out?

This revision now requires changes to proceed.Tue, Nov 25, 9:24 AM

Removing unecessary comment lines

These lines were an experiment but not necessary.

Harbormaster completed remote builds in B68838: Diff 167080.Tue, Nov 25, 1:52 PM

thj accepted this revision.Wed, Nov 26, 10:02 AM

This revision is now accepted and ready to land.Wed, Nov 26, 10:02 AM

looks good. manpage? :)

db added inline comments.Thu, Nov 27, 12:35 AM

sys/dev/usb/serial/ufintek.c
742	Sure. This chip interleaves LSR and data and my early thinking was if the driver saw a lot of bytes at once it might be better to de-interleave the bytes and use one call to ucom_put_data() instead of singly putting bytes so I tried it as an experiment. It worked fine but is unnecessary here. I'll remove the commented out lines.

In D53893#1232293, @adrian wrote:

looks good. manpage? :)

http://www.freebsd.org/~db/ufintek.4

And ziaee has a copy
<fbsdslack:#bsddocs> <ziaee> db: if you let me know when you put it in the tree I'll put the line in HW Relnotes. Or you can do it if you want, it's at doc/website/archetypes/release/hardware.adoc