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

[Anatolij Gustschin]

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 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.

...
> +	/* 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