From patchwork Sun Sep 11 17:34:25 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jaccon Bastiaansen X-Patchwork-Id: 114241 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 40C97B71D1 for ; Mon, 12 Sep 2011 03:34:47 +1000 (EST) Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1R2nvO-0005sW-Tn; Sun, 11 Sep 2011 17:34:35 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1R2nvN-0000Ze-0R; Sun, 11 Sep 2011 17:34:33 +0000 Received: from mail-pz0-f41.google.com ([209.85.210.41]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1R2nvJ-0000ZL-63 for linux-arm-kernel@lists.infradead.org; Sun, 11 Sep 2011 17:34:30 +0000 Received: by pzk4 with SMTP id 4so6951607pzk.28 for ; Sun, 11 Sep 2011 10:34:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=6KRpGg1zIvqLANkvUuQ4601hE3AZmOuICeUCk65nwDk=; b=HQtLMjYzxASUkTbq2+isO/7vkubYkvFmjHpZ/minpsnNjtkD6oCofJ5mJXuDV87FN9 9ZqvM9VHgv4I8GHEUyxEvZOufBMTpchGtg+EesntIfzfpnPggLfo2LU4NBTooK78Olo/ BzXYKGoF4kJALuHJTds3bWsuKflaeZOYZF82M= MIME-Version: 1.0 Received: by 10.68.55.100 with SMTP id r4mr1058008pbp.124.1315762465907; Sun, 11 Sep 2011 10:34:25 -0700 (PDT) Received: by 10.142.154.3 with HTTP; Sun, 11 Sep 2011 10:34:25 -0700 (PDT) In-Reply-To: <20110910141244.GQ28816@pengutronix.de> References: <1315390967-6683-1-git-send-email-jaccon.bastiaansen@gmail.com> <20110907125047.GU28816@pengutronix.de> <20110910141244.GQ28816@pengutronix.de> Date: Sun, 11 Sep 2011 19:34:25 +0200 Message-ID: Subject: Re: [PATCH] Add platform driver support to the CS890x driver From: Jaccon Bastiaansen To: =?ISO-8859-1?Q?Uwe_Kleine=2DK=F6nig?= X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110911_133429_575852_C9DE5DC5 X-CRM114-Status: GOOD ( 44.03 ) X-Spam-Score: 1.7 (+) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (1.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.210.41 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (jaccon.bastiaansen[at]gmail.com) -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 2.5 FREEMAIL_REPLY From and body contain different freemails Cc: s.hauer@pengutronix.de, kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org Hello Uwe, 2011/9/10 Uwe Kleine-König : > Hello Jaccon, > > On Sat, Sep 10, 2011 at 01:37:07PM +0200, Jaccon Bastiaansen wrote: >> Hello Uwe, >> >> 2011/9/7 Uwe Kleine-König : >> > Hello Jaccon, >> > >> > On Wed, Sep 07, 2011 at 12:22:47PM +0200, Jaccon Bastiaansen wrote: >> >> The CS89x0 ethernet controller is used on a number of evaluation >> >> boards, such as the MX31ADS. The current driver has memory address and >> >> IRQ settings for each board on which this controller is used. Driver >> >> updates are therefore required to support other boards that also use >> >> the CS89x0. To avoid these driver updates, a better mechanism >> >> (platform driver support) is added to communicate the board dependent >> >> settings to the driver. >> >> >> >> Signed-off-by: Jaccon Bastiaansen >> >> --- >> >>  drivers/net/Kconfig  |   18 +++++++++-- >> >>  drivers/net/Space.c  |    2 +- >> >>  drivers/net/cs89x0.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++-- >> >>  3 files changed, 96 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> >> index 93359fa..17be84f 100644 >> >> --- a/drivers/net/Kconfig >> >> +++ b/drivers/net/Kconfig >> >> @@ -1497,8 +1497,7 @@ config FORCEDETH >> >> >> >>  config CS89x0 >> >>       tristate "CS89x0 support" >> >> -     depends on NET_ETHERNET && (ISA || EISA || MACH_IXDP2351 \ >> >> -             || ARCH_IXDP2X01 || MACH_MX31ADS || MACH_QQ2440) >> >> +     depends on NET_ETHERNET >> >>       ---help--- >> >>         Support for CS89x0 chipset based Ethernet cards. If you have a >> >>         network (Ethernet) card of this type, say Y and read the >> >> @@ -1509,10 +1508,23 @@ config CS89x0 >> >>         To compile this driver as a module, choose M here. The module >> >>         will be called cs89x0. >> >> >> >> +config CS89x0_PLATFORM >> >> +     bool "CS89x0 platform driver support" >> >> +     depends on CS89x0 >> >> +     default n >> > default n is implicit so you don't need (and should not) add it here. >> > >> >> Ok, I will fix this in the next version of the patch. >> >> >> +     help >> >> +       Say Y to compile the cs890x0 driver as a platform driver. This >> >> +       makes this driver suitable for use on certain evaluation boards >> >> +       such as the IMX21ADS. >> >> + >> >> +       If you are unsure, say N. >> >> + >> >>  config CS89x0_NONISA_IRQ >> >>       def_bool y >> >>       depends on CS89x0 != n >> >> -     depends on MACH_IXDP2351 || ARCH_IXDP2X01 || MACH_MX31ADS || MACH_QQ2440 >> >> +     depends on MACH_IXDP2351 || ARCH_IXDP2X01 || MACH_MX31ADS || \ >> >> +                MACH_QQ2440 || CS89x0_PLATFORM >> >> + >> >> >> >>  config TC35815 >> >>       tristate "TOSHIBA TC35815 Ethernet support" >> >> diff --git a/drivers/net/Space.c b/drivers/net/Space.c >> >> index 068c356..3c53ab1 100644 >> >> --- a/drivers/net/Space.c >> >> +++ b/drivers/net/Space.c >> >> @@ -189,7 +189,7 @@ static struct devprobe2 isa_probes[] __initdata = { >> >>  #ifdef CONFIG_SEEQ8005 >> >>       {seeq8005_probe, 0}, >> >>  #endif >> >> -#ifdef CONFIG_CS89x0 >> >> +#if defined(CONFIG_CS89x0) && !defined(CONFIG_CS89x0_PLATFORM) >> >>       {cs89x0_probe, 0}, >> >>  #endif >> >>  #ifdef CONFIG_AT1700 >> >> diff --git a/drivers/net/cs89x0.c b/drivers/net/cs89x0.c >> >> index 537a4b2..604c828 100644 >> >> --- a/drivers/net/cs89x0.c >> >> +++ b/drivers/net/cs89x0.c >> >> @@ -98,6 +98,8 @@ >> >>    Domenico Andreoli : cavokz@gmail.com >> >>                      : QQ2440 platform support >> >> >> >> +  Jaccon Bastiaansen: jaccon.bastiaansen@gmail.com >> >> +                 : added platform driver support >> >>  */ >> >> >> >>  /* Always include 'config.h' first in case the user wants to turn on >> >> @@ -154,7 +156,9 @@ >> >>  #if ALLOW_DMA >> >>  #include >> >>  #endif >> >> - >> >> +#ifdef CONFIG_CS89x0_PLATFORM >> >> +#include >> >> +#endif >> > IMHO better include that unconditionally. >> > >> >> I will also fix this in the next version of this patch. >> >> >>  #include "cs89x0.h" >> >> >> >>  static char version[] __initdata = >> >> @@ -189,6 +193,9 @@ 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}; >> >> +#elif defined(CONFIG_CS89x0_PLATFORM) >> >> +static unsigned int netcard_portlist[] __used __initdata = {0, 0}; >> >> +static unsigned int cs8900_irq_map[] = {0, 0, 0, 0}; >> >>  #else >> >>  static unsigned int netcard_portlist[] __used __initdata = >> >>     { 0x300, 0x320, 0x340, 0x360, 0x200, 0x220, 0x240, 0x260, 0x280, 0x2a0, 0x2c0, 0x2e0, 0}; >> >> @@ -1746,7 +1753,7 @@ static int set_mac_address(struct net_device *dev, void *p) >> >>       return 0; >> >>  } >> >> >> >> -#ifdef MODULE >> >> +#if defined(MODULE) && !defined(CONFIG_CS89x0_PLATFORM) >> >> >> >>  static struct net_device *dev_cs89x0; >> >> >> >> @@ -1900,7 +1907,77 @@ 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; >> >> + >> >> +     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) { >> >> +             printk(KERN_WARNING >> >> +                    DRV_NAME >> >> +                    ": memory and/or interrupt resource missing.\n"); >> > I'd prefer you do: >> > >> >        #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> > >> > at the top of the driver and then just use: >> > >> >        pr_warning("memory and/or interrupt resource missing\m"); >> > >> >> I agree. >> >> >> +             err = -ENOENT; >> >> +             goto out; >> >> +     } >> >> + >> >> +     cs8900_irq_map[0] = irq_res->start; >> >> +     err = cs89x0_probe1(dev, mem_res->start, 0); >> > hmm, better switch the complete driver to be a platform driver and >> > instead of the legacy probing let it create the corresponding device. >> > >> >> What exactly do you mean with "switch the complete driver to be a >> platform driver"? That I should remove all the legacy from the driver >> (which would break the driver for users who don't use it as a platform >> driver) or that I should add a new probe() function free of legacy >> (which would duplicate code of the existing cs89x0_probe1() function)? > Of course you should not break it for legacy users. Just instead of the > legacy stuff just add a platform device with the respective values such > that the platform driver is used in all cases. > > For extra points move the adding of the device to platform code. > The platform device you mention is added by another patch that I also sent on the the 7th of September (not directly to you, but to Sascha and kernel@pengutronix.de). This patch was called: [PATCH] Add CS890x0 ethernet controller support to the i.MX21ADS In this patch I add a platform device to the i.MX21ADS evaluation board. All the required settings (memory address and IRQ) that the CS890x platform driver needs to run on the i.MX21ADS board are set in the i.MX21ADS specific code. You find the patch below. It this the approach you mean? --- 1.7.1 > Best regards > Uwe > > -- > Pengutronix e.K.                           | Uwe Kleine-König            | > Industrial Linux Solutions                 | http://www.pengutronix.de/  | > Regards, Jaccon =================================== [PATCH] Add CS890x0 ethernet controller support to the i.MX21ADS Signed-off-by: Jaccon Bastiaansen --- arch/arm/mach-imx/mach-mx21ads.c | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-) diff --git a/arch/arm/mach-imx/mach-mx21ads.c b/arch/arm/mach-imx/mach-mx21ads.c index 74ac889..62b4717 100644 --- a/arch/arm/mach-imx/mach-mx21ads.c +++ b/arch/arm/mach-imx/mach-mx21ads.c @@ -159,6 +159,26 @@ static struct platform_device mx21ads_nor_mtd_device = { .resource = &mx21ads_flash_resource, }; +static struct resource mx21ads_cs8900_resources[] = { + { + .start = (u32)MX21ADS_CS8900A_IOBASE_REG, + .end = (u32)MX21ADS_CS8900A_IOBASE_REG + 0x200000 - 1, + .flags = IORESOURCE_MEM, + }, + { + .start = MX21ADS_CS8900A_IRQ, + .end = MX21ADS_CS8900A_IRQ, + .flags = IORESOURCE_IRQ, + }, +}; + +static struct platform_device mx21ads_cs8900_device = { + .name = "cs89x0", + .id = 0, + .num_resources = ARRAY_SIZE(mx21ads_cs8900_resources), + .resource = mx21ads_cs8900_resources, +}; + static const struct imxuart_platform_data uart_pdata_rts __initconst = { .flags = IMXUART_HAVE_RTSCTS, }; @@ -275,6 +295,7 @@ static void __init mx21ads_map_io(void) static struct platform_device *platform_devices[] __initdata = { &mx21ads_nor_mtd_device, + &mx21ads_cs8900_device }; static void __init mx21ads_board_init(void)