| Message ID | 1331585999-8604-4-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Thomas Abraham [mailto:thomas.abraham@linaro.org] > Sent: Tuesday, March 13, 2012 6:00 AM > To: linux-fbdev@vger.kernel.org > Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; > kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; > JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus > Cuelenaere > Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > For all the Samsung SoC based boards which have the platform data for > s3c-fb driver, the 'default_win' element in the platform data is removed > and the lcd panel video timing values are moved out of individual window > configuration data. > > Cc: Jingoo Han <jg1.han@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: JeongHyeon Kim <jhkim@insignal.co.kr> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Ben Dooks <ben-linux@fluff.org> > Cc: Kwangwoo Lee <kwangwoo.lee@gmail.com> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > Cc: Peter Korsgaard <jacmet@sunsite.dk> > Cc: Darius Augulis <augulis.darius@gmail.com> > Cc: Maurus Cuelenaere <mcuelenaere@gmail.com> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- > arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- > arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- > arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- > arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- > arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- > arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- > arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- > arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- > arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- > arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- > arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- > arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- > arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- > arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- > arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- > arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- > arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- > arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- > 19 files changed, 285 insertions(+), 238 deletions(-) > [.....] > diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c > index c34c2ab..24dcdc9 100644 > --- a/arch/arm/mach-s3c64xx/mach-mini6410.c > +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c > @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { > > static struct s3c_fb_pd_win mini6410_fb_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, > + .xres = 480, > + .yres = 272, > }, { > - .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, > + .xres = 800, > + .yres = 480, > }, > }; > > +static struct fb_videomode mini6410_lcd_timing = { > + .left_margin = 8, > + .right_margin = 13, > + .upper_margin = 7, > + .lower_margin = 5, > + .hsync_len = 3, > + .vsync_len = 1, > + .xres = 800, > + .yres = 480, > +}; > + > static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { > .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, > + .vtiming = &mini6410_lcd_timing, > .win[0] = &mini6410_fb_win[0], > .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, > .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, > @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) > mini6410_lcd_pdata.win[0] = &mini6410_fb_win[features.lcd_index]; > > printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", > - mini6410_lcd_pdata.win[0]->win_mode.xres, > - mini6410_lcd_pdata.win[0]->win_mode.yres); > + mini6410_lcd_pdata.win[0]->xres, > + mini6410_lcd_pdata.win[0]->yres); > > s3c_nand_set_platdata(&mini6410_nand_info); > s3c_fb_set_platdata(&mini6410_lcd_pdata); > diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c > index be2a9a2..41e4f74 100644 > --- a/arch/arm/mach-s3c64xx/mach-real6410.c > +++ b/arch/arm/mach-s3c64xx/mach-real6410.c > @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { > > static struct s3c_fb_pd_win real6410_fb_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, > + .xres = 480, > + .yres = 272, > }, { > - .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, > + .xres = 800, > + .yres = 480, > }, > }; > > +static struct fb_videomode real6410_lcd_timing = { > + .left_margin = 3, > + .right_margin = 2, > + .upper_margin = 1, > + .lower_margin = 1, > + .hsync_len = 40, > + .vsync_len = 1, > + .xres = 800, > + .yres = 480, > +}; What is the difference between mini6410_lcd_timing and real6410_lcd_timing? In my opinion, it would be good as follows: +static struct fb_videomode real6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, +}; > + > static struct s3c_fb_platdata real6410_lcd_pdata __initdata = { > .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, > + .vtiming = &real6410_lcd_timing, > .win[0] = &real6410_fb_win[0], > .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, > .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, > @@ -291,8 +287,8 @@ static void __init real6410_machine_init(void) > real6410_lcd_pdata.win[0] = &real6410_fb_win[features.lcd_index]; > > printk(KERN_INFO "REAL6410: selected LCD display is %dx%d\n", > - real6410_lcd_pdata.win[0]->win_mode.xres, > - real6410_lcd_pdata.win[0]->win_mode.yres); > + real6410_lcd_pdata.win[0]->xres, > + real6410_lcd_pdata.win[0]->yres); > > s3c_fb_set_platdata(&real6410_lcd_pdata); > s3c_nand_set_platdata(&real6410_nand_info); [.....]
On 13 March 2012 15:17, Jingoo Han <jg1.han@samsung.com> wrote: >> -----Original Message----- >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] >> Sent: Tuesday, March 13, 2012 6:00 AM >> To: linux-fbdev@vger.kernel.org >> Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; >> kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; >> JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus >> Cuelenaere >> Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver >> >> For all the Samsung SoC based boards which have the platform data for >> s3c-fb driver, the 'default_win' element in the platform data is removed >> and the lcd panel video timing values are moved out of individual window >> configuration data. >> >> Cc: Jingoo Han <jg1.han@samsung.com> >> Cc: Kyungmin Park <kyungmin.park@samsung.com> >> Cc: JeongHyeon Kim <jhkim@insignal.co.kr> >> Cc: Kukjin Kim <kgene.kim@samsung.com> >> Cc: Heiko Stuebner <heiko@sntech.de> >> Cc: Ben Dooks <ben-linux@fluff.org> >> Cc: Kwangwoo Lee <kwangwoo.lee@gmail.com> >> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> >> Cc: Peter Korsgaard <jacmet@sunsite.dk> >> Cc: Darius Augulis <augulis.darius@gmail.com> >> Cc: Maurus Cuelenaere <mcuelenaere@gmail.com> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> --- >> arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- >> arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- >> arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- >> arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- >> arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- >> arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- >> arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- >> arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- >> arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- >> arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- >> arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- >> arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- >> arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- >> arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- >> arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- >> arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- >> arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- >> arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- >> arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- >> 19 files changed, 285 insertions(+), 238 deletions(-) >> > > [.....] > >> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c >> index c34c2ab..24dcdc9 100644 >> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c >> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c >> @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { >> >> static struct s3c_fb_pd_win mini6410_fb_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, >> + .xres = 480, >> + .yres = 272, >> }, { >> - .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, >> + .xres = 800, >> + .yres = 480, >> }, >> }; >> >> +static struct fb_videomode mini6410_lcd_timing = { >> + .left_margin = 8, >> + .right_margin = 13, >> + .upper_margin = 7, >> + .lower_margin = 5, >> + .hsync_len = 3, >> + .vsync_len = 1, >> + .xres = 800, >> + .yres = 480, >> +}; >> + >> static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { >> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, >> + .vtiming = &mini6410_lcd_timing, >> .win[0] = &mini6410_fb_win[0], >> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, >> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, >> @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) >> mini6410_lcd_pdata.win[0] = &mini6410_fb_win[features.lcd_index]; >> >> printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", >> - mini6410_lcd_pdata.win[0]->win_mode.xres, >> - mini6410_lcd_pdata.win[0]->win_mode.yres); >> + mini6410_lcd_pdata.win[0]->xres, >> + mini6410_lcd_pdata.win[0]->yres); >> >> s3c_nand_set_platdata(&mini6410_nand_info); >> s3c_fb_set_platdata(&mini6410_lcd_pdata); >> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c >> index be2a9a2..41e4f74 100644 >> --- a/arch/arm/mach-s3c64xx/mach-real6410.c >> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c >> @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { >> >> static struct s3c_fb_pd_win real6410_fb_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, >> + .xres = 480, >> + .yres = 272, >> }, { >> - .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, >> + .xres = 800, >> + .yres = 480, >> }, >> }; >> >> +static struct fb_videomode real6410_lcd_timing = { >> + .left_margin = 3, >> + .right_margin = 2, >> + .upper_margin = 1, >> + .lower_margin = 1, >> + .hsync_len = 40, >> + .vsync_len = 1, >> + .xres = 800, >> + .yres = 480, >> +}; > > > What is the difference between mini6410_lcd_timing and real6410_lcd_timing? > In my opinion, it would be good as follows: > > +static struct fb_videomode real6410_lcd_timing = { > + .left_margin = 8, > + .right_margin = 13, > + .upper_margin = 7, > + .lower_margin = 5, > + .hsync_len = 3, > + .vsync_len = 1, > + .xres = 800, > + .yres = 480, > +}; > > Before this patch series, both real6410 and mini6410 had 'default_win' = 0 in the platform data. And, the s3c-fb driver selected the video timing from the window selected by the default_win parameter in s3c-fb platform data, i.e window 0 for both mini6440 and real6410. So, in this patch, while moving the timing values out of window data, the timing values for window 0 was selected. The timing value for window 1 was never used on mini6410 and real6410. So I would suggest to use timing value of window 0 in this patch. Thanks for your comments. Regards, Thomas.
> -----Original Message----- > From: Thomas Abraham [mailto:thomas.abraham@linaro.org] > Sent: Tuesday, March 13, 2012 7:11 PM > To: Jingoo Han > Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux- > samsung-soc@vger.kernel.org; kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin > Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus > Cuelenaere > Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > On 13 March 2012 15:17, Jingoo Han <jg1.han@samsung.com> wrote: > >> -----Original Message----- > >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] > >> Sent: Tuesday, March 13, 2012 6:00 AM > >> To: linux-fbdev@vger.kernel.org > >> Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; > >> kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; > >> JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus > >> Cuelenaere > >> Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > >> > >> For all the Samsung SoC based boards which have the platform data for > >> s3c-fb driver, the 'default_win' element in the platform data is removed > >> and the lcd panel video timing values are moved out of individual window > >> configuration data. > >> > >> Cc: Jingoo Han <jg1.han@samsung.com> > >> Cc: Kyungmin Park <kyungmin.park@samsung.com> > >> Cc: JeongHyeon Kim <jhkim@insignal.co.kr> > >> Cc: Kukjin Kim <kgene.kim@samsung.com> > >> Cc: Heiko Stuebner <heiko@sntech.de> > >> Cc: Ben Dooks <ben-linux@fluff.org> > >> Cc: Kwangwoo Lee <kwangwoo.lee@gmail.com> > >> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > >> Cc: Peter Korsgaard <jacmet@sunsite.dk> > >> Cc: Darius Augulis <augulis.darius@gmail.com> > >> Cc: Maurus Cuelenaere <mcuelenaere@gmail.com> > >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > >> --- > >> arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- > >> arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- > >> arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- > >> arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- > >> arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- > >> arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- > >> arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- > >> arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- > >> arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- > >> arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- > >> arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- > >> arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- > >> arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- > >> arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- > >> arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- > >> arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- > >> arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- > >> arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- > >> arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- > >> 19 files changed, 285 insertions(+), 238 deletions(-) > >> > > > > [.....] > > > >> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c > >> index c34c2ab..24dcdc9 100644 > >> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c > >> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c > >> @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { > >> > >> static struct s3c_fb_pd_win mini6410_fb_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, > >> + .xres = 480, > >> + .yres = 272, > >> }, { > >> - .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, > >> + .xres = 800, > >> + .yres = 480, > >> }, > >> }; > >> > >> +static struct fb_videomode mini6410_lcd_timing = { > >> + .left_margin = 8, > >> + .right_margin = 13, > >> + .upper_margin = 7, > >> + .lower_margin = 5, > >> + .hsync_len = 3, > >> + .vsync_len = 1, > >> + .xres = 800, > >> + .yres = 480, > >> +}; > >> + > >> static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { > >> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, > >> + .vtiming = &mini6410_lcd_timing, > >> .win[0] = &mini6410_fb_win[0], > >> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, > >> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, > >> @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) > >> mini6410_lcd_pdata.win[0] = &mini6410_fb_win[features.lcd_index]; > >> > >> printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", > >> - mini6410_lcd_pdata.win[0]->win_mode.xres, > >> - mini6410_lcd_pdata.win[0]->win_mode.yres); > >> + mini6410_lcd_pdata.win[0]->xres, > >> + mini6410_lcd_pdata.win[0]->yres); > >> > >> s3c_nand_set_platdata(&mini6410_nand_info); > >> s3c_fb_set_platdata(&mini6410_lcd_pdata); > >> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c > >> index be2a9a2..41e4f74 100644 > >> --- a/arch/arm/mach-s3c64xx/mach-real6410.c > >> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c > >> @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { > >> > >> static struct s3c_fb_pd_win real6410_fb_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, > >> + .xres = 480, > >> + .yres = 272, > >> }, { > >> - .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, > >> + .xres = 800, > >> + .yres = 480, > >> }, > >> }; > >> > >> +static struct fb_videomode real6410_lcd_timing = { > >> + .left_margin = 3, > >> + .right_margin = 2, > >> + .upper_margin = 1, > >> + .lower_margin = 1, > >> + .hsync_len = 40, > >> + .vsync_len = 1, > >> + .xres = 800, > >> + .yres = 480, > >> +}; > > > > > > What is the difference between mini6410_lcd_timing and real6410_lcd_timing? > > In my opinion, it would be good as follows: > > > > +static struct fb_videomode real6410_lcd_timing = { > > + .left_margin = 8, > > + .right_margin = 13, > > + .upper_margin = 7, > > + .lower_margin = 5, > > + .hsync_len = 3, > > + .vsync_len = 1, > > + .xres = 800, > > + .yres = 480, > > +}; > > > > > > Before this patch series, both real6410 and mini6410 had 'default_win' > = 0 in the platform data. And, the s3c-fb driver selected the video > timing from the window selected by the default_win parameter in s3c-fb > platform data, i.e window 0 for both mini6440 and real6410. So, in > this patch, while moving the timing values out of window data, the > timing values for window 0 was selected. > > The timing value for window 1 was never used on mini6410 and real6410. > So I would suggest to use timing value of window 0 in this patch. OK, I see. Then, as you mentioned above, the timing value of mini6410 and real6410 should be same. Also, the mini6410 should use the timing value for window 0 as below. Also, this timing value is used for 4.3" 480x272 LCD. So, xres and yres would be 480 and 272, respectively, instead of 800 and 480. The mini6410 seems to use 4.3" 480x272 LCD as default LCD. Please refer to 'http://www.friendlyarm.net/products/mini6410'. Also, real6410 seems to use 4.3" 480x272 LCD as default LCD. http://s3c6410kits.googlecode.com/files/overview_Real6410.pdf Therefore, given this, the timing value of mini6410 and real6410 would be as follows. +static struct fb_videomode mini6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 480, + .yres = 272, +}; +static struct fb_videomode real6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 480, + .yres = 272, +}; Darius Augulis, can you confirm it? (Darius Augulis is a maintainer for real6410 and mini6410 boards.) Best regards, Jingoo Han > > Thanks for your comments. > > Regards, > Thomas.
On 03/14/2012 03:51 AM, Jingoo Han wrote: >> -----Original Message----- >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] >> Sent: Tuesday, March 13, 2012 7:11 PM >> To: Jingoo Han >> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux- >> samsung-soc@vger.kernel.org; kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin >> Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus >> Cuelenaere >> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver >> >> On 13 March 2012 15:17, Jingoo Han<jg1.han@samsung.com> wrote: >>>> -----Original Message----- >>>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] >>>> Sent: Tuesday, March 13, 2012 6:00 AM >>>> To: linux-fbdev@vger.kernel.org >>>> Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; >>>> kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; >>>> JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus >>>> Cuelenaere >>>> Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver >>>> >>>> For all the Samsung SoC based boards which have the platform data for >>>> s3c-fb driver, the 'default_win' element in the platform data is removed >>>> and the lcd panel video timing values are moved out of individual window >>>> configuration data. >>>> >>>> Cc: Jingoo Han<jg1.han@samsung.com> >>>> Cc: Kyungmin Park<kyungmin.park@samsung.com> >>>> Cc: JeongHyeon Kim<jhkim@insignal.co.kr> >>>> Cc: Kukjin Kim<kgene.kim@samsung.com> >>>> Cc: Heiko Stuebner<heiko@sntech.de> >>>> Cc: Ben Dooks<ben-linux@fluff.org> >>>> Cc: Kwangwoo Lee<kwangwoo.lee@gmail.com> >>>> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com> >>>> Cc: Peter Korsgaard<jacmet@sunsite.dk> >>>> Cc: Darius Augulis<augulis.darius@gmail.com> >>>> Cc: Maurus Cuelenaere<mcuelenaere@gmail.com> >>>> Signed-off-by: Thomas Abraham<thomas.abraham@linaro.org> >>>> --- >>>> arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- >>>> arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- >>>> arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- >>>> arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- >>>> arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- >>>> arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- >>>> arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- >>>> arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- >>>> arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- >>>> arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- >>>> arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- >>>> arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- >>>> arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- >>>> arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- >>>> arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- >>>> arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- >>>> arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- >>>> arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- >>>> arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- >>>> 19 files changed, 285 insertions(+), 238 deletions(-) >>>> >>> [.....] >>> >>>> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c >>>> index c34c2ab..24dcdc9 100644 >>>> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c >>>> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c >>>> @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { >>>> >>>> static struct s3c_fb_pd_win mini6410_fb_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, >>>> + .xres = 480, >>>> + .yres = 272, >>>> }, { >>>> - .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, >>>> + .xres = 800, >>>> + .yres = 480, >>>> }, >>>> }; >>>> >>>> +static struct fb_videomode mini6410_lcd_timing = { >>>> + .left_margin = 8, >>>> + .right_margin = 13, >>>> + .upper_margin = 7, >>>> + .lower_margin = 5, >>>> + .hsync_len = 3, >>>> + .vsync_len = 1, >>>> + .xres = 800, >>>> + .yres = 480, >>>> +}; >>>> + >>>> static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { >>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, >>>> + .vtiming =&mini6410_lcd_timing, >>>> .win[0] =&mini6410_fb_win[0], >>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, >>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, >>>> @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) >>>> mini6410_lcd_pdata.win[0] =&mini6410_fb_win[features.lcd_index]; >>>> >>>> printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", >>>> - mini6410_lcd_pdata.win[0]->win_mode.xres, >>>> - mini6410_lcd_pdata.win[0]->win_mode.yres); >>>> + mini6410_lcd_pdata.win[0]->xres, >>>> + mini6410_lcd_pdata.win[0]->yres); >>>> >>>> s3c_nand_set_platdata(&mini6410_nand_info); >>>> s3c_fb_set_platdata(&mini6410_lcd_pdata); >>>> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c >>>> index be2a9a2..41e4f74 100644 >>>> --- a/arch/arm/mach-s3c64xx/mach-real6410.c >>>> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c >>>> @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { >>>> >>>> static struct s3c_fb_pd_win real6410_fb_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, >>>> + .xres = 480, >>>> + .yres = 272, >>>> }, { >>>> - .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, >>>> + .xres = 800, >>>> + .yres = 480, >>>> }, >>>> }; >>>> >>>> +static struct fb_videomode real6410_lcd_timing = { >>>> + .left_margin = 3, >>>> + .right_margin = 2, >>>> + .upper_margin = 1, >>>> + .lower_margin = 1, >>>> + .hsync_len = 40, >>>> + .vsync_len = 1, >>>> + .xres = 800, >>>> + .yres = 480, >>>> +}; >>> >>> What is the difference between mini6410_lcd_timing and real6410_lcd_timing? >>> In my opinion, it would be good as follows: >>> >>> +static struct fb_videomode real6410_lcd_timing = { >>> + .left_margin = 8, >>> + .right_margin = 13, >>> + .upper_margin = 7, >>> + .lower_margin = 5, >>> + .hsync_len = 3, >>> + .vsync_len = 1, >>> + .xres = 800, >>> + .yres = 480, >>> +}; >>> >>> >> Before this patch series, both real6410 and mini6410 had 'default_win' >> = 0 in the platform data. And, the s3c-fb driver selected the video >> timing from the window selected by the default_win parameter in s3c-fb >> platform data, i.e window 0 for both mini6440 and real6410. So, in >> this patch, while moving the timing values out of window data, the >> timing values for window 0 was selected. >> >> The timing value for window 1 was never used on mini6410 and real6410. >> So I would suggest to use timing value of window 0 in this patch. > OK, I see. > Then, as you mentioned above, the timing value of mini6410 and real6410 should be same. > Also, the mini6410 should use the timing value for window 0 as below. > > Also, this timing value is used for 4.3" 480x272 LCD. > So, xres and yres would be 480 and 272, respectively, instead of 800 and 480. > > The mini6410 seems to use 4.3" 480x272 LCD as default LCD. > Please refer to 'http://www.friendlyarm.net/products/mini6410'. > > Also, real6410 seems to use 4.3" 480x272 LCD as default LCD. > http://s3c6410kits.googlecode.com/files/overview_Real6410.pdf > > Therefore, given this, the timing value of mini6410 and real6410 would be as follows. > > +static struct fb_videomode mini6410_lcd_timing = { > + .left_margin = 8, > + .right_margin = 13, > + .upper_margin = 7, > + .lower_margin = 5, > + .hsync_len = 3, > + .vsync_len = 1, > + .xres = 480, > + .yres = 272, > +}; > > +static struct fb_videomode real6410_lcd_timing = { > + .left_margin = 8, > + .right_margin = 13, > + .upper_margin = 7, > + .lower_margin = 5, > + .hsync_len = 3, > + .vsync_len = 1, > + .xres = 480, > + .yres = 272, > +}; > > Darius Augulis, can you confirm it? > (Darius Augulis is a maintainer for real6410 and mini6410 boards.) Are you going to leave only single LCD resolution for every of two boards? If so, then my answer is no. I have mini6410 with both 4.3" and 7" LCDs and real6410 with 7" LCD. Now we have possibility to choose LCD size dynamically - leave it there. What you mean "default" 4.3" size LCD? The 7" size LCD is also provided by board sellers - I've bought it. regards, Darius Augulis > > Best regards, > Jingoo Han > >> Thanks for your comments. >> >> Regards, >> Thomas. >
On 03/13/2012 12:10 PM, Thomas Abraham wrote: > On 13 March 2012 15:17, Jingoo Han<jg1.han@samsung.com> wrote: >>> -----Original Message----- >>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] >>> Sent: Tuesday, March 13, 2012 6:00 AM >>> To: linux-fbdev@vger.kernel.org >>> Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; >>> kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; >>> JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus >>> Cuelenaere >>> Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver >>> >>> For all the Samsung SoC based boards which have the platform data for >>> s3c-fb driver, the 'default_win' element in the platform data is removed >>> and the lcd panel video timing values are moved out of individual window >>> configuration data. >>> >>> Cc: Jingoo Han<jg1.han@samsung.com> >>> Cc: Kyungmin Park<kyungmin.park@samsung.com> >>> Cc: JeongHyeon Kim<jhkim@insignal.co.kr> >>> Cc: Kukjin Kim<kgene.kim@samsung.com> >>> Cc: Heiko Stuebner<heiko@sntech.de> >>> Cc: Ben Dooks<ben-linux@fluff.org> >>> Cc: Kwangwoo Lee<kwangwoo.lee@gmail.com> >>> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com> >>> Cc: Peter Korsgaard<jacmet@sunsite.dk> >>> Cc: Darius Augulis<augulis.darius@gmail.com> >>> Cc: Maurus Cuelenaere<mcuelenaere@gmail.com> >>> Signed-off-by: Thomas Abraham<thomas.abraham@linaro.org> >>> --- >>> arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- >>> arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- >>> arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- >>> arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- >>> arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- >>> arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- >>> arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- >>> arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- >>> arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- >>> arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- >>> arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- >>> arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- >>> arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- >>> arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- >>> arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- >>> arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- >>> arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- >>> arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- >>> arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- >>> 19 files changed, 285 insertions(+), 238 deletions(-) >>> >> [.....] >> >>> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c >>> index c34c2ab..24dcdc9 100644 >>> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c >>> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c >>> @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { >>> >>> static struct s3c_fb_pd_win mini6410_fb_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, >>> + .xres = 480, >>> + .yres = 272, >>> }, { >>> - .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, >>> + .xres = 800, >>> + .yres = 480, >>> }, >>> }; >>> >>> +static struct fb_videomode mini6410_lcd_timing = { >>> + .left_margin = 8, >>> + .right_margin = 13, >>> + .upper_margin = 7, >>> + .lower_margin = 5, >>> + .hsync_len = 3, >>> + .vsync_len = 1, >>> + .xres = 800, >>> + .yres = 480, >>> +}; >>> + >>> static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { >>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, >>> + .vtiming =&mini6410_lcd_timing, >>> .win[0] =&mini6410_fb_win[0], >>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, >>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, >>> @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) >>> mini6410_lcd_pdata.win[0] =&mini6410_fb_win[features.lcd_index]; >>> >>> printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", >>> - mini6410_lcd_pdata.win[0]->win_mode.xres, >>> - mini6410_lcd_pdata.win[0]->win_mode.yres); >>> + mini6410_lcd_pdata.win[0]->xres, >>> + mini6410_lcd_pdata.win[0]->yres); >>> >>> s3c_nand_set_platdata(&mini6410_nand_info); >>> s3c_fb_set_platdata(&mini6410_lcd_pdata); >>> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c >>> index be2a9a2..41e4f74 100644 >>> --- a/arch/arm/mach-s3c64xx/mach-real6410.c >>> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c >>> @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { >>> >>> static struct s3c_fb_pd_win real6410_fb_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, >>> + .xres = 480, >>> + .yres = 272, >>> }, { >>> - .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, >>> + .xres = 800, >>> + .yres = 480, >>> }, >>> }; >>> >>> +static struct fb_videomode real6410_lcd_timing = { >>> + .left_margin = 3, >>> + .right_margin = 2, >>> + .upper_margin = 1, >>> + .lower_margin = 1, >>> + .hsync_len = 40, >>> + .vsync_len = 1, >>> + .xres = 800, >>> + .yres = 480, >>> +}; >> >> What is the difference between mini6410_lcd_timing and real6410_lcd_timing? >> In my opinion, it would be good as follows: >> >> +static struct fb_videomode real6410_lcd_timing = { >> + .left_margin = 8, >> + .right_margin = 13, >> + .upper_margin = 7, >> + .lower_margin = 5, >> + .hsync_len = 3, >> + .vsync_len = 1, >> + .xres = 800, >> + .yres = 480, >> +}; >> >> > Before this patch series, both real6410 and mini6410 had 'default_win' > = 0 in the platform data. And, the s3c-fb driver selected the video > timing from the window selected by the default_win parameter in s3c-fb > platform data, i.e window 0 for both mini6440 and real6410. So, in > this patch, while moving the timing values out of window data, the > timing values for window 0 was selected. > > The timing value for window 1 was never used on mini6410 and real6410. > So I would suggest to use timing value of window 0 in this patch. You are wrong. Please see mini6410_parse_features(). Please do not remove second window because it could be selected via kernel parameters dinamically. It's very useful and I spend my hours to create it. > > Thanks for your comments. > > Regards, > Thomas. >
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Thursday, March 15, 2012 3:39 AM > 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; kgene.kim@samsung.com; ben- > linux@fluff.org; patches@linaro.org; 'Kyungmin Park'; 'JeongHyeon Kim'; 'Heiko Stuebner'; 'Kwangwoo Lee'; > 'Mark Brown'; 'Peter Korsgaard'; 'Maurus Cuelenaere' > Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > On 03/14/2012 03:51 AM, Jingoo Han wrote: > >> -----Original Message----- > >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] > >> Sent: Tuesday, March 13, 2012 7:11 PM > >> To: Jingoo Han > >> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; > linux- > >> samsung-soc@vger.kernel.org; kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin > >> Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; > Maurus > >> Cuelenaere > >> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > >> > >> On 13 March 2012 15:17, Jingoo Han<jg1.han@samsung.com> wrote: > >>>> -----Original Message----- > >>>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org] > >>>> Sent: Tuesday, March 13, 2012 6:00 AM > >>>> To: linux-fbdev@vger.kernel.org > >>>> Cc: FlorianSchandinat@gmx.de; linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; > >>>> kgene.kim@samsung.com; jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org; Kyungmin Park; > >>>> JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter Korsgaard; Darius Augulis; Maurus > >>>> Cuelenaere > >>>> Subject: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > >>>> > >>>> For all the Samsung SoC based boards which have the platform data for > >>>> s3c-fb driver, the 'default_win' element in the platform data is removed > >>>> and the lcd panel video timing values are moved out of individual window > >>>> configuration data. > >>>> > >>>> Cc: Jingoo Han<jg1.han@samsung.com> > >>>> Cc: Kyungmin Park<kyungmin.park@samsung.com> > >>>> Cc: JeongHyeon Kim<jhkim@insignal.co.kr> > >>>> Cc: Kukjin Kim<kgene.kim@samsung.com> > >>>> Cc: Heiko Stuebner<heiko@sntech.de> > >>>> Cc: Ben Dooks<ben-linux@fluff.org> > >>>> Cc: Kwangwoo Lee<kwangwoo.lee@gmail.com> > >>>> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com> > >>>> Cc: Peter Korsgaard<jacmet@sunsite.dk> > >>>> Cc: Darius Augulis<augulis.darius@gmail.com> > >>>> Cc: Maurus Cuelenaere<mcuelenaere@gmail.com> > >>>> Signed-off-by: Thomas Abraham<thomas.abraham@linaro.org> > >>>> --- > >>>> arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- > >>>> arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- > >>>> arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- > >>>> arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- > >>>> arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- > >>>> arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- > >>>> arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- > >>>> arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- > >>>> arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- > >>>> arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- > >>>> arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- > >>>> arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- > >>>> arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- > >>>> arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- > >>>> arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- > >>>> arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- > >>>> arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- > >>>> arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- > >>>> arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- > >>>> 19 files changed, 285 insertions(+), 238 deletions(-) > >>>> > >>> [.....] > >>> > >>>> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c > >>>> index c34c2ab..24dcdc9 100644 > >>>> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c > >>>> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c > >>>> @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { > >>>> > >>>> static struct s3c_fb_pd_win mini6410_fb_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, > >>>> + .xres = 480, > >>>> + .yres = 272, > >>>> }, { > >>>> - .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, > >>>> + .xres = 800, > >>>> + .yres = 480, > >>>> }, > >>>> }; > >>>> > >>>> +static struct fb_videomode mini6410_lcd_timing = { > >>>> + .left_margin = 8, > >>>> + .right_margin = 13, > >>>> + .upper_margin = 7, > >>>> + .lower_margin = 5, > >>>> + .hsync_len = 3, > >>>> + .vsync_len = 1, > >>>> + .xres = 800, > >>>> + .yres = 480, > >>>> +}; > >>>> + > >>>> static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { > >>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, > >>>> + .vtiming =&mini6410_lcd_timing, > >>>> .win[0] =&mini6410_fb_win[0], > >>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, > >>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, > >>>> @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) > >>>> mini6410_lcd_pdata.win[0] =&mini6410_fb_win[features.lcd_index]; > >>>> > >>>> printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", > >>>> - mini6410_lcd_pdata.win[0]->win_mode.xres, > >>>> - mini6410_lcd_pdata.win[0]->win_mode.yres); > >>>> + mini6410_lcd_pdata.win[0]->xres, > >>>> + mini6410_lcd_pdata.win[0]->yres); > >>>> > >>>> s3c_nand_set_platdata(&mini6410_nand_info); > >>>> s3c_fb_set_platdata(&mini6410_lcd_pdata); > >>>> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c > >>>> index be2a9a2..41e4f74 100644 > >>>> --- a/arch/arm/mach-s3c64xx/mach-real6410.c > >>>> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c > >>>> @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { > >>>> > >>>> static struct s3c_fb_pd_win real6410_fb_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, > >>>> + .xres = 480, > >>>> + .yres = 272, > >>>> }, { > >>>> - .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, > >>>> + .xres = 800, > >>>> + .yres = 480, > >>>> }, > >>>> }; > >>>> > >>>> +static struct fb_videomode real6410_lcd_timing = { > >>>> + .left_margin = 3, > >>>> + .right_margin = 2, > >>>> + .upper_margin = 1, > >>>> + .lower_margin = 1, > >>>> + .hsync_len = 40, > >>>> + .vsync_len = 1, > >>>> + .xres = 800, > >>>> + .yres = 480, > >>>> +}; > >>> > >>> What is the difference between mini6410_lcd_timing and real6410_lcd_timing? > >>> In my opinion, it would be good as follows: > >>> > >>> +static struct fb_videomode real6410_lcd_timing = { > >>> + .left_margin = 8, > >>> + .right_margin = 13, > >>> + .upper_margin = 7, > >>> + .lower_margin = 5, > >>> + .hsync_len = 3, > >>> + .vsync_len = 1, > >>> + .xres = 800, > >>> + .yres = 480, > >>> +}; > >>> > >>> > >> Before this patch series, both real6410 and mini6410 had 'default_win' > >> = 0 in the platform data. And, the s3c-fb driver selected the video > >> timing from the window selected by the default_win parameter in s3c-fb > >> platform data, i.e window 0 for both mini6440 and real6410. So, in > >> this patch, while moving the timing values out of window data, the > >> timing values for window 0 was selected. > >> > >> The timing value for window 1 was never used on mini6410 and real6410. > >> So I would suggest to use timing value of window 0 in this patch. > > OK, I see. > > Then, as you mentioned above, the timing value of mini6410 and real6410 should be same. > > Also, the mini6410 should use the timing value for window 0 as below. > > > > Also, this timing value is used for 4.3" 480x272 LCD. > > So, xres and yres would be 480 and 272, respectively, instead of 800 and 480. > > > > The mini6410 seems to use 4.3" 480x272 LCD as default LCD. > > Please refer to 'http://www.friendlyarm.net/products/mini6410'. > > > > Also, real6410 seems to use 4.3" 480x272 LCD as default LCD. > > http://s3c6410kits.googlecode.com/files/overview_Real6410.pdf > > > > Therefore, given this, the timing value of mini6410 and real6410 would be as follows. > > > > +static struct fb_videomode mini6410_lcd_timing = { > > + .left_margin = 8, > > + .right_margin = 13, > > + .upper_margin = 7, > > + .lower_margin = 5, > > + .hsync_len = 3, > > + .vsync_len = 1, > > + .xres = 480, > > + .yres = 272, > > +}; > > > > +static struct fb_videomode real6410_lcd_timing = { > > + .left_margin = 8, > > + .right_margin = 13, > > + .upper_margin = 7, > > + .lower_margin = 5, > > + .hsync_len = 3, > > + .vsync_len = 1, > > + .xres = 480, > > + .yres = 272, > > +}; > > > > Darius Augulis, can you confirm it? > > (Darius Augulis is a maintainer for real6410 and mini6410 boards.) > > Are you going to leave only single LCD resolution for every of two > boards? If so, then my answer is no. Yes, only single LCD resolution will be left. > I have mini6410 with both 4.3" and 7" LCDs and real6410 with 7" LCD. Now > we have possibility to choose LCD size dynamically - leave it there. > What you mean "default" 4.3" size LCD? The 7" size LCD is also provided > by board sellers - I've bought it. OK, I see. Both mini6410 and real6410 provide both 4.3" and 7" LCDs, so you needs to select both LCDs. Um, usually, single LCD is provided on the single board. Also, the daughter board with another kind LCD can be connected to the board. However, .win_mode is used not for LCD, but for windows of FIMD IP. Actually, our current framework does not support to choose multi LCD, so, I understand that you use .win_mode[1] as second LCD. However, that's not accurate way to select multi LCD. Thomas, can you consider Augulis's opinion? I think that the method to select multi LCDs is necessary. Ideal process is such as: 1. add the patch to support to select multi LCDs 2. apply above patch to make the mini6410 and real6410 to select multi LCDs. 3. apply Thomas's patchset to remove timing value from .win_mode variable. Best regards, Jingoo Han > > regards, > Darius Augulis > > > > > Best regards, > > Jingoo Han > > > >> Thanks for your comments. > >> > >> Regards, > >> Thomas. > >
Hi, > > Yes, only single LCD resolution will be left. > >> I have mini6410 with both 4.3" and 7" LCDs and real6410 with 7" LCD. Now >> we have possibility to choose LCD size dynamically - leave it there. >> What you mean "default" 4.3" size LCD? The 7" size LCD is also provided >> by board sellers - I've bought it. > > OK, I see. > Both mini6410 and real6410 provide both 4.3" and 7" LCDs, > so you needs to select both LCDs. > > Um, usually, single LCD is provided on the single board. > Also, the daughter board with another kind LCD can be connected to the board. There is single board - mini6410 (or real6410) and it's name doesn't depend on connected LCD size. We know, that this board is available with different sizes of LCD and currently we have in kernel support for both sizes. It might be so, that it's implemented not in perfect way, but it was accepted and at least it's working. If you want to rework s3c-fb platform data and driver framework, you should not drop any functionality created by other people. > > However, .win_mode is used not for LCD, but for windows of FIMD IP. > Actually, our current framework does not support to choose multi LCD, > so, I understand that you use .win_mode[1] as second LCD. > However, that's not accurate way to select multi LCD. > > Thomas, can you consider Augulis's opinion? > I think that the method to select multi LCDs is necessary. > > Ideal process is such as: > 1. add the patch to support to select multi LCDs > 2. apply above patch to make the mini6410 and real6410 to select multi LCDs. > 3. apply Thomas's patchset to remove timing value from .win_mode variable. Yes, this would be the right way to go. regards, Darius A.
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Thursday, March 15, 2012 4:42 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; kgene.kim@samsung.com; ben- > linux@fluff.org; patches@linaro.org; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark > Brown; Peter Korsgaard; Maurus Cuelenaere > Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > Hi, > > > > > Yes, only single LCD resolution will be left. > > > >> I have mini6410 with both 4.3" and 7" LCDs and real6410 with 7" LCD. Now > >> we have possibility to choose LCD size dynamically - leave it there. > >> What you mean "default" 4.3" size LCD? The 7" size LCD is also provided > >> by board sellers - I've bought it. > > > > OK, I see. > > Both mini6410 and real6410 provide both 4.3" and 7" LCDs, > > so you needs to select both LCDs. > > > > Um, usually, single LCD is provided on the single board. > > Also, the daughter board with another kind LCD can be connected to the board. > > There is single board - mini6410 (or real6410) and it's name doesn't > depend on connected LCD size. > We know, that this board is available with different sizes of LCD and > currently we have in kernel support for both sizes. > It might be so, that it's implemented not in perfect way, but it was > accepted and at least it's working. I don't think so. You argues that the wrong code should not be removed because it was accepted and at least it's working. It is just wrong usage, which can just work. Moreover, your code will make the problem, when 2~5 windows of FIMD IP are used. So, your code can be removed. > If you want to rework s3c-fb platform data and driver framework, you > should not drop any functionality created by other people. Augulis, Thomas Abraham wants to rework s3c-fb platform data and driver framework, not me. :) I am just reviewing the Thomas's patchset. > > > > > However, .win_mode is used not for LCD, but for windows of FIMD IP. > > Actually, our current framework does not support to choose multi LCD, > > so, I understand that you use .win_mode[1] as second LCD. > > However, that's not accurate way to select multi LCD. > > > > Thomas, can you consider Augulis's opinion? > > I think that the method to select multi LCDs is necessary. > > > > Ideal process is such as: > > 1. add the patch to support to select multi LCDs > > 2. apply above patch to make the mini6410 and real6410 to select multi LCDs. > > 3. apply Thomas's patchset to remove timing value from .win_mode variable. > > Yes, this would be the right way to go. However, skipping step 1 & 2 is also available, because your code is wrong. As I'm mentioned above, your code will make the problem when 2~5 windows of FIMD IP are used. > > regards, > Darius A.
On Thu, Mar 15, 2012 at 10:10 AM, Jingoo Han <jg1.han@samsung.com> wrote: >> There is single board - mini6410 (or real6410) and it's name doesn't >> depend on connected LCD size. >> We know, that this board is available with different sizes of LCD and >> currently we have in kernel support for both sizes. >> It might be so, that it's implemented not in perfect way, but it was >> accepted and at least it's working. > > > I don't think so. > You argues that the wrong code should not be removed because it was accepted and at least it's working. > It is just wrong usage, which can just work. > Moreover, your code will make the problem, when 2~5 windows of FIMD IP are used. Could please explain that? How does LCD feature selection for mini6410 and real6410 break that? Timing for different sizes of LCD is located in two statically allocated structures which are used at board init time depending on kernel param line. Data from one of these structures is copied to s3c-fb platform data dynamically by board init code. Why it's a problem? What changes if platform data isn't rewritten dynamically? > So, your code can be removed. I don't think so. It does not break anything yet. One who make changes, should ensure backwards compatibility, at least talking about functionality and hardware (LCD) support, which was added few months ago. Remember, we talk about kernel parameters line - now it lets bootloader to select correct LCD size. After your changes, board with 7" LCD wont show anything. Darius A.
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Thursday, March 15, 2012 5:27 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; kgene.kim@samsung.com; ben- > linux@fluff.org; patches@linaro.org; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark > Brown; Peter Korsgaard; Maurus Cuelenaere > Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > On Thu, Mar 15, 2012 at 10:10 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > >> There is single board - mini6410 (or real6410) and it's name doesn't > >> depend on connected LCD size. > >> We know, that this board is available with different sizes of LCD and > >> currently we have in kernel support for both sizes. > >> It might be so, that it's implemented not in perfect way, but it was > >> accepted and at least it's working. > > > > > > I don't think so. > > You argues that the wrong code should not be removed because it was accepted and at least it's working. > > It is just wrong usage, which can just work. > > Moreover, your code will make the problem, when 2~5 windows of FIMD IP are used. > > Could please explain that? How does LCD feature selection for mini6410 > and real6410 break that? > Timing for different sizes of LCD is located in two statically > allocated structures which are used at board init time depending on > kernel param line. > Data from one of these structures is copied to s3c-fb platform data > dynamically by board init code. Why it's a problem? What changes if > platform data isn't rewritten dynamically? > > > So, your code can be removed. > > I don't think so. It does not break anything yet. One who make > changes, should ensure backwards compatibility, at least talking about > functionality and hardware (LCD) support, which was added few months > ago. Remember, we talk about kernel parameters line - now it lets > bootloader to select correct LCD size. After your changes, board with > 7" LCD wont show anything. OK, your code work, although the usage is wrong. Thomas, can you keep backwards compatibility? > > Darius A.
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Thursday, March 15, 2012 5:27 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; kgene.kim@samsung.com; ben- > linux@fluff.org; patches@linaro.org; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark > Brown; Peter Korsgaard; Maurus Cuelenaere > Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver > > On Thu, Mar 15, 2012 at 10:10 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > >> There is single board - mini6410 (or real6410) and it's name doesn't > >> depend on connected LCD size. > >> We know, that this board is available with different sizes of LCD and > >> currently we have in kernel support for both sizes. > >> It might be so, that it's implemented not in perfect way, but it was > >> accepted and at least it's working. > > > > > > I don't think so. > > You argues that the wrong code should not be removed because it was accepted and at least it's working. > > It is just wrong usage, which can just work. > > Moreover, your code will make the problem, when 2~5 windows of FIMD IP are used. > > Could please explain that? How does LCD feature selection for mini6410 > and real6410 break that? > Timing for different sizes of LCD is located in two statically > allocated structures which are used at board init time depending on > kernel param line. > Data from one of these structures is copied to s3c-fb platform data > dynamically by board init code. Why it's a problem? What changes if > platform data isn't rewritten dynamically? > > > So, your code can be removed. > > I don't think so. It does not break anything yet. One who make > changes, should ensure backwards compatibility, at least talking about > functionality and hardware (LCD) support, which was added few months > ago. Remember, we talk about kernel parameters line - now it lets > bootloader to select correct LCD size. After your changes, board with > 7" LCD wont show anything. [CC'ed Tushar Behera] Yes, your code is working. However, your code is bug. 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' should not be used to select LCD. The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make problem with Thomas's patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP. We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would not make the compatibility problem with your code. Why we should keep aberration such as your code? If you want to select two LCDs, please find other way. > > Darius A.
On Fri, Mar 16, 2012 at 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote: >> > So, your code can be removed. >> >> I don't think so. It does not break anything yet. One who make >> changes, should ensure backwards compatibility, at least talking about >> functionality and hardware (LCD) support, which was added few months >> ago. Remember, we talk about kernel parameters line - now it lets >> bootloader to select correct LCD size. After your changes, board with >> 7" LCD wont show anything. > > [CC'ed Tushar Behera] > > Yes, your code is working. However, your code is bug. it's not a bug, because it's working like it was intended to. it was reviewed and accepted by maintainers before merging to main line. you do not have rights to drop it because you don't like it. You are doing changes - please do it correctly and do not break existing functionality. btw, you still not answered my question: why these two s3c_fb_pd_win structures in mach-mini6410.c is a problem? They are declared on the stack, but platform data uses only single one. mini6410 rewrites its s3c-fb platform data with data from one of these two structures dynamically. Why it's a bug? You want to remove this dynamic selection which could be leaved there with reworked framework too. > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' should not be used to select LCD. > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make problem with Thomas's > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP. > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would not make the compatibility > problem with your code. Why we should keep aberration such as your code? If you want to select two LCDs, please find other way. > > >> >> Darius A. >
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Friday, March 16, 2012 4:47 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 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > >> > So, your code can be removed. > >> > >> I don't think so. It does not break anything yet. One who make > >> changes, should ensure backwards compatibility, at least talking about > >> functionality and hardware (LCD) support, which was added few months > >> ago. Remember, we talk about kernel parameters line - now it lets > >> bootloader to select correct LCD size. After your changes, board with > >> 7" LCD wont show anything. > > > > [CC'ed Tushar Behera] > > > > Yes, your code is working. However, your code is bug. > > it's not a bug, because it's working like it was intended to. it was > reviewed and accepted by maintainers before merging to main line. > you do not have rights to drop it because you don't like it. > You are doing changes - please do it correctly and do not break > existing functionality. > btw, you still not answered my question: why these two s3c_fb_pd_win > structures in mach-mini6410.c is a problem? They are declared on the > stack, but platform data uses only single one. mini6410 rewrites its > s3c-fb platform data with data from one of these two structures > dynamically. Why it's a bug? You want to remove this dynamic selection > which could be leaved there with reworked framework too. 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. struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0], s3c_fb_pd_win[1] represents window 0, window 1 of FIMD IP. However, your code includes information of LCD, it means that struct s3c_fb_pd_win represents 4.3" LCD, 7.0" LCD. It is wrong usage. > > > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' > should not be used to select LCD. > > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make > problem with Thomas's > > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP. > > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would > not make the compatibility > > problem with your code. Why we should keep aberration such as your code? If you want to select two > LCDs, please find other way. > > > > > >> > >> Darius A. > >
On Fri, Mar 16, 2012 at 10:07 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > >> -----Original Message----- >> From: Darius Augulis [mailto:augulis.darius@gmail.com] >> Sent: Friday, March 16, 2012 4:47 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 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote: >> >> it's not a bug, because it's working like it was intended to. it was >> reviewed and accepted by maintainers before merging to main line. >> you do not have rights to drop it because you don't like it. >> You are doing changes - please do it correctly and do not break >> existing functionality. >> btw, you still not answered my question: why these two s3c_fb_pd_win >> structures in mach-mini6410.c is a problem? They are declared on the >> stack, but platform data uses only single one. mini6410 rewrites its >> s3c-fb platform data with data from one of these two structures >> dynamically. Why it's a bug? You want to remove this dynamic selection >> which could be leaved there with reworked framework too. > > > 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. You are simply lazy to deal with little bit different code of mini6410 and real6410 because they have support for multiple LCDs and other s3c-fb boards doesn't. This is a reason why you want to drop it and not because it's a bug. Hope other maintainers and code reviewers understand this situation correctly. > > struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0], s3c_fb_pd_win[1] represents window 0, > window 1 of FIMD IP. However, your code includes information of LCD, it means that struct s3c_fb_pd_win represents 4.3" LCD, 7.0" > LCD. It is wrong usage. > > >> >> > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' >> should not be used to select LCD. >> > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make >> problem with Thomas's >> > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP. >> > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would >> not make the compatibility >> > problem with your code. Why we should keep aberration such as your code? If you want to select two >> LCDs, please find other way. >> > >> > >> >> >> >> Darius A. >> > >
> -----Original Message----- > From: Darius Augulis [mailto:augulis.darius@gmail.com] > Sent: Friday, March 16, 2012 6:16 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 10:07 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > > > > >> -----Original Message----- > >> From: Darius Augulis [mailto:augulis.darius@gmail.com] > >> Sent: Friday, March 16, 2012 4:47 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 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote: > >> > >> it's not a bug, because it's working like it was intended to. it was > >> reviewed and accepted by maintainers before merging to main line. > >> you do not have rights to drop it because you don't like it. > >> You are doing changes - please do it correctly and do not break > >> existing functionality. > >> btw, you still not answered my question: why these two s3c_fb_pd_win > >> structures in mach-mini6410.c is a problem? They are declared on the > >> stack, but platform data uses only single one. mini6410 rewrites its > >> s3c-fb platform data with data from one of these two structures > >> dynamically. Why it's a bug? You want to remove this dynamic selection > >> which could be leaved there with reworked framework too. > > > > > > 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. 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. > You are simply lazy to deal with little bit different code of mini6410 > and real6410 because they have support for multiple LCDs and other > s3c-fb boards doesn't. > This is a reason why you want to drop it and not because it's a bug. > Hope other maintainers and code reviewers understand this situation correctly. > > > > > struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0], > s3c_fb_pd_win[1] represents window 0, > > window 1 of FIMD IP. However, your code includes information of LCD, it means that struct > s3c_fb_pd_win represents 4.3" LCD, 7.0" > > LCD. It is wrong usage. > > > > > >> > >> > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' > >> should not be used to select LCD. > >> > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't > make > >> problem with Thomas's > >> > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP. > >> > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would > >> not make the compatibility > >> > problem with your code. Why we should keep aberration such as your code? If you want to select two > >> LCDs, please find other way. > >> > > >> > > >> >> > >> >> Darius A. > >> > > >
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. >
> -----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
Hi Darius, On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> 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.
Hi, On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham <thomas.abraham@linaro.org> wrote: > Hi Darius, > > On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> 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. 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
On 19 March 2012 13:10, Darius Augulis <augulis.darius@gmail.com> wrote: > Hi, > > On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham > <thomas.abraham@linaro.org> wrote: >> Hi Darius, >> >> On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> 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. > > 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. Sure. I will make a patch for that. I did not notice the way two lcd's were supported in your code while preparing this patchset. Thanks for bringing up this point. > >> >> 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? Yes, I will prepare an additional patch in this series that will maintain backward compatibility for real6410 and mini6410 with the approach suggested by Jingoo Han. As I do not have either of these boards, an ack from you would be very helpful when the next version of this patchset is posted. Thanks, Thomas.
On Mon, Mar 19, 2012 at 9:49 AM, Thomas Abraham <thomas.abraham@linaro.org> wrote: > On 19 March 2012 13:10, Darius Augulis <augulis.darius@gmail.com> wrote: >> 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? > > Yes, I will prepare an additional patch in this series that will > maintain backward compatibility for real6410 and mini6410 with the > approach suggested by Jingoo Han. As I do not have either of these > boards, an ack from you would be very helpful when the next version of > this patchset is posted. It's fine - I will review and probably test it.
diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-nuri.c index 7ac81ce..a7e6731 100644 --- a/arch/arm/mach-exynos/mach-nuri.c +++ b/arch/arm/mach-exynos/mach-nuri.c @@ -212,25 +212,29 @@ static struct platform_device nuri_gpio_keys = { /* Frame Buffer */ static struct s3c_fb_pd_win nuri_fb_win0 = { - .win_mode = { - .left_margin = 64, - .right_margin = 16, - .upper_margin = 64, - .lower_margin = 1, - .hsync_len = 48, - .vsync_len = 3, - .xres = 1024, - .yres = 600, - .refresh = 60, - }, .max_bpp = 24, .default_bpp = 16, + .xres = 1024, + .yres = 600, .virtual_x = 1024, .virtual_y = 2 * 600, }; +static struct fb_videomode nuri_lcd_timing = { + .left_margin = 64, + .right_margin = 16, + .upper_margin = 64, + .lower_margin = 1, + .hsync_len = 48, + .vsync_len = 3, + .xres = 1024, + .yres = 600, + .refresh = 60, +}; + static struct s3c_fb_platdata nuri_fb_pdata __initdata = { .win[0] = &nuri_fb_win0, + .vtiming = &nuri_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB | VIDCON0_CLKSEL_LCD, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c index 25c9b46..de71237 100644 --- a/arch/arm/mach-exynos/mach-origen.c +++ b/arch/arm/mach-exynos/mach-origen.c @@ -583,22 +583,26 @@ static struct platform_device origen_lcd_hv070wsa = { }; static struct s3c_fb_pd_win origen_fb_win0 = { - .win_mode = { - .left_margin = 64, - .right_margin = 16, - .upper_margin = 64, - .lower_margin = 16, - .hsync_len = 48, - .vsync_len = 3, - .xres = 1024, - .yres = 600, - }, + .xres = 1024, + .yres = 600, .max_bpp = 32, .default_bpp = 24, }; +static struct fb_videomode origen_lcd_timing = { + .left_margin = 64, + .right_margin = 16, + .upper_margin = 64, + .lower_margin = 16, + .hsync_len = 48, + .vsync_len = 3, + .xres = 1024, + .yres = 600, +}; + static struct s3c_fb_platdata origen_lcd_pdata __initdata = { .win[0] = &origen_fb_win0, + .vtiming = &origen_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC | VIDCON1_INV_VCLK, diff --git a/arch/arm/mach-exynos/mach-smdkv310.c b/arch/arm/mach-exynos/mach-smdkv310.c index f08529f..6c00fa6 100644 --- a/arch/arm/mach-exynos/mach-smdkv310.c +++ b/arch/arm/mach-exynos/mach-smdkv310.c @@ -157,22 +157,26 @@ static struct platform_device smdkv310_lcd_lte480wv = { }; static struct s3c_fb_pd_win smdkv310_fb_win0 = { - .win_mode = { - .left_margin = 13, - .right_margin = 8, - .upper_margin = 7, - .lower_margin = 5, - .hsync_len = 3, - .vsync_len = 1, - .xres = 800, - .yres = 480, - }, - .max_bpp = 32, - .default_bpp = 24, + .max_bpp = 32, + .default_bpp = 24, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smdkv310_lcd_timing = { + .left_margin = 13, + .right_margin = 8, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; static struct s3c_fb_platdata smdkv310_lcd0_pdata __initdata = { .win[0] = &smdkv310_fb_win0, + .vtiming = &smdkv310_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, .setup_gpio = exynos4_fimd0_gpio_setup_24bpp, diff --git a/arch/arm/mach-exynos/mach-universal_c210.c b/arch/arm/mach-exynos/mach-universal_c210.c index 57e4418..67a9547 100644 --- a/arch/arm/mach-exynos/mach-universal_c210.c +++ b/arch/arm/mach-exynos/mach-universal_c210.c @@ -813,25 +813,29 @@ static struct i2c_board_info i2c1_devs[] __initdata = { /* Frame Buffer */ static struct s3c_fb_pd_win universal_fb_win0 = { - .win_mode = { - .left_margin = 16, - .right_margin = 16, - .upper_margin = 2, - .lower_margin = 28, - .hsync_len = 2, - .vsync_len = 1, - .xres = 480, - .yres = 800, - .refresh = 55, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 480, + .yres = 800, .virtual_x = 480, .virtual_y = 2 * 800, }; +static struct fb_videomode universal_lcd_timing = { + .left_margin = 16, + .right_margin = 16, + .upper_margin = 2, + .lower_margin = 28, + .hsync_len = 2, + .vsync_len = 1, + .xres = 480, + .yres = 800, + .refresh = 55, +}; + static struct s3c_fb_platdata universal_lcd_pdata __initdata = { .win[0] = &universal_fb_win0, + .vtiming = &universal_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB | VIDCON0_CLKSEL_LCD, .vidcon1 = VIDCON1_INV_VCLK | VIDCON1_INV_VDEN diff --git a/arch/arm/mach-s3c24xx/mach-smdk2416.c b/arch/arm/mach-s3c24xx/mach-smdk2416.c index 30a44f8..c3100a0 100644 --- a/arch/arm/mach-s3c24xx/mach-smdk2416.c +++ b/arch/arm/mach-s3c24xx/mach-smdk2416.c @@ -148,23 +148,25 @@ static struct s3c24xx_hsudc_platdata smdk2416_hsudc_platdata = { static struct s3c_fb_pd_win smdk2416_fb_win[] = { [0] = { - /* think this is the same as the smdk6410 */ - .win_mode = { - .pixclock = 41094, - .left_margin = 8, - .right_margin = 13, - .upper_margin = 7, - .lower_margin = 5, - .hsync_len = 3, - .vsync_len = 1, - .xres = 800, - .yres = 480, - }, .default_bpp = 16, .max_bpp = 32, + .xres = 800, + .yres = 480, }, }; +static struct fb_videomode smdk2416_lcd_timing = { + .pixclock = 41094, + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, +}; + static void s3c2416_fb_gpio_setup_24bpp(void) { unsigned int gpio; @@ -187,6 +189,7 @@ static void s3c2416_fb_gpio_setup_24bpp(void) static struct s3c_fb_platdata smdk2416_fb_platdata = { .win[0] = &smdk2416_fb_win[0], + .vtiming = &smdk2416_lcd_timing, .setup_gpio = s3c2416_fb_gpio_setup_24bpp, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-s3c64xx/mach-anw6410.c b/arch/arm/mach-s3c64xx/mach-anw6410.c index b86f277..58fd0e3 100644 --- a/arch/arm/mach-s3c64xx/mach-anw6410.c +++ b/arch/arm/mach-s3c64xx/mach-anw6410.c @@ -134,24 +134,27 @@ static struct platform_device anw6410_lcd_powerdev = { }; static struct s3c_fb_pd_win anw6410_fb_win0 = { - /* this is to ensure we use win0 */ - .win_mode = { - .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, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode anw6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; /* 405566 clocks per frame => 60Hz refresh requires 24333960Hz clock */ static struct s3c_fb_platdata anw6410_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &anw6410_lcd_timing, .win[0] = &anw6410_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c b/arch/arm/mach-s3c64xx/mach-crag6410.c index e20bf58..c1ef57e 100644 --- a/arch/arm/mach-s3c64xx/mach-crag6410.c +++ b/arch/arm/mach-s3c64xx/mach-crag6410.c @@ -151,26 +151,29 @@ static struct platform_device crag6410_lcd_powerdev = { /* 640x480 URT */ static struct s3c_fb_pd_win crag6410_fb_win0 = { - /* this is to ensure we use win0 */ - .win_mode = { - .left_margin = 150, - .right_margin = 80, - .upper_margin = 40, - .lower_margin = 5, - .hsync_len = 40, - .vsync_len = 5, - .xres = 640, - .yres = 480, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 640, + .yres = 480, .virtual_y = 480 * 2, .virtual_x = 640, }; +static struct fb_videomode crag6410_lcd_timing = { + .left_margin = 150, + .right_margin = 80, + .upper_margin = 40, + .lower_margin = 5, + .hsync_len = 40, + .vsync_len = 5, + .xres = 640, + .yres = 480, +}; + /* 405566 clocks per frame => 60Hz refresh requires 24333960Hz clock */ static struct s3c_fb_platdata crag6410_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &crag6410_lcd_timing, .win[0] = &crag6410_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c index 521e07b..4e9b1ac 100644 --- a/arch/arm/mach-s3c64xx/mach-hmt.c +++ b/arch/arm/mach-s3c64xx/mach-hmt.c @@ -129,23 +129,27 @@ static struct platform_device hmt_backlight_device = { }; static struct s3c_fb_pd_win hmt_fb_win0 = { - .win_mode = { - .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, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode hmt_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; /* 405566 clocks per frame => 60Hz refresh requires 24333960Hz clock */ static struct s3c_fb_platdata hmt_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &hmt_lcd_timing, .win[0] = &hmt_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c index c34c2ab..24dcdc9 100644 --- a/arch/arm/mach-s3c64xx/mach-mini6410.c +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c @@ -153,36 +153,32 @@ static struct s3c2410_platform_nand mini6410_nand_info = { static struct s3c_fb_pd_win mini6410_fb_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, + .xres = 480, + .yres = 272, }, { - .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, + .xres = 800, + .yres = 480, }, }; +static struct fb_videomode mini6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, +}; + static struct s3c_fb_platdata mini6410_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &mini6410_lcd_timing, .win[0] = &mini6410_fb_win[0], .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, @@ -310,8 +306,8 @@ static void __init mini6410_machine_init(void) mini6410_lcd_pdata.win[0] = &mini6410_fb_win[features.lcd_index]; printk(KERN_INFO "MINI6410: selected LCD display is %dx%d\n", - mini6410_lcd_pdata.win[0]->win_mode.xres, - mini6410_lcd_pdata.win[0]->win_mode.yres); + mini6410_lcd_pdata.win[0]->xres, + mini6410_lcd_pdata.win[0]->yres); s3c_nand_set_platdata(&mini6410_nand_info); s3c_fb_set_platdata(&mini6410_lcd_pdata); diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c index be2a9a2..41e4f74 100644 --- a/arch/arm/mach-s3c64xx/mach-real6410.c +++ b/arch/arm/mach-s3c64xx/mach-real6410.c @@ -119,36 +119,32 @@ static struct platform_device real6410_device_eth = { static struct s3c_fb_pd_win real6410_fb_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, + .xres = 480, + .yres = 272, }, { - .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, + .xres = 800, + .yres = 480, }, }; +static struct fb_videomode real6410_lcd_timing = { + .left_margin = 3, + .right_margin = 2, + .upper_margin = 1, + .lower_margin = 1, + .hsync_len = 40, + .vsync_len = 1, + .xres = 800, + .yres = 480, +}; + static struct s3c_fb_platdata real6410_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &real6410_lcd_timing, .win[0] = &real6410_fb_win[0], .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, @@ -291,8 +287,8 @@ static void __init real6410_machine_init(void) real6410_lcd_pdata.win[0] = &real6410_fb_win[features.lcd_index]; printk(KERN_INFO "REAL6410: selected LCD display is %dx%d\n", - real6410_lcd_pdata.win[0]->win_mode.xres, - real6410_lcd_pdata.win[0]->win_mode.yres); + real6410_lcd_pdata.win[0]->xres, + real6410_lcd_pdata.win[0]->yres); s3c_fb_set_platdata(&real6410_lcd_pdata); s3c_nand_set_platdata(&real6410_nand_info); diff --git a/arch/arm/mach-s3c64xx/mach-smartq5.c b/arch/arm/mach-s3c64xx/mach-smartq5.c index 3f42431..03a2f88 100644 --- a/arch/arm/mach-s3c64xx/mach-smartq5.c +++ b/arch/arm/mach-s3c64xx/mach-smartq5.c @@ -108,23 +108,27 @@ static struct platform_device smartq5_buttons_device = { }; static struct s3c_fb_pd_win smartq5_fb_win0 = { - .win_mode = { - .left_margin = 216, - .right_margin = 40, - .upper_margin = 35, - .lower_margin = 10, - .hsync_len = 1, - .vsync_len = 1, - .xres = 800, - .yres = 480, - .refresh = 80, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smartq5_lcd_timing = { + .left_margin = 216, + .right_margin = 40, + .upper_margin = 35, + .lower_margin = 10, + .hsync_len = 1, + .vsync_len = 1, + .xres = 800, + .yres = 480, + .refresh = 80, }; static struct s3c_fb_platdata smartq5_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &smartq5_lcd_timing, .win[0] = &smartq5_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC | diff --git a/arch/arm/mach-s3c64xx/mach-smartq7.c b/arch/arm/mach-s3c64xx/mach-smartq7.c index e5c09b6..4e3b038 100644 --- a/arch/arm/mach-s3c64xx/mach-smartq7.c +++ b/arch/arm/mach-s3c64xx/mach-smartq7.c @@ -124,23 +124,27 @@ static struct platform_device smartq7_buttons_device = { }; static struct s3c_fb_pd_win smartq7_fb_win0 = { - .win_mode = { - .left_margin = 3, - .right_margin = 5, - .upper_margin = 1, - .lower_margin = 20, - .hsync_len = 10, - .vsync_len = 3, - .xres = 800, - .yres = 480, - .refresh = 80, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smartq7_lcd_timing = { + .left_margin = 3, + .right_margin = 5, + .upper_margin = 1, + .lower_margin = 20, + .hsync_len = 10, + .vsync_len = 3, + .xres = 800, + .yres = 480, + .refresh = 80, }; static struct s3c_fb_platdata smartq7_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &smartq7_lcd_timing, .win[0] = &smartq7_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC | diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c b/arch/arm/mach-s3c64xx/mach-smdk6410.c index d55bc96..3cfc90f 100644 --- a/arch/arm/mach-s3c64xx/mach-smdk6410.c +++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c @@ -146,26 +146,29 @@ static struct platform_device smdk6410_lcd_powerdev = { }; static struct s3c_fb_pd_win smdk6410_fb_win0 = { - /* this is to ensure we use win0 */ - .win_mode = { - .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, + .xres = 800, + .yres = 480, .virtual_y = 480 * 2, .virtual_x = 800, }; +static struct fb_videomode smdk6410_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, +}; + /* 405566 clocks per frame => 60Hz refresh requires 24333960Hz clock */ static struct s3c_fb_platdata smdk6410_lcd_pdata __initdata = { .setup_gpio = s3c64xx_fb_gpio_setup_24bpp, + .vtiming = &smdk6410_lcd_timing, .win[0] = &smdk6410_fb_win0, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, diff --git a/arch/arm/mach-s5p64x0/mach-smdk6440.c b/arch/arm/mach-s5p64x0/mach-smdk6440.c index a40e325..92fefad 100644 --- a/arch/arm/mach-s5p64x0/mach-smdk6440.c +++ b/arch/arm/mach-s5p64x0/mach-smdk6440.c @@ -103,22 +103,26 @@ static struct s3c2410_uartcfg smdk6440_uartcfgs[] __initdata = { /* Frame Buffer */ static struct s3c_fb_pd_win smdk6440_fb_win0 = { - .win_mode = { - .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 = 24, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smdk6440_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; static struct s3c_fb_platdata smdk6440_lcd_pdata __initdata = { .win[0] = &smdk6440_fb_win0, + .vtiming = &smdk6440_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, .setup_gpio = s5p64x0_fb_gpio_setup_24bpp, diff --git a/arch/arm/mach-s5p64x0/mach-smdk6450.c b/arch/arm/mach-s5p64x0/mach-smdk6450.c index efb69e2..e2335ec 100644 --- a/arch/arm/mach-s5p64x0/mach-smdk6450.c +++ b/arch/arm/mach-s5p64x0/mach-smdk6450.c @@ -121,22 +121,26 @@ static struct s3c2410_uartcfg smdk6450_uartcfgs[] __initdata = { /* Frame Buffer */ static struct s3c_fb_pd_win smdk6450_fb_win0 = { - .win_mode = { - .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 = 24, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smdk6450_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; static struct s3c_fb_platdata smdk6450_lcd_pdata __initdata = { .win[0] = &smdk6450_fb_win0, + .vtiming = &smdk6450_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, .setup_gpio = s5p64x0_fb_gpio_setup_24bpp, diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c b/arch/arm/mach-s5pc100/mach-smdkc100.c index 674d229..0c3ae38 100644 --- a/arch/arm/mach-s5pc100/mach-smdkc100.c +++ b/arch/arm/mach-s5pc100/mach-smdkc100.c @@ -136,24 +136,27 @@ static struct platform_device smdkc100_lcd_powerdev = { /* Frame Buffer */ static struct s3c_fb_pd_win smdkc100_fb_win0 = { - /* this is to ensure we use win0 */ - .win_mode = { - .left_margin = 8, - .right_margin = 13, - .upper_margin = 7, - .lower_margin = 5, - .hsync_len = 3, - .vsync_len = 1, - .xres = 800, - .yres = 480, - .refresh = 80, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smdkc100_lcd_timing = { + .left_margin = 8, + .right_margin = 13, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, + .refresh = 80, }; static struct s3c_fb_platdata smdkc100_lcd_pdata __initdata = { .win[0] = &smdkc100_fb_win0, + .vtiming = &smdkc100_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, .setup_gpio = s5pc100_fb_gpio_setup_24bpp, diff --git a/arch/arm/mach-s5pv210/mach-aquila.c b/arch/arm/mach-s5pv210/mach-aquila.c index a9ea64e..7b91bbf 100644 --- a/arch/arm/mach-s5pv210/mach-aquila.c +++ b/arch/arm/mach-s5pv210/mach-aquila.c @@ -96,38 +96,34 @@ static struct s3c2410_uartcfg aquila_uartcfgs[] __initdata = { /* Frame Buffer */ static struct s3c_fb_pd_win aquila_fb_win0 = { - .win_mode = { - .left_margin = 16, - .right_margin = 16, - .upper_margin = 3, - .lower_margin = 28, - .hsync_len = 2, - .vsync_len = 2, - .xres = 480, - .yres = 800, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 480, + .yres = 800, }; static struct s3c_fb_pd_win aquila_fb_win1 = { - .win_mode = { - .left_margin = 16, - .right_margin = 16, - .upper_margin = 3, - .lower_margin = 28, - .hsync_len = 2, - .vsync_len = 2, - .xres = 480, - .yres = 800, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 480, + .yres = 800, +}; + +static struct fb_videomode aquila_lcd_timing = { + .left_margin = 16, + .right_margin = 16, + .upper_margin = 3, + .lower_margin = 28, + .hsync_len = 2, + .vsync_len = 2, + .xres = 480, + .yres = 800, }; static struct s3c_fb_platdata aquila_lcd_pdata __initdata = { .win[0] = &aquila_fb_win0, .win[1] = &aquila_fb_win1, + .vtiming = &aquila_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC | VIDCON1_INV_VCLK | VIDCON1_INV_VDEN, diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-s5pv210/mach-goni.c index 2cf5ed7..07a840d 100644 --- a/arch/arm/mach-s5pv210/mach-goni.c +++ b/arch/arm/mach-s5pv210/mach-goni.c @@ -105,25 +105,29 @@ static struct s3c2410_uartcfg goni_uartcfgs[] __initdata = { /* Frame Buffer */ static struct s3c_fb_pd_win goni_fb_win0 = { - .win_mode = { - .left_margin = 16, - .right_margin = 16, - .upper_margin = 2, - .lower_margin = 28, - .hsync_len = 2, - .vsync_len = 1, - .xres = 480, - .yres = 800, - .refresh = 55, - }, .max_bpp = 32, .default_bpp = 16, + .xres = 480, + .yres = 800, .virtual_x = 480, .virtual_y = 2 * 800, }; +static struct fb_videomode goni_lcd_timing = { + .left_margin = 16, + .right_margin = 16, + .upper_margin = 2, + .lower_margin = 28, + .hsync_len = 2, + .vsync_len = 1, + .xres = 480, + .yres = 800, + .refresh = 55, +}; + static struct s3c_fb_platdata goni_lcd_pdata __initdata = { .win[0] = &goni_fb_win0, + .vtiming = &goni_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB | VIDCON0_CLKSEL_LCD, .vidcon1 = VIDCON1_INV_VCLK | VIDCON1_INV_VDEN diff --git a/arch/arm/mach-s5pv210/mach-smdkv210.c b/arch/arm/mach-s5pv210/mach-smdkv210.c index 91d4ad8..5e0c955 100644 --- a/arch/arm/mach-s5pv210/mach-smdkv210.c +++ b/arch/arm/mach-s5pv210/mach-smdkv210.c @@ -189,22 +189,26 @@ static struct platform_device smdkv210_lcd_lte480wv = { }; static struct s3c_fb_pd_win smdkv210_fb_win0 = { - .win_mode = { - .left_margin = 13, - .right_margin = 8, - .upper_margin = 7, - .lower_margin = 5, - .hsync_len = 3, - .vsync_len = 1, - .xres = 800, - .yres = 480, - }, .max_bpp = 32, .default_bpp = 24, + .xres = 800, + .yres = 480, +}; + +static struct fb_videomode smdkv210_lcd_timing = { + .left_margin = 13, + .right_margin = 8, + .upper_margin = 7, + .lower_margin = 5, + .hsync_len = 3, + .vsync_len = 1, + .xres = 800, + .yres = 480, }; static struct s3c_fb_platdata smdkv210_lcd0_pdata __initdata = { .win[0] = &smdkv210_fb_win0, + .vtiming = &smdkv210_lcd_timing, .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB, .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC, .setup_gpio = s5pv210_fb_gpio_setup_24bpp,
For all the Samsung SoC based boards which have the platform data for s3c-fb driver, the 'default_win' element in the platform data is removed and the lcd panel video timing values are moved out of individual window configuration data. Cc: Jingoo Han <jg1.han@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: JeongHyeon Kim <jhkim@insignal.co.kr> Cc: Kukjin Kim <kgene.kim@samsung.com> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Ben Dooks <ben-linux@fluff.org> Cc: Kwangwoo Lee <kwangwoo.lee@gmail.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: Peter Korsgaard <jacmet@sunsite.dk> Cc: Darius Augulis <augulis.darius@gmail.com> Cc: Maurus Cuelenaere <mcuelenaere@gmail.com> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- arch/arm/mach-exynos/mach-nuri.c | 26 ++++++++++------- arch/arm/mach-exynos/mach-origen.c | 24 ++++++++++------- arch/arm/mach-exynos/mach-smdkv310.c | 28 +++++++++++-------- arch/arm/mach-exynos/mach-universal_c210.c | 26 ++++++++++------- arch/arm/mach-s3c24xx/mach-smdk2416.c | 27 ++++++++++-------- arch/arm/mach-s3c64xx/mach-anw6410.c | 25 ++++++++++------- arch/arm/mach-s3c64xx/mach-crag6410.c | 25 ++++++++++------- arch/arm/mach-s3c64xx/mach-hmt.c | 24 ++++++++++------- arch/arm/mach-s3c64xx/mach-mini6410.c | 40 ++++++++++++--------------- arch/arm/mach-s3c64xx/mach-real6410.c | 40 ++++++++++++--------------- arch/arm/mach-s3c64xx/mach-smartq5.c | 26 ++++++++++------- arch/arm/mach-s3c64xx/mach-smartq7.c | 26 ++++++++++------- arch/arm/mach-s3c64xx/mach-smdk6410.c | 25 ++++++++++------- arch/arm/mach-s5p64x0/mach-smdk6440.c | 24 ++++++++++------- arch/arm/mach-s5p64x0/mach-smdk6450.c | 24 ++++++++++------- arch/arm/mach-s5pc100/mach-smdkc100.c | 27 ++++++++++-------- arch/arm/mach-s5pv210/mach-aquila.c | 36 +++++++++++-------------- arch/arm/mach-s5pv210/mach-goni.c | 26 ++++++++++------- arch/arm/mach-s5pv210/mach-smdkv210.c | 24 ++++++++++------- 19 files changed, 285 insertions(+), 238 deletions(-)