| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120104092000.GP5446@pengutronix.de>
Date: Wed, 4 Jan 2012 10:20:00 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Jaccon Bastiaansen <jaccon.bastiaansen@...il.com>
Cc: kernel@...gutronix.de, u.kleine-koenig@...gutronix.de,
davem@...emloft.net, cavokz@...il.com,
linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/4] CS89x0 : add platform driver support
Hi Jaccon,
Thanks for continuing this. Some comments inline.
On Tue, Jan 03, 2012 at 11:12:09PM +0100, Jaccon Bastiaansen wrote:
> @@ -151,6 +153,9 @@
> #include <asm/system.h>
> #include <asm/io.h>
> #include <asm/irq.h>
> +#ifdef CONFIG_CS89x0_PLATFORM
> +#include <linux/atomic.h>
> +#endif
No need to ifdef includes.
> #if ALLOW_DMA
> #include <asm/dma.h>
> #endif
> @@ -173,6 +178,7 @@ static char version[] __initdata =
> /* The cs8900 has 4 IRQ pins, software selectable. cs8900_irq_map maps
> them to system IRQ numbers. This mapping is card specific and is set to
> the configuration of the Cirrus Eval board for this chip. */
> +#if defined(CONFIG_CS89x0_PLATFORM)
> #if defined(CONFIG_MACH_IXDP2351)
> static unsigned int netcard_portlist[] __used __initdata = {IXDP2351_VIRT_CS8900_BASE, 0};
> static unsigned int cs8900_irq_map[] = {IRQ_IXDP2351_CS8900, 0, 0, 0};
> @@ -188,7 +194,17 @@ static unsigned int cs8900_irq_map[] = { QQ2440_CS8900_IRQ, 0, 0, 0 };
> static unsigned int netcard_portlist[] __used __initdata = {
> PBC_BASE_ADDRESS + PBC_CS8900A_IOBASE + 0x300, 0
> };
> -static unsigned cs8900_irq_map[] = {EXPIO_INT_ENET_INT, 0, 0, 0};
> +static unsigned int cs8900_irq_map[] = {EXPIO_INT_ENET_INT, 0, 0, 0};
> +#else
> +/*
> + * Counter for the number of CS89x0 platform device instances, which is needed
> + * because this driver currently supports only one CS89x0 platform device
> + * instance.
> + */
> +static atomic_t platform_dev_cnt = ATOMIC_INIT(0);
> +static unsigned int cs8900_irq_map[] = {0, 0, 0, 0};
> +static unsigned int netcard_portlist[] __used __initdata = {0, 0};
> +#endif
> #else
> static unsigned int netcard_portlist[] __used __initdata =
> { 0x300, 0x320, 0x340, 0x360, 0x200, 0x220, 0x240, 0x260, 0x280, 0x2a0, 0x2c0, 0x2e0, 0};
> @@ -737,7 +753,7 @@ cs89x0_probe1(struct net_device *dev, int ioaddr, int modular)
> } else {
> i = lp->isa_config & INT_NO_MASK;
> if (lp->chip_type == CS8900) {
[...]
>
> -#ifdef MODULE
> +#if defined(MODULE) && !defined(CONFIG_CS89x0_PLATFORM)
>
> static struct net_device *dev_cs89x0;
>
> @@ -1900,7 +1916,78 @@ cleanup_module(void)
> release_region(dev_cs89x0->base_addr, NETCARD_IO_EXTENT);
> free_netdev(dev_cs89x0);
> }
> -#endif /* MODULE */
> +#endif /* MODULE && !CONFIG_CS89x0_PLATFORM */
> +
> +#ifdef CONFIG_CS89x0_PLATFORM
> +static int cs89x0_platform_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev = alloc_etherdev(sizeof(struct net_local));
> + struct resource *mem_res;
> + struct resource *irq_res;
> + int err;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + if (atomic_inc_return(&platform_dev_cnt) != 1)
> + return -EBUSY;
This deserves a comment like this:
/*
* This driver uses global variables. Until this is fixed we can
* only support a single instance.
*/
> +
> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
Don't use virtual addresses in resources. It's wrong. Pass in physical
addresses here and use ioremap() in the driver.
> + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (mem_res == NULL || irq_res == NULL) {
> + pr_warning("memory and/or interrupt resource missing.\n");
dev_warn please
> + err = -ENOENT;
> + goto out;
> + }
> +
> + cs8900_irq_map[0] = irq_res->start;
> + err = cs89x0_probe1(dev, mem_res->start, 0);
> + if (err) {
> + pr_warning("no cs8900 or cs8920 detected.\n");
ditto
> + goto out;
> + }
> +
> + platform_set_drvdata(pdev, dev);
> + return 0;
> +
> +out:
> + free_netdev(dev);
> + return err;
> +}
> +
> +static int cs89x0_platform_remove(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + unregister_netdev(dev);
> + free_netdev(dev);
> + return 0;
> +}
> +
> +static struct platform_driver cs89x0_platform_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE
> + },
> + .probe = cs89x0_platform_probe,
> + .remove = cs89x0_platform_remove
Please add a comma at the end of the last elements aswell. The rationale
if that we can add elements later without touching the existing lines.
>
> /*
> * Local variables:
> @@ -1911,3 +1998,4 @@ cleanup_module(void)
> * End:
> *
> */
> +
Please don't add blank lines at the end of files.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists