RE: [PATCH] [2.6.17] Add tsi108 Ethernet device driver support

2006-07-03 Thread Zang Roy-r61911

> On Wed, 21 Jun 2006 12:00:40 +0800
> Zang Roy-r61911 <[EMAIL PROTECTED]> wrote:
> 
> > This patch adds a net device driver and configuration options for 
> > Tundra Semiconductor Tsi108 integrated dual port Gigabit Ethernet 
> > controller
> 
> Your patch forgot to include these:
> 
> > +#include 
> > +#include 
> 
> Why would a net driver have files in include/asm/?

The net driver is for a tsi108 on chip Ethernet controller.  tsi108_irq 
provides the Ethernet IRQ number.
tsi108.h provides the hw_info structure. Now tsi108.h has been in the kernel 
tree. I'd like to provide
the tsi108_irq.h with tsi108  irq support in another patch. Any comment? 

> 
> 
> 
> 
> Have some comments-via-diff, mainly trivial:
> 
Thanks for your patch. It works OK! I will integrate it in my next version.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] [2.6.17] Add tsi108 Ethernet device driver support

2006-07-01 Thread Andrew Morton
On Wed, 21 Jun 2006 12:00:40 +0800
Zang Roy-r61911 <[EMAIL PROTECTED]> wrote:

> This patch adds a net device driver and configuration options for
> Tundra Semiconductor Tsi108 integrated dual port Gigabit 
> Ethernet controller

Your patch forgot to include these:

> +#include 
> +#include 

Why would a net driver have files in include/asm/?




Have some comments-via-diff, mainly trivial:


From: Andrew Morton <[EMAIL PROTECTED]>

- Nuke the typedef

- Reduce exorbitant inlining.

- possible bug: tsi108_open() does request_irq() as its first operation. 
  The IRQ handler can run immediately.  Is it ready for that?

- Use setup_irq().

- Unneeded casts of void*

- alloc_etherdev() already zeroes the memory.

- why are tsi108_prv_data.regs and .phyregs volatile?  They shouldn't be.

- What's this?

data->regs = (u32)ioremap(einfo->regs, 0x400);  /*FIX ME */
data->phyregs = (u32)ioremap(einfo->phyregs, 0x400);/*FIX ME */

- Use DEFINE_SPINLOCK()

- Use free_netdev(), not kfree() (Is this right?)

- Use mod_timer()

- tsi108_init_phy() has a 10-millisecond busywait in it, seemingly
  needlessly.  Convert to msleep().

- tsi108_init_phy() has multiple potentially infinite wait loops.  Add
  timeouts.

- Random coding-style cleanups.





Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
---

 drivers/net/tsi108_eth.c |  295 ++---
 1 file changed, 145 insertions(+), 150 deletions(-)

diff -puN 
drivers/net/tsi108_eth.c~add-tsi108-ethernet-device-driver-support-tidy 
drivers/net/tsi108_eth.c
--- a/drivers/net/tsi108_eth.c~add-tsi108-ethernet-device-driver-support-tidy
+++ a/drivers/net/tsi108_eth.c
@@ -44,15 +44,16 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 #include 
 #include 
 #include 
 
+#include 
+#include 
 #include 
 #include 
+
 #include "tsi108_eth.h"
 
 #undef DEBUG
@@ -85,7 +86,7 @@ typedef struct sk_buff sk_buff;
 static int tsi108_init_one(struct platform_device *pdev);
 static int tsi108_ether_remove(struct platform_device *pdev);
 
-typedef struct {
+struct tsi108_prv_data {
volatile u32 regs;  /* Base of normal regs */
volatile u32 phyregs;   /* Base of register bank used for PHY access */
int phy;/* Index of PHY for this interface */
@@ -154,7 +155,7 @@ typedef struct {
unsigned long tx_pause_drop;/* Add to tx_aborted_errors */
 
unsigned long mc_hash[16];
-} tsi108_prv_data;
+};
 
 /* Structure for a device driver */
 
@@ -168,9 +169,9 @@ static struct platform_driver tsi_eth_dr
 
 static void tsi108_timed_checker(unsigned long dev_ptr);
 
-static void dump_eth_one(net_device * dev)
+static void dump_eth_one(net_device *dev)
 {
-   tsi108_prv_data *data = netdev_priv(dev);
+   struct tsi108_prv_data *data = netdev_priv(dev);
 
printk("Dumping %s...\n", dev->name);
printk("intstat %x intmask %x phy_ok %d"
@@ -198,9 +199,9 @@ static void dump_eth_one(net_device * de
  * interfaces, so this can't be made interface-specific.
  */
 
-static spinlock_t phy_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(phy_lock);
 
-static inline u16 tsi108_read_mii(tsi108_prv_data * data, int reg, int *status)
+static u16 tsi108_read_mii(struct tsi108_prv_data *data, int reg, int *status)
 {
int i;
u16 ret;
@@ -235,7 +236,7 @@ static inline u16 tsi108_read_mii(tsi108
return ret;
 }
 
-static inline void tsi108_write_mii(tsi108_prv_data * data, int reg, u16 val)
+static void tsi108_write_mii(struct tsi108_prv_data *data, int reg, u16 val)
 {
TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_ADDR,
(data->phy << TSI108_MAC_MII_ADDR_PHY) |
@@ -247,7 +248,8 @@ static inline void tsi108_write_mii(tsi1
   TSI108_MAC_MII_IND_BUSY) ;
 }
 
-static inline void tsi108_write_tbi(tsi108_prv_data * data, int reg, u16 val)
+static inline void tsi108_write_tbi(struct tsi108_prv_data *data,
+   int reg, u16 val)
 {
 
TSI108_ETH_WRITE_REG(TSI108_MAC_MII_ADDR,
@@ -261,9 +263,9 @@ static inline void tsi108_write_tbi(tsi1
   TSI108_MAC_MII_IND_BUSY) ;
 }
 
-static void tsi108_check_phy(net_device * dev)
+static void tsi108_check_phy(net_device *dev)
 {
-   tsi108_prv_data *data = netdev_priv(dev);
+   struct tsi108_prv_data *data = netdev_priv(dev);
u16 sumstat;
u32 mac_cfg2_reg, portctrl_reg;
u32 fdx_flag = 0, reg_update = 0;
@@ -291,79 +293,76 @@ static void tsi108_check_phy(net_device 
goto out;
}
 
-   {
-   mac_cfg2_reg = TSI108_ETH_READ_REG(TSI108_MAC_CFG2);
-   portctrl_reg = TSI108_ETH_READ_REG(TSI108_EC_PORTCTRL);
-
-   sumstat = tsi108_read_mii(data, PHY_SUM_STAT, NULL);
-
-   switch (sumstat & PHY_SUM_STAT_SPEED_MASK) {
-   case PHY_SUM_STAT_1000T_FD:
-   fdx_flag++;
-   case PHY_SUM_STAT_10