A quick look only: Matti Linnanvuori <[EMAIL PROTECTED]> writes:
> +++ linux-2.6.24/drivers/net/wan/retina.c > + CHANGES > + ------- > + > + v1.0.0 (JK) - May 27, 2003: > + * Original driver. > + > + v1.1.0 (JK) - June, 2003: > + * final Flexibilis driver > + > + v1.2.0: NO_ARP option back again > + > + v1.2.1: (JT) - Aug 21, 2003: > + * Added support for Retina C5400 card including PROC stuff. > + > + v1.2.2: (Petri Ahonen) - Sep 19, 2003: And so on - I'm not sure such logs belong here. > +#define DRV_NAME "retina" > +#define DRV_VERSION "1.2.5" > +#define DRV_RELDATE "November 14, 2003" Hmm... > +/* obsolete > + see retina_noarp_with_ptp > +define FEPCI_NO_ARP */ If it's obsolete (or rather unused), just drop it. > +#undef inb > +#undef inw > +#undef inl > +#undef outb > +#undef outw > +#undef outl > +#define inb nonexistent /* force using only 32bit access */ > +#define inw nonexistent /* force using only 32bit access */ > +#define inl(x) le32_to_cpu(readl(x)) > +#define outb nonexistent /* force using only 32bit access */ > +#define outw nonexistent /* force using only 32bit access */ > +#define outl(value, address) writel(cpu_to_le32(value), address) Any code like that is write-only, why don't just use readl()/ writel() in the actual code? Are you sure about this cpu_to_le32? readl()/writel() already preserve the value. > +#define VMA_OFFSET(vma) ((vma)->vm_pgoff << PAGE_SHIFT) Not sure about such things in a driver. > +enum pci_id_flags_bits { > + /* Set PCI command register bits before calling probe */ > + PCI_USES_IO = 1, PCI_USES_MEM = 2, PCI_USES_MASTER = 4, > + /* Read and map the single following PCI BAR */ > + PCI_ADDR0 = 0 << 4, PCI_ADDR1 = 1 << 4, PCI_ADDR2 = > + 2 << 4, PCI_ADDR3 = 3 << 4, > + PCI_ADDR_64BITS = 0x100, PCI_NO_ACPI_WAKE = > + 0x200, PCI_NO_MIN_LATENCY = 0x400, > + PCI_UNUSED_IRQ = 0x800, > +}; We already have such things in PCI headers, don't we? > +/* Linux 2.4 appears to drop POINTOPOINT,BROADCAST and NOARP flags Linux 2.4? > +/* proc filesystem functions introduced: */ I'm not sure we're adding new /proc files. Perhaps you should investigate sysfs and friends? > + case FEPCI_IOCTL_R_SHARED_MEM: > + DBG_PRINT(" %s: ioctl read shared mem commanded.\n", > + fepci_NAME); > + fepci_copy_to_user(arg, ioaddr + FEPCI_SHARED_MEM_OFFSETT, > + _IOC_SIZE(cmd), 0); > + break; > + case FEPCI_IOCTL_W_SHARED_MEM: > + DBG_PRINT(" %s: ioctl write shared mem commanded.\n", > + fepci_NAME); > + fepci_copy_from_user(ioaddr + FEPCI_SHARED_MEM_OFFSETT, > + arg, _IOC_SIZE(cmd), 0); > + break; > + case FEPCI_IOCTL_G_IDENTIFICATION: > + DBG_PRINT(" %s: IOCTL_G_IDENTIFICATION commanded.\n", > + fepci_NAME); > + fepci_copy_to_user(arg, > + ioaddr + FEPCI_IDENTIFICATION_OFFSETT, > + _IOC_SIZE(cmd), 1); > + break; > + case FEPCI_IOCTL_G_FEATURES: > + DBG_PRINT(" %s: IOCTL_G_FEATURES commanded.\n", fepci_NAME); > + fepci_copy_to_user(arg, ioaddr + FEPCI_FEATURES_OFFSETT, > + _IOC_SIZE(cmd), 1); > + break; Are you sure these ioctls are a good idea? Perhaps sysfs attributes would be much better? > + if (length == 0) { > + fp->rx_packets_of_size_0_stream++; > + } else if (length == 1) { > + fp->rx_packets_of_size_1_stream++; > + } else if (length == 2) { > + fp->rx_packets_of_size_2_stream++; > + } else if (length == 3) { > + fp->rx_packets_of_size_3_stream++; > + } else if (length < 8) { > + fp->rx_packets_of_size_4_7_stream++; > + } else if (length < 16) { ... I think style details are really a personal thing but this would look much better without the braces. > + } > + temp_tx = (temp_tx + 1) & (TX_RING_SIZE - 1); > + temp_tx_unit = (temp_tx_unit + 1); > + temp_tx_unit *= temp_tx_unit < fp->units; > + } > + > + return IRQ_HANDLED; No unhandled IRQ protection anymore? > +#ifdef FEPCI_POINT_TO_POINT > +static int is_ptp_interface(struct net_device *dev) > +{ > + char **p_ptp_if_name = retina_ptp_interfaces; > + unsigned int i = interfaces; > + while (i > 0 && *p_ptp_if_name != NULL) { > + if (!strncmp(dev->name, *p_ptp_if_name, sizeof(dev->name))) { > + return 1; > + } else { > + } > + p_ptp_if_name++; > + i--; > + } > + return 0; > +} A bit weird, isn't it? > +static int __devinit fepci_init_one(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + struct net_device *dev = NULL; > + struct fepci_ch_private *fp = NULL; > + int chip_idx = ent->driver_data; > + int drv_flags = pci_id_tbl[chip_idx].drv_flags; > + int i = pci_enable_device(pdev); > + u32 j; > + resource_size_t real_ioaddr; > + void *ioaddr; > + unsigned position; > + > + if (i) { > + printk(KERN_WARNING "%s: pci_enable_device returned %x.\n", > + fepci_NAME, i); > + return i; > + } Didn't spot that pci_enable_device() at first. I think we shouldn't put state-changing functions in variable declarations, especially when there are many variables. > + goto err_2; > + FOUND: The labels can be hard to spot, too. > + /* dev->name[3]= j+0x30; channel number -> ascii */ > + /* minor number -> ascii */ > + dev->name[4] = ((fp->minor * CHANNELS + j) % 10) + 0x30; > + /* minor number -> ascii */ > + dev->name[3] = ((fp->minor * CHANNELS + j) / 10) + 0x30; That 0x30 could be written as plain '0'. > + /* HW_ADDR: 00:rnd:rnd:rnd:rnd:05 */ > + dev->dev_addr[0] = 0; > + get_random_bytes(&(dev->dev_addr[1]), 4); > + dev->dev_addr[5] = 5; I think we already have a function for this. > + > +static int fepci_start_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct fepci_ch_private *fp = dev->priv; > + const unsigned cur_tx = fp->cur_tx; > + > + fp->tx_skbuffs_in++; > + > + { > + u32 tx_length = skb->len; > + u32 bus_address; > + struct sk_buff *old; > + if (unlikely(tx_length < ETH_ZLEN)) { > + struct sk_buff *bigger = Why don't you just move the variables to the beginning of this function? The extra indentation isn't good for readability. > + if (intr_status & IntrFrameTransmitted) { > + fp->tx_interrupts_since_last_timer++; > + if (netif_tx_trylock(dev)) { > + unsigned i = TX_RING_SIZE - 1; > + do { > + u32 desc_b; > + if ((fp->tx_skbuff[i] != NULL) > + && > + (((desc_b = > + inl(&fp->tx_desc[i]. > + desc_b)) & transfer_not_done) == > + 0)) { > + /* has been sent */ > + pci_unmap_single(fp-> > + this_card_priv-> > + pci_dev, > + inl(&fp-> > + tx_desc[i]. > + desc_a), > + desc_b & > + frame_length, > + PCI_DMA_TODEVICE); The above looks like a hardcopy printout with missing CRs (carriage returns) :-) Why not split it? > + [line]++; > + /* small packet counters */ > + if (length == 0) > + fp->rx_packets_of_size_0++; > + else if (length == 1) > + fp->rx_packets_of_size_1++; > + else if (length == 2) > + fp->rx_packets_of_size_2++; > + else if (length == 3) > + fp->rx_packets_of_size_3++; > + else if (length < 8) > + fp->rx_packets_of_size_4_7++; > + else if (length < 16) > + fp->rx_packets_of_size_8_15++; > + else if (length < 32) > + fp->rx_packets_of_size_16_31++; Is there a specific reason to have such detailed counters? > +int get_line_data_rate_value(unsigned char line_rate) > +{ > + switch (line_rate) { > + case 0x00: > + return 0; > + case 0x01: > + return 1; > + case 0x05: > + return 8; > + case 0x06: > + return 10; > + case 0x14: > + return 256; > + case 0x15: > + return 300; > + case 0x20: > + return 1; > + case 0x28: > + return 8; > + case 0x29: > + return 10; > + case 0x30: > + return 32; > + case 0x34: > + return 56; > + case 0x36: > + return 64; > + case 0x60: > + return 1; > + case 0xa0: > + return 1; > + default: > + return -1; > + } > +} > + > +char get_line_data_rate_unit(unsigned char line_rate) > +{ > + switch (line_rate) { > + case 0x00: > + return 0; > + case 0x01: > + return 0; > + case 0x05: > + return 0; > + case 0x06: > + return 0; > + case 0x14: > + return 0; > + case 0x15: > + return 0; > + case 0x20: > + return 'k'; > + case 0x28: > + return 'k'; > + case 0x29: > + return 'k'; > + case 0x30: > + return 'k'; > + case 0x34: > + return 'k'; > + case 0x36: > + return 'k'; > + case 0x60: > + return 'M'; > + case 0xa0: > + return 'G'; > + default: > + return 0; > + } > +} Are you sure you really want this? > +int print_line_type(unsigned char type, char *buf, int pos) > +{ > + DBG_PRINT("print_line_type %c\n", type); > + switch (type) { > + case 0: > + pos += sprintf(buf + pos, "NONE"); > + break; > + case 1: > + pos += sprintf(buf + pos, "DCombus management bus"); > + break; > + case 2: > + pos += sprintf(buf + pos, "V.24"); > + break; > + case 3: > + pos += sprintf(buf + pos, "X.21"); > + break; > + case 4: > + pos += sprintf(buf + pos, "V.35"); > + break; > + case 5: > + pos += sprintf(buf + pos, "V.11"); > + break; > + case 6: > + pos += sprintf(buf + pos, "IDSL (ISDN Basic Rate"); > + break; > + case 7: > + pos += sprintf(buf + pos, "E1 nonframed/framed"); > + break; > + case 8: > + pos += sprintf(buf + pos, "E2 nonframed/framed"); > + break; > + case 9: A table would be more readable I think. > +++ linux-2.6.24/drivers/net/wan/retina.h > @@ -0,0 +1,164 @@ Short header file included by single .c - is it worth it? Wow, really long. -- Krzysztof Halasa - 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