From: ganesanr@xxxxxxxxxxxx
Date: Tue, 5 Feb 2013 17:00:19 +0530
> +config NETLOGIC_XLR_NET
> + tristate "Netlogic XLR/XLS network device"
> + default y
> + select PHYLIB
> + depends on CPU_XLR
> + ---help---
> + This driver support Netlogic XLR/XLS on chip gigabit
> + Ethernet.
No individual device driver should default to 'y'. Vendor guards, yes, can
default to 'y', but not individual drivers.
> +/*
> + * The readl/writel implementation byteswaps on XLR/XLS, so
> + * we need to use __raw_ IO to read the NAE registers
> + * because they are in the big-endian MMIO area on the SoC.
> + */
Comments in the networking are to be formatted:
/* Like
* this.
*/
Please fix this up in your entire driver.
> +/*
> + * Table of net_device pointers indexed by port, this will be used to
> + * lookup the net_device corresponding to a port by the message ring handler.
> + *
> + * Maximum ports in XLR/XLS is 8(8 GMAC on XLS, 4 GMAC + 2 XGMAC on XLR)
> + */
> +static struct net_device *mac_to_ndev[8];
Make this a dynamic data structure, a parent device that the individual
netdevs are hung off of, it can still be an array. That way you can have
a bonafide struct device instance and associated hierarchy of devices in
the kernel device list.
Also avoid this strange and non-standard usage of "MAC" as an integer
port index. The canonical meaning of MAC is the link-layer address of
the device.
> +
> +static inline struct sk_buff *mac_get_skb_back_ptr(void *addr)
> +{
> + struct sk_buff **back_ptr;
> +
> + /* this function should be used only for newly allocated packets.
> + * It assumes the first cacheline is for the back pointer related
> + * book keeping info.
> + */
> + back_ptr = (struct sk_buff **)(addr - MAC_SKB_BACK_PTR_SIZE);
> + return *back_ptr;
> +}
Use the skb->cb[] control block rather than mis-using the skb data area
for storing internal driver state.
> + paddr = virt_to_bus(addr);
virt_to_bus() is verboten, use the proper DMA APIs. I don't care if
this is a specialized driver for a special platform.
> + addr = bus_to_virt(msg->msg0 & 0xffffffffffULL);
bus_to_virt() is verbotten, use the proper DMA APIs and store correct
references to packets in a translation data structure in your per-netdev
software state.
> +static void __maybe_unused xlr_wakeup_queue(unsigned long dev)
This is really unused, just delete it.
That's enough for me, this driver needs a lot of work.
