Cc'ed Zhangfei.

On Wed, Aug 4, 2010 at 3:08 PM, Eric Miao [off-list ref] wrote:

On Wed, Aug 4, 2010 at 2:58 PM, Lennert Buytenhek
[off-list ref] wrote:

quoted

On Wed, Aug 04, 2010 at 04:17:50PM +0530, Sachin Sanap wrote:

quoted

This patch adds support for PXA168 Fast Ethernet on Aspenite
board.

There's nothing Aspenite-specific in this patch (nor should there be),
so you can leave that part out.

quoted

Patch generated against Linux 2.6.35-rc5
commit cd5b8f8755a89a57fc8c408d284b8b613f090345

This might be interesting to know but shouldn't be part of the commit
message.

quoted

diff --git a/arch/arm/mach-mmp/include/mach/pxa168_eth.h b/arch/arm/mach-mmp/include/mach/pxa168_eth.h
new file mode 100644
index 0000000..abfd335
--- /dev/null
+++ b/arch/arm/mach-mmp/include/mach/pxa168_eth.h

@@ -0,0 +1,18 @@
+/*
+ *pxa168 ethernet platform device data definition file.
+ */
+#ifndef __LINUX_PXA168_ETH_H
+#define __LINUX_PXA168_ETH_H

This should just be in include/linux, IMHO, as this unit isn't only used
in the PXA168, for one.

Yep.

quoted

quoted

+struct pxa168_eth_platform_data {
+     int     port_number;
+     u16     phy_addr;
+
+     /* If speed is 0, then speed and duplex are autonegotiated. */
+     u32     speed;          /* 0, SPEED_10, SPEED_100 */
+     u32     duplex;         /* DUPLEX_HALF or DUPLEX_FULL */

phylib treats these three (phy address, speed and duplex) as ints, is
there any reason you need these to be of different types?

quoted

+     int (*init)(void);

What's this needed for?  The name of this callback is entirely
nondescriptive, there's no comment as to when it's called, etc.

quoted

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index ce2fcdd..5ebf287 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig

@@ -927,6 +927,16 @@ config SMC91X

        The module will be called smc91x.  If you want to compile it as a
        module, say M here and read <file:Documentation/kbuild/modules.txt>.

+config PXA168_ETH
+     tristate "Marvell pxa168 ethernet support"
+     depends on MACH_ASPENITE

You can have it depend on ARM or PXA168, but having it depend on support
for one specific board support file is almost certainly wrong.

depends on CPU_PXA168

probably makes more sense here.

And yeah we also found the driver to be very similar to mv643xx_eth
at that time, though not sure how much it resembles. So it would be
definitely good to have some comment why to make this a separate
driver, instead of reusing mv643xx_eth.