[PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

This patchset rearranges the elements in the platform data of the s3c-fb
driver with the intent of adding device tree support to the driver in
subsequent patches.

The first patch moves the video timing information from the individual window
setup data into the platform specific configuration section in the platform
data. The video timing is independent of the window setup. The resolution of
the window could be smaller than that of the lcd panel attached. So the video
timing data is removed from window configuration data.

The second patch removes the need for the 'default_win' element in the
platform data. This element was used to decide whether the video data
output from the controller should be enabled or disabled when the window
specified by 'default_win' is enabled or disabled. With the first patch
removing the need for atleast one window to be of the same resolution as
that of the lcd panel, it is now possible to decide when to enable/disable
the video data output based on the state of each window. If any of the
window is active, the lcd data output is enabled. Otherwise, the lcd data
output is disabled. Hence, the 'default_win' parameter from the platform
data can be removed, which anyways cannot be specified when using
device tree.

The third patch reworks the platform data of the display controller based
on the changes introduced in the first two patches.

The patchset does not include the rework of platform data of all the other
boards which have platform data for the s3c-fb driver. I am posting this
patchset to know if the the changes are conceptually correct. If the first
two patches are acceptable, I will resubmit this patchset with changes to
the platform data for all boards which have s3c-fb platform data.

Thomas Abraham (3):
  video: s3c-fb: move video interface timing out of window setup data
  video: s3c-fb: remove 'default_win' element from platform data
  ARM: Exynos: Rework platform data for lcd controller for Origen board

 arch/arm/mach-exynos/mach-origen.c      |   24 ++++---
 arch/arm/plat-samsung/include/plat/fb.h |   11 ++-
 drivers/video/s3c-fb.c                  |  128 ++++++++++++++----------------
 3 files changed, 80 insertions(+), 83 deletions(-)

[PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

The video interface timing is independent of the window setup data.
The resolution of the window can be smaller than that of the lcd
panel to which the video data is output.

So move the video timing data from the per-window setup data to the
platform specific section in the platform data. This also removes
the restriction that atleast one window should have the same
resolution as that of the panel attached.

Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
 drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
 2 files changed, 63 insertions(+), 52 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
index 0fedf47..39d6bd7 100644
--- a/arch/arm/plat-samsung/include/plat/fb.h
+++ b/arch/arm/plat-samsung/include/plat/fb.h
@@ -24,15 +24,16 @@
 
 /**
  * struct s3c_fb_pd_win - per window setup data
- * @win_mode: The display parameters to initialise (not for window 0)
+ * @xres     : The window X size.
+ * @yres     : The window Y size.
  * @virtual_x: The virtual X size.
  * @virtual_y: The virtual Y size.
  */
 struct s3c_fb_pd_win {
-	struct fb_videomode	win_mode;
-
 	unsigned short		default_bpp;
 	unsigned short		max_bpp;
+	unsigned short		xres;
+	unsigned short		yres;
 	unsigned short		virtual_x;
 	unsigned short		virtual_y;
 };
@@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
  * @default_win: default window layer number to be used for UI layer.
  * @vidcon0: The base vidcon0 values to control the panel data format.
  * @vidcon1: The base vidcon1 values to control the panel data output.
+ * @vtiming: Video timing when connected to a RGB type panel.
  * @win: The setup data for each hardware window, or NULL for unused.
  * @display_mode: The LCD output display mode.
  *
@@ -58,6 +60,7 @@ struct s3c_fb_platdata {
 	void	(*setup_gpio)(void);
 
 	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
+	struct fb_videomode     *vtiming;
 
 	u32			 default_win;
 
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 1fb7ddf..8e05d4d 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
 	u32 alpha = 0;
 	u32 data;
 	u32 pagewidth;
-	int clkdiv;
 
 	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
 
@@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 
-	/* use platform specified window as the basis for the lcd timings */
-
-	if (win_no = sfb->pdata->default_win) {
-		clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
-
-		data = sfb->pdata->vidcon0;
-		data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
-
-		if (clkdiv > 1)
-			data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
-		else
-			data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
-
-		/* write the timing data to the panel */
-
-		if (sfb->variant.is_2443)
-			data |= (1 << 5);
-
-		writel(data, regs + VIDCON0);
-
+	if (win_no = sfb->pdata->default_win)
 		s3c_fb_enable(sfb, 1);
 
-		data = VIDTCON0_VBPD(var->upper_margin - 1) |
-		       VIDTCON0_VFPD(var->lower_margin - 1) |
-		       VIDTCON0_VSPW(var->vsync_len - 1);
-
-		writel(data, regs + sfb->variant.vidtcon);
-
-		data = VIDTCON1_HBPD(var->left_margin - 1) |
-		       VIDTCON1_HFPD(var->right_margin - 1) |
-		       VIDTCON1_HSPW(var->hsync_len - 1);
-
-		/* VIDTCON1 */
-		writel(data, regs + sfb->variant.vidtcon + 4);
-
-		data = VIDTCON2_LINEVAL(var->yres - 1) |
-		       VIDTCON2_HOZVAL(var->xres - 1);
-		writel(data, regs + sfb->variant.vidtcon + 8);
-	}
-
 	/* write the buffer address */
 
 	/* start and end registers stride is 8 */
@@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
 
 	dev_dbg(sfb->dev, "allocating memory for display\n");
 
-	real_size = windata->win_mode.xres * windata->win_mode.yres;
+	real_size = windata->xres * windata->yres;
 	virt_size = windata->virtual_x * windata->virtual_y;
 
 	dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
-		real_size, windata->win_mode.xres, windata->win_mode.yres,
+		real_size, windata->xres, windata->yres,
 		virt_size, windata->virtual_x, windata->virtual_y);
 
 	size = (real_size > virt_size) ? real_size : virt_size;
@@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 				      struct s3c_fb_win **res)
 {
 	struct fb_var_screeninfo *var;
-	struct fb_videomode *initmode;
+	struct fb_videomode initmode;
 	struct s3c_fb_pd_win *windata;
 	struct s3c_fb_win *win;
 	struct fb_info *fbinfo;
@@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	}
 
 	windata = sfb->pdata->win[win_no];
-	initmode = &windata->win_mode;
+	initmode = *sfb->pdata->vtiming;
 
 	WARN_ON(windata->max_bpp = 0);
-	WARN_ON(windata->win_mode.xres = 0);
-	WARN_ON(windata->win_mode.yres = 0);
+	WARN_ON(windata->xres = 0);
+	WARN_ON(windata->yres = 0);
 
 	win = fbinfo->par;
 	*res = win;
@@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	}
 
 	/* setup the initial video mode from the window */
-	fb_videomode_to_var(&fbinfo->var, initmode);
+	initmode.xres = windata->xres;
+	initmode.yres = windata->yres;
+	fb_videomode_to_var(&fbinfo->var, &initmode);
 
 	fbinfo->fix.type	= FB_TYPE_PACKED_PIXELS;
 	fbinfo->fix.accel	= FB_ACCEL_NONE;
@@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 }
 
 /**
+ * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
+ * @sfb: The base resources for the hardware.
+ *
+ * Set horizontal and vertical lcd rgb interface timing.
+ */
+static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
+{
+	struct fb_videomode *vmode = sfb->pdata->vtiming;
+	void __iomem *regs = sfb->regs;
+	int clkdiv;
+	u32 data;
+
+	if (!vmode->pixclock)
+		s3c_fb_missing_pixclock(vmode);
+
+	clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
+
+	data = sfb->pdata->vidcon0;
+	data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
+
+	if (clkdiv > 1)
+		data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
+	else
+		data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
+
+	if (sfb->variant.is_2443)
+		data |= (1 << 5);
+	writel(data, regs + VIDCON0);
+
+	data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
+	       VIDTCON0_VFPD(vmode->lower_margin - 1) |
+	       VIDTCON0_VSPW(vmode->vsync_len - 1);
+	writel(data, regs + sfb->variant.vidtcon);
+
+	data = VIDTCON1_HBPD(vmode->left_margin - 1) |
+	       VIDTCON1_HFPD(vmode->right_margin - 1) |
+	       VIDTCON1_HSPW(vmode->hsync_len - 1);
+	writel(data, regs + sfb->variant.vidtcon + 4);
+
+	data = VIDTCON2_LINEVAL(vmode->yres - 1) |
+	       VIDTCON2_HOZVAL(vmode->xres - 1);
+	writel(data, regs + sfb->variant.vidtcon + 8);
+}
+
+/**
  * s3c_fb_clear_win() - clear hardware window registers.
  * @sfb: The base resources for the hardware.
  * @win: The window to process.
@@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 		writel(0xffffff, regs + WKEYCON1);
 	}
 
+	s3c_fb_set_rgb_timing(sfb);
+
 	/* we have the register setup, start allocating framebuffers */
 
 	for (win = 0; win < fbdrv->variant.nr_windows; win++) {
 		if (!pd->win[win])
 			continue;
 
-		if (!pd->win[win]->win_mode.pixclock)
-			s3c_fb_missing_pixclock(&pd->win[win]->win_mode);
-
 		ret = s3c_fb_probe_win(sfb, win, fbdrv->win[win],
 				       &sfb->windows[win]);
 		if (ret < 0) {
-- 
1.6.6.rc2

[PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Thomas Abraham]

The decision to enable or disable the data output to the lcd panel from
the controller need not be based on the value of 'default_win' element
in the platform data. Instead, the data output to the panel is enabled
if any of the windows are active, else data output is disabled.

Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/include/plat/fb.h |    2 --
 drivers/video/s3c-fb.c                  |   24 ++++--------------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
index 39d6bd7..536002f 100644
--- a/arch/arm/plat-samsung/include/plat/fb.h
+++ b/arch/arm/plat-samsung/include/plat/fb.h
@@ -62,8 +62,6 @@ struct s3c_fb_platdata {
 	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
 	struct fb_videomode     *vtiming;
 
-	u32			 default_win;
-
 	u32			 vidcon0;
 	u32			 vidcon1;
 };
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 8e05d4d..8baba31 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 
-	if (win_no = sfb->pdata->default_win)
+	if (!sfb->output_on)
 		s3c_fb_enable(sfb, 1);
 
 	/* write the buffer address */
@@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	struct s3c_fb_win *win = info->par;
 	struct s3c_fb *sfb = win->parent;
 	unsigned int index = win->index;
-	u32 wincon;
+	u32 wincon, output_on = sfb->output_on;
 
 	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
 
@@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	 * it is highly likely that we also do not need to output
 	 * anything.
 	 */
-
-	/* We could do something like the following code, but the current
-	 * system of using framebuffer events means that we cannot make
-	 * the distinction between just window 0 being inactive and all
-	 * the windows being down.
-	 *
-	 * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
-	*/
-
-	/* we're stuck with this until we can do something about overriding
-	 * the power control using the blanking event for a single fb.
-	 */
-	if (index = sfb->pdata->default_win) {
-		shadow_protect_win(win, 1);
-		s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
-		shadow_protect_win(win, 0);
-	}
+	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
 
 	pm_runtime_put_sync(sfb->dev);
 
-	return 0;
+	return output_on = sfb->output_on;
 }
 
 /**
-- 
1.6.6.rc2

[PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board

[Thomas Abraham]

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: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos/mach-origen.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
index f57aed4..dc4ecc3 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			= 512,
+	.yres			= 300,
 	.max_bpp		= 32,
 	.default_bpp		= 24,
 };
 
+static struct fb_videomode lcd_hv070wsa_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	= &lcd_hv070wsa_timing,
 	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
 	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC |
 				VIDCON1_INV_VCLK,
-- 
1.6.6.rc2

[PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

This patchset rearranges the elements in the platform data of the s3c-fb
driver with the intent of adding device tree support to the driver in
subsequent patches.

The first patch moves the video timing information from the individual window
setup data into the platform specific configuration section in the platform
data. The video timing is independent of the window setup. The resolution of
the window could be smaller than that of the lcd panel attached. So the video
timing data is removed from window configuration data.

The second patch removes the need for the 'default_win' element in the
platform data. This element was used to decide whether the video data
output from the controller should be enabled or disabled when the window
specified by 'default_win' is enabled or disabled. With the first patch
removing the need for atleast one window to be of the same resolution as
that of the lcd panel, it is now possible to decide when to enable/disable
the video data output based on the state of each window. If any of the
window is active, the lcd data output is enabled. Otherwise, the lcd data
output is disabled. Hence, the 'default_win' parameter from the platform
data can be removed, which anyways cannot be specified when using
device tree.

The third patch reworks the platform data of the display controller based
on the changes introduced in the first two patches.

The patchset does not include the rework of platform data of all the other
boards which have platform data for the s3c-fb driver. I am posting this
patchset to know if the the changes are conceptually correct. If the first
two patches are acceptable, I will resubmit this patchset with changes to
the platform data for all boards which have s3c-fb platform data.

Thomas Abraham (3):
  video: s3c-fb: move video interface timing out of window setup data
  video: s3c-fb: remove 'default_win' element from platform data
  ARM: Exynos: Rework platform data for lcd controller for Origen board

 arch/arm/mach-exynos/mach-origen.c      |   24 ++++---
 arch/arm/plat-samsung/include/plat/fb.h |   11 ++-
 drivers/video/s3c-fb.c                  |  128 ++++++++++++++----------------
 3 files changed, 80 insertions(+), 83 deletions(-)

[PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

The video interface timing is independent of the window setup data.
The resolution of the window can be smaller than that of the lcd
panel to which the video data is output.

So move the video timing data from the per-window setup data to the
platform specific section in the platform data. This also removes
the restriction that atleast one window should have the same
resolution as that of the panel attached.

Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
 drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
 2 files changed, 63 insertions(+), 52 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
index 0fedf47..39d6bd7 100644
--- a/arch/arm/plat-samsung/include/plat/fb.h
+++ b/arch/arm/plat-samsung/include/plat/fb.h
@@ -24,15 +24,16 @@
 
 /**
  * struct s3c_fb_pd_win - per window setup data
- * @win_mode: The display parameters to initialise (not for window 0)
+ * @xres     : The window X size.
+ * @yres     : The window Y size.
  * @virtual_x: The virtual X size.
  * @virtual_y: The virtual Y size.
  */
 struct s3c_fb_pd_win {
-	struct fb_videomode	win_mode;
-
 	unsigned short		default_bpp;
 	unsigned short		max_bpp;
+	unsigned short		xres;
+	unsigned short		yres;
 	unsigned short		virtual_x;
 	unsigned short		virtual_y;
 };
@@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
  * @default_win: default window layer number to be used for UI layer.
  * @vidcon0: The base vidcon0 values to control the panel data format.
  * @vidcon1: The base vidcon1 values to control the panel data output.
+ * @vtiming: Video timing when connected to a RGB type panel.
  * @win: The setup data for each hardware window, or NULL for unused.
  * @display_mode: The LCD output display mode.
  *
@@ -58,6 +60,7 @@ struct s3c_fb_platdata {
 	void	(*setup_gpio)(void);
 
 	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
+	struct fb_videomode     *vtiming;
 
 	u32			 default_win;
 
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 1fb7ddf..8e05d4d 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
 	u32 alpha = 0;
 	u32 data;
 	u32 pagewidth;
-	int clkdiv;
 
 	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
 
@@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 
-	/* use platform specified window as the basis for the lcd timings */
-
-	if (win_no == sfb->pdata->default_win) {
-		clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
-
-		data = sfb->pdata->vidcon0;
-		data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
-
-		if (clkdiv > 1)
-			data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
-		else
-			data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
-
-		/* write the timing data to the panel */
-
-		if (sfb->variant.is_2443)
-			data |= (1 << 5);
-
-		writel(data, regs + VIDCON0);
-
+	if (win_no == sfb->pdata->default_win)
 		s3c_fb_enable(sfb, 1);
 
-		data = VIDTCON0_VBPD(var->upper_margin - 1) |
-		       VIDTCON0_VFPD(var->lower_margin - 1) |
-		       VIDTCON0_VSPW(var->vsync_len - 1);
-
-		writel(data, regs + sfb->variant.vidtcon);
-
-		data = VIDTCON1_HBPD(var->left_margin - 1) |
-		       VIDTCON1_HFPD(var->right_margin - 1) |
-		       VIDTCON1_HSPW(var->hsync_len - 1);
-
-		/* VIDTCON1 */
-		writel(data, regs + sfb->variant.vidtcon + 4);
-
-		data = VIDTCON2_LINEVAL(var->yres - 1) |
-		       VIDTCON2_HOZVAL(var->xres - 1);
-		writel(data, regs + sfb->variant.vidtcon + 8);
-	}
-
 	/* write the buffer address */
 
 	/* start and end registers stride is 8 */
@@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
 
 	dev_dbg(sfb->dev, "allocating memory for display\n");
 
-	real_size = windata->win_mode.xres * windata->win_mode.yres;
+	real_size = windata->xres * windata->yres;
 	virt_size = windata->virtual_x * windata->virtual_y;
 
 	dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
-		real_size, windata->win_mode.xres, windata->win_mode.yres,
+		real_size, windata->xres, windata->yres,
 		virt_size, windata->virtual_x, windata->virtual_y);
 
 	size = (real_size > virt_size) ? real_size : virt_size;
@@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 				      struct s3c_fb_win **res)
 {
 	struct fb_var_screeninfo *var;
-	struct fb_videomode *initmode;
+	struct fb_videomode initmode;
 	struct s3c_fb_pd_win *windata;
 	struct s3c_fb_win *win;
 	struct fb_info *fbinfo;
@@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	}
 
 	windata = sfb->pdata->win[win_no];
-	initmode = &windata->win_mode;
+	initmode = *sfb->pdata->vtiming;
 
 	WARN_ON(windata->max_bpp == 0);
-	WARN_ON(windata->win_mode.xres == 0);
-	WARN_ON(windata->win_mode.yres == 0);
+	WARN_ON(windata->xres == 0);
+	WARN_ON(windata->yres == 0);
 
 	win = fbinfo->par;
 	*res = win;
@@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	}
 
 	/* setup the initial video mode from the window */
-	fb_videomode_to_var(&fbinfo->var, initmode);
+	initmode.xres = windata->xres;
+	initmode.yres = windata->yres;
+	fb_videomode_to_var(&fbinfo->var, &initmode);
 
 	fbinfo->fix.type	= FB_TYPE_PACKED_PIXELS;
 	fbinfo->fix.accel	= FB_ACCEL_NONE;
@@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 }
 
 /**
+ * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
+ * @sfb: The base resources for the hardware.
+ *
+ * Set horizontal and vertical lcd rgb interface timing.
+ */
+static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
+{
+	struct fb_videomode *vmode = sfb->pdata->vtiming;
+	void __iomem *regs = sfb->regs;
+	int clkdiv;
+	u32 data;
+
+	if (!vmode->pixclock)
+		s3c_fb_missing_pixclock(vmode);
+
+	clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
+
+	data = sfb->pdata->vidcon0;
+	data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
+
+	if (clkdiv > 1)
+		data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
+	else
+		data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
+
+	if (sfb->variant.is_2443)
+		data |= (1 << 5);
+	writel(data, regs + VIDCON0);
+
+	data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
+	       VIDTCON0_VFPD(vmode->lower_margin - 1) |
+	       VIDTCON0_VSPW(vmode->vsync_len - 1);
+	writel(data, regs + sfb->variant.vidtcon);
+
+	data = VIDTCON1_HBPD(vmode->left_margin - 1) |
+	       VIDTCON1_HFPD(vmode->right_margin - 1) |
+	       VIDTCON1_HSPW(vmode->hsync_len - 1);
+	writel(data, regs + sfb->variant.vidtcon + 4);
+
+	data = VIDTCON2_LINEVAL(vmode->yres - 1) |
+	       VIDTCON2_HOZVAL(vmode->xres - 1);
+	writel(data, regs + sfb->variant.vidtcon + 8);
+}
+
+/**
  * s3c_fb_clear_win() - clear hardware window registers.
  * @sfb: The base resources for the hardware.
  * @win: The window to process.
@@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 		writel(0xffffff, regs + WKEYCON1);
 	}
 
+	s3c_fb_set_rgb_timing(sfb);
+
 	/* we have the register setup, start allocating framebuffers */
 
 	for (win = 0; win < fbdrv->variant.nr_windows; win++) {
 		if (!pd->win[win])
 			continue;
 
-		if (!pd->win[win]->win_mode.pixclock)
-			s3c_fb_missing_pixclock(&pd->win[win]->win_mode);
-
 		ret = s3c_fb_probe_win(sfb, win, fbdrv->win[win],
 				       &sfb->windows[win]);
 		if (ret < 0) {
-- 
1.6.6.rc2

[PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Thomas Abraham]

The decision to enable or disable the data output to the lcd panel from
the controller need not be based on the value of 'default_win' element
in the platform data. Instead, the data output to the panel is enabled
if any of the windows are active, else data output is disabled.

Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/include/plat/fb.h |    2 --
 drivers/video/s3c-fb.c                  |   24 ++++--------------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
index 39d6bd7..536002f 100644
--- a/arch/arm/plat-samsung/include/plat/fb.h
+++ b/arch/arm/plat-samsung/include/plat/fb.h
@@ -62,8 +62,6 @@ struct s3c_fb_platdata {
 	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
 	struct fb_videomode     *vtiming;
 
-	u32			 default_win;
-
 	u32			 vidcon0;
 	u32			 vidcon1;
 };
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 8e05d4d..8baba31 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 
-	if (win_no == sfb->pdata->default_win)
+	if (!sfb->output_on)
 		s3c_fb_enable(sfb, 1);
 
 	/* write the buffer address */
@@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	struct s3c_fb_win *win = info->par;
 	struct s3c_fb *sfb = win->parent;
 	unsigned int index = win->index;
-	u32 wincon;
+	u32 wincon, output_on = sfb->output_on;
 
 	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
 
@@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	 * it is highly likely that we also do not need to output
 	 * anything.
 	 */
-
-	/* We could do something like the following code, but the current
-	 * system of using framebuffer events means that we cannot make
-	 * the distinction between just window 0 being inactive and all
-	 * the windows being down.
-	 *
-	 * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
-	*/
-
-	/* we're stuck with this until we can do something about overriding
-	 * the power control using the blanking event for a single fb.
-	 */
-	if (index == sfb->pdata->default_win) {
-		shadow_protect_win(win, 1);
-		s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
-		shadow_protect_win(win, 0);
-	}
+	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
 
 	pm_runtime_put_sync(sfb->dev);
 
-	return 0;
+	return output_on == sfb->output_on;
 }
 
 /**
-- 
1.6.6.rc2

[PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board

[Thomas Abraham]

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: Ben Dooks <ben-linux@fluff.org>
Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos/mach-origen.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
index f57aed4..dc4ecc3 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			= 512,
+	.yres			= 300,
 	.max_bpp		= 32,
 	.default_bpp		= 24,
 };
 
+static struct fb_videomode lcd_hv070wsa_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	= &lcd_hv070wsa_timing,
 	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
 	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC |
 				VIDCON1_INV_VCLK,
-- 
1.6.6.rc2

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Mark Brown]

On Sat, Mar 03, 2012 at 09:20:05PM +0530, Thomas Abraham wrote:
> This patchset rearranges the elements in the platform data of the s3c-fb
> driver with the intent of adding device tree support to the driver in
> subsequent patches.

It seems somewhat surprising that none of these updates touch any
platform data for any existing machines - are no machines using any of
the features being updated?

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Mark Brown]

On Sat, Mar 03, 2012 at 09:20:05PM +0530, Thomas Abraham wrote:
> This patchset rearranges the elements in the platform data of the s3c-fb
> driver with the intent of adding device tree support to the driver in
> subsequent patches.

It seems somewhat surprising that none of these updates touch any
platform data for any existing machines - are no machines using any of
the features being updated?

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

On 3 March 2012 22:00, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Mar 03, 2012 at 09:20:05PM +0530, Thomas Abraham wrote:
>> This patchset rearranges the elements in the platform data of the s3c-fb
>> driver with the intent of adding device tree support to the driver in
>> subsequent patches.
>
> It seems somewhat surprising that none of these updates touch any
> platform data for any existing machines - are no machines using any of
> the features being updated?

The third patch in this series includes platform data updates for the
Exynos4210 based Origen board. If the first two patches in this series
are acceptable, I will submit another version of this patchset which
would include updates to all existing machines that have platform data
for s3c-fb driver. I did not want to modify all the existing machines
without being sure of the acceptance of the first two patches.

Thanks,
Thomas.

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Sat, Mar 03, 2012 at 10:15:32PM +0530, Thomas Abraham wrote:

> The third patch in this series includes platform data updates for the
> Exynos4210 based Origen board. If the first two patches in this series
> are acceptable, I will submit another version of this patchset which
> would include updates to all existing machines that have platform data
> for s3c-fb driver. I did not want to modify all the existing machines
> without being sure of the acceptance of the first two patches.

OK, it'd have been good to highlight that more strongly so that the
patches don't just get applied if they look OK - you did mention it but
it was buried quite far down in the cover letter and would be very easy
to miss.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Sat, Mar 03, 2012 at 10:15:32PM +0530, Thomas Abraham wrote:

> The third patch in this series includes platform data updates for the
> Exynos4210 based Origen board. If the first two patches in this series
> are acceptable, I will submit another version of this patchset which
> would include updates to all existing machines that have platform data
> for s3c-fb driver. I did not want to modify all the existing machines
> without being sure of the acceptance of the first two patches.

OK, it'd have been good to highlight that more strongly so that the
patches don't just get applied if they look OK - you did mention it but
it was buried quite far down in the cover letter and would be very easy
to miss.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

On 3 March 2012 22:00, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Mar 03, 2012 at 09:20:05PM +0530, Thomas Abraham wrote:
>> This patchset rearranges the elements in the platform data of the s3c-fb
>> driver with the intent of adding device tree support to the driver in
>> subsequent patches.
>
> It seems somewhat surprising that none of these updates touch any
> platform data for any existing machines - are no machines using any of
> the features being updated?

The third patch in this series includes platform data updates for the
Exynos4210 based Origen board. If the first two patches in this series
are acceptable, I will submit another version of this patchset which
would include updates to all existing machines that have platform data
for s3c-fb driver. I did not want to modify all the existing machines
without being sure of the acceptance of the first two patches.

Thanks,
Thomas.

RE: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data
> 
> This patchset rearranges the elements in the platform data of the s3c-fb
> driver with the intent of adding device tree support to the driver in
> subsequent patches.
> 
> The first patch moves the video timing information from the individual window
> setup data into the platform specific configuration section in the platform
> data. The video timing is independent of the window setup. The resolution of
> the window could be smaller than that of the lcd panel attached. So the video
> timing data is removed from window configuration data.

Hi, Thomas.

OK, I know what you have done.
Actually, it was my long concern.

Current s3c-fb driver needs lcd panel resolutions, since each window's
xres, yres values can be changed by fb driver or user application.
Therefore, additional 'costant' lcd panel resolutions are necessary in order to
set timing values.

I'll review your patch.
Thank you.

Best regards,
Jingoo Han

> 
> The second patch removes the need for the 'default_win' element in the
> platform data. This element was used to decide whether the video data
> output from the controller should be enabled or disabled when the window
> specified by 'default_win' is enabled or disabled. With the first patch
> removing the need for atleast one window to be of the same resolution as
> that of the lcd panel, it is now possible to decide when to enable/disable
> the video data output based on the state of each window. If any of the
> window is active, the lcd data output is enabled. Otherwise, the lcd data
> output is disabled. Hence, the 'default_win' parameter from the platform
> data can be removed, which anyways cannot be specified when using
> device tree.
> 
> The third patch reworks the platform data of the display controller based
> on the changes introduced in the first two patches.
> 
> The patchset does not include the rework of platform data of all the other
> boards which have platform data for the s3c-fb driver. I am posting this
> patchset to know if the the changes are conceptually correct. If the first
> two patches are acceptable, I will resubmit this patchset with changes to
> the platform data for all boards which have s3c-fb platform data.
> 
> Thomas Abraham (3):
>   video: s3c-fb: move video interface timing out of window setup data
>   video: s3c-fb: remove 'default_win' element from platform data
>   ARM: Exynos: Rework platform data for lcd controller for Origen board
> 
>  arch/arm/mach-exynos/mach-origen.c      |   24 ++++---
>  arch/arm/plat-samsung/include/plat/fb.h |   11 ++-
>  drivers/video/s3c-fb.c                  |  128 ++++++++++++++----------------
>  3 files changed, 80 insertions(+), 83 deletions(-)

RE: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data
> 
> This patchset rearranges the elements in the platform data of the s3c-fb
> driver with the intent of adding device tree support to the driver in
> subsequent patches.
> 
> The first patch moves the video timing information from the individual window
> setup data into the platform specific configuration section in the platform
> data. The video timing is independent of the window setup. The resolution of
> the window could be smaller than that of the lcd panel attached. So the video
> timing data is removed from window configuration data.

Hi, Thomas.

OK, I know what you have done.
Actually, it was my long concern.

Current s3c-fb driver needs lcd panel resolutions, since each window's
xres, yres values can be changed by fb driver or user application.
Therefore, additional 'costant' lcd panel resolutions are necessary in order to
set timing values.

I'll review your patch.
Thank you.

Best regards,
Jingoo Han

> 
> The second patch removes the need for the 'default_win' element in the
> platform data. This element was used to decide whether the video data
> output from the controller should be enabled or disabled when the window
> specified by 'default_win' is enabled or disabled. With the first patch
> removing the need for atleast one window to be of the same resolution as
> that of the lcd panel, it is now possible to decide when to enable/disable
> the video data output based on the state of each window. If any of the
> window is active, the lcd data output is enabled. Otherwise, the lcd data
> output is disabled. Hence, the 'default_win' parameter from the platform
> data can be removed, which anyways cannot be specified when using
> device tree.
> 
> The third patch reworks the platform data of the display controller based
> on the changes introduced in the first two patches.
> 
> The patchset does not include the rework of platform data of all the other
> boards which have platform data for the s3c-fb driver. I am posting this
> patchset to know if the the changes are conceptually correct. If the first
> two patches are acceptable, I will resubmit this patchset with changes to
> the platform data for all boards which have s3c-fb platform data.
> 
> Thomas Abraham (3):
>   video: s3c-fb: move video interface timing out of window setup data
>   video: s3c-fb: remove 'default_win' element from platform data
>   ARM: Exynos: Rework platform data for lcd controller for Origen board
> 
>  arch/arm/mach-exynos/mach-origen.c      |   24 ++++---
>  arch/arm/plat-samsung/include/plat/fb.h |   11 ++-
>  drivers/video/s3c-fb.c                  |  128 ++++++++++++++----------------
>  3 files changed, 80 insertions(+), 83 deletions(-)

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

On 5 March 2012 12:59, Jingoo Han <jg1.han@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data
>>
>> This patchset rearranges the elements in the platform data of the s3c-fb
>> driver with the intent of adding device tree support to the driver in
>> subsequent patches.
>>
>> The first patch moves the video timing information from the individual window
>> setup data into the platform specific configuration section in the platform
>> data. The video timing is independent of the window setup. The resolution of
>> the window could be smaller than that of the lcd panel attached. So the video
>> timing data is removed from window configuration data.
>
> Hi, Thomas.
>
> OK, I know what you have done.
> Actually, it was my long concern.
>
> Current s3c-fb driver needs lcd panel resolutions, since each window's
> xres, yres values can be changed by fb driver or user application.
> Therefore, additional 'costant' lcd panel resolutions are necessary in order to
> set timing values.
>
> I'll review your patch.
> Thank you.

Thanks. Please let me know if you have any comments.

Regards,
Thomas.

[...]

Re: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data

[Thomas Abraham]

On 5 March 2012 12:59, Jingoo Han <jg1.han@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 0/3] video: s3c-fb: Rearrange the elements in platform data
>>
>> This patchset rearranges the elements in the platform data of the s3c-fb
>> driver with the intent of adding device tree support to the driver in
>> subsequent patches.
>>
>> The first patch moves the video timing information from the individual window
>> setup data into the platform specific configuration section in the platform
>> data. The video timing is independent of the window setup. The resolution of
>> the window could be smaller than that of the lcd panel attached. So the video
>> timing data is removed from window configuration data.
>
> Hi, Thomas.
>
> OK, I know what you have done.
> Actually, it was my long concern.
>
> Current s3c-fb driver needs lcd panel resolutions, since each window's
> xres, yres values can be changed by fb driver or user application.
> Therefore, additional 'costant' lcd panel resolutions are necessary in order to
> set timing values.
>
> I'll review your patch.
> Thank you.

Thanks. Please let me know if you have any comments.

Regards,
Thomas.

[...]

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> The video interface timing is independent of the window setup data.
> The resolution of the window can be smaller than that of the lcd
> panel to which the video data is output.
> 
> So move the video timing data from the per-window setup data to the
> platform specific section in the platform data. This also removes
> the restriction that atleast one window should have the same
> resolution as that of the panel attached.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>  2 files changed, 63 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> index 0fedf47..39d6bd7 100644
> --- a/arch/arm/plat-samsung/include/plat/fb.h
> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> @@ -24,15 +24,16 @@
> 
>  /**
>   * struct s3c_fb_pd_win - per window setup data
> - * @win_mode: The display parameters to initialise (not for window 0)
> + * @xres     : The window X size.
> + * @yres     : The window Y size.
>   * @virtual_x: The virtual X size.
>   * @virtual_y: The virtual Y size.
>   */
>  struct s3c_fb_pd_win {
> -	struct fb_videomode	win_mode;
> -
>  	unsigned short		default_bpp;
>  	unsigned short		max_bpp;
> +	unsigned short		xres;
> +	unsigned short		yres;
>  	unsigned short		virtual_x;
>  	unsigned short		virtual_y;
>  };
> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>   * @default_win: default window layer number to be used for UI layer.
>   * @vidcon0: The base vidcon0 values to control the panel data format.
>   * @vidcon1: The base vidcon1 values to control the panel data output.
> + * @vtiming: Video timing when connected to a RGB type panel.

fb_videomode can be set, even if it is not RGB type panel.
In my opinion, it would be better.
+ * @vtiming: The video timing values to set the interface timing of the panel.

>   * @win: The setup data for each hardware window, or NULL for unused.
>   * @display_mode: The LCD output display mode.
>   *
> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>  	void	(*setup_gpio)(void);
> 
>  	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
> +	struct fb_videomode     *vtiming;
> 
>  	u32			 default_win;
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 1fb7ddf..8e05d4d 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	u32 alpha = 0;
>  	u32 data;
>  	u32 pagewidth;
> -	int clkdiv;
> 
>  	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> 
> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	/* disable the window whilst we update it */
>  	writel(0, regs + WINCON(win_no));
> 
> -	/* use platform specified window as the basis for the lcd timings */
> -
> -	if (win_no = sfb->pdata->default_win) {
> -		clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> -
> -		data = sfb->pdata->vidcon0;
> -		data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> -
> -		if (clkdiv > 1)
> -			data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> -		else
> -			data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
> -
> -		/* write the timing data to the panel */
> -
> -		if (sfb->variant.is_2443)
> -			data |= (1 << 5);
> -
> -		writel(data, regs + VIDCON0);
> -
> +	if (win_no = sfb->pdata->default_win)
>  		s3c_fb_enable(sfb, 1);
> 
> -		data = VIDTCON0_VBPD(var->upper_margin - 1) |
> -		       VIDTCON0_VFPD(var->lower_margin - 1) |
> -		       VIDTCON0_VSPW(var->vsync_len - 1);
> -
> -		writel(data, regs + sfb->variant.vidtcon);
> -
> -		data = VIDTCON1_HBPD(var->left_margin - 1) |
> -		       VIDTCON1_HFPD(var->right_margin - 1) |
> -		       VIDTCON1_HSPW(var->hsync_len - 1);
> -
> -		/* VIDTCON1 */
> -		writel(data, regs + sfb->variant.vidtcon + 4);
> -
> -		data = VIDTCON2_LINEVAL(var->yres - 1) |
> -		       VIDTCON2_HOZVAL(var->xres - 1);
> -		writel(data, regs + sfb->variant.vidtcon + 8);
> -	}
> -

It looks good.
VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.

>  	/* write the buffer address */
> 
>  	/* start and end registers stride is 8 */
> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> 
>  	dev_dbg(sfb->dev, "allocating memory for display\n");
> 
> -	real_size = windata->win_mode.xres * windata->win_mode.yres;
> +	real_size = windata->xres * windata->yres;
>  	virt_size = windata->virtual_x * windata->virtual_y;
> 
>  	dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> -		real_size, windata->win_mode.xres, windata->win_mode.yres,
> +		real_size, windata->xres, windata->yres,
>  		virt_size, windata->virtual_x, windata->virtual_y);
> 
>  	size = (real_size > virt_size) ? real_size : virt_size;
> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  				      struct s3c_fb_win **res)
>  {
>  	struct fb_var_screeninfo *var;
> -	struct fb_videomode *initmode;
> +	struct fb_videomode initmode;

*initmode cannot be used???
Can you tell me why pointer type should be changed?

>  	struct s3c_fb_pd_win *windata;
>  	struct s3c_fb_win *win;
>  	struct fb_info *fbinfo;
> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  	}
> 
>  	windata = sfb->pdata->win[win_no];
> -	initmode = &windata->win_mode;
> +	initmode = *sfb->pdata->vtiming;
> 
>  	WARN_ON(windata->max_bpp = 0);
> -	WARN_ON(windata->win_mode.xres = 0);
> -	WARN_ON(windata->win_mode.yres = 0);
> +	WARN_ON(windata->xres = 0);
> +	WARN_ON(windata->yres = 0);
> 
>  	win = fbinfo->par;
>  	*res = win;
> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  	}
> 
>  	/* setup the initial video mode from the window */
> -	fb_videomode_to_var(&fbinfo->var, initmode);
> +	initmode.xres = windata->xres;
> +	initmode.yres = windata->yres;
> +	fb_videomode_to_var(&fbinfo->var, &initmode);
> 
>  	fbinfo->fix.type	= FB_TYPE_PACKED_PIXELS;
>  	fbinfo->fix.accel	= FB_ACCEL_NONE;
> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  }
> 
>  /**
> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
> + * @sfb: The base resources for the hardware.
> + *
> + * Set horizontal and vertical lcd rgb interface timing.
> + */
> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
> +{
> +	struct fb_videomode *vmode = sfb->pdata->vtiming;
> +	void __iomem *regs = sfb->regs;
> +	int clkdiv;
> +	u32 data;
> +
> +	if (!vmode->pixclock)
> +		s3c_fb_missing_pixclock(vmode);
> +
> +	clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
> +
> +	data = sfb->pdata->vidcon0;
> +	data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> +
> +	if (clkdiv > 1)
> +		data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> +	else
> +		data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
> +
> +	if (sfb->variant.is_2443)
> +		data |= (1 << 5);
> +	writel(data, regs + VIDCON0);
> +
> +	data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
> +	       VIDTCON0_VFPD(vmode->lower_margin - 1) |
> +	       VIDTCON0_VSPW(vmode->vsync_len - 1);
> +	writel(data, regs + sfb->variant.vidtcon);
> +
> +	data = VIDTCON1_HBPD(vmode->left_margin - 1) |
> +	       VIDTCON1_HFPD(vmode->right_margin - 1) |
> +	       VIDTCON1_HSPW(vmode->hsync_len - 1);
> +	writel(data, regs + sfb->variant.vidtcon + 4);
> +
> +	data = VIDTCON2_LINEVAL(vmode->yres - 1) |
> +	       VIDTCON2_HOZVAL(vmode->xres - 1);
> +	writel(data, regs + sfb->variant.vidtcon + 8);
> +}
> +
> +/**
>   * s3c_fb_clear_win() - clear hardware window registers.
>   * @sfb: The base resources for the hardware.
>   * @win: The window to process.
> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>  		writel(0xffffff, regs + WKEYCON1);
>  	}
> 
> +	s3c_fb_set_rgb_timing(sfb);
> +

This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
The timing registers should be set when s3c_fb_resume() is called.
If not, after resuming, timing registers such as VIDTCONx will not bet set.

>  	/* we have the register setup, start allocating framebuffers */
> 
>  	for (win = 0; win < fbdrv->variant.nr_windows; win++) {
>  		if (!pd->win[win])
>  			continue;
> 
> -		if (!pd->win[win]->win_mode.pixclock)
> -			s3c_fb_missing_pixclock(&pd->win[win]->win_mode);
> -
>  		ret = s3c_fb_probe_win(sfb, win, fbdrv->win[win],
>  				       &sfb->windows[win]);
>  		if (ret < 0) {
> --
> 1.6.6.rc2

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> The video interface timing is independent of the window setup data.
> The resolution of the window can be smaller than that of the lcd
> panel to which the video data is output.
> 
> So move the video timing data from the per-window setup data to the
> platform specific section in the platform data. This also removes
> the restriction that atleast one window should have the same
> resolution as that of the panel attached.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>  2 files changed, 63 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> index 0fedf47..39d6bd7 100644
> --- a/arch/arm/plat-samsung/include/plat/fb.h
> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> @@ -24,15 +24,16 @@
> 
>  /**
>   * struct s3c_fb_pd_win - per window setup data
> - * @win_mode: The display parameters to initialise (not for window 0)
> + * @xres     : The window X size.
> + * @yres     : The window Y size.
>   * @virtual_x: The virtual X size.
>   * @virtual_y: The virtual Y size.
>   */
>  struct s3c_fb_pd_win {
> -	struct fb_videomode	win_mode;
> -
>  	unsigned short		default_bpp;
>  	unsigned short		max_bpp;
> +	unsigned short		xres;
> +	unsigned short		yres;
>  	unsigned short		virtual_x;
>  	unsigned short		virtual_y;
>  };
> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>   * @default_win: default window layer number to be used for UI layer.
>   * @vidcon0: The base vidcon0 values to control the panel data format.
>   * @vidcon1: The base vidcon1 values to control the panel data output.
> + * @vtiming: Video timing when connected to a RGB type panel.

fb_videomode can be set, even if it is not RGB type panel.
In my opinion, it would be better.
+ * @vtiming: The video timing values to set the interface timing of the panel.

>   * @win: The setup data for each hardware window, or NULL for unused.
>   * @display_mode: The LCD output display mode.
>   *
> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>  	void	(*setup_gpio)(void);
> 
>  	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
> +	struct fb_videomode     *vtiming;
> 
>  	u32			 default_win;
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 1fb7ddf..8e05d4d 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	u32 alpha = 0;
>  	u32 data;
>  	u32 pagewidth;
> -	int clkdiv;
> 
>  	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> 
> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	/* disable the window whilst we update it */
>  	writel(0, regs + WINCON(win_no));
> 
> -	/* use platform specified window as the basis for the lcd timings */
> -
> -	if (win_no == sfb->pdata->default_win) {
> -		clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> -
> -		data = sfb->pdata->vidcon0;
> -		data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> -
> -		if (clkdiv > 1)
> -			data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> -		else
> -			data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
> -
> -		/* write the timing data to the panel */
> -
> -		if (sfb->variant.is_2443)
> -			data |= (1 << 5);
> -
> -		writel(data, regs + VIDCON0);
> -
> +	if (win_no == sfb->pdata->default_win)
>  		s3c_fb_enable(sfb, 1);
> 
> -		data = VIDTCON0_VBPD(var->upper_margin - 1) |
> -		       VIDTCON0_VFPD(var->lower_margin - 1) |
> -		       VIDTCON0_VSPW(var->vsync_len - 1);
> -
> -		writel(data, regs + sfb->variant.vidtcon);
> -
> -		data = VIDTCON1_HBPD(var->left_margin - 1) |
> -		       VIDTCON1_HFPD(var->right_margin - 1) |
> -		       VIDTCON1_HSPW(var->hsync_len - 1);
> -
> -		/* VIDTCON1 */
> -		writel(data, regs + sfb->variant.vidtcon + 4);
> -
> -		data = VIDTCON2_LINEVAL(var->yres - 1) |
> -		       VIDTCON2_HOZVAL(var->xres - 1);
> -		writel(data, regs + sfb->variant.vidtcon + 8);
> -	}
> -

It looks good.
VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.

>  	/* write the buffer address */
> 
>  	/* start and end registers stride is 8 */
> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> 
>  	dev_dbg(sfb->dev, "allocating memory for display\n");
> 
> -	real_size = windata->win_mode.xres * windata->win_mode.yres;
> +	real_size = windata->xres * windata->yres;
>  	virt_size = windata->virtual_x * windata->virtual_y;
> 
>  	dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> -		real_size, windata->win_mode.xres, windata->win_mode.yres,
> +		real_size, windata->xres, windata->yres,
>  		virt_size, windata->virtual_x, windata->virtual_y);
> 
>  	size = (real_size > virt_size) ? real_size : virt_size;
> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  				      struct s3c_fb_win **res)
>  {
>  	struct fb_var_screeninfo *var;
> -	struct fb_videomode *initmode;
> +	struct fb_videomode initmode;

*initmode cannot be used???
Can you tell me why pointer type should be changed?

>  	struct s3c_fb_pd_win *windata;
>  	struct s3c_fb_win *win;
>  	struct fb_info *fbinfo;
> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  	}
> 
>  	windata = sfb->pdata->win[win_no];
> -	initmode = &windata->win_mode;
> +	initmode = *sfb->pdata->vtiming;
> 
>  	WARN_ON(windata->max_bpp == 0);
> -	WARN_ON(windata->win_mode.xres == 0);
> -	WARN_ON(windata->win_mode.yres == 0);
> +	WARN_ON(windata->xres == 0);
> +	WARN_ON(windata->yres == 0);
> 
>  	win = fbinfo->par;
>  	*res = win;
> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  	}
> 
>  	/* setup the initial video mode from the window */
> -	fb_videomode_to_var(&fbinfo->var, initmode);
> +	initmode.xres = windata->xres;
> +	initmode.yres = windata->yres;
> +	fb_videomode_to_var(&fbinfo->var, &initmode);
> 
>  	fbinfo->fix.type	= FB_TYPE_PACKED_PIXELS;
>  	fbinfo->fix.accel	= FB_ACCEL_NONE;
> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>  }
> 
>  /**
> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
> + * @sfb: The base resources for the hardware.
> + *
> + * Set horizontal and vertical lcd rgb interface timing.
> + */
> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
> +{
> +	struct fb_videomode *vmode = sfb->pdata->vtiming;
> +	void __iomem *regs = sfb->regs;
> +	int clkdiv;
> +	u32 data;
> +
> +	if (!vmode->pixclock)
> +		s3c_fb_missing_pixclock(vmode);
> +
> +	clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
> +
> +	data = sfb->pdata->vidcon0;
> +	data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> +
> +	if (clkdiv > 1)
> +		data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> +	else
> +		data &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
> +
> +	if (sfb->variant.is_2443)
> +		data |= (1 << 5);
> +	writel(data, regs + VIDCON0);
> +
> +	data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
> +	       VIDTCON0_VFPD(vmode->lower_margin - 1) |
> +	       VIDTCON0_VSPW(vmode->vsync_len - 1);
> +	writel(data, regs + sfb->variant.vidtcon);
> +
> +	data = VIDTCON1_HBPD(vmode->left_margin - 1) |
> +	       VIDTCON1_HFPD(vmode->right_margin - 1) |
> +	       VIDTCON1_HSPW(vmode->hsync_len - 1);
> +	writel(data, regs + sfb->variant.vidtcon + 4);
> +
> +	data = VIDTCON2_LINEVAL(vmode->yres - 1) |
> +	       VIDTCON2_HOZVAL(vmode->xres - 1);
> +	writel(data, regs + sfb->variant.vidtcon + 8);
> +}
> +
> +/**
>   * s3c_fb_clear_win() - clear hardware window registers.
>   * @sfb: The base resources for the hardware.
>   * @win: The window to process.
> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>  		writel(0xffffff, regs + WKEYCON1);
>  	}
> 
> +	s3c_fb_set_rgb_timing(sfb);
> +

This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
The timing registers should be set when s3c_fb_resume() is called.
If not, after resuming, timing registers such as VIDTCONx will not bet set.

>  	/* we have the register setup, start allocating framebuffers */
> 
>  	for (win = 0; win < fbdrv->variant.nr_windows; win++) {
>  		if (!pd->win[win])
>  			continue;
> 
> -		if (!pd->win[win]->win_mode.pixclock)
> -			s3c_fb_missing_pixclock(&pd->win[win]->win_mode);
> -
>  		ret = s3c_fb_probe_win(sfb, win, fbdrv->win[win],
>  				       &sfb->windows[win]);
>  		if (ret < 0) {
> --
> 1.6.6.rc2

Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>>
>> The video interface timing is independent of the window setup data.
>> The resolution of the window can be smaller than that of the lcd
>> panel to which the video data is output.
>>
>> So move the video timing data from the per-window setup data to the
>> platform specific section in the platform data. This also removes
>> the restriction that atleast one window should have the same
>> resolution as that of the panel attached.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>>  2 files changed, 63 insertions(+), 52 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> index 0fedf47..39d6bd7 100644
>> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> @@ -24,15 +24,16 @@
>>
>>  /**
>>   * struct s3c_fb_pd_win - per window setup data
>> - * @win_mode: The display parameters to initialise (not for window 0)
>> + * @xres     : The window X size.
>> + * @yres     : The window Y size.
>>   * @virtual_x: The virtual X size.
>>   * @virtual_y: The virtual Y size.
>>   */
>>  struct s3c_fb_pd_win {
>> -     struct fb_videomode     win_mode;
>> -
>>       unsigned short          default_bpp;
>>       unsigned short          max_bpp;
>> +     unsigned short          xres;
>> +     unsigned short          yres;
>>       unsigned short          virtual_x;
>>       unsigned short          virtual_y;
>>  };
>> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>>   * @default_win: default window layer number to be used for UI layer.
>>   * @vidcon0: The base vidcon0 values to control the panel data format.
>>   * @vidcon1: The base vidcon1 values to control the panel data output.
>> + * @vtiming: Video timing when connected to a RGB type panel.
>
> fb_videomode can be set, even if it is not RGB type panel.
> In my opinion, it would be better.
> + * @vtiming: The video timing values to set the interface timing of the panel.

The other interface that is supported is the i80 interface. Can these
timing values be used for i80 interface as well ?

>
>>   * @win: The setup data for each hardware window, or NULL for unused.
>>   * @display_mode: The LCD output display mode.
>>   *
>> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>>       void    (*setup_gpio)(void);
>>
>>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>> +     struct fb_videomode     *vtiming;
>>
>>       u32                      default_win;
>>
>> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> index 1fb7ddf..8e05d4d 100644
>> --- a/drivers/video/s3c-fb.c
>> +++ b/drivers/video/s3c-fb.c
>> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       u32 alpha = 0;
>>       u32 data;
>>       u32 pagewidth;
>> -     int clkdiv;
>>
>>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
>>
>> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       /* disable the window whilst we update it */
>>       writel(0, regs + WINCON(win_no));
>>
>> -     /* use platform specified window as the basis for the lcd timings */
>> -
>> -     if (win_no == sfb->pdata->default_win) {
>> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
>> -
>> -             data = sfb->pdata->vidcon0;
>> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> -
>> -             if (clkdiv > 1)
>> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> -             else
>> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> -
>> -             /* write the timing data to the panel */
>> -
>> -             if (sfb->variant.is_2443)
>> -                     data |= (1 << 5);
>> -
>> -             writel(data, regs + VIDCON0);
>> -
>> +     if (win_no == sfb->pdata->default_win)
>>               s3c_fb_enable(sfb, 1);
>>
>> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
>> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
>> -                    VIDTCON0_VSPW(var->vsync_len - 1);
>> -
>> -             writel(data, regs + sfb->variant.vidtcon);
>> -
>> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
>> -                    VIDTCON1_HFPD(var->right_margin - 1) |
>> -                    VIDTCON1_HSPW(var->hsync_len - 1);
>> -
>> -             /* VIDTCON1 */
>> -             writel(data, regs + sfb->variant.vidtcon + 4);
>> -
>> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
>> -                    VIDTCON2_HOZVAL(var->xres - 1);
>> -             writel(data, regs + sfb->variant.vidtcon + 8);
>> -     }
>> -
>
> It looks good.
> VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
>
>>       /* write the buffer address */
>>
>>       /* start and end registers stride is 8 */
>> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
>>
>>       dev_dbg(sfb->dev, "allocating memory for display\n");
>>
>> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
>> +     real_size = windata->xres * windata->yres;
>>       virt_size = windata->virtual_x * windata->virtual_y;
>>
>>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
>> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
>> +             real_size, windata->xres, windata->yres,
>>               virt_size, windata->virtual_x, windata->virtual_y);
>>
>>       size = (real_size > virt_size) ? real_size : virt_size;
>> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>                                     struct s3c_fb_win **res)
>>  {
>>       struct fb_var_screeninfo *var;
>> -     struct fb_videomode *initmode;
>> +     struct fb_videomode initmode;
>
> *initmode cannot be used???
> Can you tell me why pointer type should be changed?
>

The initmode is used to pass video timing to the fb_videomode_to_var()
function. Each window setup data in platform data included video
timing information. Since, the video timing data is now moved out of
per-window data, the xres and yres values have to be setup based on
the window for which the fb_videomode_to_var() is called. Hence, the
common timing values is first copied into initmode and then the window
specific xres and yres are set. If initmode is a maintained as a
pointer (to the video timing data in platform data), then any xres and
yres update to initmode would overwrite the 'constant' video timing.


>>       struct s3c_fb_pd_win *windata;
>>       struct s3c_fb_win *win;
>>       struct fb_info *fbinfo;
>> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>       }
>>
>>       windata = sfb->pdata->win[win_no];
>> -     initmode = &windata->win_mode;
>> +     initmode = *sfb->pdata->vtiming;
>>
>>       WARN_ON(windata->max_bpp == 0);
>> -     WARN_ON(windata->win_mode.xres == 0);
>> -     WARN_ON(windata->win_mode.yres == 0);
>> +     WARN_ON(windata->xres == 0);
>> +     WARN_ON(windata->yres == 0);
>>
>>       win = fbinfo->par;
>>       *res = win;
>> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>       }
>>
>>       /* setup the initial video mode from the window */
>> -     fb_videomode_to_var(&fbinfo->var, initmode);
>> +     initmode.xres = windata->xres;
>> +     initmode.yres = windata->yres;

Here, the xres and yres values are copied to initmode and described above.

>> +     fb_videomode_to_var(&fbinfo->var, &initmode);
>>
>>       fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;
>>       fbinfo->fix.accel       = FB_ACCEL_NONE;
>> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>  }
>>
>>  /**
>> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
>> + * @sfb: The base resources for the hardware.
>> + *
>> + * Set horizontal and vertical lcd rgb interface timing.
>> + */
>> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
>> +{
>> +     struct fb_videomode *vmode = sfb->pdata->vtiming;
>> +     void __iomem *regs = sfb->regs;
>> +     int clkdiv;
>> +     u32 data;
>> +
>> +     if (!vmode->pixclock)
>> +             s3c_fb_missing_pixclock(vmode);
>> +
>> +     clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
>> +
>> +     data = sfb->pdata->vidcon0;
>> +     data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> +
>> +     if (clkdiv > 1)
>> +             data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> +     else
>> +             data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> +
>> +     if (sfb->variant.is_2443)
>> +             data |= (1 << 5);
>> +     writel(data, regs + VIDCON0);
>> +
>> +     data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
>> +            VIDTCON0_VFPD(vmode->lower_margin - 1) |
>> +            VIDTCON0_VSPW(vmode->vsync_len - 1);
>> +     writel(data, regs + sfb->variant.vidtcon);
>> +
>> +     data = VIDTCON1_HBPD(vmode->left_margin - 1) |
>> +            VIDTCON1_HFPD(vmode->right_margin - 1) |
>> +            VIDTCON1_HSPW(vmode->hsync_len - 1);
>> +     writel(data, regs + sfb->variant.vidtcon + 4);
>> +
>> +     data = VIDTCON2_LINEVAL(vmode->yres - 1) |
>> +            VIDTCON2_HOZVAL(vmode->xres - 1);
>> +     writel(data, regs + sfb->variant.vidtcon + 8);
>> +}
>> +
>> +/**
>>   * s3c_fb_clear_win() - clear hardware window registers.
>>   * @sfb: The base resources for the hardware.
>>   * @win: The window to process.
>> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>>               writel(0xffffff, regs + WKEYCON1);
>>       }
>>
>> +     s3c_fb_set_rgb_timing(sfb);
>> +
>
> This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
> The timing registers should be set when s3c_fb_resume() is called.
> If not, after resuming, timing registers such as VIDTCONx will not bet set.

Yes, I missed that. I will add it in the resume path.

Thanks for you comments. If you could let me know if i80 video timing
values can be passed with 'struct fb_videomode', I will do the changes
as you have suggested and quickly repost this patchset.

Thanks,
Thomas.

Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>>
>> The video interface timing is independent of the window setup data.
>> The resolution of the window can be smaller than that of the lcd
>> panel to which the video data is output.
>>
>> So move the video timing data from the per-window setup data to the
>> platform specific section in the platform data. This also removes
>> the restriction that atleast one window should have the same
>> resolution as that of the panel attached.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>>  2 files changed, 63 insertions(+), 52 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> index 0fedf47..39d6bd7 100644
>> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> @@ -24,15 +24,16 @@
>>
>>  /**
>>   * struct s3c_fb_pd_win - per window setup data
>> - * @win_mode: The display parameters to initialise (not for window 0)
>> + * @xres     : The window X size.
>> + * @yres     : The window Y size.
>>   * @virtual_x: The virtual X size.
>>   * @virtual_y: The virtual Y size.
>>   */
>>  struct s3c_fb_pd_win {
>> -     struct fb_videomode     win_mode;
>> -
>>       unsigned short          default_bpp;
>>       unsigned short          max_bpp;
>> +     unsigned short          xres;
>> +     unsigned short          yres;
>>       unsigned short          virtual_x;
>>       unsigned short          virtual_y;
>>  };
>> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>>   * @default_win: default window layer number to be used for UI layer.
>>   * @vidcon0: The base vidcon0 values to control the panel data format.
>>   * @vidcon1: The base vidcon1 values to control the panel data output.
>> + * @vtiming: Video timing when connected to a RGB type panel.
>
> fb_videomode can be set, even if it is not RGB type panel.
> In my opinion, it would be better.
> + * @vtiming: The video timing values to set the interface timing of the panel.

The other interface that is supported is the i80 interface. Can these
timing values be used for i80 interface as well ?

>
>>   * @win: The setup data for each hardware window, or NULL for unused.
>>   * @display_mode: The LCD output display mode.
>>   *
>> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>>       void    (*setup_gpio)(void);
>>
>>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>> +     struct fb_videomode     *vtiming;
>>
>>       u32                      default_win;
>>
>> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> index 1fb7ddf..8e05d4d 100644
>> --- a/drivers/video/s3c-fb.c
>> +++ b/drivers/video/s3c-fb.c
>> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       u32 alpha = 0;
>>       u32 data;
>>       u32 pagewidth;
>> -     int clkdiv;
>>
>>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
>>
>> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       /* disable the window whilst we update it */
>>       writel(0, regs + WINCON(win_no));
>>
>> -     /* use platform specified window as the basis for the lcd timings */
>> -
>> -     if (win_no = sfb->pdata->default_win) {
>> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
>> -
>> -             data = sfb->pdata->vidcon0;
>> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> -
>> -             if (clkdiv > 1)
>> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> -             else
>> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> -
>> -             /* write the timing data to the panel */
>> -
>> -             if (sfb->variant.is_2443)
>> -                     data |= (1 << 5);
>> -
>> -             writel(data, regs + VIDCON0);
>> -
>> +     if (win_no = sfb->pdata->default_win)
>>               s3c_fb_enable(sfb, 1);
>>
>> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
>> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
>> -                    VIDTCON0_VSPW(var->vsync_len - 1);
>> -
>> -             writel(data, regs + sfb->variant.vidtcon);
>> -
>> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
>> -                    VIDTCON1_HFPD(var->right_margin - 1) |
>> -                    VIDTCON1_HSPW(var->hsync_len - 1);
>> -
>> -             /* VIDTCON1 */
>> -             writel(data, regs + sfb->variant.vidtcon + 4);
>> -
>> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
>> -                    VIDTCON2_HOZVAL(var->xres - 1);
>> -             writel(data, regs + sfb->variant.vidtcon + 8);
>> -     }
>> -
>
> It looks good.
> VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
>
>>       /* write the buffer address */
>>
>>       /* start and end registers stride is 8 */
>> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
>>
>>       dev_dbg(sfb->dev, "allocating memory for display\n");
>>
>> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
>> +     real_size = windata->xres * windata->yres;
>>       virt_size = windata->virtual_x * windata->virtual_y;
>>
>>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
>> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
>> +             real_size, windata->xres, windata->yres,
>>               virt_size, windata->virtual_x, windata->virtual_y);
>>
>>       size = (real_size > virt_size) ? real_size : virt_size;
>> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>                                     struct s3c_fb_win **res)
>>  {
>>       struct fb_var_screeninfo *var;
>> -     struct fb_videomode *initmode;
>> +     struct fb_videomode initmode;
>
> *initmode cannot be used???
> Can you tell me why pointer type should be changed?
>

The initmode is used to pass video timing to the fb_videomode_to_var()
function. Each window setup data in platform data included video
timing information. Since, the video timing data is now moved out of
per-window data, the xres and yres values have to be setup based on
the window for which the fb_videomode_to_var() is called. Hence, the
common timing values is first copied into initmode and then the window
specific xres and yres are set. If initmode is a maintained as a
pointer (to the video timing data in platform data), then any xres and
yres update to initmode would overwrite the 'constant' video timing.


>>       struct s3c_fb_pd_win *windata;
>>       struct s3c_fb_win *win;
>>       struct fb_info *fbinfo;
>> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>       }
>>
>>       windata = sfb->pdata->win[win_no];
>> -     initmode = &windata->win_mode;
>> +     initmode = *sfb->pdata->vtiming;
>>
>>       WARN_ON(windata->max_bpp = 0);
>> -     WARN_ON(windata->win_mode.xres = 0);
>> -     WARN_ON(windata->win_mode.yres = 0);
>> +     WARN_ON(windata->xres = 0);
>> +     WARN_ON(windata->yres = 0);
>>
>>       win = fbinfo->par;
>>       *res = win;
>> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>       }
>>
>>       /* setup the initial video mode from the window */
>> -     fb_videomode_to_var(&fbinfo->var, initmode);
>> +     initmode.xres = windata->xres;
>> +     initmode.yres = windata->yres;

Here, the xres and yres values are copied to initmode and described above.

>> +     fb_videomode_to_var(&fbinfo->var, &initmode);
>>
>>       fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;
>>       fbinfo->fix.accel       = FB_ACCEL_NONE;
>> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>>  }
>>
>>  /**
>> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
>> + * @sfb: The base resources for the hardware.
>> + *
>> + * Set horizontal and vertical lcd rgb interface timing.
>> + */
>> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
>> +{
>> +     struct fb_videomode *vmode = sfb->pdata->vtiming;
>> +     void __iomem *regs = sfb->regs;
>> +     int clkdiv;
>> +     u32 data;
>> +
>> +     if (!vmode->pixclock)
>> +             s3c_fb_missing_pixclock(vmode);
>> +
>> +     clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
>> +
>> +     data = sfb->pdata->vidcon0;
>> +     data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> +
>> +     if (clkdiv > 1)
>> +             data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> +     else
>> +             data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> +
>> +     if (sfb->variant.is_2443)
>> +             data |= (1 << 5);
>> +     writel(data, regs + VIDCON0);
>> +
>> +     data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
>> +            VIDTCON0_VFPD(vmode->lower_margin - 1) |
>> +            VIDTCON0_VSPW(vmode->vsync_len - 1);
>> +     writel(data, regs + sfb->variant.vidtcon);
>> +
>> +     data = VIDTCON1_HBPD(vmode->left_margin - 1) |
>> +            VIDTCON1_HFPD(vmode->right_margin - 1) |
>> +            VIDTCON1_HSPW(vmode->hsync_len - 1);
>> +     writel(data, regs + sfb->variant.vidtcon + 4);
>> +
>> +     data = VIDTCON2_LINEVAL(vmode->yres - 1) |
>> +            VIDTCON2_HOZVAL(vmode->xres - 1);
>> +     writel(data, regs + sfb->variant.vidtcon + 8);
>> +}
>> +
>> +/**
>>   * s3c_fb_clear_win() - clear hardware window registers.
>>   * @sfb: The base resources for the hardware.
>>   * @win: The window to process.
>> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>>               writel(0xffffff, regs + WKEYCON1);
>>       }
>>
>> +     s3c_fb_set_rgb_timing(sfb);
>> +
>
> This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
> The timing registers should be set when s3c_fb_resume() is called.
> If not, after resuming, timing registers such as VIDTCONx will not bet set.

Yes, I missed that. I will add it in the resume path.

Thanks for you comments. If you could let me know if i80 video timing
values can be passed with 'struct fb_videomode', I will do the changes
as you have suggested and quickly repost this patchset.

Thanks,
Thomas.

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Tuesday, March 06, 2012 2:26 PM
> To: Jingoo Han
> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
> 
> >> -----Original Message-----
> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> Sent: Sunday, March 04, 2012 12:50 AM
> >> To: linux-fbdev@vger.kernel.org
> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >>
> >> The video interface timing is independent of the window setup data.
> >> The resolution of the window can be smaller than that of the lcd
> >> panel to which the video data is output.
> >>
> >> So move the video timing data from the per-window setup data to the
> >> platform specific section in the platform data. This also removes
> >> the restriction that atleast one window should have the same
> >> resolution as that of the panel attached.
> >>
> >> Cc: Ben Dooks <ben-linux@fluff.org>
> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
> >>  2 files changed, 63 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> >> index 0fedf47..39d6bd7 100644
> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> >> @@ -24,15 +24,16 @@
> >>
> >>  /**
> >>   * struct s3c_fb_pd_win - per window setup data
> >> - * @win_mode: The display parameters to initialise (not for window 0)
> >> + * @xres     : The window X size.
> >> + * @yres     : The window Y size.
> >>   * @virtual_x: The virtual X size.
> >>   * @virtual_y: The virtual Y size.
> >>   */
> >>  struct s3c_fb_pd_win {
> >> -     struct fb_videomode     win_mode;
> >> -
> >>       unsigned short          default_bpp;
> >>       unsigned short          max_bpp;
> >> +     unsigned short          xres;
> >> +     unsigned short          yres;
> >>       unsigned short          virtual_x;
> >>       unsigned short          virtual_y;
> >>  };
> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
> >>   * @default_win: default window layer number to be used for UI layer.
> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
> >> + * @vtiming: Video timing when connected to a RGB type panel.
> >
> > fb_videomode can be set, even if it is not RGB type panel.
> > In my opinion, it would be better.
> > + * @vtiming: The video timing values to set the interface timing of the panel.
> 
> The other interface that is supported is the i80 interface. Can these
> timing values be used for i80 interface as well ?

No, you're right. The i80 is not supported.
Please ignore my comment.

> 
> >
> >>   * @win: The setup data for each hardware window, or NULL for unused.
> >>   * @display_mode: The LCD output display mode.
> >>   *
> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
> >>       void    (*setup_gpio)(void);
> >>
> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
> >> +     struct fb_videomode     *vtiming;
> >>
> >>       u32                      default_win;
> >>
> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> >> index 1fb7ddf..8e05d4d 100644
> >> --- a/drivers/video/s3c-fb.c
> >> +++ b/drivers/video/s3c-fb.c
> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
> >>       u32 alpha = 0;
> >>       u32 data;
> >>       u32 pagewidth;
> >> -     int clkdiv;
> >>
> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> >>
> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
> >>       /* disable the window whilst we update it */
> >>       writel(0, regs + WINCON(win_no));
> >>
> >> -     /* use platform specified window as the basis for the lcd timings */
> >> -
> >> -     if (win_no = sfb->pdata->default_win) {
> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> >> -
> >> -             data = sfb->pdata->vidcon0;
> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> -
> >> -             if (clkdiv > 1)
> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> -             else
> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> -
> >> -             /* write the timing data to the panel */
> >> -
> >> -             if (sfb->variant.is_2443)
> >> -                     data |= (1 << 5);
> >> -
> >> -             writel(data, regs + VIDCON0);
> >> -
> >> +     if (win_no = sfb->pdata->default_win)
> >>               s3c_fb_enable(sfb, 1);
> >>
> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
> >> -
> >> -             writel(data, regs + sfb->variant.vidtcon);
> >> -
> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
> >> -
> >> -             /* VIDTCON1 */
> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
> >> -
> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
> >> -     }
> >> -
> >
> > It looks good.
> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
> >
> >>       /* write the buffer address */
> >>
> >>       /* start and end registers stride is 8 */
> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> >>
> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
> >>
> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
> >> +     real_size = windata->xres * windata->yres;
> >>       virt_size = windata->virtual_x * windata->virtual_y;
> >>
> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
> >> +             real_size, windata->xres, windata->yres,
> >>               virt_size, windata->virtual_x, windata->virtual_y);
> >>
> >>       size = (real_size > virt_size) ? real_size : virt_size;
> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>                                     struct s3c_fb_win **res)
> >>  {
> >>       struct fb_var_screeninfo *var;
> >> -     struct fb_videomode *initmode;
> >> +     struct fb_videomode initmode;
> >
> > *initmode cannot be used???
> > Can you tell me why pointer type should be changed?
> >
> 
> The initmode is used to pass video timing to the fb_videomode_to_var()
> function. Each window setup data in platform data included video
> timing information. Since, the video timing data is now moved out of
> per-window data, the xres and yres values have to be setup based on
> the window for which the fb_videomode_to_var() is called. Hence, the
> common timing values is first copied into initmode and then the window
> specific xres and yres are set. If initmode is a maintained as a
> pointer (to the video timing data in platform data), then any xres and
> yres update to initmode would overwrite the 'constant' video timing.

My point is that variable type 'initmode' can be not changed by using pointer type as follows:

@@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
        }

        windata = sfb->pdata->win[win_no];
-       initmode = &windata->win_mode;
+       initmode = sfb->pdata->vtiming;

        WARN_ON(windata->max_bpp = 0);
-       WARN_ON(windata->win_mode.xres = 0);
-       WARN_ON(windata->win_mode.yres = 0);
+       WARN_ON(windata->xres = 0);
+       WARN_ON(windata->yres = 0);

        win = fbinfo->par;
        *res = win;
@@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
        }

        /* setup the initial video mode from the window */
+       initmode->xres = windata->xres;
+       initmode->yres = windata->yres;
        fb_videomode_to_var(&fbinfo->var, initmode);

        fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;

In this case, you don't need additional change such as following.
-	struct fb_videomode *initmode;
+	struct fb_videomode initmode;
...
-	fb_videomode_to_var(&fbinfo->var, initmode);
+	fb_videomode_to_var(&fbinfo->var, &initmode);

> 
> 
> >>       struct s3c_fb_pd_win *windata;
> >>       struct s3c_fb_win *win;
> >>       struct fb_info *fbinfo;
> >> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>       }
> >>
> >>       windata = sfb->pdata->win[win_no];
> >> -     initmode = &windata->win_mode;
> >> +     initmode = *sfb->pdata->vtiming;
> >>
> >>       WARN_ON(windata->max_bpp = 0);
> >> -     WARN_ON(windata->win_mode.xres = 0);
> >> -     WARN_ON(windata->win_mode.yres = 0);
> >> +     WARN_ON(windata->xres = 0);
> >> +     WARN_ON(windata->yres = 0);
> >>
> >>       win = fbinfo->par;
> >>       *res = win;
> >> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>       }
> >>
> >>       /* setup the initial video mode from the window */
> >> -     fb_videomode_to_var(&fbinfo->var, initmode);
> >> +     initmode.xres = windata->xres;
> >> +     initmode.yres = windata->yres;
> 
> Here, the xres and yres values are copied to initmode and described above.
> 
> >> +     fb_videomode_to_var(&fbinfo->var, &initmode);
> >>
> >>       fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;
> >>       fbinfo->fix.accel       = FB_ACCEL_NONE;
> >> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>  }
> >>
> >>  /**
> >> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
> >> + * @sfb: The base resources for the hardware.
> >> + *
> >> + * Set horizontal and vertical lcd rgb interface timing.
> >> + */
> >> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
> >> +{
> >> +     struct fb_videomode *vmode = sfb->pdata->vtiming;
> >> +     void __iomem *regs = sfb->regs;
> >> +     int clkdiv;
> >> +     u32 data;
> >> +
> >> +     if (!vmode->pixclock)
> >> +             s3c_fb_missing_pixclock(vmode);
> >> +
> >> +     clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
> >> +
> >> +     data = sfb->pdata->vidcon0;
> >> +     data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> +
> >> +     if (clkdiv > 1)
> >> +             data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> +     else
> >> +             data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> +
> >> +     if (sfb->variant.is_2443)
> >> +             data |= (1 << 5);
> >> +     writel(data, regs + VIDCON0);
> >> +
> >> +     data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
> >> +            VIDTCON0_VFPD(vmode->lower_margin - 1) |
> >> +            VIDTCON0_VSPW(vmode->vsync_len - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon);
> >> +
> >> +     data = VIDTCON1_HBPD(vmode->left_margin - 1) |
> >> +            VIDTCON1_HFPD(vmode->right_margin - 1) |
> >> +            VIDTCON1_HSPW(vmode->hsync_len - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon + 4);
> >> +
> >> +     data = VIDTCON2_LINEVAL(vmode->yres - 1) |
> >> +            VIDTCON2_HOZVAL(vmode->xres - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon + 8);
> >> +}
> >> +
> >> +/**
> >>   * s3c_fb_clear_win() - clear hardware window registers.
> >>   * @sfb: The base resources for the hardware.
> >>   * @win: The window to process.
> >> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> >>               writel(0xffffff, regs + WKEYCON1);
> >>       }
> >>
> >> +     s3c_fb_set_rgb_timing(sfb);
> >> +
> >
> > This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
> > The timing registers should be set when s3c_fb_resume() is called.
> > If not, after resuming, timing registers such as VIDTCONx will not bet set.
> 
> Yes, I missed that. I will add it in the resume path.
> 
> Thanks for you comments. If you could let me know if i80 video timing
> values can be passed with 'struct fb_videomode', I will do the changes
> as you have suggested and quickly repost this patchset.
> 
> Thanks,
> Thomas.

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Tuesday, March 06, 2012 2:26 PM
> To: Jingoo Han
> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
> 
> >> -----Original Message-----
> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> Sent: Sunday, March 04, 2012 12:50 AM
> >> To: linux-fbdev@vger.kernel.org
> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >>
> >> The video interface timing is independent of the window setup data.
> >> The resolution of the window can be smaller than that of the lcd
> >> panel to which the video data is output.
> >>
> >> So move the video timing data from the per-window setup data to the
> >> platform specific section in the platform data. This also removes
> >> the restriction that atleast one window should have the same
> >> resolution as that of the panel attached.
> >>
> >> Cc: Ben Dooks <ben-linux@fluff.org>
> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
> >>  2 files changed, 63 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> >> index 0fedf47..39d6bd7 100644
> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> >> @@ -24,15 +24,16 @@
> >>
> >>  /**
> >>   * struct s3c_fb_pd_win - per window setup data
> >> - * @win_mode: The display parameters to initialise (not for window 0)
> >> + * @xres     : The window X size.
> >> + * @yres     : The window Y size.
> >>   * @virtual_x: The virtual X size.
> >>   * @virtual_y: The virtual Y size.
> >>   */
> >>  struct s3c_fb_pd_win {
> >> -     struct fb_videomode     win_mode;
> >> -
> >>       unsigned short          default_bpp;
> >>       unsigned short          max_bpp;
> >> +     unsigned short          xres;
> >> +     unsigned short          yres;
> >>       unsigned short          virtual_x;
> >>       unsigned short          virtual_y;
> >>  };
> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
> >>   * @default_win: default window layer number to be used for UI layer.
> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
> >> + * @vtiming: Video timing when connected to a RGB type panel.
> >
> > fb_videomode can be set, even if it is not RGB type panel.
> > In my opinion, it would be better.
> > + * @vtiming: The video timing values to set the interface timing of the panel.
> 
> The other interface that is supported is the i80 interface. Can these
> timing values be used for i80 interface as well ?

No, you're right. The i80 is not supported.
Please ignore my comment.

> 
> >
> >>   * @win: The setup data for each hardware window, or NULL for unused.
> >>   * @display_mode: The LCD output display mode.
> >>   *
> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
> >>       void    (*setup_gpio)(void);
> >>
> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
> >> +     struct fb_videomode     *vtiming;
> >>
> >>       u32                      default_win;
> >>
> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> >> index 1fb7ddf..8e05d4d 100644
> >> --- a/drivers/video/s3c-fb.c
> >> +++ b/drivers/video/s3c-fb.c
> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
> >>       u32 alpha = 0;
> >>       u32 data;
> >>       u32 pagewidth;
> >> -     int clkdiv;
> >>
> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> >>
> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
> >>       /* disable the window whilst we update it */
> >>       writel(0, regs + WINCON(win_no));
> >>
> >> -     /* use platform specified window as the basis for the lcd timings */
> >> -
> >> -     if (win_no == sfb->pdata->default_win) {
> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> >> -
> >> -             data = sfb->pdata->vidcon0;
> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> -
> >> -             if (clkdiv > 1)
> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> -             else
> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> -
> >> -             /* write the timing data to the panel */
> >> -
> >> -             if (sfb->variant.is_2443)
> >> -                     data |= (1 << 5);
> >> -
> >> -             writel(data, regs + VIDCON0);
> >> -
> >> +     if (win_no == sfb->pdata->default_win)
> >>               s3c_fb_enable(sfb, 1);
> >>
> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
> >> -
> >> -             writel(data, regs + sfb->variant.vidtcon);
> >> -
> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
> >> -
> >> -             /* VIDTCON1 */
> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
> >> -
> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
> >> -     }
> >> -
> >
> > It looks good.
> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
> >
> >>       /* write the buffer address */
> >>
> >>       /* start and end registers stride is 8 */
> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> >>
> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
> >>
> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
> >> +     real_size = windata->xres * windata->yres;
> >>       virt_size = windata->virtual_x * windata->virtual_y;
> >>
> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
> >> +             real_size, windata->xres, windata->yres,
> >>               virt_size, windata->virtual_x, windata->virtual_y);
> >>
> >>       size = (real_size > virt_size) ? real_size : virt_size;
> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>                                     struct s3c_fb_win **res)
> >>  {
> >>       struct fb_var_screeninfo *var;
> >> -     struct fb_videomode *initmode;
> >> +     struct fb_videomode initmode;
> >
> > *initmode cannot be used???
> > Can you tell me why pointer type should be changed?
> >
> 
> The initmode is used to pass video timing to the fb_videomode_to_var()
> function. Each window setup data in platform data included video
> timing information. Since, the video timing data is now moved out of
> per-window data, the xres and yres values have to be setup based on
> the window for which the fb_videomode_to_var() is called. Hence, the
> common timing values is first copied into initmode and then the window
> specific xres and yres are set. If initmode is a maintained as a
> pointer (to the video timing data in platform data), then any xres and
> yres update to initmode would overwrite the 'constant' video timing.

My point is that variable type 'initmode' can be not changed by using pointer type as follows:

@@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
        }

        windata = sfb->pdata->win[win_no];
-       initmode = &windata->win_mode;
+       initmode = sfb->pdata->vtiming;

        WARN_ON(windata->max_bpp == 0);
-       WARN_ON(windata->win_mode.xres == 0);
-       WARN_ON(windata->win_mode.yres == 0);
+       WARN_ON(windata->xres == 0);
+       WARN_ON(windata->yres == 0);

        win = fbinfo->par;
        *res = win;
@@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
        }

        /* setup the initial video mode from the window */
+       initmode->xres = windata->xres;
+       initmode->yres = windata->yres;
        fb_videomode_to_var(&fbinfo->var, initmode);

        fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;

In this case, you don't need additional change such as following.
-	struct fb_videomode *initmode;
+	struct fb_videomode initmode;
...
-	fb_videomode_to_var(&fbinfo->var, initmode);
+	fb_videomode_to_var(&fbinfo->var, &initmode);

> 
> 
> >>       struct s3c_fb_pd_win *windata;
> >>       struct s3c_fb_win *win;
> >>       struct fb_info *fbinfo;
> >> @@ -1243,11 +1205,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>       }
> >>
> >>       windata = sfb->pdata->win[win_no];
> >> -     initmode = &windata->win_mode;
> >> +     initmode = *sfb->pdata->vtiming;
> >>
> >>       WARN_ON(windata->max_bpp == 0);
> >> -     WARN_ON(windata->win_mode.xres == 0);
> >> -     WARN_ON(windata->win_mode.yres == 0);
> >> +     WARN_ON(windata->xres == 0);
> >> +     WARN_ON(windata->yres == 0);
> >>
> >>       win = fbinfo->par;
> >>       *res = win;
> >> @@ -1286,7 +1248,9 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>       }
> >>
> >>       /* setup the initial video mode from the window */
> >> -     fb_videomode_to_var(&fbinfo->var, initmode);
> >> +     initmode.xres = windata->xres;
> >> +     initmode.yres = windata->yres;
> 
> Here, the xres and yres values are copied to initmode and described above.
> 
> >> +     fb_videomode_to_var(&fbinfo->var, &initmode);
> >>
> >>       fbinfo->fix.type        = FB_TYPE_PACKED_PIXELS;
> >>       fbinfo->fix.accel       = FB_ACCEL_NONE;
> >> @@ -1331,6 +1295,51 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >>  }
> >>
> >>  /**
> >> + * s3c_fb_set_rgb_timing() - set video timing for rgb interface.
> >> + * @sfb: The base resources for the hardware.
> >> + *
> >> + * Set horizontal and vertical lcd rgb interface timing.
> >> + */
> >> +static void s3c_fb_set_rgb_timing(struct s3c_fb *sfb)
> >> +{
> >> +     struct fb_videomode *vmode = sfb->pdata->vtiming;
> >> +     void __iomem *regs = sfb->regs;
> >> +     int clkdiv;
> >> +     u32 data;
> >> +
> >> +     if (!vmode->pixclock)
> >> +             s3c_fb_missing_pixclock(vmode);
> >> +
> >> +     clkdiv = s3c_fb_calc_pixclk(sfb, vmode->pixclock);
> >> +
> >> +     data = sfb->pdata->vidcon0;
> >> +     data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> +
> >> +     if (clkdiv > 1)
> >> +             data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> +     else
> >> +             data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> +
> >> +     if (sfb->variant.is_2443)
> >> +             data |= (1 << 5);
> >> +     writel(data, regs + VIDCON0);
> >> +
> >> +     data = VIDTCON0_VBPD(vmode->upper_margin - 1) |
> >> +            VIDTCON0_VFPD(vmode->lower_margin - 1) |
> >> +            VIDTCON0_VSPW(vmode->vsync_len - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon);
> >> +
> >> +     data = VIDTCON1_HBPD(vmode->left_margin - 1) |
> >> +            VIDTCON1_HFPD(vmode->right_margin - 1) |
> >> +            VIDTCON1_HSPW(vmode->hsync_len - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon + 4);
> >> +
> >> +     data = VIDTCON2_LINEVAL(vmode->yres - 1) |
> >> +            VIDTCON2_HOZVAL(vmode->xres - 1);
> >> +     writel(data, regs + sfb->variant.vidtcon + 8);
> >> +}
> >> +
> >> +/**
> >>   * s3c_fb_clear_win() - clear hardware window registers.
> >>   * @sfb: The base resources for the hardware.
> >>   * @win: The window to process.
> >> @@ -1473,15 +1482,14 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> >>               writel(0xffffff, regs + WKEYCON1);
> >>       }
> >>
> >> +     s3c_fb_set_rgb_timing(sfb);
> >> +
> >
> > This s3c_fb_set_rgb_timing(sfb) should be added to s3c_fb_resume().
> > The timing registers should be set when s3c_fb_resume() is called.
> > If not, after resuming, timing registers such as VIDTCONx will not bet set.
> 
> Yes, I missed that. I will add it in the resume path.
> 
> Thanks for you comments. If you could let me know if i80 video timing
> values can be passed with 'struct fb_videomode', I will do the changes
> as you have suggested and quickly repost this patchset.
> 
> Thanks,
> Thomas.

Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

On 6 March 2012 11:52, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Tuesday, March 06, 2012 2:26 PM
>> To: Jingoo Han
>> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
>> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>>
>> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
>>
>> >> -----Original Message-----
>> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> >> Sent: Sunday, March 04, 2012 12:50 AM
>> >> To: linux-fbdev@vger.kernel.org
>> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>> >>
>> >> The video interface timing is independent of the window setup data.
>> >> The resolution of the window can be smaller than that of the lcd
>> >> panel to which the video data is output.
>> >>
>> >> So move the video timing data from the per-window setup data to the
>> >> platform specific section in the platform data. This also removes
>> >> the restriction that atleast one window should have the same
>> >> resolution as that of the panel attached.
>> >>
>> >> Cc: Ben Dooks <ben-linux@fluff.org>
>> >> Cc: Jingoo Han <jg1.han@samsung.com>
>> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> >> ---
>> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>> >>  2 files changed, 63 insertions(+), 52 deletions(-)
>> >>
>> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> >> index 0fedf47..39d6bd7 100644
>> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> >> @@ -24,15 +24,16 @@
>> >>
>> >>  /**
>> >>   * struct s3c_fb_pd_win - per window setup data
>> >> - * @win_mode: The display parameters to initialise (not for window 0)
>> >> + * @xres     : The window X size.
>> >> + * @yres     : The window Y size.
>> >>   * @virtual_x: The virtual X size.
>> >>   * @virtual_y: The virtual Y size.
>> >>   */
>> >>  struct s3c_fb_pd_win {
>> >> -     struct fb_videomode     win_mode;
>> >> -
>> >>       unsigned short          default_bpp;
>> >>       unsigned short          max_bpp;
>> >> +     unsigned short          xres;
>> >> +     unsigned short          yres;
>> >>       unsigned short          virtual_x;
>> >>       unsigned short          virtual_y;
>> >>  };
>> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>> >>   * @default_win: default window layer number to be used for UI layer.
>> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
>> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
>> >> + * @vtiming: Video timing when connected to a RGB type panel.
>> >
>> > fb_videomode can be set, even if it is not RGB type panel.
>> > In my opinion, it would be better.
>> > + * @vtiming: The video timing values to set the interface timing of the panel.
>>
>> The other interface that is supported is the i80 interface. Can these
>> timing values be used for i80 interface as well ?
>
> No, you're right. The i80 is not supported.
> Please ignore my comment.
>
>>
>> >
>> >>   * @win: The setup data for each hardware window, or NULL for unused.
>> >>   * @display_mode: The LCD output display mode.
>> >>   *
>> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>> >>       void    (*setup_gpio)(void);
>> >>
>> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>> >> +     struct fb_videomode     *vtiming;
>> >>
>> >>       u32                      default_win;
>> >>
>> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> >> index 1fb7ddf..8e05d4d 100644
>> >> --- a/drivers/video/s3c-fb.c
>> >> +++ b/drivers/video/s3c-fb.c
>> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>> >>       u32 alpha = 0;
>> >>       u32 data;
>> >>       u32 pagewidth;
>> >> -     int clkdiv;
>> >>
>> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
>> >>
>> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>> >>       /* disable the window whilst we update it */
>> >>       writel(0, regs + WINCON(win_no));
>> >>
>> >> -     /* use platform specified window as the basis for the lcd timings */
>> >> -
>> >> -     if (win_no == sfb->pdata->default_win) {
>> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
>> >> -
>> >> -             data = sfb->pdata->vidcon0;
>> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> >> -
>> >> -             if (clkdiv > 1)
>> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> >> -             else
>> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> >> -
>> >> -             /* write the timing data to the panel */
>> >> -
>> >> -             if (sfb->variant.is_2443)
>> >> -                     data |= (1 << 5);
>> >> -
>> >> -             writel(data, regs + VIDCON0);
>> >> -
>> >> +     if (win_no == sfb->pdata->default_win)
>> >>               s3c_fb_enable(sfb, 1);
>> >>
>> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
>> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
>> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
>> >> -
>> >> -             writel(data, regs + sfb->variant.vidtcon);
>> >> -
>> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
>> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
>> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
>> >> -
>> >> -             /* VIDTCON1 */
>> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
>> >> -
>> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
>> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
>> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
>> >> -     }
>> >> -
>> >
>> > It looks good.
>> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
>> >
>> >>       /* write the buffer address */
>> >>
>> >>       /* start and end registers stride is 8 */
>> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
>> >>
>> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
>> >>
>> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
>> >> +     real_size = windata->xres * windata->yres;
>> >>       virt_size = windata->virtual_x * windata->virtual_y;
>> >>
>> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
>> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
>> >> +             real_size, windata->xres, windata->yres,
>> >>               virt_size, windata->virtual_x, windata->virtual_y);
>> >>
>> >>       size = (real_size > virt_size) ? real_size : virt_size;
>> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>> >>                                     struct s3c_fb_win **res)
>> >>  {
>> >>       struct fb_var_screeninfo *var;
>> >> -     struct fb_videomode *initmode;
>> >> +     struct fb_videomode initmode;
>> >
>> > *initmode cannot be used???
>> > Can you tell me why pointer type should be changed?
>> >
>>
>> The initmode is used to pass video timing to the fb_videomode_to_var()
>> function. Each window setup data in platform data included video
>> timing information. Since, the video timing data is now moved out of
>> per-window data, the xres and yres values have to be setup based on
>> the window for which the fb_videomode_to_var() is called. Hence, the
>> common timing values is first copied into initmode and then the window
>> specific xres and yres are set. If initmode is a maintained as a
>> pointer (to the video timing data in platform data), then any xres and
>> yres update to initmode would overwrite the 'constant' video timing.
>
> My point is that variable type 'initmode' can be not changed by using pointer type as follows:
>
> @@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>        }
>
>        windata = sfb->pdata->win[win_no];
> -       initmode = &windata->win_mode;
> +       initmode = sfb->pdata->vtiming;

In this case, initmode is not pointing to the 'constant' lcd panel
timing values.

>
>        WARN_ON(windata->max_bpp == 0);
> -       WARN_ON(windata->win_mode.xres == 0);
> -       WARN_ON(windata->win_mode.yres == 0);
> +       WARN_ON(windata->xres == 0);
> +       WARN_ON(windata->yres == 0);
>
>        win = fbinfo->par;
>        *res = win;
> @@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>        }
>
>        /* setup the initial video mode from the window */
> +       initmode->xres = windata->xres;
> +       initmode->yres = windata->yres;

And then here, the xres and yres of the 'constant' lcd panel timing
value will be changed.

But, we don't want to change the actual lcd panel timing value. That
is the reason to first copy the lcd panel timing value into a local
copy and then update the xres and yres and then pass this local copy
of the timing value to fb_videomode_to_var() function.

Thanks,
Thomas.

[...]

Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Thomas Abraham]

On 6 March 2012 11:52, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Tuesday, March 06, 2012 2:26 PM
>> To: Jingoo Han
>> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
>> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>>
>> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
>>
>> >> -----Original Message-----
>> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> >> Sent: Sunday, March 04, 2012 12:50 AM
>> >> To: linux-fbdev@vger.kernel.org
>> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
>> >>
>> >> The video interface timing is independent of the window setup data.
>> >> The resolution of the window can be smaller than that of the lcd
>> >> panel to which the video data is output.
>> >>
>> >> So move the video timing data from the per-window setup data to the
>> >> platform specific section in the platform data. This also removes
>> >> the restriction that atleast one window should have the same
>> >> resolution as that of the panel attached.
>> >>
>> >> Cc: Ben Dooks <ben-linux@fluff.org>
>> >> Cc: Jingoo Han <jg1.han@samsung.com>
>> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> >> ---
>> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
>> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
>> >>  2 files changed, 63 insertions(+), 52 deletions(-)
>> >>
>> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> >> index 0fedf47..39d6bd7 100644
>> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> >> @@ -24,15 +24,16 @@
>> >>
>> >>  /**
>> >>   * struct s3c_fb_pd_win - per window setup data
>> >> - * @win_mode: The display parameters to initialise (not for window 0)
>> >> + * @xres     : The window X size.
>> >> + * @yres     : The window Y size.
>> >>   * @virtual_x: The virtual X size.
>> >>   * @virtual_y: The virtual Y size.
>> >>   */
>> >>  struct s3c_fb_pd_win {
>> >> -     struct fb_videomode     win_mode;
>> >> -
>> >>       unsigned short          default_bpp;
>> >>       unsigned short          max_bpp;
>> >> +     unsigned short          xres;
>> >> +     unsigned short          yres;
>> >>       unsigned short          virtual_x;
>> >>       unsigned short          virtual_y;
>> >>  };
>> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
>> >>   * @default_win: default window layer number to be used for UI layer.
>> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
>> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
>> >> + * @vtiming: Video timing when connected to a RGB type panel.
>> >
>> > fb_videomode can be set, even if it is not RGB type panel.
>> > In my opinion, it would be better.
>> > + * @vtiming: The video timing values to set the interface timing of the panel.
>>
>> The other interface that is supported is the i80 interface. Can these
>> timing values be used for i80 interface as well ?
>
> No, you're right. The i80 is not supported.
> Please ignore my comment.
>
>>
>> >
>> >>   * @win: The setup data for each hardware window, or NULL for unused.
>> >>   * @display_mode: The LCD output display mode.
>> >>   *
>> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
>> >>       void    (*setup_gpio)(void);
>> >>
>> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>> >> +     struct fb_videomode     *vtiming;
>> >>
>> >>       u32                      default_win;
>> >>
>> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> >> index 1fb7ddf..8e05d4d 100644
>> >> --- a/drivers/video/s3c-fb.c
>> >> +++ b/drivers/video/s3c-fb.c
>> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
>> >>       u32 alpha = 0;
>> >>       u32 data;
>> >>       u32 pagewidth;
>> >> -     int clkdiv;
>> >>
>> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
>> >>
>> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
>> >>       /* disable the window whilst we update it */
>> >>       writel(0, regs + WINCON(win_no));
>> >>
>> >> -     /* use platform specified window as the basis for the lcd timings */
>> >> -
>> >> -     if (win_no = sfb->pdata->default_win) {
>> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
>> >> -
>> >> -             data = sfb->pdata->vidcon0;
>> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
>> >> -
>> >> -             if (clkdiv > 1)
>> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
>> >> -             else
>> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
>> >> -
>> >> -             /* write the timing data to the panel */
>> >> -
>> >> -             if (sfb->variant.is_2443)
>> >> -                     data |= (1 << 5);
>> >> -
>> >> -             writel(data, regs + VIDCON0);
>> >> -
>> >> +     if (win_no = sfb->pdata->default_win)
>> >>               s3c_fb_enable(sfb, 1);
>> >>
>> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
>> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
>> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
>> >> -
>> >> -             writel(data, regs + sfb->variant.vidtcon);
>> >> -
>> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
>> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
>> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
>> >> -
>> >> -             /* VIDTCON1 */
>> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
>> >> -
>> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
>> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
>> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
>> >> -     }
>> >> -
>> >
>> > It looks good.
>> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
>> >
>> >>       /* write the buffer address */
>> >>
>> >>       /* start and end registers stride is 8 */
>> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
>> >>
>> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
>> >>
>> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
>> >> +     real_size = windata->xres * windata->yres;
>> >>       virt_size = windata->virtual_x * windata->virtual_y;
>> >>
>> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
>> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
>> >> +             real_size, windata->xres, windata->yres,
>> >>               virt_size, windata->virtual_x, windata->virtual_y);
>> >>
>> >>       size = (real_size > virt_size) ? real_size : virt_size;
>> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>> >>                                     struct s3c_fb_win **res)
>> >>  {
>> >>       struct fb_var_screeninfo *var;
>> >> -     struct fb_videomode *initmode;
>> >> +     struct fb_videomode initmode;
>> >
>> > *initmode cannot be used???
>> > Can you tell me why pointer type should be changed?
>> >
>>
>> The initmode is used to pass video timing to the fb_videomode_to_var()
>> function. Each window setup data in platform data included video
>> timing information. Since, the video timing data is now moved out of
>> per-window data, the xres and yres values have to be setup based on
>> the window for which the fb_videomode_to_var() is called. Hence, the
>> common timing values is first copied into initmode and then the window
>> specific xres and yres are set. If initmode is a maintained as a
>> pointer (to the video timing data in platform data), then any xres and
>> yres update to initmode would overwrite the 'constant' video timing.
>
> My point is that variable type 'initmode' can be not changed by using pointer type as follows:
>
> @@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>        }
>
>        windata = sfb->pdata->win[win_no];
> -       initmode = &windata->win_mode;
> +       initmode = sfb->pdata->vtiming;

In this case, initmode is not pointing to the 'constant' lcd panel
timing values.

>
>        WARN_ON(windata->max_bpp = 0);
> -       WARN_ON(windata->win_mode.xres = 0);
> -       WARN_ON(windata->win_mode.yres = 0);
> +       WARN_ON(windata->xres = 0);
> +       WARN_ON(windata->yres = 0);
>
>        win = fbinfo->par;
>        *res = win;
> @@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
>        }
>
>        /* setup the initial video mode from the window */
> +       initmode->xres = windata->xres;
> +       initmode->yres = windata->yres;

And then here, the xres and yres of the 'constant' lcd panel timing
value will be changed.

But, we don't want to change the actual lcd panel timing value. That
is the reason to first copy the lcd panel timing value into a local
copy and then update the xres and yres and then pass this local copy
of the timing value to fb_videomode_to_var() function.

Thanks,
Thomas.

[...]

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Tuesday, March 06, 2012 3:35 PM
> To: Jingoo Han
> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> On 6 March 2012 11:52, Jingoo Han <jg1.han@samsung.com> wrote:
> 
> >> -----Original Message-----
> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> Sent: Tuesday, March 06, 2012 2:26 PM
> >> To: Jingoo Han
> >> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> >> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >>
> >> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
> >>
> >> >> -----Original Message-----
> >> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> >> Sent: Sunday, March 04, 2012 12:50 AM
> >> >> To: linux-fbdev@vger.kernel.org
> >> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> >> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >> >>
> >> >> The video interface timing is independent of the window setup data.
> >> >> The resolution of the window can be smaller than that of the lcd
> >> >> panel to which the video data is output.
> >> >>
> >> >> So move the video timing data from the per-window setup data to the
> >> >> platform specific section in the platform data. This also removes
> >> >> the restriction that atleast one window should have the same
> >> >> resolution as that of the panel attached.
> >> >>
> >> >> Cc: Ben Dooks <ben-linux@fluff.org>
> >> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> >> ---
> >> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
> >> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
> >> >>  2 files changed, 63 insertions(+), 52 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> >> >> index 0fedf47..39d6bd7 100644
> >> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
> >> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> >> >> @@ -24,15 +24,16 @@
> >> >>
> >> >>  /**
> >> >>   * struct s3c_fb_pd_win - per window setup data
> >> >> - * @win_mode: The display parameters to initialise (not for window 0)
> >> >> + * @xres     : The window X size.
> >> >> + * @yres     : The window Y size.
> >> >>   * @virtual_x: The virtual X size.
> >> >>   * @virtual_y: The virtual Y size.
> >> >>   */
> >> >>  struct s3c_fb_pd_win {
> >> >> -     struct fb_videomode     win_mode;
> >> >> -
> >> >>       unsigned short          default_bpp;
> >> >>       unsigned short          max_bpp;
> >> >> +     unsigned short          xres;
> >> >> +     unsigned short          yres;
> >> >>       unsigned short          virtual_x;
> >> >>       unsigned short          virtual_y;
> >> >>  };
> >> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
> >> >>   * @default_win: default window layer number to be used for UI layer.
> >> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
> >> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
> >> >> + * @vtiming: Video timing when connected to a RGB type panel.
> >> >
> >> > fb_videomode can be set, even if it is not RGB type panel.
> >> > In my opinion, it would be better.
> >> > + * @vtiming: The video timing values to set the interface timing of the panel.
> >>
> >> The other interface that is supported is the i80 interface. Can these
> >> timing values be used for i80 interface as well ?
> >
> > No, you're right. The i80 is not supported.
> > Please ignore my comment.
> >
> >>
> >> >
> >> >>   * @win: The setup data for each hardware window, or NULL for unused.
> >> >>   * @display_mode: The LCD output display mode.
> >> >>   *
> >> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
> >> >>       void    (*setup_gpio)(void);
> >> >>
> >> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
> >> >> +     struct fb_videomode     *vtiming;
> >> >>
> >> >>       u32                      default_win;
> >> >>
> >> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> >> >> index 1fb7ddf..8e05d4d 100644
> >> >> --- a/drivers/video/s3c-fb.c
> >> >> +++ b/drivers/video/s3c-fb.c
> >> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
> >> >>       u32 alpha = 0;
> >> >>       u32 data;
> >> >>       u32 pagewidth;
> >> >> -     int clkdiv;
> >> >>
> >> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> >> >>
> >> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
> >> >>       /* disable the window whilst we update it */
> >> >>       writel(0, regs + WINCON(win_no));
> >> >>
> >> >> -     /* use platform specified window as the basis for the lcd timings */
> >> >> -
> >> >> -     if (win_no = sfb->pdata->default_win) {
> >> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> >> >> -
> >> >> -             data = sfb->pdata->vidcon0;
> >> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> >> -
> >> >> -             if (clkdiv > 1)
> >> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> >> -             else
> >> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> >> -
> >> >> -             /* write the timing data to the panel */
> >> >> -
> >> >> -             if (sfb->variant.is_2443)
> >> >> -                     data |= (1 << 5);
> >> >> -
> >> >> -             writel(data, regs + VIDCON0);
> >> >> -
> >> >> +     if (win_no = sfb->pdata->default_win)
> >> >>               s3c_fb_enable(sfb, 1);
> >> >>
> >> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
> >> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
> >> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
> >> >> -
> >> >> -             writel(data, regs + sfb->variant.vidtcon);
> >> >> -
> >> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
> >> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
> >> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
> >> >> -
> >> >> -             /* VIDTCON1 */
> >> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
> >> >> -
> >> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
> >> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
> >> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
> >> >> -     }
> >> >> -
> >> >
> >> > It looks good.
> >> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
> >> >
> >> >>       /* write the buffer address */
> >> >>
> >> >>       /* start and end registers stride is 8 */
> >> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> >> >>
> >> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
> >> >>
> >> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
> >> >> +     real_size = windata->xres * windata->yres;
> >> >>       virt_size = windata->virtual_x * windata->virtual_y;
> >> >>
> >> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> >> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
> >> >> +             real_size, windata->xres, windata->yres,
> >> >>               virt_size, windata->virtual_x, windata->virtual_y);
> >> >>
> >> >>       size = (real_size > virt_size) ? real_size : virt_size;
> >> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int
> win_no,
> >> >>                                     struct s3c_fb_win **res)
> >> >>  {
> >> >>       struct fb_var_screeninfo *var;
> >> >> -     struct fb_videomode *initmode;
> >> >> +     struct fb_videomode initmode;
> >> >
> >> > *initmode cannot be used???
> >> > Can you tell me why pointer type should be changed?
> >> >
> >>
> >> The initmode is used to pass video timing to the fb_videomode_to_var()
> >> function. Each window setup data in platform data included video
> >> timing information. Since, the video timing data is now moved out of
> >> per-window data, the xres and yres values have to be setup based on
> >> the window for which the fb_videomode_to_var() is called. Hence, the
> >> common timing values is first copied into initmode and then the window
> >> specific xres and yres are set. If initmode is a maintained as a
> >> pointer (to the video timing data in platform data), then any xres and
> >> yres update to initmode would overwrite the 'constant' video timing.
> >
> > My point is that variable type 'initmode' can be not changed by using pointer type as follows:
> >
> > @@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >        }
> >
> >        windata = sfb->pdata->win[win_no];
> > -       initmode = &windata->win_mode;
> > +       initmode = sfb->pdata->vtiming;
> 
> In this case, initmode is not pointing to the 'constant' lcd panel
> timing values.
> 
> >
> >        WARN_ON(windata->max_bpp = 0);
> > -       WARN_ON(windata->win_mode.xres = 0);
> > -       WARN_ON(windata->win_mode.yres = 0);
> > +       WARN_ON(windata->xres = 0);
> > +       WARN_ON(windata->yres = 0);
> >
> >        win = fbinfo->par;
> >        *res = win;
> > @@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >        }
> >
> >        /* setup the initial video mode from the window */
> > +       initmode->xres = windata->xres;
> > +       initmode->yres = windata->yres;
> 
> And then here, the xres and yres of the 'constant' lcd panel timing
> value will be changed.
> 
> But, we don't want to change the actual lcd panel timing value. That
> is the reason to first copy the lcd panel timing value into a local
> copy and then update the xres and yres and then pass this local copy
> of the timing value to fb_videomode_to_var() function.

OK. I see. You're right.

If you add add it in the resume path in the resume path,
you can use my acked-by to this 1st patch.

Thank you for sending the patch.

Best regards,
Jingoo Han

> 
> Thanks,
> Thomas.
> 
> [...]

RE: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Tuesday, March 06, 2012 3:35 PM
> To: Jingoo Han
> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> 
> On 6 March 2012 11:52, Jingoo Han <jg1.han@samsung.com> wrote:
> 
> >> -----Original Message-----
> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> Sent: Tuesday, March 06, 2012 2:26 PM
> >> To: Jingoo Han
> >> Cc: linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org;
> >> kgene.kim@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> Subject: Re: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >>
> >> On 6 March 2012 10:15, Jingoo Han <jg1.han@samsung.com> wrote:
> >>
> >> >> -----Original Message-----
> >> >> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> >> >> Sent: Sunday, March 04, 2012 12:50 AM
> >> >> To: linux-fbdev@vger.kernel.org
> >> >> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> >> >> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> >> >> Subject: [PATCH 1/3] video: s3c-fb: move video interface timing out of window setup data
> >> >>
> >> >> The video interface timing is independent of the window setup data.
> >> >> The resolution of the window can be smaller than that of the lcd
> >> >> panel to which the video data is output.
> >> >>
> >> >> So move the video timing data from the per-window setup data to the
> >> >> platform specific section in the platform data. This also removes
> >> >> the restriction that atleast one window should have the same
> >> >> resolution as that of the panel attached.
> >> >>
> >> >> Cc: Ben Dooks <ben-linux@fluff.org>
> >> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> >> ---
> >> >>  arch/arm/plat-samsung/include/plat/fb.h |    9 ++-
> >> >>  drivers/video/s3c-fb.c                  |  106 +++++++++++++++++--------------
> >> >>  2 files changed, 63 insertions(+), 52 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> >> >> index 0fedf47..39d6bd7 100644
> >> >> --- a/arch/arm/plat-samsung/include/plat/fb.h
> >> >> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> >> >> @@ -24,15 +24,16 @@
> >> >>
> >> >>  /**
> >> >>   * struct s3c_fb_pd_win - per window setup data
> >> >> - * @win_mode: The display parameters to initialise (not for window 0)
> >> >> + * @xres     : The window X size.
> >> >> + * @yres     : The window Y size.
> >> >>   * @virtual_x: The virtual X size.
> >> >>   * @virtual_y: The virtual Y size.
> >> >>   */
> >> >>  struct s3c_fb_pd_win {
> >> >> -     struct fb_videomode     win_mode;
> >> >> -
> >> >>       unsigned short          default_bpp;
> >> >>       unsigned short          max_bpp;
> >> >> +     unsigned short          xres;
> >> >> +     unsigned short          yres;
> >> >>       unsigned short          virtual_x;
> >> >>       unsigned short          virtual_y;
> >> >>  };
> >> >> @@ -45,6 +46,7 @@ struct s3c_fb_pd_win {
> >> >>   * @default_win: default window layer number to be used for UI layer.
> >> >>   * @vidcon0: The base vidcon0 values to control the panel data format.
> >> >>   * @vidcon1: The base vidcon1 values to control the panel data output.
> >> >> + * @vtiming: Video timing when connected to a RGB type panel.
> >> >
> >> > fb_videomode can be set, even if it is not RGB type panel.
> >> > In my opinion, it would be better.
> >> > + * @vtiming: The video timing values to set the interface timing of the panel.
> >>
> >> The other interface that is supported is the i80 interface. Can these
> >> timing values be used for i80 interface as well ?
> >
> > No, you're right. The i80 is not supported.
> > Please ignore my comment.
> >
> >>
> >> >
> >> >>   * @win: The setup data for each hardware window, or NULL for unused.
> >> >>   * @display_mode: The LCD output display mode.
> >> >>   *
> >> >> @@ -58,6 +60,7 @@ struct s3c_fb_platdata {
> >> >>       void    (*setup_gpio)(void);
> >> >>
> >> >>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
> >> >> +     struct fb_videomode     *vtiming;
> >> >>
> >> >>       u32                      default_win;
> >> >>
> >> >> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> >> >> index 1fb7ddf..8e05d4d 100644
> >> >> --- a/drivers/video/s3c-fb.c
> >> >> +++ b/drivers/video/s3c-fb.c
> >> >> @@ -495,7 +495,6 @@ static int s3c_fb_set_par(struct fb_info *info)
> >> >>       u32 alpha = 0;
> >> >>       u32 data;
> >> >>       u32 pagewidth;
> >> >> -     int clkdiv;
> >> >>
> >> >>       dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> >> >>
> >> >> @@ -532,46 +531,9 @@ static int s3c_fb_set_par(struct fb_info *info)
> >> >>       /* disable the window whilst we update it */
> >> >>       writel(0, regs + WINCON(win_no));
> >> >>
> >> >> -     /* use platform specified window as the basis for the lcd timings */
> >> >> -
> >> >> -     if (win_no == sfb->pdata->default_win) {
> >> >> -             clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
> >> >> -
> >> >> -             data = sfb->pdata->vidcon0;
> >> >> -             data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
> >> >> -
> >> >> -             if (clkdiv > 1)
> >> >> -                     data |= VIDCON0_CLKVAL_F(clkdiv-1) | VIDCON0_CLKDIR;
> >> >> -             else
> >> >> -                     data &= ~VIDCON0_CLKDIR;        /* 1:1 clock */
> >> >> -
> >> >> -             /* write the timing data to the panel */
> >> >> -
> >> >> -             if (sfb->variant.is_2443)
> >> >> -                     data |= (1 << 5);
> >> >> -
> >> >> -             writel(data, regs + VIDCON0);
> >> >> -
> >> >> +     if (win_no == sfb->pdata->default_win)
> >> >>               s3c_fb_enable(sfb, 1);
> >> >>
> >> >> -             data = VIDTCON0_VBPD(var->upper_margin - 1) |
> >> >> -                    VIDTCON0_VFPD(var->lower_margin - 1) |
> >> >> -                    VIDTCON0_VSPW(var->vsync_len - 1);
> >> >> -
> >> >> -             writel(data, regs + sfb->variant.vidtcon);
> >> >> -
> >> >> -             data = VIDTCON1_HBPD(var->left_margin - 1) |
> >> >> -                    VIDTCON1_HFPD(var->right_margin - 1) |
> >> >> -                    VIDTCON1_HSPW(var->hsync_len - 1);
> >> >> -
> >> >> -             /* VIDTCON1 */
> >> >> -             writel(data, regs + sfb->variant.vidtcon + 4);
> >> >> -
> >> >> -             data = VIDTCON2_LINEVAL(var->yres - 1) |
> >> >> -                    VIDTCON2_HOZVAL(var->xres - 1);
> >> >> -             writel(data, regs + sfb->variant.vidtcon + 8);
> >> >> -     }
> >> >> -
> >> >
> >> > It looks good.
> >> > VIDTCON registers don't need to be written whenever s3c_fb_set_par is called.
> >> >
> >> >>       /* write the buffer address */
> >> >>
> >> >>       /* start and end registers stride is 8 */
> >> >> @@ -1136,11 +1098,11 @@ static int __devinit s3c_fb_alloc_memory(struct s3c_fb *sfb,
> >> >>
> >> >>       dev_dbg(sfb->dev, "allocating memory for display\n");
> >> >>
> >> >> -     real_size = windata->win_mode.xres * windata->win_mode.yres;
> >> >> +     real_size = windata->xres * windata->yres;
> >> >>       virt_size = windata->virtual_x * windata->virtual_y;
> >> >>
> >> >>       dev_dbg(sfb->dev, "real_size=%u (%u.%u), virt_size=%u (%u.%u)\n",
> >> >> -             real_size, windata->win_mode.xres, windata->win_mode.yres,
> >> >> +             real_size, windata->xres, windata->yres,
> >> >>               virt_size, windata->virtual_x, windata->virtual_y);
> >> >>
> >> >>       size = (real_size > virt_size) ? real_size : virt_size;
> >> >> @@ -1222,7 +1184,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int
> win_no,
> >> >>                                     struct s3c_fb_win **res)
> >> >>  {
> >> >>       struct fb_var_screeninfo *var;
> >> >> -     struct fb_videomode *initmode;
> >> >> +     struct fb_videomode initmode;
> >> >
> >> > *initmode cannot be used???
> >> > Can you tell me why pointer type should be changed?
> >> >
> >>
> >> The initmode is used to pass video timing to the fb_videomode_to_var()
> >> function. Each window setup data in platform data included video
> >> timing information. Since, the video timing data is now moved out of
> >> per-window data, the xres and yres values have to be setup based on
> >> the window for which the fb_videomode_to_var() is called. Hence, the
> >> common timing values is first copied into initmode and then the window
> >> specific xres and yres are set. If initmode is a maintained as a
> >> pointer (to the video timing data in platform data), then any xres and
> >> yres update to initmode would overwrite the 'constant' video timing.
> >
> > My point is that variable type 'initmode' can be not changed by using pointer type as follows:
> >
> > @@ -1243,11 +1243,11 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >        }
> >
> >        windata = sfb->pdata->win[win_no];
> > -       initmode = &windata->win_mode;
> > +       initmode = sfb->pdata->vtiming;
> 
> In this case, initmode is not pointing to the 'constant' lcd panel
> timing values.
> 
> >
> >        WARN_ON(windata->max_bpp == 0);
> > -       WARN_ON(windata->win_mode.xres == 0);
> > -       WARN_ON(windata->win_mode.yres == 0);
> > +       WARN_ON(windata->xres == 0);
> > +       WARN_ON(windata->yres == 0);
> >
> >        win = fbinfo->par;
> >        *res = win;
> > @@ -1286,6 +1286,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> >        }
> >
> >        /* setup the initial video mode from the window */
> > +       initmode->xres = windata->xres;
> > +       initmode->yres = windata->yres;
> 
> And then here, the xres and yres of the 'constant' lcd panel timing
> value will be changed.
> 
> But, we don't want to change the actual lcd panel timing value. That
> is the reason to first copy the lcd panel timing value into a local
> copy and then update the xres and yres and then pass this local copy
> of the timing value to fb_videomode_to_var() function.

OK. I see. You're right.

If you add add it in the resume path in the resume path,
you can use my acked-by to this 1st patch.

Thank you for sending the patch.

Best regards,
Jingoo Han

> 
> Thanks,
> Thomas.
> 
> [...]

RE: [PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board
> 
> 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: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>

It looks good.

Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  arch/arm/mach-exynos/mach-origen.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
> index f57aed4..dc4ecc3 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			= 512,
> +	.yres			= 300,
>  	.max_bpp		= 32,
>  	.default_bpp		= 24,
>  };
> 
> +static struct fb_videomode lcd_hv070wsa_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	= &lcd_hv070wsa_timing,
>  	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>  	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC |
>  				VIDCON1_INV_VCLK,
> --
> 1.6.6.rc2

RE: [PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 3/3] ARM: Exynos: Rework platform data for lcd controller for Origen board
> 
> 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: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>

It looks good.

Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  arch/arm/mach-exynos/mach-origen.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
> index f57aed4..dc4ecc3 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			= 512,
> +	.yres			= 300,
>  	.max_bpp		= 32,
>  	.default_bpp		= 24,
>  };
> 
> +static struct fb_videomode lcd_hv070wsa_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	= &lcd_hv070wsa_timing,
>  	.vidcon0	= VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>  	.vidcon1	= VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC |
>  				VIDCON1_INV_VCLK,
> --
> 1.6.6.rc2

RE: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data
> 
> The decision to enable or disable the data output to the lcd panel from
> the controller need not be based on the value of 'default_win' element
> in the platform data. Instead, the data output to the panel is enabled
> if any of the windows are active, else data output is disabled.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/include/plat/fb.h |    2 --
>  drivers/video/s3c-fb.c                  |   24 ++++--------------------
>  2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> index 39d6bd7..536002f 100644
> --- a/arch/arm/plat-samsung/include/plat/fb.h
> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> @@ -62,8 +62,6 @@ struct s3c_fb_platdata {
>  	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
>  	struct fb_videomode     *vtiming;
> 
> -	u32			 default_win;
> -
>  	u32			 vidcon0;
>  	u32			 vidcon1;
>  };
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 8e05d4d..8baba31 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	/* disable the window whilst we update it */
>  	writel(0, regs + WINCON(win_no));
> 
> -	if (win_no = sfb->pdata->default_win)
> +	if (!sfb->output_on)
>  		s3c_fb_enable(sfb, 1);
> 
>  	/* write the buffer address */
> @@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  	struct s3c_fb_win *win = info->par;
>  	struct s3c_fb *sfb = win->parent;
>  	unsigned int index = win->index;
> -	u32 wincon;
> +	u32 wincon, output_on = sfb->output_on;

Can you add new line as below? It's more readable.
+	u32 output_on = sfb->output_on;
Sorry for nitpicking.

> 
>  	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
> 
> @@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  	 * it is highly likely that we also do not need to output
>  	 * anything.
>  	 */
> -
> -	/* We could do something like the following code, but the current
> -	 * system of using framebuffer events means that we cannot make
> -	 * the distinction between just window 0 being inactive and all
> -	 * the windows being down.
> -	 *
> -	 * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
> -	*/
> -
> -	/* we're stuck with this until we can do something about overriding
> -	 * the power control using the blanking event for a single fb.
> -	 */
> -	if (index = sfb->pdata->default_win) {
> -		shadow_protect_win(win, 1);
> -		s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
> -		shadow_protect_win(win, 0);
> -	}
> +	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);

However, shadow_protect_win() is necessary as belows.
Because shadow registers such as VIDCON0 should be protectd
whenever the registers are updated. The s3c_fb_enable() updates
VIDCON0.
+	shadow_protect_win(win, 1);
+	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
+	shadow_protect_win(win, 0);

> 
>  	pm_runtime_put_sync(sfb->dev);
> 
> -	return 0;
> +	return output_on = sfb->output_on;
>  }
> 
>  /**
> --
> 1.6.6.rc2

RE: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Jingoo Han]

> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Sunday, March 04, 2012 12:50 AM
> To: linux-fbdev@vger.kernel.org
> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
> Subject: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data
> 
> The decision to enable or disable the data output to the lcd panel from
> the controller need not be based on the value of 'default_win' element
> in the platform data. Instead, the data output to the panel is enabled
> if any of the windows are active, else data output is disabled.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/include/plat/fb.h |    2 --
>  drivers/video/s3c-fb.c                  |   24 ++++--------------------
>  2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
> index 39d6bd7..536002f 100644
> --- a/arch/arm/plat-samsung/include/plat/fb.h
> +++ b/arch/arm/plat-samsung/include/plat/fb.h
> @@ -62,8 +62,6 @@ struct s3c_fb_platdata {
>  	struct s3c_fb_pd_win	*win[S3C_FB_MAX_WIN];
>  	struct fb_videomode     *vtiming;
> 
> -	u32			 default_win;
> -
>  	u32			 vidcon0;
>  	u32			 vidcon1;
>  };
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 8e05d4d..8baba31 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
>  	/* disable the window whilst we update it */
>  	writel(0, regs + WINCON(win_no));
> 
> -	if (win_no == sfb->pdata->default_win)
> +	if (!sfb->output_on)
>  		s3c_fb_enable(sfb, 1);
> 
>  	/* write the buffer address */
> @@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  	struct s3c_fb_win *win = info->par;
>  	struct s3c_fb *sfb = win->parent;
>  	unsigned int index = win->index;
> -	u32 wincon;
> +	u32 wincon, output_on = sfb->output_on;

Can you add new line as below? It's more readable.
+	u32 output_on = sfb->output_on;
Sorry for nitpicking.

> 
>  	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
> 
> @@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  	 * it is highly likely that we also do not need to output
>  	 * anything.
>  	 */
> -
> -	/* We could do something like the following code, but the current
> -	 * system of using framebuffer events means that we cannot make
> -	 * the distinction between just window 0 being inactive and all
> -	 * the windows being down.
> -	 *
> -	 * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
> -	*/
> -
> -	/* we're stuck with this until we can do something about overriding
> -	 * the power control using the blanking event for a single fb.
> -	 */
> -	if (index == sfb->pdata->default_win) {
> -		shadow_protect_win(win, 1);
> -		s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
> -		shadow_protect_win(win, 0);
> -	}
> +	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);

However, shadow_protect_win() is necessary as belows.
Because shadow registers such as VIDCON0 should be protectd
whenever the registers are updated. The s3c_fb_enable() updates
VIDCON0.
+	shadow_protect_win(win, 1);
+	s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
+	shadow_protect_win(win, 0);

> 
>  	pm_runtime_put_sync(sfb->dev);
> 
> -	return 0;
> +	return output_on == sfb->output_on;
>  }
> 
>  /**
> --
> 1.6.6.rc2

Re: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Thomas Abraham]

On 6 March 2012 15:35, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data
>>
>> The decision to enable or disable the data output to the lcd panel from
>> the controller need not be based on the value of 'default_win' element
>> in the platform data. Instead, the data output to the panel is enabled
>> if any of the windows are active, else data output is disabled.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  arch/arm/plat-samsung/include/plat/fb.h |    2 --
>>  drivers/video/s3c-fb.c                  |   24 ++++--------------------
>>  2 files changed, 4 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> index 39d6bd7..536002f 100644
>> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> @@ -62,8 +62,6 @@ struct s3c_fb_platdata {
>>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>>       struct fb_videomode     *vtiming;
>>
>> -     u32                      default_win;
>> -
>>       u32                      vidcon0;
>>       u32                      vidcon1;
>>  };
>> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> index 8e05d4d..8baba31 100644
>> --- a/drivers/video/s3c-fb.c
>> +++ b/drivers/video/s3c-fb.c
>> @@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       /* disable the window whilst we update it */
>>       writel(0, regs + WINCON(win_no));
>>
>> -     if (win_no == sfb->pdata->default_win)
>> +     if (!sfb->output_on)
>>               s3c_fb_enable(sfb, 1);
>>
>>       /* write the buffer address */
>> @@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>>       struct s3c_fb_win *win = info->par;
>>       struct s3c_fb *sfb = win->parent;
>>       unsigned int index = win->index;
>> -     u32 wincon;
>> +     u32 wincon, output_on = sfb->output_on;
>
> Can you add new line as below? It's more readable.
> +       u32 output_on = sfb->output_on;
> Sorry for nitpicking.

Ok. I will add a new line as you have suggested.

>
>>
>>       dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
>>
>> @@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>>        * it is highly likely that we also do not need to output
>>        * anything.
>>        */
>> -
>> -     /* We could do something like the following code, but the current
>> -      * system of using framebuffer events means that we cannot make
>> -      * the distinction between just window 0 being inactive and all
>> -      * the windows being down.
>> -      *
>> -      * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
>> -     */
>> -
>> -     /* we're stuck with this until we can do something about overriding
>> -      * the power control using the blanking event for a single fb.
>> -      */
>> -     if (index == sfb->pdata->default_win) {
>> -             shadow_protect_win(win, 1);
>> -             s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
>> -             shadow_protect_win(win, 0);
>> -     }
>> +     s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
>
> However, shadow_protect_win() is necessary as belows.
> Because shadow registers such as VIDCON0 should be protectd
> whenever the registers are updated. The s3c_fb_enable() updates
> VIDCON0.
> +       shadow_protect_win(win, 1);
> +       s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
> +       shadow_protect_win(win, 0);

Right. Thanks for the correction. I will submit updated patch series soon.

Regards,
Thomas.

Re: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data

[Thomas Abraham]

On 6 March 2012 15:35, Jingoo Han <jg1.han@samsung.com> wrote:

>> -----Original Message-----
>> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
>> Sent: Sunday, March 04, 2012 12:50 AM
>> To: linux-fbdev@vger.kernel.org
>> Cc: FlorianSchandinat@gmx.de; linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> jg1.han@samsung.com; ben-linux@fluff.org; patches@linaro.org
>> Subject: [PATCH 2/3] video: s3c-fb: remove 'default_win' element from platform data
>>
>> The decision to enable or disable the data output to the lcd panel from
>> the controller need not be based on the value of 'default_win' element
>> in the platform data. Instead, the data output to the panel is enabled
>> if any of the windows are active, else data output is disabled.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  arch/arm/plat-samsung/include/plat/fb.h |    2 --
>>  drivers/video/s3c-fb.c                  |   24 ++++--------------------
>>  2 files changed, 4 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
>> index 39d6bd7..536002f 100644
>> --- a/arch/arm/plat-samsung/include/plat/fb.h
>> +++ b/arch/arm/plat-samsung/include/plat/fb.h
>> @@ -62,8 +62,6 @@ struct s3c_fb_platdata {
>>       struct s3c_fb_pd_win    *win[S3C_FB_MAX_WIN];
>>       struct fb_videomode     *vtiming;
>>
>> -     u32                      default_win;
>> -
>>       u32                      vidcon0;
>>       u32                      vidcon1;
>>  };
>> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
>> index 8e05d4d..8baba31 100644
>> --- a/drivers/video/s3c-fb.c
>> +++ b/drivers/video/s3c-fb.c
>> @@ -531,7 +531,7 @@ static int s3c_fb_set_par(struct fb_info *info)
>>       /* disable the window whilst we update it */
>>       writel(0, regs + WINCON(win_no));
>>
>> -     if (win_no = sfb->pdata->default_win)
>> +     if (!sfb->output_on)
>>               s3c_fb_enable(sfb, 1);
>>
>>       /* write the buffer address */
>> @@ -792,7 +792,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>>       struct s3c_fb_win *win = info->par;
>>       struct s3c_fb *sfb = win->parent;
>>       unsigned int index = win->index;
>> -     u32 wincon;
>> +     u32 wincon, output_on = sfb->output_on;
>
> Can you add new line as below? It's more readable.
> +       u32 output_on = sfb->output_on;
> Sorry for nitpicking.

Ok. I will add a new line as you have suggested.

>
>>
>>       dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
>>
>> @@ -838,27 +838,11 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>>        * it is highly likely that we also do not need to output
>>        * anything.
>>        */
>> -
>> -     /* We could do something like the following code, but the current
>> -      * system of using framebuffer events means that we cannot make
>> -      * the distinction between just window 0 being inactive and all
>> -      * the windows being down.
>> -      *
>> -      * s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
>> -     */
>> -
>> -     /* we're stuck with this until we can do something about overriding
>> -      * the power control using the blanking event for a single fb.
>> -      */
>> -     if (index = sfb->pdata->default_win) {
>> -             shadow_protect_win(win, 1);
>> -             s3c_fb_enable(sfb, blank_mode != FB_BLANK_POWERDOWN ? 1 : 0);
>> -             shadow_protect_win(win, 0);
>> -     }
>> +     s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
>
> However, shadow_protect_win() is necessary as belows.
> Because shadow registers such as VIDCON0 should be protectd
> whenever the registers are updated. The s3c_fb_enable() updates
> VIDCON0.
> +       shadow_protect_win(win, 1);
> +       s3c_fb_enable(sfb, sfb->enabled ? 1 : 0);
> +       shadow_protect_win(win, 0);

Right. Thanks for the correction. I will submit updated patch series soon.

Regards,
Thomas.