Re: [PATCH] ARM: OMAP3: Add support for the IGEP v2 board (rev B)

[Nishanth Menon <nm@ti.com>]

Hi,
Thanks for the patch.. a few minor comments follow from a read through..

Enric Balletbò i Serra had written, on 10/09/2009 10:59 AM, the following:
>   This patch adds minimal IGEP v2 support.

could you add more details on where do we get more data on this platform?

> 
> Signed-off-by: Enric Balletbo i Serra <eballetbo@iseebcn.com>
> ---
>  arch/arm/configs/igep0020_defconfig  | 1443 ++++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/Kconfig          |    4 +
>  arch/arm/mach-omap2/Makefile         |    2 +
>  arch/arm/mach-omap2/board-igep0020.c |  239 ++++++
>  4 files changed, 1688 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/configs/igep0020_defconfig
>  create mode 100644 arch/arm/mach-omap2/board-igep0020.c
[...]
> 
> --- /dev/null
> +++ b/arch/arm/mach-omap2/board-igep0020.c
> @@ -0,0 +1,239 @@
> +/*
[..]

> +static inline void __init igep2_init_smsc911x(void)
> +{
> +       unsigned long cs_mem_base;
> +
> +       if (gpmc_cs_request(IGEP2_SMSC911X_CS, SZ_16M, &cs_mem_base) < 0) {
> +               printk(KERN_ERR "Failed request for GPMC mem for smsc911x\n");
> +               return;
> +       }
> +
> +       igep2_smsc911x_resources[0].start = cs_mem_base + 0x0;
> +       igep2_smsc911x_resources[0].end   = cs_mem_base + 0xff;
> +
> +       if ((gpio_request(IGEP2_SMSC911X_GPIO, "SMSC911X IRQ") == 0) &&
> +           (gpio_direction_input(IGEP2_SMSC911X_GPIO) == 0)) {
> +               gpio_export(IGEP2_SMSC911X_GPIO, 0);
> +       } else {
could you run scripts/checkpatch --strict ../patchName after generating 
the patch with git format-patch -s -M -o .. -1 ? one liners dont usually 
need a {} also there are few warning inducing code below..
> +               printk(KERN_ERR "could not obtain gpio for SMSC911X IRQ\n");
probably dumb question: should'nt we be moving to pr_err and family 
instead of printks?

further, if this does not work.. are'nt you supposed to release the cs? 
gpmc_cs_free perhaps?

> +               return;
> +       }
> +
> +       igep2_smsc911x_resources[1].start = OMAP_GPIO_IRQ(IGEP2_SMSC911X_GPIO);
> +       igep2_smsc911x_resources[1].end   = 0;
> +
> +       platform_device_register(&igep2_smsc911x_device);
> +}
> +
> +#else
> +
> +static inline void __init igep2_init_smsc911x(void) { }
> +
> +#endif
> +
[..]
> +               .flags          = I2C_CLIENT_WAKE,
> +               .irq            = INT_34XX_SYS_NIRQ,
> +               .platform_data  = &igep2_twldata,
> +       },
> +};
> +
> +static int __init igep2_i2c_init(void)
> +{
> +       omap_register_i2c_bus(1, 2600, igep2_i2c_boardinfo,
> +                       ARRAY_SIZE(igep2_i2c_boardinfo));
you may want to step down to 2400 
http://marc.info/?l=linux-omap&m=125510664919890&w=2 if you are using 
26Mhz.. if you are using twl5030..
> +       omap_register_i2c_bus(3, 400, NULL, 0);
> +       return 0;
> +}
> +
> +static void __init igep2_init(void)
> +{
> +       igep2_i2c_init();
> +       omap_serial_init();
> +       usb_musb_init();
> +
> +       igep2_init_smsc911x();
> +
> +       /* GPIO userspace leds */
> +       if ((gpio_request(IGEP2_GPIO_LED_0_RED, "GPIO_LED_0_RED") == 0) &&
> (gpio_direction_output(IGEP2_GPIO_LED_0_RED, 1) == 0)) {
> +               gpio_export(IGEP2_GPIO_LED_0_RED, 1);
> +       } else {
> +               printk(KERN_ERR "could not obtain gpio for " "GPIO_LED_0_RED\n");
do you really want to flag a an error for a led glow?
> +       }
> +       if ((gpio_request(IGEP2_GPIO_LED_0_GREEN, "GPIO_LED_0_GREEN") == 0)
> && (gpio_direction_output(IGEP2_GPIO_LED_0_GREEN, 1) == 0)) {
> +               gpio_export(IGEP2_GPIO_LED_0_GREEN, 1);
> +       } else {
> +               printk(KERN_ERR "could not obtain gpio for " "GPIO_LED_0_GREEN\n");
> +       }
> +       if ((gpio_request(IGEP2_GPIO_LED_1_RED, "GPIO_LED_1_RED") == 0) &&
> (gpio_direction_output(IGEP2_GPIO_LED_1_RED, 1) == 0)) {
here is a an long line?
[...]
-- 
Regards,
Nishanth Menon
--
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