lkml.org 
[lkml]   [2012]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] ARM: LPC32xx: Ethernet driver
From
Date
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.




\
 
 \ /
  Last update: 2012-02-25 21:43    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site