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

Hi,

On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham
[off-list ref] wrote:

Hi Darius,

On 19 March 2012 05:16, Jingoo Han [off-list ref] wrote:
[...]

quoted

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

I agree with solution which Jingoo Han has proposed. Instead of
selecting one instance of s3c_fb_pd_win over the other at runtime, the
features->lcd_index can be used to select one of the instances of
platform data for s3c-fb driver.

I agree with it too. Would you make such patch based on your s3c-fb patch set?
Somebody should make it and definitely not to remove multiple LCD
suport for these boards.

s3c_fb_pd_win is used to describe a window supported by the fimd
controller. It is not meant to carry lcd timing information. The lcd
panel timing remains the same irrespective of the window sizes. And
this patchset is a step towards untangling window information and
video timing.

Right now s3c_fb_pd_win contains exactly LCD timing values.
Your patch will move these timing values to struct fb_videomode.

I could make necessary patch for that too, but I think you will spend
less time doing it by yourself than syncing with me.
What do you think?

regards,
Darius