Re: [PATCH v2] ARM: LPC32xx: Ethernet driver

[Joe Perches <joe@perches.com>]

On Sat, 2012-02-25 at 21:21 +0100, Roland Stigge wrote:
> This patch adds an ethernet driver for the LPC32xx ARM SoC.

Just some style notes:

> +++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

[]

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/ioport.h>
> +#include <linux/crc32.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/ethtool.h>
> +#include <linux/mii.h>
> +#include <linux/clk.h>
> +#include <linux/workqueue.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/phy.h>
> +#include <linux/dma-mapping.h>
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <mach/board.h>
> +#include <mach/platform.h>
> +#include <mach/hardware.h>
> +#include "lpc_eth.h"
> +
> +#define MODNAME "lpc-net"

lpc-net or lpc_eth?
[]
> +/*
> + * MAC address is provided as a boot parameter (ethaddr)
> + */
> +static u8 mac_address[6] = {0, 0, 0, 0, 0, 0};
[]
> +static void __lpc_set_mac(struct netdata_local *pldat, u8 *mac)
[]
> +	pr_debug("Ethernet MAC address %02x:%02x:%02x:%02x:%02x:%02x\n",
> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +}

	pr_debug("MAC address: %pM\n", mac)

[]
> +static int lpc_mii_probe(struct net_device *ndev)
> +{
[]
> +	if (!phydev) {
> +		pr_err("%s: no PHY found\n", ndev->name);

It looks like most of these pr_<level>(...) uses
should be netdev_<level>(ndev, ...)

[]
> +static void __lpc_handle_recv(struct net_device *ndev)
> +{
> +	struct netdata_local *pldat = netdev_priv(ndev);
> +	struct sk_buff *skb;
> +	int rxconsidx, len, ethst;
> +	struct rx_status_t *prxstat;
> +	u8 *prdbuf;
> +
> +	/* Get the current RX buffer indexes */
> +	rxconsidx = (int) readl(LPC_ENET_RXCONSUMEINDEX(pldat->net_base));

Please try to be consistent with space or no space after cast.
I think no space is better.

> +	while (rxconsidx !=
> +			(int)readl(LPC_ENET_RXPRODUCEINDEX(pldat->net_base))) {

> +		/* Get pointer to receive status */
> +		prxstat = (struct rx_status_t *) pldat->rx_stat_v[rxconsidx];
> +		len = (prxstat->statusinfo & 0x7FF) + 1;
> +
> +		/* Status error? */
> +		ethst = prxstat->statusinfo;
> +		if ((ethst & 0xBF800000) == 0x84000000)
> +			ethst &= ~0x80000000;
> +
> +		if (ethst & 0x80000000) {
> +			/* Check statuses */
> +			if (prxstat->statusinfo & (1 << 28)) {

Might be better to use a temporary for prxstat->statusinfo

> +				/* Overrun error */
> +				ndev->stats.rx_fifo_errors++;
> +			} else if (prxstat->statusinfo & (1 << 23)) {
> +				/* CRC error */
> +				ndev->stats.rx_crc_errors++;
> +			} else if (prxstat->statusinfo & (1 << 25)) {
> +				/* Length error */
> +				ndev->stats.rx_length_errors++;
> +			} else if (prxstat->statusinfo & 0x80000000) {
> +				/* Other error */
> +				ndev->stats.rx_length_errors++;
> +			}
> +			ndev->stats.rx_errors++;
> +		} else {
> +			/* Packet is good */
> +			skb = dev_alloc_skb(len + 8);
> +			if (!skb)
> +				ndev->stats.rx_dropped++;
> +			else {
> +				skb_reserve(skb, 8);
> +				prdbuf = skb_put(skb, (len - 0));
> +
> +				/* Copy packer from buffer */
> +				memcpy(prdbuf,
> +					(void *)pldat->rx_buff_v[rxconsidx],

Probably don't need the cast to (void *)
> +					len);
> +
[]
> +static irqreturn_t __lpc_eth_interrupt(int irq, void *dev_id)
> +{
[]
> +	/* Get the interrupt status */
> +	tmp = readl(LPC_ENET_INTSTATUS(pldat->net_base));
> +
> +	while (tmp) {

probably better as

	while ((tmp = readl(...)) {
[...]

> +		/* Recheck the interrupt status */
> +		tmp = readl(LPC_ENET_INTSTATUS(pldat->net_base));
> +	}
[]
> +static int lpc_net_close(struct net_device *ndev)
> +{
> +	unsigned long flags;
> +	struct netdata_local *pldat = netdev_priv(ndev);
> +
> +	if (netif_msg_ifdown(pldat))
> +		dev_dbg(&pldat->pdev->dev, "shutting down %s\n", ndev->name);

	netif_dbg(pltdat, ifdown, ndev, ...)
[]
> +static int lpc_net_drv_probe(struct platform_device *pdev)
[]
> +	if (use_iram_for_net()) {
> +		dma_handle = (dma_addr_t) LPC32XX_IRAM_BASE;
> +		if (pldat->dma_buff_size <= lpc32xx_return_iram_size())
> +			pldat->dma_buff_base_v =
> +				(u32) io_p2v(LPC32XX_IRAM_BASE);
> +		else
> +			pr_err("%s: IRAM not big enough for net buffers, "
> +				"using SDRAM instead.\n", MODNAME);

Please coalesce formats and ignore 80 column limits
to make grep easier.