Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751940Ab3FIWPl (ORCPT ); Sun, 9 Jun 2013 18:15:41 -0400 Received: from violet.fr.zoreil.com ([92.243.8.30]:37871 "EHLO violet.fr.zoreil.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751763Ab3FIWPk (ORCPT ); Sun, 9 Jun 2013 18:15:40 -0400 Date: Mon, 10 Jun 2013 00:15:08 +0200 From: Francois Romieu To: Alexey Brodkin Cc: "netdev@vger.kernel.org" , Vineet Gupta , Mischa Jonker , Arnd Bergmann , Grant Likely , Rob Herring , Paul Gortmaker , "David S. Miller" , "linux-kernel@vger.kernel.org" , "devicetree-discuss@lists.ozlabs.org" , "joe@perches.com" Subject: Re: [PATCH v2] ethernet/arc/arc_emac - Add new driver Message-ID: <20130609221508.GA12264@electric-eye.fr.zoreil.com> References: <1370617656-18349-1-git-send-email-abrodkin@synopsys.com> <20130607203231.GA14077@electric-eye.fr.zoreil.com> <4881796E12491D4BB15146FE0209CE643F5CB230@DE02WEMBXB.internal.synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4881796E12491D4BB15146FE0209CE643F5CB230@DE02WEMBXB.internal.synopsys.com> X-Organisation: Land of Sunshine Inc. User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3485 Lines: 118 Alexey Brodkin : > On 06/08/2013 12:33 AM, Francois Romieu wrote: > > Alexey Brodkin : [...] > As replied to Joe I just want to name people contributed in this driver. > What is a appropriate way to do it? A polite way could be to see with contributors if it's ok for them to appear in the (c) section. [...] > > You may #define (FRST_MASK | LAST_MASK) > > This combo is used in only 2 places so is it worth to introduce another > define? With these (FRST_MASK | LAST_MASK) I suppose reader will > understand that these are 2 separate bits. But still it might be just my > vision and if #define (FRST_MASK | LAST_MASK) looks better then I'll add > it immediately. Your call. The #define only needs to appear in or near the function. [...] > >> + if (!skbnew) { > >> + netdev_err(net_dev, "Out of memory, " > >> + "dropping packet\n"); > > > > Rate limit or do nothing. > > Not clear what do you mean. Could you please clarify ? net_ratelimit() will prevent a storm of log messages. Actually I would avoid the message completely. [...] > >> +{ > >> + struct arc_emac_priv *priv = netdev_priv(net_dev); > >> + > >> + napi_disable(&priv->napi); > >> + netif_stop_queue(net_dev); > >> + > >> + /* Disable interrupts */ > >> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE, > >> + TXINT_MASK | /* Tx interrupt */ > >> + RXINT_MASK | /* Rx interrupt */ > >> + ERR_MASK | /* Error interrupt */ > >> + TXCH_MASK); /* Transmit chaining error interrupt */ > > > > Useless comments. > > Is it clear that TXCH means "Transmit chaining error interrupt"? ERR_TXCHAIN_MASK ? > Or those defines should just have comments where they are defined and > later just use them with no comments? s/should have/may have/ Otherwise, yes. [...] > >> +static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int > reg_num) > >> +{ > >> + int error; > >> + unsigned int value; > >> + struct arc_mdio_priv *priv = bus->priv; > > > > Revert the xmas tree. > > Not clear what does it mean? Could you please calrify? struct arc_mdio_priv *priv = bus->priv; unsigned int value; int error; [...] > >> +struct arc_mdio_priv { > >> + struct mii_bus *bus; > >> + struct device *dev; > >> + void __iomem *reg_base_addr; /* MAC registers base address */ > >> +}; > > > > Overengineered ? > > Why so? Not clear, sorry. Most of this struct is shared with the device private one. I am not sure that they need to be separated. [...] > >> +#define EMAC_REG_SET(reg_base_addr, reg, val) \ > >> + iowrite32((val), reg_base_addr + reg * sizeof(int)) > >> + > >> +#define EMAC_REG_GET(reg_base_addr, reg) \ > >> + ioread32(reg_base_addr + reg * sizeof(int)) > > > > May use real non-caps functions. > > Do you mean to use "io{read|write}32" directly without macro? Add your own arc_{r/w} functions and use the device private struct pointer as one of their parameters (whence no void * parameter). [...] > Thanks a lot for this deep analisys. Not that deep: - arc_emac_tx should disable tx queue as soon as the tx ring is exhausted - you may consider moving work from the irq handler to napi poll. -- Ueimor -- 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/