[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