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
