Messages in this thread | | | From | Po-Yu Chuang <> | Date | Wed, 19 Jan 2011 17:40:07 +0800 | Subject | Re: [PATCH v2] net: add Faraday FTMAC100 10/100 Ethernet driver |
| |
Dear Joe,
On Tue, Jan 18, 2011 at 1:19 AM, Joe Perches <joe@perches.com> wrote: > On Mon, 2011-01-17 at 17:21 +0800, Po-Yu Chuang wrote: > >> + * priveate data > > private
Fixed.
>> +static void ftmac100_enable_all_int(struct ftmac100 *priv) >> +{ >> + unsigned int imr; >> + >> + imr = FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF >> + | FTMAC100_INT_XPKT_OK | FTMAC100_INT_XPKT_LOST >> + | FTMAC100_INT_RPKT_LOST | FTMAC100_INT_AHB_ERR >> + | FTMAC100_INT_PHYSTS_CHG; > > This could be a #define.
OK, done.
>> + maccr = FTMAC100_MACCR_XMT_EN | >> + FTMAC100_MACCR_RCV_EN | >> + FTMAC100_MACCR_XDMA_EN | >> + FTMAC100_MACCR_RDMA_EN | >> + FTMAC100_MACCR_CRC_APD | >> + FTMAC100_MACCR_FULLDUP | >> + FTMAC100_MACCR_RX_RUNT | >> + FTMAC100_MACCR_RX_BROADPKT; > > Here too.
OK, done.
>> +static int ftmac100_rx_packet_error(struct ftmac100 *priv, >> + struct ftmac100_rxdes *rxdes) > [] >> + if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) { >> + if (net_ratelimit()) >> + netdev_info(netdev, "rx frame too long\n"); >> + >> + netdev->stats.rx_length_errors++; >> + error = 1; >> + } >> + >> + if (unlikely(ftmac100_rxdes_runt(rxdes))) { > > else if ?
OK, fixed.
>> +static int ftmac100_rx_packet(struct ftmac100 *priv, int *processed) >> +{ >> + struct net_device *netdev = priv->netdev; >> + struct ftmac100_rxdes *rxdes; >> + struct sk_buff *skb; >> + int length; >> + int copied = 0; >> + int done = 0; > > You could use bool/true/false here for copied and done > and all the other uses of an int for a logical bool.
OK, fixed.
>> +static void ftmac100_txdes_set_dma_own(struct ftmac100_txdes *txdes) >> +{ >> + /* >> + * Make sure dma own bit will not be set before any other >> + * descriptor fiels. > > field/fields
Fixed.
>> +static int ftmac100_mdio_read(struct net_device *netdev, int phy_id, int reg) >> +{ >> + struct ftmac100 *priv = netdev_priv(netdev); >> + int phycr; >> + int i; >> + >> + phycr = FTMAC100_PHYCR_PHYAD(phy_id) | >> + FTMAC100_PHYCR_REGAD(reg) | >> + FTMAC100_PHYCR_MIIRD; >> + >> + iowrite32(phycr, priv->base + FTMAC100_OFFSET_PHYCR); >> + for (i = 0; i < 10; i++) { >> + phycr = ioread32(priv->base + FTMAC100_OFFSET_PHYCR); >> + >> + if ((phycr & FTMAC100_PHYCR_MIIRD) == 0) >> + return phycr & FTMAC100_PHYCR_MIIRDATA; >> + >> + usleep_range(100, 1000); >> + } >> + >> + netdev_err(netdev, "mdio read timed out\n"); >> + return 0xffff; > > 0xffff is a rather odd return, perhaps a #define?
After a little digging in drivers/net/mii.c, it seems that mii lib does not check return value if it is error. So I guess I should return 0 if error.
>> +/****************************************************************************** >> + * initialization / finalization >> + *****************************************************************************/ >> +static int __init ftmac100_init(void) >> +{ >> + printk(KERN_INFO "Loading " DRV_NAME ": version " DRV_VERSION " ...\n"); > > You could use > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > before any #include and > pr_info("Loading version " DRV_VERSION " ...\n");
OK
> One last comment on split long line indentation style > and long function declarations. > > There's no required style so you can use what you are > most comfortable doing. > > Most of drivers/net uses an alignment to open parenthesis > using maximal tabs and minimal necessary spaces instead of > an extra tabstop. > > Like: > > static int some_long_function(type var1, type var2... > type varN) > and > some_long_function(var1, var2, ... > varN); > > not > static int some_long_function(type var1, type var2... > type varN) > and > some_long_function(var1, var2, ... > varN);
Well, TBH, I don't like this style because if I changed the function name, the indentation might need to be adjusted.
Even worse, I got an infeasible case :-(
static struct ftmac100_rxdes *ftmac100_rx_locate_first_segment( struct ftmac100 *priv)
I know my function names are quite long, but I like them to be descriptive. Do you really insist on it?
Thanks, Po-Yu Chuang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |