Patchwork [11/11] benet: Kconfig, MAINTAINETS, drivers/net Makefile

login
register
mail settings
Submitter Sathya Perla
Date Dec. 9, 2008, 2:21 p.m.
Message ID <1228832471.6435.104.camel@sperla-laptop>
Download mbox | patch
Permalink /patch/12959/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Sathya Perla - Dec. 9, 2008, 2:21 p.m.
Signed-off-by: Sathya Perla <sathyap@serverengines.com>
---
 drivers/net/Kconfig           |    2 ++
 drivers/net/Makefile          |    1 +
 drivers/net/benet/Kconfig     |    7 +++++++
 drivers/net/benet/MAINTAINERS |    8 ++++++++
 4 files changed, 18 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/benet/Kconfig
 create mode 100644 drivers/net/benet/MAINTAINERS
David Miller - Dec. 10, 2008, 6:54 a.m.
Ok I looked at this driver some more and I have many comments.

The hardware library abstraction has to go.  You aren't writing a
native linux driver if you split things up like this.  I know you want
code sharing with your other supported platforms, but that's too bad.
If we let every vendor do this the whole tree would be one big
unmaintainable mess.

Many structures have all-caps or capitalized names.  Good coding style
indicates that only CPP macros are to be named with capital letters.
(capital letterd symbols say to the reader "I'm a CPP macro and
probably have side-effects, beware") What's happening here looks ugly
and is inconsistent with the rest of the linux kernel.

The definition of the access to the chip registers is overly
obfuscated.  All of this structure stuff and offsetof() business
adds complexity to the driver and makes it harder to understand.

The bit twiddling is difficult to understand and makes the compiler
work too hard.

I would suggest to fix all of this by simply using macros which
define chip register offsets, and next to those offset define
macros which define the bit values within the register.  Endianness
is not an issue, and all read*()/write*() calls will write out
to the chip in little endian regardless of cpu endianness.

See other drivers such as drivers/net/tg3.[ch] or drivers/net/niu.[ch]
As you'll notice even such huge drivers as those can be done cleanly
in a single source file with no hardware abstraction library layer and
no funny register access structures and bit twiddling, so you can
strive for that as well.

So, this driver still needs a lot of work. :)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 231eeaf..85992af 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2579,6 +2579,8 @@  config QLGE
 
 source "drivers/net/sfc/Kconfig"
 
+source "drivers/net/benet/Kconfig"
+
 endif # NETDEV_10000
 
 source "drivers/net/tokenring/Kconfig"
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 017383a..0d48432 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -21,6 +21,7 @@  obj-$(CONFIG_GIANFAR) += gianfar_driver.o
 obj-$(CONFIG_TEHUTI) += tehuti.o
 obj-$(CONFIG_ENIC) += enic/
 obj-$(CONFIG_JME) += jme.o
+obj-$(CONFIG_BENET) += benet/
 
 gianfar_driver-objs := gianfar.o \
 		gianfar_ethtool.o \
diff --git a/drivers/net/benet/Kconfig b/drivers/net/benet/Kconfig
new file mode 100644
index 0000000..f680607
--- /dev/null
+++ b/drivers/net/benet/Kconfig
@@ -0,0 +1,7 @@ 
+config BENET
+	tristate "ServerEngines 10Gb NIC - BladeEngine"
+	depends on PCI && INET
+	select INET_LRO
+	help
+	  This driver implements the NIC functionality for ServerEngines
+	  10Gb network adapter BladeEngine (EC 3210).
diff --git a/drivers/net/benet/MAINTAINERS b/drivers/net/benet/MAINTAINERS
new file mode 100644
index 0000000..60f3144
--- /dev/null
+++ b/drivers/net/benet/MAINTAINERS
@@ -0,0 +1,8 @@ 
+SERVER ENGINES 10Gbe NIC - BLADE-ENGINE
+P:	Subbu Seetharaman
+M:	subbus@serverengines.com
+P:	Sathya Perla	
+M:	sathyap@serverengines.com
+L:	netdev@vger.kernel.org
+W:	http://www.serverengines.com
+S:	Supported