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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 7 Nov 2011 13:14:14 +0100
From:	Jaccon Bastiaansen <jaccon.bastiaansen@...il.com>
To:	Sascha Hauer <s.hauer@...gutronix.de>
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

Hello Sascha,

I will do this. Since I'm on holiday the rest of the week, it will
take some time before I can post the new patches.

Regards,
  Jaccon

2011/11/3 Sascha Hauer <s.hauer@...gutronix.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@...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