| Message ID | 4F7965BC.9040609@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
Hi! Sorry for not looking at this earlier. The patch looks pretty good, but there are some style issues, please see some comments below. Also the patch subject should reflect on which subsystem the patch is doing the changes, so I would suggest using the patch subject like "LCD: add data structure for EXYNOS display driver" On Mon, 02 Apr 2012 17:39:24 +0900 Donghwa Lee <dh09.lee@samsung.com> wrote: > add vidinfo data structure for EXYNOS display driver > > Signed-off-by: Donghwa Lee <dh09.lee@samsung.com> > Signed-off-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > include/lcd.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 63 insertions(+), 0 deletions(-) > > diff --git a/include/lcd.h b/include/lcd.h > index d95feeb..651ff42 100644 > --- a/include/lcd.h > +++ b/include/lcd.h > @@ -56,6 +56,11 @@ extern void lcd_initcolregs (void); > /* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ > extern struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp); > > +enum { > + FIMD_RGB_INTERFACE = 1, > + FIMD_CPU_INTERFACE = 2, > +}; This is EXYNOS specific so please move it under #elif defined(CONFIG_EXYNOS_FB) line. ... > + /* LCD configuration register */ > + u_char vl_freq; /* Frequency */ > + u_char vl_clkp; /* Clock polarity */ > + u_char vl_oep; /* Output Enable polarity */ > + u_char vl_hsp; /* Horizontal Sync polarity */ > + u_char vl_vsp; /* Vertical Sync polarity */ > + u_char vl_dp; /* Data polarity */ > + u_char vl_bpix; /* Bits per pixel, 0 = 1, 1 = 2, 2 = 4, 3 = 8, 4 = 16 */ The above line is too long, we have 80 chars/line limit. I know there are other bad examples in this file, but please change to u_char vl_bpix; /* Bits per pixel, bpp = pow(2, vl_bpix) */ ... > @@ -213,6 +275,7 @@ void lcd_puts (const char *s); > void lcd_printf (const char *fmt, ...); > void lcd_clear(void); > int lcd_display_bitmap(ulong bmp_image, int x, int y); > +void init_panel_info (vidinfo_t *vid); Please remove space between function name and open parenthesis '('. There are other examples with spaces between function names and '(' in this file, I'll fix them later. Btw, where is init_panel_info() defined? I only see that it is used in drivers/video/exynos_fb.c, but do not see the function code. So the driver cannot be build at all. How did you test it? Also try to check your patches with 'tools/checkpatch.pl'. It will warn you about such style issues. Some of the warnings will be Linux style specific, these can be disabled using "--ignore=" option, e.g. using --ignore=NEW_TYPEDEFS would be appropriate for checking this patch. Thanks, Anatolij
Hi, Thank you for your comments. On Wen, 04 Apr 2012 21:53, Anatolij Gustschin wrote: > Hi! > > Sorry for not looking at this earlier. The patch looks pretty good, but > there are some style issues, please see some comments below. > > Also the patch subject should reflect on which subsystem the patch is > doing the changes, so I would suggest using the patch subject like > "LCD: add data structure for EXYNOS display driver" > Ok, I will change the title. > On Mon, 02 Apr 2012 17:39:24 +0900 > Donghwa Lee <dh09.lee@samsung.com> wrote: > >> add vidinfo data structure for EXYNOS display driver >> >> Signed-off-by: Donghwa Lee <dh09.lee@samsung.com> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> include/lcd.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 63 insertions(+), 0 deletions(-) >> >> diff --git a/include/lcd.h b/include/lcd.h >> index d95feeb..651ff42 100644 >> --- a/include/lcd.h >> +++ b/include/lcd.h >> @@ -56,6 +56,11 @@ extern void lcd_initcolregs (void); >> /* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ >> extern struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp); >> >> +enum { >> + FIMD_RGB_INTERFACE = 1, >> + FIMD_CPU_INTERFACE = 2, >> +}; > > This is EXYNOS specific so please move it under > #elif defined(CONFIG_EXYNOS_FB) line. > > ... Ok, I will move it. >> + /* LCD configuration register */ >> + u_char vl_freq; /* Frequency */ >> + u_char vl_clkp; /* Clock polarity */ >> + u_char vl_oep; /* Output Enable polarity */ >> + u_char vl_hsp; /* Horizontal Sync polarity */ >> + u_char vl_vsp; /* Vertical Sync polarity */ >> + u_char vl_dp; /* Data polarity */ >> + u_char vl_bpix; /* Bits per pixel, 0 = 1, 1 = 2, 2 = 4, 3 = 8, 4 = 16 */ > > The above line is too long, we have 80 chars/line limit. I know > there are other bad examples in this file, but please change to > > u_char vl_bpix; /* Bits per pixel, bpp = pow(2, vl_bpix) */ > > ... >> @@ -213,6 +275,7 @@ void lcd_puts (const char *s); >> void lcd_printf (const char *fmt, ...); >> void lcd_clear(void); >> int lcd_display_bitmap(ulong bmp_image, int x, int y); >> +void init_panel_info (vidinfo_t *vid); > > Please remove space between function name and open parenthesis '('. > > There are other examples with spaces between function names and '(' > in this file, I'll fix them later. > > Btw, where is init_panel_info() defined? I only see that it is used > in drivers/video/exynos_fb.c, but do not see the function code. So the > driver cannot be build at all. How did you test it? > > Also try to check your patches with 'tools/checkpatch.pl'. It will > warn you about such style issues. Some of the warnings will be Linux > style specific, these can be disabled using "--ignore=" option, e.g. > using --ignore=NEW_TYPEDEFS would be appropriate for checking this > patch. > This function is used in TRATS board file to get vidinfo data that sets up in board file, Although I had already check by using checkpatch.pl, maybe some file was missed. I will send patch next version. Thank you, Donghwa Lee
diff --git a/include/lcd.h b/include/lcd.h index d95feeb..651ff42 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -56,6 +56,11 @@ extern void lcd_initcolregs (void); /* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ extern struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp); +enum { + FIMD_RGB_INTERFACE = 1, + FIMD_CPU_INTERFACE = 2, +}; + #if defined CONFIG_MPC823 /* * LCD controller stucture for MPC823 CPU @@ -183,6 +188,63 @@ typedef struct vidinfo { u_long mmio; /* Memory mapped registers */ } vidinfo_t; +#elif defined(CONFIG_EXYNOS_FB) + +typedef struct vidinfo { + ushort vl_col; /* Number of columns (i.e. 640) */ + ushort vl_row; /* Number of rows (i.e. 480) */ + ushort vl_width; /* Width of display area in millimeters */ + ushort vl_height; /* Height of display area in millimeters */ + + /* LCD configuration register */ + u_char vl_freq; /* Frequency */ + u_char vl_clkp; /* Clock polarity */ + u_char vl_oep; /* Output Enable polarity */ + u_char vl_hsp; /* Horizontal Sync polarity */ + u_char vl_vsp; /* Vertical Sync polarity */ + u_char vl_dp; /* Data polarity */ + u_char vl_bpix; /* Bits per pixel, 0 = 1, 1 = 2, 2 = 4, 3 = 8, 4 = 16 */ + + /* Horizontal control register. Timing from data sheet */ + u_char vl_hspw; /* Horz sync pulse width */ + u_char vl_hfpd; /* Wait before of line */ + u_char vl_hbpd; /* Wait end of line */ + + /* Vertical control register. */ + u_char vl_vspw; /* Vertical sync pulse width */ + u_char vl_vfpd; /* Wait before of frame */ + u_char vl_vbpd; /* Wait end of frame */ + u_char vl_cmd_allow_len; /* Wait end of frame */ + + void (*cfg_gpio)(void); + void (*backlight_on)(unsigned int onoff); + void (*reset_lcd)(void); + void (*lcd_power_on)(void); + void (*cfg_ldo)(void); + void (*enable_ldo)(unsigned int onoff); + void (*mipi_power)(void); + void (*backlight_reset)(void); + + unsigned int win_id; + unsigned int init_delay; + unsigned int power_on_delay; + unsigned int reset_delay; + unsigned int interface_mode; + unsigned int cs_setup; + unsigned int wr_setup; + unsigned int wr_act; + unsigned int wr_hold; + + /* parent clock name(MPLL, EPLL or VPLL) */ + unsigned int pclk_name; + /* ratio value for source clock from parent clock. */ + unsigned int sclk_div; + + unsigned int lcd_rotate; + unsigned int dual_lcd_enabled; + +} vidinfo_t; + #else typedef struct vidinfo { @@ -213,6 +275,7 @@ void lcd_puts (const char *s); void lcd_printf (const char *fmt, ...); void lcd_clear(void); int lcd_display_bitmap(ulong bmp_image, int x, int y); +void init_panel_info (vidinfo_t *vid); /* Allow boards to customize the information displayed */ void lcd_show_board_info(void);