[PATCH] [MTD] [RFC] New Solarflare NIC EEPROM/Flash driver
Jörn Engel
joern at logfs.org
Thu Jan 10 18:16:45 EST 2008
Editor window was still open on some terminal, so I had a second look:
/* Free verification buffer */
kfree(verify_buffer);
Please kill such non-documentation. Plain C is readable enough and
does not need translation in English. Some of your comments are
quite useful, because they explain background information. Do keep
those.
efx_mtd_sync() is a function of 29 lines that does... very little. It
takes the semaphore and releases it again. Everything else is pure
noise. When reading the function, 90% of the time is wasted on noise.
So please take a rough brush and sweep out everything you don't need.
One of my first questions was, why does this driver need 1000 lines?
And the answer is: it doesn't. 500 sounds like a fair goal. That
means 50% of your driver is noise and should get removed. Which, btw,
is a fairly good starting point for in-house code. In my last job,
75% noise was quite common.
Another interesting metric for in-house code was that while removing
noise I usually fixed one bug for every 10-20 lines I removed. Your
code gives me a significantly better first impression, so I don't
expect you to find and fix 50 bugs during the cleanup. But I'd be
equally surprised if you found none. ;)
Jörn
--
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike
More information about the linux-mtd
mailing list