lkml.org 
[lkml]   [2011]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] net: add Faraday FTMAC100 10/100 Ethernet driver
    From
    Date
    On Mon, 2011-01-17 at 17:21 +0800, Po-Yu Chuang wrote:
    > From: Po-Yu Chuang <ratbert@faraday-tech.com>
    > FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and
    > MII. This driver has been working on some ARM/NDS32 SoC's including
    > Faraday A320 and Andes AG101.

    Hi again.

    > Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
    > ---
    > v2:
    > always use NAPI
    > do not use our own net_device_stats structure
    > don't set trans_start and last_rx
    > stats.rx_packets and stats.rx_bytes include dropped packets
    > add missed netif_napi_del()
    > initialize spinlocks in probe function
    > remove rx_lock and hw_lock
    > use netdev_[err/info/dbg] instead of dev_* ones
    > use netdev_alloc_skb_ip_align()
    > remove ftmac100_get_stats()
    > use is_valid_ether_addr() instead of is_zero_ether_addr()
    > add const to ftmac100_ethtool_ops and ftmac100_netdev_ops
    > use net_ratelimit() instead of printk_ratelimit()
    > no explicit inline
    > use %pM to print MAC address
    > add comment before wmb
    > use napi poll() to handle all interrupts

    This looks very clean, thanks for doing the rework.

    Now the the really trivial...

    > + * priveate data

    private

    > +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.

    > + 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.

    > +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 ?

    > +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.

    > +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

    > +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?

    > +/******************************************************************************
    > + * 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");

    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);



    \
     
     \ /
      Last update: 2011-01-17 18:23    [W:0.029 / U:33.232 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site