On Tue, Aug 11, 2009 at 19:08:24, Nori, Sekhar wrote: > Sudhakar, > > On Wed, Aug 12, 2009 at 03:28:02, Rajashekhara, Sudhakar wrote: > > This patch adds platform support for the graphic display > > (Sharp LK043T1DG01) found on DA850/OMAP-L138 based EVM. > > > > Signed-off-by: Sudhakar Rajashekhara <sudhakar....@ti.com> > > --- > > arch/arm/mach-davinci/board-da850-evm.c | 71 > > ++++++++++++++++++++++++++++ > > arch/arm/mach-davinci/da850.c | 42 ++++++++++++++++ > > arch/arm/mach-davinci/devices-da8xx.c | 59 +++++++++++++++++++++++ > > arch/arm/mach-davinci/include/mach/da8xx.h | 3 + > > arch/arm/mach-davinci/include/mach/mux.h | 26 ++++++++++ > > 5 files changed, 201 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mach-davinci/board-da850-evm.c > > b/arch/arm/mach-davinci/board-da850-evm.c > > index d989346..45575e5 100644 > > --- a/arch/arm/mach-davinci/board-da850-evm.c > > +++ b/arch/arm/mach-davinci/board-da850-evm.c > > @@ -17,6 +17,8 @@ > > #include <linux/console.h> > > #include <linux/i2c.h> > > #include <linux/i2c/at24.h> > > +#include <linux/gpio.h> > > +#include <linux/delay.h> > > > > #include <asm/mach-types.h> > > #include <asm/mach/arch.h> > > @@ -25,6 +27,7 @@ > > #include <mach/irqs.h> > > #include <mach/cp_intc.h> > > #include <mach/da8xx.h> > > +#include <mach/psc.h> > > > > #define DA850_EVM_PHY_MASK 0x1 > > #define DA850_EVM_MDIO_FREQUENCY 2200000 /* PHY bus frequency */ > > @@ -38,6 +41,59 @@ static struct davinci_uart_config da850_evm_uart_config > > __initdata = { > > .enabled_uarts = 0x7, > > }; > > > > +int da850_lcd_hw_init(void) > > This should be static? >
Yes, this function can be static. > > +{ > > + /* GPIO 2[15] is used for LCD back light - 16 * 2 + 15 = 47 */ > > + int bl_gpio_num = 47; > > + > > + /* GPIO 8[10] is used for LCD power - 16 * 8 + 10 = 138 */ > > + int pwr_gpio_num = 138; > > These could probably be #defines instead of > variables. > I'll define the GPIO pin numbers as macros. > > + int status; > > + > > + status = gpio_request(bl_gpio_num, "lcd bl\n"); > > + if (status < 0) > > + return status; > > + > > + status = gpio_request(pwr_gpio_num, "lcd pwr\n"); > > + if (status < 0) > > + return status; > > We are missing gpio_free of back light GPIO > here. > I'll take care of this. > > + > > + gpio_direction_output(bl_gpio_num, 0); > > + gpio_direction_output(pwr_gpio_num, 0); > > + > > + /* disable lcd backlight */ > > + gpio_set_value(bl_gpio_num, 0); > > + > > + /* disable lcd power */ > > + gpio_set_value(pwr_gpio_num, 0); > > + > > + /* wait for sometime */ > > + mdelay(3); > > Can you explain the reasoning behind > the delay in the comment? > I found out during testing that these delays are not required. I'll remove them in my next version. > > + > > + /* disable lcdc */ > > + davinci_psc_config(DAVINCI_GPSC_ARMDOMAIN, 1, DA8XX_LPSC1_LCDC, 0); > > Hmm, why is this needed? Hopefully the LCD clock has > not been enabled by this time. > Same as previous. > > + > > + /* wait for sometime */ > > + mdelay(1); > > Some explanation required here too. > Same here.. [....] > > +static struct lcd_ctrl_config lcd_cfg = { > > + &disp_panel, /* p_disp_panel */ > > + .ac_bias = 255, /* ac bias */ > > + .ac_bias_intrpt = 0, /* ac bias intrpt */ > > + .dma_burst_sz = 16, /* dma_burst_sz */ > > + .bpp = 16, /* bpp */ > > + .fdd = 255, /* fdd */ > > + .tft_alt_mode = 0, /* tft_alt_mode */ > > + .stn_565_mode = 0, /* stn_565_mode */ > > + .mono_8bit_mode = 0, /* mono_8bit_mode */ > > + .invert_line_clock = 1, /* invert_line_clock */ > > + .invert_frm_clock = 1, /* invert_frm_clock */ > > + .sync_edge = 0, /* sync_edge */ > > + .sync_ctrl = 1, /* sync_ctrl */ > > + .raster_order = 0, /* raster_order */ > > +}; > > Comments at end of each line are not required. > I'll remove these comments. Thanks for your review. I'll re-submit the patch after addressing the comments. Regards, Sudhakar _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source