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

Hi Darius,

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

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.

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.

(Apologizes for the delayed reply. I was out of office for last 4 days.)

Thanks,
Thomas.