On 06/07/2013 10:13 PM, Joe Perches wrote: > On Fri, 2013-06-07 at 19:07 +0400, Alexey Brodkin wrote: >> Driver for non-standard on-chip ethernet device ARC EMAC 10/100, >> instantiated in some legacy ARC (Synopsys) FPGA Boards such as >> ARCAngel4/ML50x. > > trivial comments only: > >> diff --git a/drivers/net/ethernet/arc/arc_emac_main.c b/drivers/net/ethernet/arc/arc_emac_main.c >> + * Alexey Brodkin: June 2013 >> + * -Upsteaming > [] >> + * Vineet Gupta: June 2011 >> + * -Issues when working with 64b cache line size > [] >> + * Vineet Gupta: May 2010 >> + * -Reduced foot-print of the main ISR (handling for error cases moved out > [] >> + * Vineet Gupta: Nov 2009 >> + * -Unified NAPI and Non-NAPI Code. > [] >> + * Vineet Gupta: Nov 2009 > [] >> + * Amit Bhor, Sameer Dhavale: 2004 > > Does the internal changelog add anything useful?
Well, I think at this point it's more of honoring people who contributed into this driver through past years. I saw this kind of changelogs in other drivers like "arc_uart" that was recently upstreamed: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/arc_uart.c So if I still want to name contributor should I just list their names? >> +static int arc_emac_poll(struct napi_struct *napi, int budget) >> +{ >> + struct net_device *net_dev = napi->dev; >> + struct arc_emac_priv *priv = netdev_priv(net_dev); >> + struct sk_buff *skb, *skbnew; >> + unsigned int i, loop, len, info, work_done = 0; >> + >> + /* Loop thru the BD chain, but not always from 0. >> + * Start from right after where we last saw a packet. >> + */ >> + i = priv->last_rx_bd; >> + >> + for (loop = 0; loop < RX_BD_NUM; loop++) { >> + i = (i + 1) & (RX_BD_NUM - 1); >> + >> + info = priv->rxbd[i].info; >> + >> + /* BD contains a packet for CPU to grab */ >> + if (likely((info & OWN_MASK) == FOR_CPU)) { > > You could reduce indentation a level for all > the lines that follow by using > if (unlikely(!(...) > continue; To be honest I thought about doing it, but personally I don't like inverted logic - I'd say it makes code logic not that straight-forward for understanding. Still it might be only my feeling and as I've got at last to advises to act so - I'll definitely follow your proposal. >> + /* Get a new SKB from stack */ >> + skbnew = netdev_alloc_skb(net_dev, >> + net_dev->mtu + >> + EMAC_BUFFER_PAD); >> + >> + if (!skbnew) { >> + netdev_err(net_dev, "Out of memory, " >> + "dropping packet\n"); > > OOM messages aren't particularly useful, > and coalesce format please... I noticed that some drivers don't print error messages for this kind of errors. My intention was to make possible problem troubleshooting a bit easier. But if it is not supposed to print "standard" errors in console but simply return proper error code from the function - I'm fine with it - makes code smaller) >> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance) >> +{ >> + struct net_device *net_dev = (struct net_device *)dev_instance; > > Don't need to cast void * > netdev is a more common variable name Good point - thanks. >> + if (status & TXINT_MASK) { >> + unsigned int i, info; >> + struct sk_buff *skb; > [] >> + if (status & TXCH_MASK) { >> + priv->stats.tx_errors++; >> + priv->stats.tx_aborted_errors++; >> + netdev_err(priv->net_dev, >> + "Tx chaining err! txbd_dirty = %u\n", >> + priv->txbd_dirty); > > You already have a local net_dev. > why not use that? Good point - overengineered stuff) >> +int arc_emac_open(struct net_device *net_dev) >> +{ >> + struct arc_emac_priv *priv = netdev_priv(net_dev); >> + struct arc_emac_bd_t *bd; >> + struct sk_buff *skb; >> + int i; >> + >> + if (!priv->phy_node) { >> + netdev_err(net_dev, "arc_emac_open: phy device is absent\n"); > > If you really need the function name, > it's better to use "%s: ", __func__ Reminder from previous "printk". Will clean-up. >> +int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev) >> +{ > [] >> +tx_next_chunk: >> + >> + info = priv->txbd[priv->txbd_curr].info; >> + if (likely((info & OWN_MASK) == FOR_CPU)) { > > if (unlikely(!(etc...) > return NETDEV_TX_BUSY; > > and save an indent level Same comment as above - will invert logic. >> +/** > >> +int arc_emac_set_address(struct net_device *net_dev, void *p) > [] >> + EMAC_REG_SET(priv->reg_base_addr, R_ADDRH, >> + *(unsigned int *)&net_dev->dev_addr[4] & 0x0000ffff); > > That doesn't seem endian friendly. Right. Need to revisit this one. >> +static int arc_emac_probe(struct platform_device *pdev) >> +{ > [] >> + /* Get phy from device tree */ >> + priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy", 0); >> + if (!priv->phy_node) { >> + dev_err(&pdev->dev, "failed to retrieve phy description " >> + "from device tree\n"); > > Coalesce formats please Could you please clarify how should I format lines in question? I'm a bit lost here. >> + err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs); >> + if (err) { >> + dev_err(&pdev->dev, "failed to retrieve registers base " >> + "from device tree\n"); > [] >> + /* Get IRQ from device tree */ >> + err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq); >> + if (!err) { >> + dev_err(&pdev->dev, "failed to retrieve <irq> value " >> + "from device tree\n"); > [] -Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
