I don't mind applying your patch, it looks basically ok. Maybe you will
be kind enough to submit an update patch which considers the following
issues:
* why are calls to pci_dma_sync_single called with "if (0)" ? Are you
sure this patch should be applied to the kernel?
+ if (0) pci_dma_sync_single(lp->pci_dev, lp->rx_dma_addr[i],
rx_skbuff->len, PCI_DMA_FROMDEVICE);
* pcnet32_tbl[] seems to duplicate needlessly information found in
pcnet32_pci_tbl[]
* MODULE_DEVICE_TABLE not present in code
* do not use check-region. instead, use request-region and test its
return value for NULL (== region unavailable).
* use "return -ENODEV" instead of "return ENODEV". The '-' style is
preferred since typical error tests throughout the code look like
if (rc < 0) { /* we got an error */ }
* two errors in the following code. (1) "if (0)", (2) you don't need to
print out a message, the PCI subsystem does that for you
* consider turning "switch (chip-version)" into a lookup table.
* You don't need to allocate and align your private structure manually.
init_etherdev does this for you. Simply pass the -true- size of your
private structure to init_ethernet, and it will allocate and align the
space for you. All you need to do is take the value of dev->priv:
dev = init_etherdev (NULL, sizeof (struct my_private_struct));
if (!dev) return -ENOMEM;
my_priv = dev->priv;
...
* all calls to printk() should have their formats contain a KERN_xxx
prefix:
printk(KERN_WARNING "warrior is about to die\n");
* ioctl() needs to be protected by hardware spinlock
* why don't you implement a PCI driver cleanup in the remove() method?
By not doing so, you are preventing any chance of hotplug with this
driver. Plus, since pci_unregister_driver calls remove() once for each
PCI driver, it should simplify the code a bit -- you don't have to
manually loop in cleanup_module or similar. Also note you can use
pdev->driver_data to store information (like dev or dev->priv) which is
generally kept in a linked list. For example of drivers which do this,
while supporting non-PCI buses like EISA or VLB, check out the updated
3c59x.c.
* check the return value of pci_module_init!
-- Jeff Garzik | Nothing cures insomnia like the Building 1024 | realization that it's time to get up. MandrakeSoft, Inc. | -- random fortune- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun May 07 2000 - 21:00:10 EST