RE: [PATCH] Add support for OMAP3Stalker boards

["Jason" <lzg@ema-tech.com>]

Thanks for replaying, I did have a long wait.

A question about board version. Is it good to do a hardware check to find
out hardware version?
 Like function static void __init omap3_evm_get_revision(void) in
board-omap3evm.c


Best regards,
Jason

> -----Original Message-----
> From: Tony Lindgren [mailto:tony@atomide.com]
> Sent: 2010年5月5日 7:00
> To: Jason
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] Add support for OMAP3Stalker boards
> 
> Hi,
> 
> Sorry for the delay in replying. Few comments, see below.
> 
> * Jason <lzg@ema-tech.com> [100303 06:16]:
> > This patche add omap3 board support for the EMA-Tech StalkerBoards. Base
> on
> > TI's EVM.
> 
> <snip>
> 
> > --- a/arch/arm/mach-omap2/Kconfig
> > +++ b/arch/arm/mach-omap2/Kconfig
> > @@ -82,6 +82,26 @@ config MACH_OMAP3517EVM
> >  	depends on ARCH_OMAP3
> >  	select OMAP_PACKAGE_CBB
> >
> > +config MACH_SBC3530
> > +	bool "OMAP3 SBC STALKER board"
> > +	depends on ARCH_OMAP3
> > +
> > +choice
> > +	prompt STALKER_BOARD_TYPE
> > +	depends on MACH_SBC3530
> > +	default STALKER_EVM
> > +
> > +config STALKER_BOARD_TYPE_EVM
> > +	bool "Stalker EVM board"
> > +	help
> > +	  Select this option if you have a Stalker EVM board
> > +
> > +config STALKER_BOARD_TYPE_LK_S
> > +	bool "Stalker LK-S board"
> > +	help
> > +	  Select this option if you have a Stalker LK-S board
> > +endchoice
> > +
> >  config MACH_OMAP3_PANDORA
> >  	bool "OMAP3 Pandora"
> >  	depends on ARCH_OMAP3
> 
> Things like this are best done by looking at the ATAG_REVISION passed by
> the bootloader. Then you can dynamically initialize the platform data
> based on the revision.
> 
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/board-omap3stalker.c
> > @@ -0,0 +1,922 @@
> > +/*
> > + * linux/arch/arm/mach-omap2/board-omap3evm.c
> > + *
> > + * Copyright (C) 2008 Guangzhou EMA-Tech
> > + *
> > + * Modified from mach-omap2/board-omap3evm.c
> > + *
> > + * Initial code: Syed Mohammed Khasim
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio.h>
> > +#include <linux/input.h>
> > +#include <linux/input/matrix_keypad.h>
> > +#include <linux/leds.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/ads7846.h>
> > +#include <linux/i2c/twl.h>
> > +#include <linux/usb/otg.h>
> > +#if defined(CONFIG_STALKER_BOARD_TYPE_EVM)
> > +#include <linux/dm9000.h>
> > +#elif defined(CONFIG_STALKER_BOARD_TYPE_LK_S)
> 
> We should not ifdef includes, it should be OK to include them
automatically.
> 
> > +#include <linux/smsc911x.h>
> > +#include <linux/gpio_keys.h>
> > +#include <linux/i2c/at24.h>
> > +#endif
> > +
> > +#include <linux/regulator/machine.h>
> > +
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <linux/mtd/nand.h>
> > +
> > +#include <mach/hardware.h>
> > +#include <asm/mach-types.h>
> > +#include <asm/mach/arch.h>
> > +#include <asm/mach/map.h>
> > +#include <asm/mach/flash.h>
> > +
> > +#include <plat/board.h>
> > +#include <plat/mux.h>
> > +#include <plat/gpmc.h>
> > +#include <plat/nand.h>
> > +#include <plat/usb.h>
> > +#include <plat/common.h>
> > +#include <plat/control.h>
> > +#include <plat/mcspi.h>
> > +#include <plat/clock.h>
> > +#include <plat/omap-pm.h>
> > +#include <plat/display.h>
> > +#include <plat/timer-gp.h>
> > +
> > +#include "mux.h"
> > +#include "sdram-micron-mt46h32m32lf-6.h"
> > +#include "hsmmc.h"
> > +#include "pm.h"
> 
> That's a lot of include files, are they all needed?
> 
> > +static struct mtd_partition omap3stalker_nand_partitions[] = {
> > +	/* All the partition sizes are listed in terms of NAND block size */
> > +	{
> > +	 .name = "X-Loader",
> > +	 .offset = 0,
> > +	 .size = 4 * (SZ_128K),
> > +	 .mask_flags = MTD_WRITEABLE,
> > +	 },
> > +	{
> > +	 .name = "U-Boot",
> > +	 .offset = MTDPART_OFS_APPEND,
> > +	 .size = 15 * (SZ_128K),
> > +	 .mask_flags = MTD_WRITEABLE,
> > +	 },
> > +	{
> > +	 .name = "U-Boot Env",
> > +	 .offset = MTDPART_OFS_APPEND,
> > +	 .size = 1 * (SZ_128K)},
> > +	{
> > +	 .name = "Kernel",
> > +	 .offset = MTDPART_OFS_APPEND,
> > +	 .size = 32 * (SZ_128K)},
> > +	{
> > +	 .name = "File System",
> > +	 .size = MTDPART_SIZ_FULL,
> > +	 .offset = MTDPART_OFS_APPEND,
> > +	 },
> > +};
> 
> Please use tabs instead of spaces.
> 
> > +void __init omap3stalker_flash_init(void)
> > +{
> > +	u8 cs = 0;
> > +	u8 nandcs = GPMC_CS_NUM + 1;
> > +	u32 gpmc_base_add = OMAP34XX_GPMC_VIRT;
> 
> Please leave out the NAND stuff for now, we need to first fix the
> NAND driver before adding more of the OMAP34XX_GPMC_VIRT..
> 
> > +static struct resource omap3stalker_smsc911x_resources[] = {
> > +	[0] = {
> > +	       .start = OMAP3STALKER_ETHR_START,
> > +	       .end =
> > +	       (OMAP3STALKER_ETHR_START + OMAP3STALKER_ETHR_SIZE -
> 1),
> > +	       .flags = IORESOURCE_MEM,
> > +	       },
> > +	[1] = {
> > +	       .start = OMAP_GPIO_IRQ(OMAP3STALKER_ETHR_GPIO_IRQ),
> > +	       .end = OMAP_GPIO_IRQ(OMAP3STALKER_ETHR_GPIO_IRQ),
> > +	       .flags = (IORESOURCE_IRQ | IRQF_TRIGGER_LOW),
> > +	       },
> > +};
> 
> Please use tabs here too instead of spaces.
> 
> > +static struct omap2_hsmmc_info mmc[] = {
> > +	{
> > +	 .mmc = 1,
> > +	 .wires = 4,
> > +	 .gpio_cd = -EINVAL,
> > +	 .gpio_wp = 23,
> > +	 },
> > +	{}			/* Terminator */
> > +};
> > +
> > +#if defined(CONFIG_STALKER_BOARD_TYPE_LK_S)
> > +static struct gpio_keys_button gpio_buttons[] = {
> > +	{
> > +	 .code = BTN_EXTRA,
> > +	 .gpio = 18,
> > +	 .desc = "user",
> > +	 .wakeup = 1,
> > +	 },
> > +};
> 
> Here too.
> 
> > +	.irq_base = TWL4030_IRQ_BASE,
> > +	.irq_end = TWL4030_IRQ_END,
> > +
> > +	/* platform_data for children goes here */
> > +	.keypad = &omap3stalker_kp_data,
> > +	.madc = &omap3stalker_madc_data,
> > +	.usb = &omap3stalker_usb_data,
> > +	.gpio = &omap3stalker_gpio_data,
> > +	.codec = &omap3stalker_codec_data,
> > +	.vdac = &omap3_stalker_vdac,
> > +	.vpll2 = &omap3_stalker_vpll2,
> > +};
> 
> Please align with tabs:
> 
> 	.keypad		= &omap3stalker_kp_data,
> 	.madc		= &omap3stalker_madc_data,
> 
> > +static struct i2c_board_info __initdata omap3stalker_i2c_boardinfo[] =
{
> > +	{
> > +	 I2C_BOARD_INFO("twl4030", 0x48),
> > +	 .flags = I2C_CLIENT_WAKE,
> > +	 .irq = INT_34XX_SYS_NIRQ,
> > +	 .platform_data = &omap3stalker_twldata,
> > +	 },
> > +};
> 
> Tabify
> 
> > +#ifdef CONFIG_STALKER_BOARD_TYPE_LK_S
> > +static struct at24_platform_data fram_info = {
> > +	.byte_len = (64 * 1024) / 8,
> > +	.page_size = 8192,
> > +	.flags = AT24_FLAG_ADDR16 | AT24_FLAG_IRUGO,
> > +};
> > +
> > +static struct i2c_board_info __initdata omap3stalker_i2c_boardinfo3[] =
{
> > +	{
> > +	 I2C_BOARD_INFO("24c64", 0x50),
> > +	 .flags = I2C_CLIENT_WAKE,
> > +	 .platform_data = &fram_info,
> > +	 },
> > +};
> > +#endif
> 
> Tabify
> 
> Anyways, mostly minor stuff except for the preference for dynamic
detection
> of the board.
> 
> Regards,
> 
> Tony

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html