> On Fri, 22 Jun 2007 21:53:58 +0200 [EMAIL PROTECTED] (Thomas Bogendoerfer)
> wrote:
> Hi,
>
> This is new ethernet driver, which use the code taken out of lasi_82596
> (done by the other patch I just sent).
>
> Thomas.
>
>
> Ethernet driver for EISA only SNI RM200/RM400 machines
>
> ...
>
> +static char sni_82596_string[] = "snirm_82596";
const?
> +
> +#define DMA_ALLOC dma_alloc_coherent
> +#define DMA_FREE dma_free_coherent
> +#define DMA_WBACK(priv, addr, len) do { } while (0)
> +#define DMA_INV(priv, addr, len) do { } while (0)
> +#define DMA_WBACK_INV(priv, addr, len) do { } while (0)
> +
> +#define SYSBUS 0x00004400
> +
> +/* big endian CPU, 82596 little endian */
> +#define SWAP32(x) cpu_to_le32((u32)(x))
> +#define SWAP16(x) cpu_to_le16((u16)(x))
> +
> +#define OPT_MPU_16BIT 0x01
> +
> +static inline void CA(struct net_device *dev);
> +static inline void MPU_PORT(struct net_device *dev, int c, dma_addr_t x);
These two function's implementations could be moved to before the #include,
s we wouldn't need to forward-declare them?
> +#include "lib82596.c"
ugh. Is this really unavoidable?
> +MODULE_AUTHOR("Thomas Bogendoerfer");
> +MODULE_DESCRIPTION("i82596 driver");
> +MODULE_LICENSE("GPL");
> +module_param(i596_debug, int, 0);
> +MODULE_PARM_DESC(i596_debug, "82596 debug mask");
> +
> +static inline void CA(struct net_device *dev)
> +{
> + struct i596_private *lp = netdev_priv(dev);
> +
> + writel(0, lp->ca);
> +}
> +
> +
> +static inline void MPU_PORT(struct net_device *dev, int c, dma_addr_t x)
> +{
> + struct i596_private *lp = netdev_priv(dev);
> +
> + u32 v = (u32) (c) | (u32) (x);
> +
> + if (lp->options & OPT_MPU_16BIT) {
> + writew(v & 0xffff, lp->mpu_port);
> + wmb(); udelay(1); /* order writes to MPU port */
Nope, please put these on separate lines. No exceptions..
> + writew(v >> 16, lp->mpu_port);
> + } else {
> + writel(v, lp->mpu_port);
> + wmb(); udelay(1); /* order writes to MPU port */
> + writel(v, lp->mpu_port);
> + }
> +}
Three callsites: This looks too large to inline.
I see no reason why this and CA() are have upper-case names?
> +
> +static int __devinit sni_82596_probe(struct platform_device *dev)
> +{
> + struct net_device *netdevice;
> + struct i596_private *lp;
> + struct resource *res, *ca, *idprom, *options;
> + int retval = -ENODEV;
> + static int init;
> + void __iomem *mpu_addr = NULL;
> + void __iomem *ca_addr = NULL;
> + u8 __iomem *eth_addr = NULL;
> +
> + if (init == 0) {
> + printk(KERN_INFO SNI_82596_DRIVER_VERSION "\n");
> + init++;
> + }
Might as well do this message in the module_init() function? There's a
per-probed-device message later on anwyay.
The patchset tries to add rather a lot of new trailing whitespace btw.
> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> + if (!res)
> + goto probe_failed;
> + mpu_addr = ioremap_nocache(res->start, 4);
> + if (!mpu_addr) {
> + retval = -ENOMEM;
> + goto probe_failed;
> + }
> + ca = platform_get_resource(dev, IORESOURCE_MEM, 1);
> + if (!ca)
> + goto probe_failed;
> + ca_addr = ioremap_nocache(ca->start, 4);
> + if (!ca_addr) {
> + retval = -ENOMEM;
> + goto probe_failed;
> + }
> + idprom = platform_get_resource(dev, IORESOURCE_MEM, 2);
> + if (!idprom)
> + goto probe_failed;
> + eth_addr = ioremap_nocache(idprom->start, 0x10);
> + if (!eth_addr) {
> + retval = -ENOMEM;
> + goto probe_failed;
> + }
> + options = platform_get_resource(dev, 0, 0);
> + if (!options)
> + goto probe_failed;
> +
> + printk(KERN_INFO "Found i82596 at 0x%x\n", res->start);
> +
> + netdevice = alloc_etherdev(sizeof(struct i596_private));
> + if (!netdevice) {
> + retval = -ENOMEM;
> + goto probe_failed;
> + }
> + SET_NETDEV_DEV(netdevice, &dev->dev);
> + platform_set_drvdata (dev, netdevice);
> +
> + netdevice->base_addr = res->start;
> + netdevice->irq = platform_get_irq(dev, 0);
> +
> + /* someone seams to like messed up stuff */
> + netdevice->dev_addr[0] = readb(eth_addr + 0x0b);
> + netdevice->dev_addr[1] = readb(eth_addr + 0x0a);
> + netdevice->dev_addr[2] = readb(eth_addr + 0x09);
> + netdevice->dev_addr[3] = readb(eth_addr + 0x08);
> + netdevice->dev_addr[4] = readb(eth_addr + 0x07);
> + netdevice->dev_addr[5] = readb(eth_addr + 0x06);
> + iounmap(eth_addr);
> +
> + if (!netdevice->irq) {
> + printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n",
> + __FILE__, netdevice->base_addr);
> + goto probe_failed;
> + }
> +
> + lp = netdev_priv(netdevice);
> + lp->options = options->flags & IORESOURCE_BITS;
> + lp->ca = ca_addr;
> + lp->mpu_port = mpu_addr;
> +
> + retval = i82596_probe(netdevice);
> + if (retval) {
> + free_netdev(netdevice);
> +probe_failed:
> + if (mpu_addr)
> + iounmap(mpu_addr);
> + if (ca_addr)
> + iounmap(ca_addr);
> + if (eth_addr)
> + iounmap(ca_addr);
> + }
> + return retval;
> +}
> +
> +static int __devexit sni_82596_driver_remove (struct platform_device *pdev)
Extraneous space ^ here
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> + struct i596_private *lp = netdev_priv(dev);
> +
> + unregister_netdev (dev);
and ^ here
checkpatch.pl will find these things.
> + DMA_FREE(dev->dev.parent, sizeof(struct i596_private),
> + lp->dma, lp->dma_addr);
> + iounmap(lp->ca);
> + iounmap(lp->mpu_port);
> + free_netdev (dev);
> + return 0;
> +}
> +
> +static struct platform_driver sni_82596_driver = {
> + .probe = sni_82596_probe,
> + .remove = __devexit_p(sni_82596_driver_remove),
> + .driver = {
> + .name = sni_82596_string,
> + },
> +};
> +
> +static int __devinit sni_82596_init(void)
> +{
> + return platform_driver_register(&sni_82596_driver);
> +}
> +
> +
> +static void __exit sni_82596_exit(void)
> +{
> + platform_driver_unregister(&sni_82596_driver);
> +}
> +
> +module_init(sni_82596_init);
> +module_exit(sni_82596_exit);
-
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
