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

Jörn Engel
2008-01-10T20:19:15Z

On Thu, 10 January 2008 18:51:13 +0000, Robert Stonehouse wrote:
>
> In the last submission that we made to linux-netdev it was requested that
> people knowledgeable about MTD drivers look over the code ... so I am sure
> I am in the right place

Not quite a review, just a couple of things that stuck out.

Even if the patch is too large, appending the relevant hunk for the
mtd driver would have been appreciated.

The prefix "efx_mtd_" causes me personally some trouble. By the
time I have reached the relevant part of the name, my brain has
already fallen half asleep. But maybe I'm just overreacting after
working with in-house code for five years.

I have no idea why you need eraseregions, if there is just one.
Kill them?

How many of the EFX_LOG() statements are still useful? They may
have initially helped writing the code, but today they hurt people
reading the code.
As a general rule, if you cannot give a good reason why this
particular log statement is needed in 20s, there usually is none and
the code can get axed.

efx_mtd->dead is fun. Does this still happen with production
hardware?
Even if it does, instead of setting the flag and checking it in
every function, you could replace the operations with
dead_device_operations that simply return -EIO for every call.

struct semaphore access_lock; should become a mutex.

Should be enough comments to get things started.

Jörn

--
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare