Re: [PATCH] [MTD] [RFC] New Solarflare NIC EEPROM/Flash driver

Jörn Engel
2008-01-10T23:22:48Z

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