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

[Sascha Hauer <s.hauer@pengutronix.de>]

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@vger.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@crynwr.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@cti-ltd.com
> @@ -98,39 +106,14 @@
>   Domenico Andreoli : cavokz@gmail.com
>                     : QQ2440 platform support
> 
> +  Jaccon Bastiaansen: jaccon.bastiaansen@gmail.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 |