On Mon, Sep 01, 2008 at 02:57:56PM -0700, sakoman@xxxxxxxxx wrote:
> + .size = 15 * NAND_BLOCK_SIZE,
Unnecessary additional spacing after '='.
> +static struct omap_onenand_platform_data overo_onenand_data = {
> + .parts = overo_onenand_partitions,
> + .nr_parts = ARRAY_SIZE(overo_onenand_partitions),
> + .dma_channel = -1, /* disable DMA in OMAP OneNAND driver */
Tabbing vs spacing indentation styles, please settle on one way to
do it.
> +void __init overo_flash_init(void)
Should be static?
> +{
> + u8 cs = 0;
> + u8 onenandcs = GPMC_CS_NUM + 1;
Do these two tabs after 'u8' help? Everywhere else you use a single space.
> +
> + while (cs < GPMC_CS_NUM) {
> + u32 ret = 0;
> + ret = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG7);
> +
> + if ((ret & 0x3F) == (ONENAND_MAP >> 24))
> + onenandcs = cs;
> + cs++;
> + }
> + if (onenandcs > GPMC_CS_NUM) {
> + printk(KERN_ERR "OneNAND: Unable to find configuration"
> + " in GPMC\n ");
Unnecessary additional space after '\n' (which will result in the next
line output by the kernel having its level tag ignored.)
> + if ((gpio_request(W2W_NRESET, "W2W_NRESET") == 0) &&
> + (gpio_direction_output(W2W_NRESET, 1) == 0)) {
> + gpio_export(W2W_NRESET, 0);
> + gpio_direction_output(W2W_NRESET, 0);
Since you've already just set W2W_NRESET as an output, shouldn't this
be:
gpio_set_value(W2W_NRESET, 0);
instead?
> + udelay(10);
> + gpio_direction_output(W2W_NRESET, 1);
gpio_set_value(W2W_NRESET, 1);
? Ditto for the rest like this.
> + } else
> + printk(KERN_ERR "could not obtain gpio for W2W_NRESET\n");
> +
> + if ((gpio_request(BT_NRESET, "BT_NRESET") == 0) &&
> + (gpio_direction_output(BT_NRESET, 1) == 0)) {
> + gpio_export(BT_NRESET, 0);
> + gpio_direction_output(BT_NRESET, 0);
> + mdelay(6);
> + gpio_direction_output(BT_NRESET, 1);
> + } else
> + printk(KERN_ERR "could not obtain gpio for BT_NRESET\n");
> +
> + if ((gpio_request(USBH_CPEN, "USBH_CPEN") == 0) &&
> + (gpio_direction_output(USBH_CPEN, 1) == 0))
> + gpio_export(USBH_CPEN, 0);
> + else
> + printk(KERN_ERR "could not obtain gpio for USBH_CPEN\n");
> +
> + if ((gpio_request(USBH_NRESET, "USBH_NRESET") == 0) &&
> + (gpio_direction_output(USBH_NRESET, 1) == 0))
> + gpio_export(USBH_NRESET, 0);
> + else
> + printk(KERN_ERR "could not obtain gpio for USBH_NRESET\n");
> +}
> +
> +arch_initcall(overo_i2c_init);
Is there a reason why overo_i2c_init can't be called from your
.init_machine function?
> diff --git a/arch/arm/plat-omap/include/mach/hardware.h b/arch/arm/plat-omap/include/mach/hardware.h
> index 438de80..f859c00 100644
> --- a/arch/arm/plat-omap/include/mach/hardware.h
> +++ b/arch/arm/plat-omap/include/mach/hardware.h
> @@ -374,6 +374,10 @@
> #include "board-sx1.h"
> #endif
>
> +#ifdef CONFIG_MACH_OVERO
> +#include "board-overo.h"
> +#endif
> +
I do hate it when machine classes like to add all their platform specific
header includes into another include, meaning that whenever you touch
_any_ platform specific kernel, everything rebuilds whether it's affected
or not.
With the driver model, it just isn't necessary anymore; I would much
rather prefer to see "board-overo.h" included by files which need it,
which to me looks like being board-overo.c.
> +static int overo_panel_init(struct lcd_panel *panel,
> + struct omapfb_device *fbdev)
> +{
> + if ((gpio_request(LCD_ENABLE, "LCD_ENABLE") == 0) &&
> + (gpio_direction_output(LCD_ENABLE, 1) == 0))
> + gpio_export(LCD_ENABLE, 0);
> + else
> + printk(KERN_ERR "could not obtain gpio for LCD_ENABLE\n");
> +
> + return 0;
> +}
> +
> +static void overo_panel_cleanup(struct lcd_panel *panel)
> +{
No freeing of LCD_ENABLE?
> +}
> +
> +static int overo_panel_enable(struct lcd_panel *panel)
> +{
> + gpio_direction_output(LCD_ENABLE, 1);
gpio_set_value() ?
> + return 0;
> +}
> +
> +static void overo_panel_disable(struct lcd_panel *panel)
> +{
> + gpio_direction_output(LCD_ENABLE, 0);
gpio_set_value() ?
> +static int overo_panel_probe(struct platform_device *pdev)
> +{
> + omapfb_register_panel(&overo_panel);
> + return 0;
> +}
> +
> +static int overo_panel_remove(struct platform_device *pdev)
> +{
No possible unregistering of the panel?
> +static int overo_panel_suspend(struct platform_device *pdev,
> + pm_message_t mesg)
> +{
> + return 0;
> +}
> +
> +static int overo_panel_resume(struct platform_device *pdev)
> +{
> + return 0;
> +}
No suspend/resume? So these empty functions aren't required.
> +
> +struct platform_driver overo_panel_driver = {
Should be static.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
