On Sat, 2006-04-01 at 05:17 -0800, Linsys Contractor Amit S. Kale wrote:
> +extern struct netxen_adapter_s *g_adapter;
> +extern spinlock_t hal_lock;
Should be in a header file.
> +void
> +netxen_nic_pci_change_crbwindow(netxen_adapter *adapter, uint32_t wndw)
Should use u32, not POSIX type names. I see you use a mixture of both;
that's messy.
> + NetXen_NIC_PCI_WRITE_32(*(unsigned int *)&window, (void*)(offset));
Use writel and family directly.
> + //printk("set crbw : 0x%08x\n",off);
Please drop debugging statements that aren't used.
> +/*
> + * Set the CRB window based on the offset.
> + */
> +static inline unsigned long
> +netxen_nic_pci_set_crbwindow(netxen_adapter *adapter, u64 off)
Why is this inlined?
> + return (off);
Lose the parens.
> + if (adapter->curr_window != 0) {
> + netxen_nic_pci_change_crbwindow(adapter, 0);
> + }
Single-statement blocks don't need curlies.
> + /* strange address given */
> + dump_stack();
> + printk(KERN_WARNING"%s: Warning: netxen_nic_pci_set_crbwindow called
> with"
> + " an unknown address(%llx)\n", netxen_nic_driver_name, off);
> +
> + return (off);
This seems like rather obnoxious behaviour. Something goes wrong, you
puke out a stack trace, and then continue anyway?
> + if(ADDR_IN_WINDOW1(off))
> + {//Window 1
> + addr = CRB_NORMALIZE(adapter, off);
> + read_lock(&adapter->adapter_lock);
> + }
> + else{//Window 0
> + addr = (void *)(uptr_t)(adapter->ahw.pci_base + off);
> + write_lock_irqsave(&adapter->adapter_lock, flags);
> + netxen_nic_pci_change_crbwindow(adapter, 0);
> + }
This looks like Windows driver code. Please fix up the style.
> +/* read num_words from hardware (num_words in 64 bits */
> +void
> +netxen_nic_mem_block_read(struct netxen_adapter_s *adapter, u64 off,
> + void *data, int num_words)
Stuff like this should be using sparse annotations so readers can tell
what's in __iomem and what isn't. You should make your driver pass a
compile with "make C=1" without any warnings.
> +/* Write num_words (in 64 bits) in the phantom memory */
> +/*void
> +netxen_nic_mem_block_write(struct netxen_adapter_s *adapter, u64 off,
> + void *data, int num_words)
Please omit routines that aren't even compiled.
> + DPRINTK(1, INFO, "writing data %lx to offset %lx\n",
> + *(unsigned long *)data, (unsigned long)off);
What does DPRINTK(1... mean? I only ever see it called with 1 as the
first param, so why's it even there?
> +long
> +netxen_nic_init_port (struct netxen_port *port)
> +{
> + struct netxen_adapter_s *adapter = port->adapter;
> + long portnum = port->portnum;
> + long ret = 0;
> + unsigned long flags;
> +
> + switch (adapter->ahw.board_type) {
> + case NetXen_NIC_GBE:
> + netxen_nic_disable_phy_interrupts (adapter, portnum);
> + udelay(20000);
What's with the magic uncommented udelay?
> + long portnum = port->portnum;
> + struct netxen_adapter_s *adapter = port->adapter;
> + unsigned long flags;
I personally dislike having variable names line up like this, but if
you're going to make some of them line up, you should do it for them
all. Or preferably use one space for all.
> +int
> +netxen_crb_writelit_adapter (unsigned long off, int data, struct
> netxen_adapter_s *adapter)
> +{ //Modified. All the direct calls to this func are changed.
> + //This gets called only thru' some macros like NetXen_CRB_WRITELIT
> + unsigned long flags;
> + void *addr;
> +
> + if(ADDR_IN_WINDOW1(off))
> + {
> + read_lock(&adapter->adapter_lock);
> + NetXen_NIC_PCI_WRITE_32(data, CRB_NORMALIZE(adapter, off));
> + read_unlock(&adapter->adapter_lock);
> + }
> + else{
Again, please fix the Windows-like coding and commenting style.
> + //netxen_nic_write_w0 (adapter, off, data);
> + write_lock_irqsave(&adapter->adapter_lock, flags);
> + netxen_nic_pci_change_crbwindow(adapter, 0);
> + addr = (void *)(adapter->ahw.pci_base + off);
> + NetXen_NIC_PCI_WRITE_32(data, addr);
> + netxen_nic_pci_change_crbwindow(adapter, 1);
> + write_unlock_irqrestore(&adapter->adapter_lock, flags);
> + }
> +
> + return 0;
> +}
Why declare this as returning int if it always returns zero?
> +native_t
> +netxen_crb_read_val (unsigned long off)
What the heck is a native_t?
> +int
> +netxen_crb_write_adapter (unsigned long off, void *data, struct
> netxen_adapter_s *adapter)
> +int
> +netxen_crb_read_adapter (unsigned long off, void *data, struct
> netxen_adapter_s *adapter)
> +int
> +netxen_crb_read_val_adapter (unsigned long off, struct netxen_adapter_s
> *adapter)
It's normal practice to have the offset come second in function
signatures like these.
> +void DELAY(A)
> +{
> + unsigned long remainder;
> +
> + remainder = A/50000;
> + do {
> + if (remainder > 1000) {
> + udelay(1000);
> + remainder -= 1000;
> + } else {
> + udelay(remainder + 1);
> + remainder = 0;
> + }
> + } while (remainder > 0);
> +}
Ew! This should not be global, not uppercased, should have a
driver-specific name prefix, and what on earth is it doing anyway?
Another global comment: you need to run unexpand on all of your source
files. They're filled with spaces instead of tabs.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
