Steve Glendinning wrote: [...] > +static inline void smsc911x_reg_write(u32 val, struct smsc911x_data *pdata, > + u32 reg)
It's more common to make the value the last parameter to a write-register function. [...] > +/* Fetches a MAC register value. Assumes phy_lock is acquired */ > +static u32 smsc911x_mac_read(struct smsc911x_data *pdata, unsigned int > offset) > +{ > + unsigned int temp; > + > +#ifdef CONFIG_DEBUG_SPINLOCK > + if (!spin_is_locked(&pdata->phy_lock)) > + SMSC_WARNING("phy_lock not held"); > +#endif /* CONFIG_DEBUG_SPINLOCK */ This is replicated in several functions; why not make it a macro, and use the standard warning macro: #ifdef CONFIG_DEBUG_SPINLOCK #define ASSERT_PHY_LOCK(pdata) WARN_ON(!spin_is_locked(&pdata->phy_lock)) #else #define ASSERT_PHY_LOCK(pdata) do {} while (0) #endif (Also, why is it called phy_lock if it's also used by the MAC access functions?) [...] > +/* Set a mac register, phy_lock must be acquired before calling */ > +static void smsc911x_mac_write(struct smsc911x_data *pdata, > + unsigned int offset, u32 val) > +{ > + unsigned int temp; > + > +#ifdef CONFIG_DEBUG_SPINLOCK > + if (!spin_is_locked(&pdata->phy_lock)) > + SMSC_WARNING("phy_lock not held"); > +#endif /* CONFIG_DEBUG_SPINLOCK */ > + > + temp = smsc911x_reg_read(pdata, MAC_CSR_CMD); > + if (unlikely(temp & MAC_CSR_CMD_CSR_BUSY_)) { > + SMSC_WARNING("smsc911x_mac_write failed, MAC busy at entry"); > + return; > + } Shouldn't this return an error code? [...] > +/* Sets a phy register, phy_lock must be acquired before calling */ > +static void smsc911x_phy_write(struct smsc911x_data *pdata, > + unsigned int index, u16 val) > +{ > + unsigned int addr; > + int i; > + > +#ifdef CONFIG_DEBUG_SPINLOCK > + if (!spin_is_locked(&pdata->phy_lock)) > + SMSC_WARNING("phy_lock not held"); > +#endif /* CONFIG_DEBUG_SPINLOCK */ > + > + /* Confirm MII not busy */ > + if (unlikely(smsc911x_mac_read(pdata, MII_ACC) & MII_ACC_MII_BUSY_)) { > + SMSC_WARNING("MII is busy in smsc911x_write_phy???"); > + return; > + } Similarly for this function. [...] > +/* called by phy_initialise and loopback test */ > +static int smsc911x_phy_reset(struct smsc911x_data *pdata) > +{ > + unsigned int temp; > + unsigned int i = 100000; > + unsigned long flags; > + > + SMSC_TRACE("Performing PHY BCR Reset"); > + spin_lock_irqsave(&pdata->phy_lock, flags); > + smsc911x_phy_write(pdata, MII_BMCR, BMCR_RESET); > + do { > + udelay(10); > + temp = smsc911x_phy_read(pdata, MII_BMCR); > + } while ((i--) && (temp & BMCR_RESET)); > + spin_unlock_irqrestore(&pdata->phy_lock, flags); I think this was already mentioned, but that's a very long time to busy- wait. Maybe you could find a way to block PHY access that doesn't require holding phy_lock; then you could sleep while waiting. [...] > +static int smsc911x_phy_loopbacktest(struct smsc911x_data *pdata) > +{ > + int result = 0; > + unsigned int i; > + unsigned int val; > + unsigned long flags; > + > + /* Initialise tx packet using broadcast destination address */ > + for (i = 0; i < 6; i++) > + pdata->loopback_tx_pkt[i] = (char)0xFF; The cast to char is just noise. [...] > + /* Set length type field */ > + pdata->loopback_tx_pkt[12] = 0x00; > + pdata->loopback_tx_pkt[13] = 0x00; > + for (i = 14; i < MIN_PACKET_SIZE; i++) > + pdata->loopback_tx_pkt[i] = (char)i; The comment applies to the following two lines only, so you could do with a blank line after them. > + val = smsc911x_reg_read(pdata, HW_CFG); > + val &= HW_CFG_TX_FIF_SZ_; > + val |= HW_CFG_SF_; > + smsc911x_reg_write(val, pdata, HW_CFG); > + > + smsc911x_reg_write(TX_CFG_TX_ON_, pdata, TX_CFG); > + smsc911x_reg_write((((unsigned int)pdata->loopback_rx_pkt) > + & 0x03) << 8, pdata, RX_CFG); > + > + for (i = 0; i < 10; i++) { > + /* Set PHY to 10/FD, no ANEG, and loopback mode */ > + spin_lock_irqsave(&pdata->phy_lock, flags); > + smsc911x_phy_write(pdata, MII_BMCR, 0x4100); You could write BMCR_LOOPBACK | BMCR_FULLDPLX instead of 0x4100; then the comment is unnecessary. [...] > +/* Update link mode if any thing has changed */ > +static void smsc911x_phy_update_linkmode(struct net_device *dev, int init) [...] > +#ifdef USE_LED1_WORK_AROUND Shouldn't this workaround be controlled by platform data or a module parameter rather than a compile-time option which isn't in Kconfig? I notice this macro is defined by default, so maybe it shouldn't be conditional at all. [...] > +static int smsc911x_soft_reset(struct smsc911x_data *pdata) > +{ > + unsigned int timeout; > + unsigned int temp; > + > + /* Reset the LAN911x */ > + smsc911x_reg_write(HW_CFG_SRST_, pdata, HW_CFG); > + timeout = 10; > + do { > + udelay(10); > + temp = smsc911x_reg_read(pdata, HW_CFG); > + } while ((--timeout) && (temp & HW_CFG_SRST_)); > + > + if (unlikely(temp & HW_CFG_SRST_)) { > + SMSC_WARNING("Failed to complete reset"); > + return -ENODEV; I think this should be -EIO unless this is only called during probe. [...] > +static int smsc911x_open(struct net_device *dev) [...] > + timeout = 1000; > + while (timeout--) { > + smp_rmb(); I think you're trying to ensure that software_irq_signal is re-read each time round the loop. For that you should use barrier(), not smp_rmb(). However, msleep() acts as a compiler barrier already. So just remove the smp_rmb(). > + if (pdata->software_irq_signal) > + break; > + msleep(1); > + } > + > + if (!pdata->software_irq_signal) { > + printk(KERN_WARNING "%s: ISR failed signaling test (IRQ %d)\n", > + dev->name, dev->irq); > + return -ENODEV; > + } > + SMSC_TRACE("IRQ handler passed test using IRQ %d", dev->irq); > + > + printk(KERN_INFO "%s: SMSC911x/921x identified at %#08lx, IRQ: %d\n", > + dev->name, (unsigned long)pdata->ioaddr, dev->irq); Should use dev_info(). [...] > +static void smsc911x_ethtool_getdrvinfo(struct net_device *dev, > + struct ethtool_drvinfo *info) > +{ > + strncpy(info->driver, SMSC_CHIPNAME, sizeof(info->driver)); > + strncpy(info->version, SMSC_DRV_VERSION, sizeof(info->version)); > + strncpy(info->bus_info, dev->dev.parent->bus_id, > + sizeof(info->bus_info)); > +} Use strlcpy(), not strncpy(). [...] > +static void smsc911x_ethtool_setmsglevel(struct net_device *dev, u32 level) > +{ > + struct smsc911x_data *pdata = netdev_priv(dev); > + pdata->msg_enable = level; > +} It would be nice if the logging macros actually tested msg_enable too. ;-) [...] > +static int smsc911x_ethtool_get_eeprom(struct net_device *dev, > + struct ethtool_eeprom *eeprom, u8 *data) > +{ > + struct smsc911x_data *pdata = netdev_priv(dev); > + u8 eeprom_data[SMSC911X_EEPROM_SIZE]; > + int len; > + int i; > + > + smsc911x_eeprom_enable_access(pdata); > + > + len = min(eeprom->len, SMSC911X_EEPROM_SIZE); > + for (i = 0; i < len; i++) { > + int ret = smsc911x_eeprom_read_location(pdata, i, eeprom_data); > + if (ret < 0) { > + eeprom->len = 0; > + return ret; > + } > + } Doesn't this need to take eeprom->offset into account? [...] > + printk(KERN_INFO > + "%s: SMSC911x MAC Address: %02x:%02x:%02x:%02x:%02x:%02x\n", > + dev->name, dev->dev_addr[0], dev->dev_addr[1], dev->dev_addr[2], > + dev->dev_addr[3], dev->dev_addr[4], dev->dev_addr[5]); This should use print_mac(). [...] > +/* SMSC911x registers and bitfields */ > +#define RX_DATA_FIFO 0x00 > + > +#define TX_DATA_FIFO 0x20 > +#define TX_CMD_A_ON_COMP_ 0x80000000 Why do these flag/mask names have trailing underscores? Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. _______________________________________________ Uclinux-dist-devel mailing list Uclinux-dist-devel@blackfin.uclinux.org http://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel