RE: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver

[Jingoo Han <jg1.han@samsung.com>]

> -----Original Message-----
> From: linux-samsung-soc-owner@vger.kernel.org [mailto:linux-samsung-soc-owner@vger.kernel.org] On Behalf
> Of Darius Augulis
> Sent: Friday, March 16, 2012 7:19 PM
> To: Jingoo Han
> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org; patches@linaro.org;
> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
> Korsgaard; Maurus Cuelenaere; Tushar Behera
> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
> 
> On Fri, Mar 16, 2012 at 11:48 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> >> > It's just bug.
> >> >
> >> > struct s3c_fb_pd_win should be used for windows of FIMD IP.
> >> > It should not be used for selecting multi-LCDs.
> >>
> >> why not? s3c_fb_pd_win contains timing values which depend directly on
> >> LCD size and resolution.
> >> Please look into the code again - mini6410 doesn't use platform data
> >> so select different LCD sizes.
> >> It has only single window in platform data - exactly what you want.
> >> Now tell my, why you are arguing, that this platform data cannot be
> >> rewritten dynamically by board setup routines at kernel startup time?
> >> Many ARM architectures are doing the same - takes kernel command line
> >> argument to select LCD and passes necessary data for its fb drivers
> >> via platform data structures. It not a bug, it's correct approach to
> >> support different sizes of LCD display for the same board.
> >
> >
> > Hey, it is not my point.
> > To take kernel command line is not problem to support multiple LCDs.
> >
> > My point is that s3c_fb_pd_win should not include variable LCD option.
> > So, if you want to support multiple LCDs, you should use another variable,
> > instead of using s3c_fb_pd_win[0] and s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
> 
> But I use s3c_fb_pd_win only as container to hold timing values in
> board setup code.
> s3c-fb platform data has only single window and does not perform any
> LCD selection.
> What is wrong here? You suggest to declare some custom structure to
> hold these timing values? What's the point?
> 
> >
> > You use s3c_fb_pd_win[0], s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
> > However, s3c_fb_pd_win[0], s3c_fb_pd_win[1] means the windows of FIMD IP.
> 
> There is no multiple windows in platform data! driver sees only single
> window. And this wont change in any way if you drop LCD selection from
> board setup code.
> Driver will see the same single window. So - your are arguing, that
> board setup code sets platform data in bad way. I don't accept your
> opinion, because tens of boards are doing the same and nobody call it
> bug.

Because there are not multiple windows and driver sees only single window as you mentiond, 
Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and 
'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1 of FIMD IP.

Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it means that
2 windows of FIMD IP are used in single LCD.

If you want to support multiple LCDs, you should use struct s3c_fb_platdata[0], struct s3c_fb_platdata[1],
because 'struct s3c_fb_platdata' means LCD panel.

If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you should do, at least, as below.

static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
	{
		.win_mode	= {	/* 4.3" 480x272 */
			.left_margin	= 3,
			.right_margin	= 2,
			.upper_margin	= 1,
			.lower_margin	= 1,
			.hsync_len	= 40,
			.vsync_len	= 1,
			.xres		= 480,
			.yres		= 272,
		},
		.max_bpp	= 32,
		.default_bpp	= 16,
	},
};

static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
	.setup_gpio	= s3c64xx_fb_gpio_setup_24bpp,
	.win[0]		= &mini6410_fb_lcd0_win[0],
	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
};

static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
	{
		.win_mode	= {	/* 7.0" 800x480 */
			.left_margin	= 8,
			.right_margin	= 13,
			.upper_margin	= 7,
			.lower_margin	= 5,
			.hsync_len	= 3,
			.vsync_len	= 1,
			.xres		= 800,
			.yres		= 480,
		},
		.max_bpp	= 32,
		.default_bpp	= 16,
	},
};

static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
	.setup_gpio	= s3c64xx_fb_gpio_setup_24bpp,
	.win[0]		= &mini6410_fb_lcd1_win[0],
	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
};

Each struct means:
  mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
  mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD

  mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
  mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD

In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[], instead of selecting mini6410_fb_win[0] or
mini6410_fb_win[1].

My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.

> 
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html