[U-Boot] [PATCH v3 2/4] ARM: EXYNOS: add data structure for EXYNOS display driver

[Donghwa Lee]

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 at samsung.com> wrote:
> 
>> add vidinfo data structure for EXYNOS display driver
>>
>> Signed-off-by: Donghwa Lee <dh09.lee at samsung.com>
>> Signed-off-by: Inki Dae <inki.dae at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at 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