netdev - Re: [PATCH V2] Add platform driver support to the CS890x driver

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Nov 2011 09:06:15 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Jaccon Bastiaansen <jaccon.bastiaansen@...il.com>
Cc:	netdev@...r.kernel.org,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>, kernel@...gutronix.de
Subject: Re: [PATCH V2] Add platform driver support to the CS890x driver

Hi Jaccon,

On Sun, Oct 09, 2011 at 10:51:23PM +0200, Jaccon Bastiaansen wrote:
> Hello,
> 
> This patch hasn't been sent to the netdev mailing list before, sorry for that.

I appreciate what you are trying to do. The cs89x0 is still not dead and
it's quite annoying that we do not have proper platform device driver
support for it. Unfortunately your patch was ignored by the important
people, so can you respin it? You should create a proper series from it
with one patch for the driver and one patch per board. This helps to
increase your visibility and also you can set the individual board
maintainers on Cc for their board. Please also Cc
netdev@...r.kernel.org and the arm linux kernel mailing list.
Your patch also contains some cleanups like the removal of the unused
QQ2440. You should create a seperate patch for this, then it will be
easier to review (and also people love to read 'cleanup' in a patch
subject ;)

Your mailer turns tabs into spaces, you should fix this before
resending.

Some more comments inline.


> 
> +static struct resource ixdp2x01_cs8900_resources[] = {
> +       {
> +               .start  = (u32)IXDP2X01_CS8900_VIRT_BASE,
> +               .end    = (u32)IXDP2X01_CS8900_VIRT_BASE + 0x1000 - 1,
> +               .flags  = IORESOURCE_MEM,
> +       },

This is wrong. resources are about physical addresses, not virtual. You
have to ioremap them in the driver.

> diff --git a/drivers/net/cs89x0.c b/drivers/net/cs89x0.c
> index 537a4b2..b7fb3bc 100644
> --- a/drivers/net/cs89x0.c
> +++ b/drivers/net/cs89x0.c
> @@ -12,6 +12,14 @@
>         The author may be reached at nelson@...nwr.com, Crynwr
>         Software, 521 Pleasant Valley Rd., Potsdam, NY 13676
> 
> +Sources
> +
> +       Crynwr packet driver epktisa.
> +
> +       Crystal Semiconductor data sheets.
> +
> +
> +

This seems unrelated to this patch. Please drop.

>   Changelog:
> 
>   Mike Cruse        : mcruse@...-ltd.com
> @@ -98,39 +106,14 @@
>   Domenico Andreoli : cavokz@...il.com
>                     : QQ2440 platform support
> 
> +  Jaccon Bastiaansen: jaccon.bastiaansen@...il.com
> +                   : added platform driver support

The history in git is enough (and even better) than the changelog in the
file headers. Please drop this.

> +
> +#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;
> +
> +       mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       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");
> +               err = -ENOENT;
> +               goto out;
> +       }
> +
> +       cs8900_irq_map[0] = irq_res->start;

This limits the driver to a single instance. I think this is ok for now
as an intermediate step, but you should check this and bail out with
-EBUSY if a second instance is registered.

> +       err = cs89x0_probe1(dev, mem_res->start, 0);
> +       if (err) {
> +               pr_warning("no cs8900 or cs8920 detected.\n");
> +               goto out;
> +       }
> +
> +       platform_set_drvdata(pdev, dev);
> +       return 0;
> +out:
> +       free_netdev(dev);
> +       return err;
> +}

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