[PATCHv2] Add dynamic video initialization to barebox

[PATCHv2] Add dynamic video initialization to barebox

[Juergen Beisert]

Currently barebox uses a fixed videomode setup. Everything is compiled in.
This change adds the possibility to select a videomode according to a
connected display at runtime. The current behaviour is still present if not
otherwise configured. If configured for runtime setup, initialization of the
video hardware will be delayed until the required videomode will be selected
from the shell code. If more than one videomode is supported by the platform,
running the 'devinfo' command on the framebuffer device shows the supported
videomode list. After selecting the videomode, the output can be enabled.

On my hardware I can connect a regular monitor, which supports more than one
videomode. The runtime setup sequence looks like that:

tx28:/ devinfo framebuffer0
base  : 0x00000000
size  : 0x00000000
driver: framebuffer

 Video/Mode info:
   Video output not enabled
 Current video mode:
   No video mode selected yet
 Supported video mode(s):
   'VGA'
   'SVGA'
   'XGA'
   'SXGA'
 Parameters:
           cdepth = 16
             mode = <NULL>
           enable = <NULL>

tx28:/ framebuffer0.mode=SVGA
tx28:/ bmp splash.bmp
tx28:/ framebuffer0.enable=1

After the last command the monitor shows the splash screen.

This is revision 2 of this patch stack.
 - enabling the video output is now always required to get a picture
 - driver specific data can now be added on a per videomode base
 - documentation is adapted to the new behaviour

jbe


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 01/12] Separate framebuffer platformdata and the videomode

[Juergen Beisert]

This patch separates the imx platformdata and its videomode in two structures,
in order to support more than one defined videomode in the boardfile. This
is intended to support runtime videomode selection later on. It also uses
now the same videomode setup style than the imx-fpu based systems (like the
i.MX35).

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c |   28 +++++++++--------
 arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c |   28 +++++++++--------
 arch/arm/boards/guf-neso/board.c                  |   28 +++++++++--------
 arch/arm/boards/imx21ads/imx21ads.c               |   34 +++++++++++----------
 arch/arm/boards/pcm038/pcm038.c                   |   28 +++++++++--------
 arch/arm/mach-imx/include/mach/imxfb.h            |    2 +-
 drivers/video/imx.c                               |    6 ++--
 7 files changed, 82 insertions(+), 72 deletions(-)

diff --git a/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c b/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
index 805ffe2..032897a 100644
--- a/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
+++ b/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
@@ -120,20 +120,22 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
+static struct fb_videomode cmo_display = {
+	.name		= "CMO-QVGA",
+	.refresh	= 60,
+	.xres		= 320,
+	.yres		= 240,
+	.pixclock	= KHZ2PICOS(6500),
+	.hsync_len	= 30,
+	.left_margin	= 38,
+	.right_margin	= 20,
+	.vsync_len	= 3,
+	.upper_margin	= 15,
+	.lower_margin	= 4,
+};
+
 static struct imx_fb_videomode imxfb_mode = {
-	.mode = {
-		.name		= "CMO-QVGA",
-		.refresh	= 60,
-		.xres		= 320,
-		.yres		= 240,
-		.pixclock	= KHZ2PICOS(6500),
-		.hsync_len	= 30,
-		.left_margin	= 38,
-		.right_margin	= 20,
-		.vsync_len	= 3,
-		.upper_margin	= 15,
-		.lower_margin	= 4,
-	},
+	.mode		= &cmo_display,
 	.pcr		= 0xCAD08B80,
 	.bpp		= 16,
 };
diff --git a/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c b/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
index 62fc14e..3ea2466 100644
--- a/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
+++ b/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
@@ -186,20 +186,22 @@ static void eukrea_cpuimx27_mmu_init(void)
 #endif
 
 #ifdef CONFIG_DRIVER_VIDEO_IMX
+static struct fb_videomode cmo_display = {
+	.name		= "CMO-QVGA",
+	.refresh	= 60,
+	.xres		= 320,
+	.yres		= 240,
+	.pixclock	= 156000,
+	.hsync_len	= 30,
+	.left_margin	= 38,
+	.right_margin	= 20,
+	.vsync_len	= 3,
+	.upper_margin	= 15,
+	.lower_margin	= 4,
+};
+
 static struct imx_fb_videomode imxfb_mode = {
-	.mode = {
-		.name		= "CMO-QVGA",
-		.refresh	= 60,
-		.xres		= 320,
-		.yres		= 240,
-		.pixclock	= 156000,
-		.hsync_len	= 30,
-		.left_margin	= 38,
-		.right_margin	= 20,
-		.vsync_len	= 3,
-		.upper_margin	= 15,
-		.lower_margin	= 4,
-	},
+	.mode		= &cmo_display,
 	.pcr		= 0xFAD08B80,
 	.bpp		= 16,};
 
diff --git a/arch/arm/boards/guf-neso/board.c b/arch/arm/boards/guf-neso/board.c
index 9c85c08..6949675 100644
--- a/arch/arm/boards/guf-neso/board.c
+++ b/arch/arm/boards/guf-neso/board.c
@@ -91,20 +91,22 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
+static struct fb_videomode cpt_display = {
+	.name		= "CPT CLAA070LC0JCT",
+	.refresh	= 60,
+	.xres		= 800,
+	.yres		= 480,
+	.pixclock	= KHZ2PICOS(27000),
+	.hsync_len	= 1,	/* DE only sync */
+	.left_margin	= 50,
+	.right_margin	= 50,
+	.vsync_len	= 1,	/* DE only sync */
+	.upper_margin	= 10,
+	.lower_margin	= 10,
+};
+
 static struct imx_fb_videomode imxfb_mode = {
-	.mode = {
-		.name		= "CPT CLAA070LC0JCT",
-		.refresh	= 60,
-		.xres		= 800,
-		.yres		= 480,
-		.pixclock	= KHZ2PICOS(27000),
-		.hsync_len	= 1,	/* DE only sync */
-		.left_margin	= 50,
-		.right_margin	= 50,
-		.vsync_len	= 1,	/* DE only sync */
-		.upper_margin	= 10,
-		.lower_margin	= 10,
-	},
+	.mode = &cpt_display,
 	/*
 	 * - TFT style panel
 	 * - clk enabled while idle
diff --git a/arch/arm/boards/imx21ads/imx21ads.c b/arch/arm/boards/imx21ads/imx21ads.c
index 44d37aa..81006de 100644
--- a/arch/arm/boards/imx21ads/imx21ads.c
+++ b/arch/arm/boards/imx21ads/imx21ads.c
@@ -79,24 +79,26 @@ static struct device_d cs8900_dev = {
 	// IRQ is connected to UART3_RTS
 };
 
+static struct fb_videomode sharp_display = {
+	.name           = "Sharp-LQ035Q7",
+	.refresh        = 60,
+	.xres           = 240,
+	.yres           = 320,
+	.pixclock       = 188679,
+	.left_margin    = 6,
+	.right_margin   = 16,
+	.upper_margin   = 8,
+	.lower_margin   = 10,
+	.hsync_len      = 2,
+	.vsync_len      = 1,
+	.sync           = 0,
+	.vmode          = FB_VMODE_NONINTERLACED,
+	.flag           = 0,
+};
+
 /* Sharp LQ035Q7DB02 QVGA display */
 static struct imx_fb_videomode imx_fb_modedata = {
-        .mode = {
-		.name           = "Sharp-LQ035Q7",
-		.refresh        = 60,
-		.xres           = 240,
-		.yres           = 320,
-		.pixclock       = 188679,
-		.left_margin    = 6,
-		.right_margin   = 16,
-		.upper_margin   = 8,
-		.lower_margin   = 10,
-		.hsync_len      = 2,
-		.vsync_len      = 1,
-		.sync           = 0,
-		.vmode          = FB_VMODE_NONINTERLACED,
-		.flag           = 0,
-	},
+        .mode           = &sharp_display,
         .pcr            = 0xfb108bc7,
         .bpp            = 16,
 };
diff --git a/arch/arm/boards/pcm038/pcm038.c b/arch/arm/boards/pcm038/pcm038.c
index 3a9b413..026e9c0 100644
--- a/arch/arm/boards/pcm038/pcm038.c
+++ b/arch/arm/boards/pcm038/pcm038.c
@@ -127,20 +127,22 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
+static struct fb_videomode sharp_display = {
+	.name		= "Sharp-LQ035Q7",
+	.refresh	= 60,
+	.xres		= 240,
+	.yres		= 320,
+	.pixclock	= 188679, /* in ps (5.3MHz) */
+	.hsync_len	= 7,
+	.left_margin	= 5,
+	.right_margin	= 16,
+	.vsync_len	= 1,
+	.upper_margin	= 7,
+	.lower_margin	= 9,
+};
+
 static struct imx_fb_videomode imxfb_mode = {
-	.mode = {
-		.name		= "Sharp-LQ035Q7",
-		.refresh	= 60,
-		.xres		= 240,
-		.yres		= 320,
-		.pixclock	= 188679, /* in ps (5.3MHz) */
-		.hsync_len	= 7,
-		.left_margin	= 5,
-		.right_margin	= 16,
-		.vsync_len	= 1,
-		.upper_margin	= 7,
-		.lower_margin	= 9,
-	},
+	.mode		= &sharp_display,
 	/*
 	 * - HSYNC active high
 	 * - VSYNC active high
diff --git a/arch/arm/mach-imx/include/mach/imxfb.h b/arch/arm/mach-imx/include/mach/imxfb.h
index 16b43ea..7baa244 100644
--- a/arch/arm/mach-imx/include/mach/imxfb.h
+++ b/arch/arm/mach-imx/include/mach/imxfb.h
@@ -50,7 +50,7 @@
 #define DMACR_TM(x)	((x) & 0xf)
 
 struct imx_fb_videomode {
-	struct fb_videomode mode;
+	struct fb_videomode *mode;
 	u32 pcr;
 	unsigned char	bpp;
 };
diff --git a/drivers/video/imx.c b/drivers/video/imx.c
index ac51858..6ccd77e 100644
--- a/drivers/video/imx.c
+++ b/drivers/video/imx.c
@@ -555,9 +555,9 @@ static int imxfb_probe(struct device_d *dev)
 	fbi->enable = pdata->enable;
 	fbi->dev = dev;
 	info->priv = fbi;
-	info->mode = &pdata->mode->mode;
-	info->xres = pdata->mode->mode.xres;
-	info->yres = pdata->mode->mode.yres;
+	info->mode = pdata->mode->mode;
+	info->xres = pdata->mode->mode->xres;
+	info->yres = pdata->mode->mode->yres;
 	info->bits_per_pixel = pdata->mode->bpp;
 	info->fbops = &imxfb_ops;
 
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 02/12] Add more flags for sync control

[Juergen Beisert]

In order to make video mode setup and initializing a runtime job (currently
it is a compile time job) this patch tries to make the 'fb_videomode' structure
more generic. It should also carry special settings required only for some LC
displays. So, I add some additional sync flags to control the DE and CLCK to
the display (something a regular CRT do not know). Also the possibility to
stop the clock when outside active display data (required for (C)STN).

Further suggestions for flags?

What about the special settings for some kind of Sharp displays the i.MX
processor family supports?

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 include/fb.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/fb.h b/include/fb.h
index 379f931..218b244 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -17,11 +17,21 @@
 					/* vtotal = 144d/288n/576i => PAL  */
 					/* vtotal = 121d/242n/484i => NTSC */
 #define FB_SYNC_ON_GREEN	32	/* sync on green */
+/* LC display related settings */
+#define FB_SYNC_DE_HIGH_ACT	(1 << 6)
+#define FB_SYNC_CLK_INVERT	(1 << 7)
+#define FB_SYNC_DATA_INVERT	(1 << 8)
+#define FB_SYNC_CLK_IDLE_EN	(1 << 9)
+#define FB_SYNC_SWAP_RGB	(1 << 10)
+#define FB_SYNC_CLK_SEL_EN	(1 << 11)
+#define FB_SYNC_SHARP_MODE	(1 << 31)
 
 #define FB_VMODE_NONINTERLACED  0	/* non interlaced */
 #define FB_VMODE_INTERLACED	1	/* interlaced	*/
 #define FB_VMODE_DOUBLE		2	/* double scan */
 #define FB_VMODE_ODD_FLD_FIRST	4	/* interlaced: top line first */
+/* LC display related settings */
+#define FB_VMODE_DUAL_SCAN	8
 #define FB_VMODE_MASK		255
 
 #define FB_VMODE_YWRAP		256	/* ywrap instead of panning     */
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 03/12] Bring in dynamic videomode selection at runtime

[Juergen Beisert]

This patch mostly rewrites all parts of /drivers/video/fb.c. As it changes
the API to the drivers, it must be done in one step to keep the repository
bisectable. But to do it in one step makes the patches itself unreadable.

So, I decided to do it in a few steps, only for the review. All patches marked
with a "step n" should be merged, prior the final commit onto the repository.

This step brings in the required function for dynamic videomode selection at
runtime but keep the old functions untouched (the new one are commented out).

This is patch 1 of 7 to keep the repository bisectable.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/video/Kconfig |   13 +++
 drivers/video/fb.c    |  243 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/fb.h          |   40 ++++++++
 3 files changed, 296 insertions(+), 0 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 7a89a3f..8138226 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -5,6 +5,19 @@ menuconfig VIDEO
 
 if VIDEO
 
+comment "runtime options"
+
+config VIDEO_DELAY_INIT
+	bool "Delayed initialization"
+	help
+	  If the platform supports more than one video mode say 'y' her to delay
+	  the initialization of the video device until any kind of barebox's
+          shell code sets up the correct mode at runtime.
+          This entry adds the "mode" parameter to the video device, to setup
+          the desired videomode prior enabling it at runtime.
+
+comment "drivers"
+
 config DRIVER_VIDEO_IMX
 	bool "i.MX framebuffer driver"
 	depends on ARCH_IMX1 || ARCH_IMX21 || ARCH_IMX25 || ARCH_IMX27
diff --git a/drivers/video/fb.c b/drivers/video/fb.c
index f9a425e..c3c4761 100644
--- a/drivers/video/fb.c
+++ b/drivers/video/fb.c
@@ -1,3 +1,4 @@
+#include <init.h>
 #include <common.h>
 #include <fb.h>
 #include <errno.h>
@@ -5,6 +6,8 @@
 #include <getopt.h>
 #include <fcntl.h>
 #include <fs.h>
+#include <malloc.h>
+#include <xfuncs.h>
 
 static int fb_ioctl(struct cdev* cdev, int req, void *data)
 {
@@ -57,6 +60,139 @@ static int fb_enable_set(struct device_d *dev, struct param_d *param,
 	return 0;
 }
 
+#if 0
+#ifdef CONFIG_VIDEO_DELAY_INIT
+static int fb_check_if_already_initialized(struct device_d *fb_dev)
+{
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+
+	if (info->active_mode != NULL) {
+		pr_err("Video mode '%s' is already set. Cannot change colour depth anymore.\n", info->active_mode->name);
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+static int fb_cdepth_set(struct device_d *fb_dev, struct param_d *param, const char *val)
+{
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+	unsigned cdepth;
+	int rc;
+
+	rc = fb_check_if_already_initialized(fb_dev);
+	if (rc != 0)
+		return rc;
+
+	cdepth = simple_strtoul(val, NULL, 0);
+	if (cdepth != 0)
+		info->bits_per_pixel = cdepth;
+	else
+		return -EINVAL;
+
+	return dev_param_set_generic(fb_dev, param, val);
+}
+#endif
+
+static int fb_enable_set(struct device_d *fb_dev, struct param_d *param, const char *val)
+{
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+	struct fb_host *host = fb_dev->platform_data;
+	int enable;
+	char *new;
+
+	if (!val)
+		return dev_param_set_generic(fb_dev, param, NULL);
+
+	enable = simple_strtoul(val, NULL, 0);
+
+	if (info->enabled == !!enable)
+		return 0;
+
+	if (enable) {
+		(host->fb_enable)(info);
+		new = "1";
+	} else {
+		(host->fb_disable)(info);
+		new = "0";
+	}
+
+	info->enabled = !!enable;
+
+	return dev_param_set_generic(fb_dev, param, new);
+}
+
+static void fb_list_modes(struct fb_host *host)
+{
+	unsigned u;
+
+	if (host->mode_cnt == 0)
+		return;
+
+	printf(" Supported video mode(s):\n");
+	for (u = 0; u < host->mode_cnt; u++)
+		printf("  '%s'\n", host->mode[u].name);
+}
+
+static int fb_activate_mode(struct device_d *fb_dev, const struct fb_videomode *mode)
+{
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+	struct fb_host *host = fb_dev->platform_data;
+	int rc;
+
+	rc = (host->fb_mode)(info, mode);
+	if (rc != 0)
+		return rc;
+
+	info->active_mode = mode;
+	/*
+	 * At this point of time we know the remaining information we need
+	 * for the cdev and fb_info structure.
+	 */
+	cdev->size = fb_dev->size;
+	info->xres = mode->xres;
+	info->yres = mode->yres;
+
+	return 0;
+}
+
+#ifdef CONFIG_VIDEO_DELAY_INIT
+static int fb_mode_set(struct device_d *fb_dev, struct param_d *param, const char *name)
+{
+	struct fb_host *host = fb_dev->platform_data;
+	unsigned u;
+	int rc;
+
+	pr_debug("%s called\n", __func__);
+
+	rc = fb_check_if_already_initialized(fb_dev);
+	if (rc != 0)
+		return rc;
+
+	/* Search for the requested video mode by name */
+	for (u = 0; u < host->mode_cnt; u++) {
+		if (!strcmp(host->mode[u].name, name))
+			break;
+	}
+	if (u >= host->mode_cnt) {
+		fb_list_modes(host);	/* no mode with 'name' found */
+		return -ENODEV;
+	} else {
+		rc = fb_activate_mode(fb_dev, &host->mode[u]);
+	}
+
+	if (rc == 0)
+		dev_param_set_generic(fb_dev, param, name);
+
+	return rc;
+}
+#endif
+#endif
+
 static struct file_operations fb_ops = {
 	.read	= mem_read,
 	.write	= mem_write,
@@ -65,6 +201,113 @@ static struct file_operations fb_ops = {
 	.ioctl	= fb_ioctl,
 };
 
+#if 0
+static int add_fb_parameter(struct device_d *fb_dev)
+{
+#ifdef CONFIG_VIDEO_DELAY_INIT
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+	char cd[10];
+
+	/** @todo provide base address parameter for the user */
+
+	dev_add_param(fb_dev, "cdepth", fb_cdepth_set, NULL, 0);
+	if (info->bits_per_pixel == 0) {
+		dev_set_param(fb_dev, "cdepth", "16");
+		info->bits_per_pixel = 16;
+	} else {
+		sprintf(cd, "%u", info->bits_per_pixel);
+		dev_set_param(fb_dev, "cdepth", cd);
+	}
+	dev_add_param(fb_dev, "mode", fb_mode_set, NULL, 0);
+	/* default is 'none' for delayed video mode setup */
+#endif
+	dev_add_param(fb_dev, "enable", fb_enable_set, NULL, 0);
+	/* default is 'off' for delayed video output */
+	return 0;
+}
+
+struct framebuffer_desc {
+	struct cdev cdev;
+	struct fb_info info;
+};
+
+static int fb_probe(struct device_d *fb_dev)
+{
+	int id = get_free_deviceid("fb");
+	struct cdev *cdev;
+	struct fb_info *info;
+	struct fb_host *host = fb_dev->platform_data;
+
+	cdev = xzalloc(sizeof(struct framebuffer_desc));
+	info = (struct fb_info*)&cdev[1];
+
+	fb_dev->priv = cdev;	/* pointer forward */
+	cdev->dev = fb_dev;	/* pointer backward */
+
+	cdev->ops = &fb_ops;
+	cdev->name = asprintf("fb%d", id);
+
+	cdev->size = fb_dev->size;	/* use the default if any */
+	cdev->priv = info;
+
+	info->host = host;
+	info->fb_dev = fb_dev;
+
+	/* setup defaults */
+	if (host->bits_per_pixel != 0)
+		info->bits_per_pixel = host->bits_per_pixel;
+	else
+		info->bits_per_pixel = 16;	/* means RGB565 */
+
+	/* add params on demand */
+	add_fb_parameter(fb_dev);
+
+	devfs_create(cdev);
+#ifndef CONFIG_VIDEO_DELAY_INIT
+	/* initialize video mode immediately (the first one) */
+	fb_activate_mode(fb_dev, &host->mode[0]);
+#endif
+	return 0;
+}
+
+static struct driver_d fb_driver = {
+	.name	= "framebuffer",
+	.probe	= fb_probe,
+};
+
+static int framebuffer_init(void)
+{
+	return register_driver(&fb_driver);
+}
+
+device_initcall(framebuffer_init);
+
+struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned size)
+{
+	struct device_d *fb_dev;
+	int rc;
+
+	fb_dev = xzalloc(sizeof(struct device_d));
+
+	strcpy(fb_dev->name, fb_driver.name);
+	fb_dev->platform_data = (void*)host;
+
+	/* setup the defaults for this framebuffer if given */
+	fb_dev->size = size;
+	fb_dev->map_base = (unsigned long)base;
+
+	rc = register_device(fb_dev);
+	if (rc != 0) {
+		pr_debug("Cannot register framebuffer device\n");
+		free(fb_dev);
+		return NULL;
+	}
+
+	return fb_dev;
+}
+#endif
+
 int register_framebuffer(struct fb_info *info)
 {
 	int id = get_free_deviceid("fb");
diff --git a/include/fb.h b/include/fb.h
index 218b244..96edc24 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -85,6 +85,46 @@ struct fb_ops {
 	void (*fb_disable)(struct fb_info *info);
 };
 
+#if 0
+struct fb_host {
+	const struct fb_videomode *mode;
+	unsigned mode_cnt;
+
+	struct device_d *hw_dev;
+
+	/* callbacks into the video hardware driver */
+	int (*fb_setcolreg)(struct fb_info*, unsigned, unsigned, unsigned, unsigned, unsigned);
+	int (*fb_mode)(struct fb_info*, const struct fb_videomode*);
+	void (*fb_enable)(struct fb_info*);
+	void (*fb_disable)(struct fb_info*);
+
+	unsigned bits_per_pixel;
+};
+
+struct fb_info {
+	struct fb_host *host;
+	struct device_d *fb_dev;
+	const struct fb_videomode *active_mode;
+
+	u32 xres;			/* visible resolution		*/
+	u32 yres;
+	u32 bits_per_pixel;		/* guess what			*/
+
+	u32 grayscale;			/* != 0 Graylevels instead of colors */
+
+	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
+	struct fb_bitfield green;	/* else only length is significant */
+	struct fb_bitfield blue;
+	struct fb_bitfield transp;	/* transparency			*/
+
+#ifdef CONFIG_VIDEO_DELAY_ENABLING
+	int enabled;
+#endif
+};
+
+struct device_d *register_framebuffer(struct fb_host*, void*, unsigned);
+#endif
+
 struct fb_info {
 	struct fb_videomode *mode;
 
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 04/12] Remove the old videomode functions

[Juergen Beisert]

Remove the old functions from the framebuffer framework. This patch is kept
separate for review only. Should be merged with the previous one patch
prior commit.

This is patch 2 of 7 to keep the repository bisectable.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/video/fb.c |   67 ++-------------------------------------------------
 include/fb.h       |   40 -------------------------------
 2 files changed, 3 insertions(+), 104 deletions(-)

diff --git a/drivers/video/fb.c b/drivers/video/fb.c
index c3c4761..428ad34 100644
--- a/drivers/video/fb.c
+++ b/drivers/video/fb.c
@@ -11,6 +11,7 @@
 
 static int fb_ioctl(struct cdev* cdev, int req, void *data)
 {
+	struct fb_host *host = cdev->dev->platform_data;
 	struct fb_info *info = cdev->priv;
 
 	switch (req) {
@@ -18,10 +19,10 @@ static int fb_ioctl(struct cdev* cdev, int req, void *data)
 		memcpy(data, info, sizeof(*info));
 		break;
 	case FBIO_ENABLE:
-		info->fbops->fb_enable(info);
+		(host->fb_enable)(cdev->priv);
 		break;
 	case FBIO_DISABLE:
-		info->fbops->fb_disable(info);
+		(host->fb_disable)(cdev->priv);
 		break;
 	default:
 		return -ENOSYS;
@@ -30,37 +31,6 @@ static int fb_ioctl(struct cdev* cdev, int req, void *data)
 	return 0;
 }
 
-static int fb_enable_set(struct device_d *dev, struct param_d *param,
-		const char *val)
-{
-	struct fb_info *info = dev->priv;
-	int enable;
-	char *new;
-
-	if (!val)
-		return dev_param_set_generic(dev, param, NULL);
-
-	enable = simple_strtoul(val, NULL, 0);
-
-	if (info->enabled == !!enable)
-		return 0;
-
-	if (enable) {
-		info->fbops->fb_enable(info);
-		new = "1";
-	} else {
-		info->fbops->fb_disable(info);
-		new = "0";
-	}
-
-	dev_param_set_generic(dev, param, new);
-
-	info->enabled = !!enable;
-
-	return 0;
-}
-
-#if 0
 #ifdef CONFIG_VIDEO_DELAY_INIT
 static int fb_check_if_already_initialized(struct device_d *fb_dev)
 {
@@ -191,7 +161,6 @@ static int fb_mode_set(struct device_d *fb_dev, struct param_d *param, const cha
 	return rc;
 }
 #endif
-#endif
 
 static struct file_operations fb_ops = {
 	.read	= mem_read,
@@ -201,7 +170,6 @@ static struct file_operations fb_ops = {
 	.ioctl	= fb_ioctl,
 };
 
-#if 0
 static int add_fb_parameter(struct device_d *fb_dev)
 {
 #ifdef CONFIG_VIDEO_DELAY_INIT
@@ -306,32 +274,3 @@ struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned
 
 	return fb_dev;
 }
-#endif
-
-int register_framebuffer(struct fb_info *info)
-{
-	int id = get_free_deviceid("fb");
-	struct device_d *dev;
-
-	info->cdev.ops = &fb_ops;
-	info->cdev.name = asprintf("fb%d", id);
-	info->cdev.size = info->xres * info->yres * (info->bits_per_pixel >> 3);
-	info->cdev.dev = &info->dev;
-	info->cdev.priv = info;
-	info->cdev.dev->map_base = (unsigned long)info->screen_base;
-	info->cdev.dev->size = info->cdev.size;
-
-	dev = &info->dev;
-	dev->priv = info;
-
-	sprintf(dev->name, "fb");
-
-	register_device(&info->dev);
-	dev_add_param(dev, "enable", fb_enable_set, NULL, 0);
-	dev_set_param(dev, "enable", "0");
-
-	devfs_create(&info->cdev);
-
-	return 0;
-}
-
diff --git a/include/fb.h b/include/fb.h
index 96edc24..c94c1d0 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -77,15 +77,6 @@ struct fb_bitfield {
 
 struct fb_info;
 
-struct fb_ops {
-	/* set color register */
-	int (*fb_setcolreg)(unsigned regno, unsigned red, unsigned green,
-			    unsigned blue, unsigned transp, struct fb_info *info);
-	void (*fb_enable)(struct fb_info *info);
-	void (*fb_disable)(struct fb_info *info);
-};
-
-#if 0
 struct fb_host {
 	const struct fb_videomode *mode;
 	unsigned mode_cnt;
@@ -117,41 +108,10 @@ struct fb_info {
 	struct fb_bitfield blue;
 	struct fb_bitfield transp;	/* transparency			*/
 
-#ifdef CONFIG_VIDEO_DELAY_ENABLING
 	int enabled;
-#endif
 };
 
 struct device_d *register_framebuffer(struct fb_host*, void*, unsigned);
-#endif
-
-struct fb_info {
-	struct fb_videomode *mode;
-
-	struct fb_ops *fbops;
-	struct device_d dev;		/* This is this fb device */
-
-	void *screen_base;
-
-	void *priv;
-
-	struct cdev cdev;
-
-	u32 xres;			/* visible resolution		*/
-	u32 yres;
-	u32 bits_per_pixel;		/* guess what			*/
-
-	u32 grayscale;			/* != 0 Graylevels instead of colors */
-
-	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
-	struct fb_bitfield green;	/* else only length is significant */
-	struct fb_bitfield blue;
-	struct fb_bitfield transp;	/* transparency			*/
-
-	int enabled;
-};
-
-int register_framebuffer(struct fb_info *info);
 
 #define FBIOGET_SCREENINFO	_IOR('F', 1, loff_t)
 #define	FBIO_ENABLE		_IO('F', 2)
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 05/12] Add verbose framebuffer device info

[Juergen Beisert]

This is patch 3 of 7 to keep the repository bisectable.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/video/Kconfig |    6 ++++++
 drivers/video/fb.c    |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 8138226..d7f3d01 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -16,6 +16,12 @@ config VIDEO_DELAY_INIT
           This entry adds the "mode" parameter to the video device, to setup
           the desired videomode prior enabling it at runtime.
 
+config VIDEO_INFO_VERBOSE
+	bool "Verbose video info"
+	help
+	  Say 'y' here to be more verbose when running the 'devinfo' command
+	  on the framebuffer device.
+
 comment "drivers"
 
 config DRIVER_VIDEO_IMX
diff --git a/drivers/video/fb.c b/drivers/video/fb.c
index 428ad34..cb66744 100644
--- a/drivers/video/fb.c
+++ b/drivers/video/fb.c
@@ -170,6 +170,43 @@ static struct file_operations fb_ops = {
 	.ioctl	= fb_ioctl,
 };
 
+static void fb_info(struct device_d *fb_dev)
+{
+	struct cdev *cdev = fb_dev->priv;
+	struct fb_info *info = cdev->priv;
+
+	printf(" Video/Mode info:\n");
+	printf("  Video output %senabled\n", info->enabled != 0 ? "" : "not ");
+	printf(" Current video mode:\n");
+	if (info->active_mode != NULL) {
+		printf("  Name: %s\n", info->active_mode->name);
+#ifdef CONFIG_VIDEO_INFO_VERBOSE
+		if (info->active_mode->refresh == 0)
+			printf("  Refresh rate: undefined\n");
+		else
+			printf("  Refresh rate: %u Hz\n",  info->active_mode->refresh);
+		printf("  Horizontal active pixel: %u\n", info->active_mode->xres);
+		printf("  Vertical active lines: %u\n", info->active_mode->yres);
+		printf("  Pixel clock: %u kHz\n", PICOS2KHZ(info->active_mode->pixclock));
+		printf("  Left/Right margin (pixel): %u/%u\n", info->active_mode->left_margin,
+			info->active_mode->right_margin);
+		printf("  Upper/Lower margin (lines): %u/%u\n", info->active_mode->upper_margin,
+			info->active_mode->lower_margin);
+		printf("  HSYNC length in pixel: %u, polarity: %s\n", info->active_mode->hsync_len,
+			(info->active_mode->sync & FB_SYNC_HOR_HIGH_ACT) ? "high" : "low");
+		printf("  VSYNC length in lines: %u, polarity: %s\n", info->active_mode->vsync_len,
+			(info->active_mode->sync & FB_SYNC_VERT_HIGH_ACT) ? "high" : "low");
+		printf("  Colour depth: %u bpp\n", info->bits_per_pixel);
+		printf("  Framebuffer size is: %u bytes\n", cdev->size);
+		/* TODO add the remaining information from fb_videomode. How valuable are they? */
+#endif
+	} else {
+		printf ("  No video mode selected yet\n");
+	}
+
+	fb_list_modes(fb_dev->platform_data);
+}
+
 static int add_fb_parameter(struct device_d *fb_dev)
 {
 #ifdef CONFIG_VIDEO_DELAY_INIT
@@ -242,6 +279,7 @@ static int fb_probe(struct device_d *fb_dev)
 static struct driver_d fb_driver = {
 	.name	= "framebuffer",
 	.probe	= fb_probe,
+	.info	= fb_info,
 };
 
 static int framebuffer_init(void)
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 06/12] Adapt the existing imx fb driver to support runtime videomode selection

[Juergen Beisert]

Adapt the API to the new framebuffer videomode selection at runtime. If the new
feature is not used, there is no visible change at runtime for platforms using
this driver.

NOTE: Due to the lack of hardware, this is compile time tested only.

This is patch 4 of 7 to keep the repository bisectable.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 arch/arm/mach-imx/include/mach/imxfb.h |    1 +
 drivers/video/imx.c                    |  211 ++++++++++++++++++--------------
 2 files changed, 120 insertions(+), 92 deletions(-)

diff --git a/arch/arm/mach-imx/include/mach/imxfb.h b/arch/arm/mach-imx/include/mach/imxfb.h
index 7baa244..c536119 100644
--- a/arch/arm/mach-imx/include/mach/imxfb.h
+++ b/arch/arm/mach-imx/include/mach/imxfb.h
@@ -60,6 +60,7 @@ struct imx_fb_videomode {
  */
 struct imx_fb_platform_data {
 	struct imx_fb_videomode *mode;
+	unsigned mode_cnt;	/**< number of entries in 'mode' */
 
 	u_int		cmap_greyscale:1,
 			cmap_inverse:1,
diff --git a/drivers/video/imx.c b/drivers/video/imx.c
index 6ccd77e..07438e5 100644
--- a/drivers/video/imx.c
+++ b/drivers/video/imx.c
@@ -137,6 +137,7 @@ struct imxfb_rgb {
 };
 
 struct imxfb_info {
+	struct fb_host		fb_host;	/**< myself */
 	void __iomem		*regs;
 
 	u_int			pcr;
@@ -147,16 +148,15 @@ struct imxfb_info {
 				cmap_static:1,
 				unused:30;
 
-	struct imx_fb_videomode *mode;
-
-	struct fb_info		info;
-	struct device_d		*dev;
-
 	void			(*enable)(int enable);
 
-	struct fb_info		overlay;
+	struct fb_host		fb_overlay;
 };
 
+#define fb_overlay_to_imxfb_info(x) (container_of((struct fb_host*)(x->host), struct imxfb_info, fb_overlay))
+#define fb_overlay_dev_to_imxfb_info(x) (container_of((struct fb_host*)(x->platform_data), struct imxfb_info, fb_overlay))
+#define fb_info_to_imxfb_info(x) ((struct imxfb_info*)((x)->host))
+
 #define IMX_NAME	"IMX"
 
 /*
@@ -204,11 +204,10 @@ static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
 	return chan << bf->offset;
 }
 
-
-static int imxfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
-		   u_int trans, struct fb_info *info)
+static int imxfb_setcolreg(struct fb_info *info, u_int regno, u_int red,
+			u_int green, u_int blue, u_int trans)
 {
-	struct imxfb_info *fbi = info->priv;
+	struct imxfb_info *fbi = fb_info_to_imxfb_info(info);
 	int ret = 1;
 	u32 val;
 
@@ -247,7 +246,7 @@ static int imxfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 
 static void imxfb_enable_controller(struct fb_info *info)
 {
-	struct imxfb_info *fbi = info->priv;
+	struct imxfb_info *fbi = fb_info_to_imxfb_info(info);
 
 	writel(RMCR_LCDC_EN, fbi->regs + LCDC_RMCR);
 #ifdef CONFIG_ARCH_IMX21
@@ -269,7 +268,7 @@ static void imxfb_enable_controller(struct fb_info *info)
 
 static void imxfb_disable_controller(struct fb_info *info)
 {
-	struct imxfb_info *fbi = info->priv;
+	struct imxfb_info *fbi = fb_info_to_imxfb_info(info);
 
 	if (fbi->enable)
 		fbi->enable(0);
@@ -295,14 +294,35 @@ static void imxfb_disable_controller(struct fb_info *info)
  *	Configures LCD Controller based on entries in var parameter.  Settings are
  *	only written to the controller if changes were made.
  */
-static int imxfb_activate_var(struct fb_info *info)
+static int imxfb_initialize_mode(struct fb_info *info, const struct fb_videomode *mode)
 {
-	struct fb_videomode *mode = info->mode;
 	struct imxfb_rgb *rgb;
 	unsigned long lcd_clk;
 	unsigned long long tmp;
-	struct imxfb_info *fbi = info->priv;
+	struct imxfb_info *fbi = fb_info_to_imxfb_info(info);
 	u32 pcr;
+	unsigned size;
+
+	/*
+	 * we need at least this amount of memory for the framebuffer
+	 */
+	size = mode->xres * mode->yres * (info->bits_per_pixel >> 3);
+	if (info->fb_dev->size != 0) {
+		if (size > info->fb_dev->size) {
+			pr_err("Cannot initialize video mode '%s': Its too large. "
+				"Required bytes are %u, available only %u\n",
+				mode->name, size, info->fb_dev->size);
+			return -EINVAL;
+		}
+	} else
+		info->fb_dev->size = size;
+
+	/*
+	 * if no framebuffer memory was specified yet, allocate one,
+	 * and allocate more memory, on user request
+	 */
+	if (info->fb_dev->map_base == 0U /* FIXME should be 'NULL'*/)
+		info->fb_dev->map_base = (unsigned long)xzalloc(info->fb_dev->size);
 
 	/* physical screen start address	    */
 	writel(VPW_VPW(mode->xres * info->bits_per_pixel / 8 / 4),
@@ -318,13 +338,13 @@ static int imxfb_activate_var(struct fb_info *info)
 		VCR_V_WAIT_2(mode->upper_margin),
 		fbi->regs + LCDC_VCR);
 
-	writel(SIZE_XMAX(info->xres) | SIZE_YMAX(info->yres),
+	writel(SIZE_XMAX(mode->xres) | SIZE_YMAX(mode->yres),
 			fbi->regs + LCDC_SIZE);
 
 	writel(fbi->pwmr, fbi->regs + LCDC_PWMR);
 	writel(fbi->lscr1, fbi->regs + LCDC_LSCR1);
 	writel(fbi->dmacr, fbi->regs + LCDC_DMACR);
-	writel((unsigned long)fbi->info.screen_base, fbi->regs + LCDC_SSA);
+	writel((uint32_t)info->fb_dev->map_base, fbi->regs + LCDC_SSA);
 
 	/* panning offset 0 (0 pixel offset)        */
 	writel(0x0, fbi->regs + LCDC_POS);
@@ -386,16 +406,10 @@ static int imxfb_activate_var(struct fb_info *info)
 	return 0;
 }
 
-static struct fb_ops imxfb_ops = {
-	.fb_setcolreg	= imxfb_setcolreg,
-	.fb_enable	= imxfb_enable_controller,
-	.fb_disable	= imxfb_disable_controller,
-};
-
 #ifdef CONFIG_IMXFB_DRIVER_VIDEO_IMX_OVERLAY
 static void imxfb_overlay_enable_controller(struct fb_info *overlay)
 {
-	struct imxfb_info *fbi = overlay->priv;
+	struct imxfb_info *fbi = fb_overlay_to_imxfb_info(overlay);
 	unsigned int tmp;
 
 	tmp = readl(fbi->regs + LCDC_LGWCR);
@@ -405,7 +419,7 @@ static void imxfb_overlay_enable_controller(struct fb_info *overlay)
 
 static void imxfb_overlay_disable_controller(struct fb_info *overlay)
 {
-	struct imxfb_info *fbi = overlay->priv;
+	struct imxfb_info *fbi = fb_overlay_to_imxfb_info(overlay);
 	unsigned int tmp;
 
 	tmp = readl(fbi->regs + LCDC_LGWCR);
@@ -413,23 +427,16 @@ static void imxfb_overlay_disable_controller(struct fb_info *overlay)
 	writel(tmp , fbi->regs + LCDC_LGWCR);
 }
 
-static int imxfb_overlay_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
-		   u_int trans, struct fb_info *info)
+static int imxfb_overlay_setcolreg(struct fb_info *overlay, u_int regno,
+		u_int red, u_int green, u_int blue, u_int trans)
 {
 	return 0;
 }
 
-static struct fb_ops imxfb_overlay_ops = {
-	.fb_setcolreg	= imxfb_overlay_setcolreg,
-	.fb_enable	= imxfb_overlay_enable_controller,
-	.fb_disable	= imxfb_overlay_disable_controller,
-};
-
 static int imxfb_alpha_set(struct device_d *dev, struct param_d *param,
 		const char *val)
 {
-	struct fb_info *overlay = dev->priv;
-	struct imxfb_info *fbi = overlay->priv;
+	struct imxfb_info *fbi = fb_overlay_dev_to_imxfb_info(dev);
 	int alpha;
 	char alphastr[16];
 	unsigned int tmp;
@@ -452,32 +459,35 @@ static int imxfb_alpha_set(struct device_d *dev, struct param_d *param,
 	return 0;
 }
 
-static int imxfb_register_overlay(struct imxfb_info *fbi, void *fb)
+static int imxfb_overlay_mode(struct fb_info *overlay, const struct fb_videomode *mode)
 {
-	struct fb_info *overlay;
+	struct imxfb_info *fbi = fb_overlay_to_imxfb_info(overlay);
 	struct imxfb_rgb *rgb;
-	int ret;
-
-	overlay = &fbi->overlay;
-
-	overlay->priv = fbi;
-	overlay->mode = fbi->info.mode;
-	overlay->xres = fbi->info.xres;
-	overlay->yres = fbi->info.yres;
-	overlay->bits_per_pixel = fbi->info.bits_per_pixel;
-	overlay->fbops = &imxfb_overlay_ops;
-
-	if (fb)
-		overlay->screen_base = fb;
-	else
-		overlay->screen_base = xzalloc(overlay->xres * overlay->yres *
-			(overlay->bits_per_pixel >> 3));
-
-	writel((unsigned long)overlay->screen_base, fbi->regs + LCDC_LGWSAR);
-	writel(SIZE_XMAX(overlay->xres) | SIZE_YMAX(overlay->yres),
-			fbi->regs + LCDC_LGWSR);
-	writel(VPW_VPW(overlay->xres * overlay->bits_per_pixel / 8 / 4),
-		fbi->regs + LCDC_LGWVPWR);
+	unsigned size;
+
+	/* we need at least this amount of memory for the framebuffer */
+	size = mode->xres * mode->yres * (overlay->bits_per_pixel >> 3);
+
+	if (overlay->fb_dev->size != 0) {
+		if (size > overlay->fb_dev->size) {
+			pr_err("Cannot initialize video mode '%s': Its too large. "
+				"Required bytes are %u, available only %u\n",
+				mode->name, size, overlay->fb_dev->size);
+			return -EINVAL;
+		}
+	} else
+		overlay->fb_dev->size = size;
+
+	/*
+	 * if no framebuffer memory was specified yet, allocate one,
+	 * and allocate more memory, on user request
+	 */
+	if (overlay->fb_dev->map_base == 0U /* FIXME should be 'NULL'*/)
+		overlay->fb_dev->map_base = (unsigned long)xzalloc(overlay->fb_dev->size);
+
+	writel((uint32_t)overlay->fb_dev->map_base, fbi->regs + LCDC_LGWSAR);
+	writel(SIZE_XMAX(mode->xres) | SIZE_YMAX(mode->yres), fbi->regs + LCDC_LGWSR);
+	writel(VPW_VPW(mode->xres * overlay->bits_per_pixel / 8 / 4), fbi->regs + LCDC_LGWVPWR);
 	writel(0, fbi->regs + LCDC_LGWPR);
 	writel(LGWCR_GWAV(0x0), fbi->regs + LCDC_LGWCR);
 
@@ -506,14 +516,36 @@ static int imxfb_register_overlay(struct imxfb_info *fbi, void *fb)
 	overlay->blue   = rgb->blue;
 	overlay->transp = rgb->transp;
 
-	ret = register_framebuffer(overlay);
-	if (ret < 0) {
-		dev_err(fbi->dev, "failed to register framebuffer\n");
-		return ret;
+	return 0;
+}
+
+static int imxfb_register_overlay(struct imxfb_info *fbi, struct device_d *hw_dev,
+				struct imx_fb_platform_data *pdata)
+{
+	struct fb_host *overlay;
+	struct device_d *overlay_dev;
+
+	overlay = &fbi->fb_overlay;
+
+	/* add runtime hardware info */
+	overlay->hw_dev = hw_dev;	/* same as the master device */
+	overlay->fb_mode = imxfb_overlay_mode;
+	overlay->fb_enable = imxfb_overlay_enable_controller;
+	overlay->fb_disable = imxfb_overlay_disable_controller;
+	overlay->fb_setcolreg = imxfb_overlay_setcolreg;
+
+	/* add runtime video info */
+	overlay->mode = pdata->mode->mode;
+	overlay->mode_cnt = 1;	/* no choice */
+
+	overlay_dev = register_framebuffer(overlay, pdata->framebuffer_ovl, 0);
+	if (overlay_dev == NULL) {
+		dev_err(hw_dev, "failed to register overlay framebuffer\n");
+		return -EINVAL;
 	}
 
-	dev_add_param(&overlay->dev, "alpha", imxfb_alpha_set, NULL, 0);
-	dev_set_param(&overlay->dev, "alpha", "0");
+	dev_add_param(overlay_dev, "alpha", imxfb_alpha_set, NULL, 0);
+	dev_set_param(overlay_dev, "alpha", "0");
 
 	return 0;
 }
@@ -522,13 +554,13 @@ static int imxfb_register_overlay(struct imxfb_info *fbi, void *fb)
 static int imxfb_probe(struct device_d *dev)
 {
 	struct imxfb_info *fbi;
-	struct fb_info *info;
+	struct device_d *fb_dev;
 	struct imx_fb_platform_data *pdata = dev->platform_data;
-	int ret;
 
 	if (!pdata)
 		return -ENODEV;
 
+	/* TODO should be done when enabling the video output */
 #ifdef CONFIG_ARCH_IMX21
 	PCCR0 &= ~(PCCR0_PERCLK3_EN | PCCR0_HCLK_LCDC_EN);
 #endif
@@ -544,40 +576,35 @@ static int imxfb_probe(struct device_d *dev)
 #endif
 
 	fbi = xzalloc(sizeof(*fbi));
-	info = &fbi->info;
 
-	fbi->mode = pdata->mode;
-	fbi->regs = (void *)dev->map_base;
+	/* add runtime hardware info */
+	fbi->fb_host.hw_dev = dev;
+	fbi->fb_host.fb_mode = imxfb_initialize_mode;
+	fbi->fb_host.fb_enable = imxfb_enable_controller;
+	fbi->fb_host.fb_disable = imxfb_disable_controller;
+	fbi->fb_host.fb_setcolreg = imxfb_setcolreg;
+
+	fbi->regs = (void*)dev->map_base;
 	fbi->pcr = pdata->mode->pcr;
 	fbi->pwmr = pdata->pwmr;
 	fbi->lscr1 = pdata->lscr1;
 	fbi->dmacr = pdata->dmacr;
 	fbi->enable = pdata->enable;
-	fbi->dev = dev;
-	info->priv = fbi;
-	info->mode = pdata->mode->mode;
-	info->xres = pdata->mode->mode->xres;
-	info->yres = pdata->mode->mode->yres;
-	info->bits_per_pixel = pdata->mode->bpp;
-	info->fbops = &imxfb_ops;
-
-	dev_info(dev, "i.MX Framebuffer driver\n");
-
-	if (pdata->framebuffer)
-		fbi->info.screen_base = pdata->framebuffer;
-	else
-		fbi->info.screen_base = xzalloc(info->xres * info->yres *
-			(info->bits_per_pixel >> 3));
-
-	imxfb_activate_var(&fbi->info);
-
-	ret = register_framebuffer(&fbi->info);
-	if (ret < 0) {
+
+	/* add runtime video info */
+	fbi->fb_host.mode = pdata->mode->mode;
+	/* to be backward compatible */
+	fbi->fb_host.mode_cnt = pdata->mode_cnt == 0 ? 1 : pdata->mode_cnt;
+	fbi->fb_host.bits_per_pixel = 16;	/* RGB565, the default */
+
+	fb_dev = register_framebuffer(&fbi->fb_host, pdata->framebuffer, 0);
+	if (dev == NULL) {
 		dev_err(dev, "failed to register framebuffer\n");
-		return ret;
+		return -EINVAL;
 	}
+
 #ifdef CONFIG_IMXFB_DRIVER_VIDEO_IMX_OVERLAY
-	imxfb_register_overlay(fbi, pdata->framebuffer_ovl);
+	imxfb_register_overlay(fbi, fb_dev, pdata);
 #endif
 	return 0;
 }
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 07/12] Adapt the existing imx-ipu fb driver to support runtime videomode selection

[Juergen Beisert]

Adapt the API to the new framebuffer videomode selection at runtime. If the new
feature is not used, there is no visible change at runtime for platforms using
this driver.

NOTE: Due to the lack of hardware, this is compile time tested only.

This is patch 5 of 7 to keep the repository bisectable.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 arch/arm/boards/freescale-mx35-3-stack/3stack.c |    2 +-
 arch/arm/boards/pcm043/pcm043.c                 |    2 +-
 arch/arm/mach-imx/include/mach/imx-ipu-fb.h     |   12 +-
 drivers/video/imx-ipu-fb.c                      |  216 +++++++++++++----------
 4 files changed, 124 insertions(+), 108 deletions(-)

diff --git a/arch/arm/boards/freescale-mx35-3-stack/3stack.c b/arch/arm/boards/freescale-mx35-3-stack/3stack.c
index d6699cd..fb40f50 100644
--- a/arch/arm/boards/freescale-mx35-3-stack/3stack.c
+++ b/arch/arm/boards/freescale-mx35-3-stack/3stack.c
@@ -139,7 +139,7 @@ static struct fb_videomode CTP_CLAA070LC0ACW = {
 	.lower_margin	= 10,	/* whole frame should have 500 lines */
 	.hsync_len	= 1,	/* note: DE only display */
 	.vsync_len	= 1,	/* note: DE only display */
-	.sync		= FB_SYNC_CLK_IDLE_EN | FB_SYNC_OE_ACT_HIGH,
+	.sync		= FB_SYNC_CLK_IDLE_EN | FB_SYNC_DE_HIGH_ACT,
 	.vmode		= FB_VMODE_NONINTERLACED,
 	.flag		= 0,
 };
diff --git a/arch/arm/boards/pcm043/pcm043.c b/arch/arm/boards/pcm043/pcm043.c
index 5932f95..7e63cc5 100644
--- a/arch/arm/boards/pcm043/pcm043.c
+++ b/arch/arm/boards/pcm043/pcm043.c
@@ -124,7 +124,7 @@ static struct fb_videomode pcm043_fb_mode = {
 	.lower_margin	= 40,
 	.hsync_len	= 96,
 	.vsync_len	= 1,
-	.sync		= FB_SYNC_VERT_HIGH_ACT | FB_SYNC_OE_ACT_HIGH,
+	.sync		= FB_SYNC_VERT_HIGH_ACT | FB_SYNC_DE_HIGH_ACT,
 	.vmode		= FB_VMODE_NONINTERLACED,
 	.flag		= 0,
 };
diff --git a/arch/arm/mach-imx/include/mach/imx-ipu-fb.h b/arch/arm/mach-imx/include/mach/imx-ipu-fb.h
index 8e1cc87..ce95243 100644
--- a/arch/arm/mach-imx/include/mach/imx-ipu-fb.h
+++ b/arch/arm/mach-imx/include/mach/imx-ipu-fb.h
@@ -12,20 +12,12 @@
 
 #include <fb.h>
 
-/* Proprietary FB_SYNC_ flags */
-#define FB_SYNC_OE_ACT_HIGH	0x80000000
-#define FB_SYNC_CLK_INVERT	0x40000000
-#define FB_SYNC_DATA_INVERT	0x20000000
-#define FB_SYNC_CLK_IDLE_EN	0x10000000
-#define FB_SYNC_SHARP_MODE	0x08000000
-#define FB_SYNC_SWAP_RGB	0x04000000
-#define FB_SYNC_CLK_SEL_EN	0x02000000
-
 /*
- * struct mx3fb_platform_data - mx3fb platform data
+ * struct imx_ipu_fb_platform_data - imx-ipu-fb's platform data
  */
 struct imx_ipu_fb_platform_data {
 	struct fb_videomode	*mode;
+	unsigned mode_cnt;	/**< number of entries in 'mode' */
 	unsigned char		bpp;
 	void __iomem		*framebuffer;
 	/** hook to enable backlight and stuff */
diff --git a/drivers/video/imx-ipu-fb.c b/drivers/video/imx-ipu-fb.c
index c38082d..45399cb 100644
--- a/drivers/video/imx-ipu-fb.c
+++ b/drivers/video/imx-ipu-fb.c
@@ -34,12 +34,12 @@
 #include <mach/clock.h>
 
 struct ipu_fb_info {
+	struct  fb_host		fb_host;	/**< myself */
 	void __iomem		*regs;
 
 	void			(*enable)(int enable);
 
-	struct fb_info		info;
-	struct device_d		*dev;
+	const struct fb_videomode *mode;	/**< requested videomodue */
 };
 
 /* IPU DMA Controller channel definitions. */
@@ -413,29 +413,30 @@ static inline void reg_write(struct ipu_fb_info *fbi, u32 value,
 	writel(value, fbi->regs + reg);
 }
 
+#define fb_info_to_imxfb_info(x) ((struct ipu_fb_info*)((x)->host))
+
 /*
  * sdc_init_panel() - initialize a synchronous LCD panel.
- * @width:		width of panel in pixels.
- * @height:		height of panel in pixels.
- * @pixel_fmt:		pixel format of buffer as FOURCC ASCII code.
+ * @param info The framebuffer to work on
+ * @param pixel_fmt pixel format of buffer as FOURCC ASCII code.
  * @return:		0 on success or negative error code on failure.
  */
-static int sdc_init_panel(struct fb_info *info, enum pixel_fmt pixel_fmt)
+static int sdc_init_panel(struct fb_info *fb_info, enum pixel_fmt pixel_fmt)
 {
-	struct ipu_fb_info *fbi = info->priv;
-	struct fb_videomode *mode = info->mode;
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
+	const struct fb_videomode *mode = fbi->mode;
 	u32 reg, old_conf, div;
 	enum ipu_panel panel = IPU_PANEL_TFT;
 	unsigned long pixel_clk;
 
 	/* Init panel size and blanking periods */
 	reg = ((mode->hsync_len - 1) << 26) |
-		((info->xres + mode->left_margin + mode->right_margin +
+		((mode->xres + mode->left_margin + mode->right_margin +
 		  mode->hsync_len - 1) << 16);
 	reg_write(fbi, reg, SDC_HOR_CONF);
 
 	reg = ((mode->vsync_len - 1) << 26) | SDC_V_SYNC_WIDTH_L |
-		((info->yres + mode->upper_margin + mode->lower_margin +
+		((mode->yres + mode->upper_margin + mode->lower_margin +
 		  mode->vsync_len - 1) << 16);
 	reg_write(fbi, reg, SDC_VER_CONF);
 
@@ -448,7 +449,7 @@ static int sdc_init_panel(struct fb_info *info, enum pixel_fmt pixel_fmt)
 		old_conf |= DI_D3_CLK_POL;
 	if (mode->sync & FB_SYNC_DATA_INVERT)
 		old_conf |= DI_D3_DATA_POL;
-	if (mode->sync & FB_SYNC_OE_ACT_HIGH)
+	if (mode->sync & FB_SYNC_DE_HIGH_ACT)
 		old_conf |= DI_D3_DRDY_SHARP_POL;
 	reg_write(fbi, old_conf, DI_DISP_SIG_POL);
 
@@ -483,12 +484,12 @@ static int sdc_init_panel(struct fb_info *info, enum pixel_fmt pixel_fmt)
 	div = imx_get_lcdclk() * 16 / pixel_clk;
 
 	if (div < 0x40) {	/* Divider less than 4 */
-		dev_dbg(&info->dev,
+		dev_dbg(fbi->fb_host.hw_dev,
 			"InitPanel() - Pixel clock divider less than 4\n");
 		div = 0x40;
 	}
 
-	dev_dbg(&info->dev, "pixel clk = %u, divider %u.%u\n",
+	dev_dbg(fbi->fb_host.hw_dev, "pixel clk = %u, divider %u.%u\n",
 		pixel_clk, div >> 4, (div & 7) * 125);
 
 	/*
@@ -588,19 +589,20 @@ static u32 dma_param_addr(enum ipu_channel channel)
 	return 0x10000 | (channel << 4);
 }
 
-static void ipu_init_channel_buffer(struct ipu_fb_info *fbi,
+static void ipu_init_channel_buffer(struct fb_info *fb_info,
 		enum ipu_channel channel, void *fbmem)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	union chan_param_mem params = {};
 	u32 reg;
 	u32 stride_bytes;
 
-	stride_bytes = fbi->info.xres * ((fbi->info.bits_per_pixel + 7) / 8);
+	stride_bytes = fbi->mode->xres * ((fb_info->bits_per_pixel + 7) / 8);
 	stride_bytes = (stride_bytes + 3) & ~3;
 
 	/* Build parameter memory data for DMA channel */
-	ipu_ch_param_set_size(&params, bpp_to_pixfmt(fbi->info.bits_per_pixel),
-			      fbi->info.xres, fbi->info.yres, stride_bytes);
+	ipu_ch_param_set_size(&params, bpp_to_pixfmt(fb_info->bits_per_pixel),
+			      fbi->mode->xres, fbi->mode->yres, stride_bytes);
 	ipu_ch_param_set_buffer(&params, fbmem, NULL);
 	params.pp.bam = 0;
 	/* Some channels (rotation) have restriction on burst length */
@@ -622,9 +624,10 @@ static void ipu_init_channel_buffer(struct ipu_fb_info *fbi,
 	reg_write(fbi, reg, IPU_CHA_DB_MODE_SEL);
 }
 
-static void ipu_channel_set_priority(struct ipu_fb_info *fbi,
+static void ipu_channel_set_priority(struct fb_info *fb_info,
 		enum ipu_channel channel, int prio)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	u32 reg;
 
 	reg = reg_read(fbi, IDMAC_CHA_PRI);
@@ -639,11 +642,13 @@ static void ipu_channel_set_priority(struct ipu_fb_info *fbi,
 
 /*
  * ipu_enable_channel() - enable an IPU channel.
+ * @param fb_info The framebuffer to work on
  * @channel:	channel ID.
  * @return:	0 on success or negative error code on failure.
  */
-static int ipu_enable_channel(struct ipu_fb_info *fbi, enum ipu_channel channel)
+static int ipu_enable_channel(struct fb_info *fb_info, enum ipu_channel channel)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	u32 reg;
 
 	/* Reset to buffer 0 */
@@ -651,7 +656,7 @@ static int ipu_enable_channel(struct ipu_fb_info *fbi, enum ipu_channel channel)
 
 	switch (channel) {
 	case IDMAC_SDC_0:
-		ipu_channel_set_priority(fbi, channel, 1);
+		ipu_channel_set_priority(fb_info, channel, 1);
 		break;
 	default:
 		break;
@@ -663,9 +668,10 @@ static int ipu_enable_channel(struct ipu_fb_info *fbi, enum ipu_channel channel)
 	return 0;
 }
 
-static int ipu_update_channel_buffer(struct ipu_fb_info *fbi,
+static int ipu_update_channel_buffer(struct fb_info *fb_info,
 		enum ipu_channel channel, void *buf)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	u32 reg;
 
 	reg = reg_read(fbi, IPU_CHA_BUF0_RDY);
@@ -679,16 +685,17 @@ static int ipu_update_channel_buffer(struct ipu_fb_info *fbi,
 	return 0;
 }
 
-static int idmac_tx_submit(struct ipu_fb_info *fbi, enum ipu_channel channel,
+static int idmac_tx_submit(struct fb_info *fb_info, enum ipu_channel channel,
 		void *buf)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	int ret;
 
-	ipu_init_channel_buffer(fbi, channel, buf);
+	ipu_init_channel_buffer(fb_info, channel, buf);
 
 
 	/* ipu_idmac.c::ipu_submit_channel_buffers() */
-	ret = ipu_update_channel_buffer(fbi, channel, buf);
+	ret = ipu_update_channel_buffer(fb_info, channel, buf);
 	if (ret < 0)
 		return ret;
 
@@ -697,16 +704,17 @@ static int idmac_tx_submit(struct ipu_fb_info *fbi, enum ipu_channel channel,
 	reg_write(fbi, 1UL << channel, IPU_CHA_BUF0_RDY);
 
 
-	ret = ipu_enable_channel(fbi, channel);
+	ret = ipu_enable_channel(fb_info, channel);
 	return ret;
 }
 
-static void sdc_enable_channel(struct ipu_fb_info *fbi, void *fbmem)
+static void sdc_enable_channel(struct fb_info *fb_info, void *fbmem)
 {
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
 	int ret;
 	u32 reg;
 
-	ret = idmac_tx_submit(fbi, IDMAC_SDC_0, fbmem);
+	ret = idmac_tx_submit(fb_info, IDMAC_SDC_0, fbmem);
 
 	/* mx3fb.c::sdc_fb_init() */
 	if (ret >= 0) {
@@ -722,11 +730,74 @@ static void sdc_enable_channel(struct ipu_fb_info *fbi, void *fbmem)
 	mdelay(2);
 }
 
+static void imxfb_init_info(struct fb_info *fb_info, const struct fb_videomode *mode)
+{
+	struct imx_ipu_fb_rgb *rgb;
+
+	fb_info->xres = mode->xres;
+	fb_info->yres = mode->yres;
+
+	switch (fb_info->bits_per_pixel) {
+	case 32:
+		rgb = &def_rgb_32;
+		break;
+	case 24:
+		rgb = &def_rgb_24;
+		break;
+	case 16:
+	default:
+		rgb = &def_rgb_16;
+		break;
+	}
+
+	/*
+	 * Copy the RGB parameters for this display
+	 * from the machine specific parameters.
+	 */
+	fb_info->red    = rgb->red;
+	fb_info->green  = rgb->green;
+	fb_info->blue   = rgb->blue;
+	fb_info->transp = rgb->transp;
+}
+
+static int ipu_fb_initialize_mode(struct fb_info *fb_info, const struct fb_videomode *mode)
+{
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
+	unsigned size;
+
+	/*
+	 * we need at least this amount of memory for the framebuffer
+	 */
+	size = mode->xres * mode->yres * (fb_info->bits_per_pixel >> 3);
+	if (fb_info->fb_dev->size != 0) {
+		if (size > fb_info->fb_dev->size) {
+			pr_err("Cannot initialize video mode '%s': Its too large. "
+				"Required bytes are %u, available only %u\n",
+				mode->name, size, fb_info->fb_dev->size);
+			return -EINVAL;
+		}
+	} else
+		fb_info->fb_dev->size = size;
+
+	/*
+	 * if no framebuffer memory was specified yet, allocate one,
+	 * and allocate more memory, on user request
+	 */
+	if (fb_info->fb_dev->map_base == 0U /* FIXME should be 'NULL'*/)
+		fb_info->fb_dev->map_base = (unsigned long)xzalloc(fb_info->fb_dev->size);
+
+	fbi->mode = mode;
+
+	imxfb_init_info(fb_info, mode);
+
+	return 0;
+}
+
 /* References in this function refer to respective Linux kernel sources */
-static void ipu_fb_enable(struct fb_info *info)
+static void ipu_fb_enable(struct fb_info *fb_info)
 {
-	struct ipu_fb_info *fbi = info->priv;
-	struct fb_videomode *mode = info->mode;
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(fb_info);
+	const struct fb_videomode *mode = fbi->mode;
 	u32 reg;
 
 	/* pcm037.c::mxc_board_init() */
@@ -779,12 +850,12 @@ static void ipu_fb_enable(struct fb_info *info)
 		~(SDC_COM_GWSEL | SDC_COM_KEY_COLOR_G);
 	reg_write(fbi, reg, SDC_COM_CONF);
 
-	sdc_init_panel(info, IPU_PIX_FMT_RGB666);
+	sdc_init_panel(fb_info, IPU_PIX_FMT_RGB666);
 
 	reg_write(fbi, (mode->left_margin << 16) | mode->upper_margin,
 			SDC_BG_POS);
 
-	sdc_enable_channel(fbi, info->screen_base);
+	sdc_enable_channel(fb_info, (void*)fb_info->fb_dev->map_base);
 
 	/*
 	 * Linux driver calls sdc_set_brightness() here again,
@@ -796,7 +867,7 @@ static void ipu_fb_enable(struct fb_info *info)
 
 static void ipu_fb_disable(struct fb_info *info)
 {
-	struct ipu_fb_info *fbi = info->priv;
+	struct ipu_fb_info *fbi = fb_info_to_imxfb_info(info);
 	u32 reg;
 
 	if (fbi->enable)
@@ -807,85 +878,38 @@ static void ipu_fb_disable(struct fb_info *info)
 	reg_write(fbi, reg, SDC_COM_CONF);
 }
 
-static struct fb_ops imxfb_ops = {
-	.fb_enable = ipu_fb_enable,
-	.fb_disable = ipu_fb_disable,
-};
-
-static void imxfb_init_info(struct fb_info *info, struct fb_videomode *mode,
-		int bpp)
-{
-	struct imx_ipu_fb_rgb *rgb;
-
-	info->mode = mode;
-	info->xres = mode->xres;
-	info->yres = mode->yres;
-	info->bits_per_pixel = bpp;
-
-	switch (info->bits_per_pixel) {
-	case 32:
-		rgb = &def_rgb_32;
-		break;
-	case 24:
-		rgb = &def_rgb_24;
-		break;
-	case 16:
-	default:
-		rgb = &def_rgb_16;
-		break;
-	}
-
-	/*
-	 * Copy the RGB parameters for this display
-	 * from the machine specific parameters.
-	 */
-	info->red    = rgb->red;
-	info->green  = rgb->green;
-	info->blue   = rgb->blue;
-	info->transp = rgb->transp;
-}
-
 static int imxfb_probe(struct device_d *dev)
 {
 	struct ipu_fb_info *fbi;
-	struct fb_info *info;
+	struct device_d *fb_dev;
 	const struct imx_ipu_fb_platform_data *pdata = dev->platform_data;
-	int ret;
 
 	if (!pdata)
 		return -ENODEV;
 
 	fbi = xzalloc(sizeof(*fbi));
-	info = &fbi->info;
+
+	/* add runtime hardware info */
+	fbi->fb_host.hw_dev = dev;
+	fbi->fb_host.fb_mode = ipu_fb_initialize_mode;
+	fbi->fb_host.fb_enable = ipu_fb_enable;
+	fbi->fb_host.fb_disable = ipu_fb_disable;
+	fbi->fb_host.fb_setcolreg = NULL;
 
 	fbi->regs = (void *)dev->map_base;
-	fbi->dev = dev;
-	info->priv = fbi;
-	info->fbops = &imxfb_ops;
-	fbi->enable = pdata->enable;
 
-	imxfb_init_info(info, pdata->mode, pdata->bpp);
+	/* add runtime video info */
+	fbi->fb_host.mode = pdata->mode;
+	/* to be backward compatible */
+	fbi->fb_host.mode_cnt = pdata->mode_cnt == 0 ? 1 : pdata->mode_cnt;
+	fbi->fb_host.bits_per_pixel = pdata->bpp;
 
 	dev_info(dev, "i.MX Framebuffer driver\n");
 
-	/*
-	 * Use a given frambuffer or reserve some
-	 * memory for screen usage
-	 */
-	fbi->info.screen_base = pdata->framebuffer;
-	if (fbi->info.screen_base == NULL) {
-		fbi->info.screen_base = malloc(info->xres * info->yres *
-					       (info->bits_per_pixel >> 3));
-		if (!fbi->info.screen_base)
-			return -ENOMEM;
-	}
-
-	sdc_enable_channel(fbi, info->screen_base);
-
-	ret = register_framebuffer(&fbi->info);
-	if (ret < 0) {
+	fb_dev = register_framebuffer(&fbi->fb_host, pdata->framebuffer, 0);
+	if (fb_dev == NULL) {
 		dev_err(dev, "failed to register framebuffer\n");
-		return ret;
+		return -EINVAL;
 	}
 
 	return 0;
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 08/12] Add a video driver for S3C2440 bases platforms

[Juergen Beisert]

From: Juergen Beisert <juergen@kreuzholzen.de>

Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>
---
 arch/arm/mach-s3c24xx/include/mach/fb.h |   40 +++
 drivers/video/Kconfig                   |    6 +
 drivers/video/Makefile                  |    1 +
 drivers/video/s3c.c                     |  452 +++++++++++++++++++++++++++++++
 4 files changed, 499 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-s3c24xx/include/mach/fb.h
 create mode 100644 drivers/video/s3c.c

diff --git a/arch/arm/mach-s3c24xx/include/mach/fb.h b/arch/arm/mach-s3c24xx/include/mach/fb.h
new file mode 100644
index 0000000..eec6164
--- /dev/null
+++ b/arch/arm/mach-s3c24xx/include/mach/fb.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2010 Juergen Beisert
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+
+#ifndef __MACH_FB_H_
+# define __MACH_FB_H_
+
+#include <fb.h>
+
+struct s3c_fb_platform_data {
+
+	const struct fb_videomode *mode_list;
+	unsigned mode_cnt;
+
+	int passive_display;	/**< enable support for STN or CSTN displays */
+
+	void *framebuffer;	/**< force framebuffer base address */
+	unsigned size;		/**< force framebuffer size */
+
+	/** hook to enable backlight and stuff */
+	void (*enable)(int);
+};
+
+#endif /* __MACH_FB_H_ */
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d7f3d01..5a5edd2 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,4 +39,10 @@ config DRIVER_VIDEO_IMX_IPU
 	  Add support for the IPU framebuffer device found on
 	  i.MX31 and i.MX35 CPUs.
 
+config S3C_VIDEO
+	bool "S3C244x framebuffer driver"
+	depends on ARCH_S3C24xx
+	help
+	  Add support for the S3C244x LCD controller.
+
 endif
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 179f0c4..4287fc8 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_VIDEO) += fb.o
 
 obj-$(CONFIG_DRIVER_VIDEO_IMX) += imx.o
 obj-$(CONFIG_DRIVER_VIDEO_IMX_IPU) += imx-ipu-fb.o
+obj-$(CONFIG_S3C_VIDEO) += s3c.o
diff --git a/drivers/video/s3c.c b/drivers/video/s3c.c
new file mode 100644
index 0000000..8ae5785
--- /dev/null
+++ b/drivers/video/s3c.c
@@ -0,0 +1,452 @@
+/*
+ * Copyright (C) 2010 Juergen Beisert
+ *
+ * This driver is based on a patch found in the web:
+ * (C) Copyright 2006 by OpenMoko, Inc.
+ * Author: Harald Welte <laforge at openmoko.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+
+/* #define DEBUG */
+
+#include <common.h>
+#include <init.h>
+#include <fb.h>
+#include <driver.h>
+#include <malloc.h>
+#include <errno.h>
+#include <asm/io.h>
+#include <mach/gpio.h>
+#include <mach/s3c24xx-generic.h>
+#include <mach/s3c24x0-clocks.h>
+#include <mach/fb.h>
+
+#define LCDCON1 0x00
+# define PNRMODE(x) (((x) & 3) << 5)
+# define BPPMODE(x) (((x) & 0xf) << 1)
+# define SET_CLKVAL(x) (((x) & 0x3ff) << 8)
+# define GET_CLKVAL(x) (((x) >> 8) & 0x3ff)
+# define ENVID (1 << 0)
+
+#define LCDCON2 0x04
+# define SET_VBPD(x) (((x) & 0xff) << 24)
+# define SET_LINEVAL(x) (((x) & 0x3ff) << 14)
+# define SET_VFPD(x) (((x) & 0xff) << 6)
+# define SET_VSPW(x) ((x) & 0x3f)
+
+#define LCDCON3 0x08
+# define SET_HBPD(x) (((x) & 0x7f) << 19)
+# define SET_HOZVAL(x) (((x) & 0x7ff) << 8)
+# define SET_HFPD(x) ((x) & 0xff)
+
+#define LCDCON4 0x0c
+# define SET_HSPW(x) ((x) & 0xff)
+
+#define LCDCON5 0x10
+# define BPP24BL (1 << 12)
+# define FRM565 (1 << 11)
+# define INV_CLK (1 << 10)
+# define INV_HS (1 << 9)
+# define INV_VS (1 << 8)
+# define INV_DTA (1 << 7)
+# define INV_DE (1 << 6)
+# define INV_PWREN (1 << 5)
+# define INV_LEND (1 << 4)	/* FIXME */
+# define ENA_PWREN (1 << 3)
+# define ENA_LEND (1 << 2)	/* FIXME */
+# define BSWP (1 << 1)
+# define HWSWP (1 << 0)
+
+#define LCDSADDR1 0x14
+# define SET_LCDBANK(x) (((x) & 0x1ff) << 21)
+# define GET_LCDBANK(x) (((x) >> 21) & 0x1ff)
+# define SET_LCDBASEU(x) ((x) & 0x1fffff)
+# define GET_LCDBASEU(x) ((x) & 0x1fffff)
+
+#define LCDSADDR2 0x18
+# define SET_LCDBASEL(x) ((x) & 0x1fffff)
+# define GET_LCDBASEL(x) ((x) & 0x1fffff)
+
+#define LCDSADDR3 0x1c
+# define SET_OFFSIZE(x) (((x) & 0x7ff) << 11)
+# define GET_OFFSIZE(x) (((x) >> 11) & 0x7ff)
+# define SET_PAGE_WIDTH(x) ((x) & 0x3ff)
+# define GET_PAGE_WIDTH(x) ((x) & 0x3ff)
+
+#define RED_LUT 0x20
+#define GREEN_LUT 0x24
+#define BLUE_LUT 0x28
+
+#define DITHMODE 0x4c
+
+#define TPAL 0x50
+
+#define LCDINTPND 0x54
+
+#define LCDSRCPND 0x58
+
+#define LCDINTMSK 0x5c
+# define FIWSEL (1 << 2)
+
+#define TCONSEL 0x60
+
+#define RED 0
+#define GREEN 1
+#define BLUE 2
+#define TRANSP 3
+
+struct s3cfb_host {
+	struct fb_host fb_data;
+	struct device_d *hw_dev;
+	void __iomem *base;
+};
+
+#define fb_info_to_s3cfb_host(x) ((struct s3cfb_host*)((x)->host))
+#define s3cfb_host_to_s3c_fb_platform_data(x) ((struct s3c_fb_platform_data*)((x)->hw_dev->platform_data))
+
+/* the RGB565 true colour mode */
+static const struct fb_bitfield def_rgb565[] = {
+	[RED] = {
+		.offset = 11,
+		.length = 5,
+	},
+	[GREEN] = {
+		.offset = 5,
+		.length = 6,
+	},
+	[BLUE] = {
+		.offset = 0,
+		.length = 5,
+	},
+	[TRANSP] = {	/* no support for transparency */
+		.length = 0,
+	}
+};
+
+/* the RGB888 true colour mode */
+static const struct fb_bitfield def_rgb888[] = {
+	[RED] = {
+		.offset = 16,
+		.length = 8,
+	},
+	[GREEN] = {
+		.offset = 8,
+		.length = 8,
+	},
+	[BLUE] = {
+		.offset = 0,
+		.length = 8,
+	},
+	[TRANSP] = {	/* no support for transparency */
+		.length = 0,
+	}
+};
+
+/**
+ * @param fb_info Framebuffer information
+ */
+static void s3cfb_enable_controller(struct fb_info *fb_info)
+{
+	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
+	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
+	uint32_t con1;
+
+	pr_debug("%s called\n", __func__);
+
+	con1 = readl(fbh->base + LCDCON1);
+
+	con1 |= ENVID;
+
+	writel(con1, fbh->base + LCDCON1);
+
+	if (pdata->enable)
+		(pdata->enable)(1);
+}
+
+/**
+ * @param fb_info Framebuffer information
+ */
+static void s3cfb_disable_controller(struct fb_info *fb_info)
+{
+	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
+	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
+	uint32_t con1;
+
+	pr_debug("%s called\n", __func__);
+
+	if (pdata->enable)
+		(pdata->enable)(0);
+
+	con1 = readl(fbh->base + LCDCON1);
+
+	con1 &= ~ENVID;
+
+	writel(con1, fbh->base + LCDCON1);
+}
+
+/**
+ * Prepare the video hardware for a specified video mode
+ * @param fb_info Framebuffer information
+ * @param mode The video mode description to initialize
+ * @return 0 on success
+ */
+static int s3cfb_initialize_mode(struct fb_info *fb_info, const struct fb_videomode *mode)
+{
+	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
+	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
+	unsigned size, hclk, div;
+	uint32_t con1, con2, con3, con4, con5 = 0;
+
+	pr_debug("%s called\n", __func__);
+
+	if (pdata->passive_display != 0) {
+		pr_err("Passive displays are currently not supported\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * we need at least this amount of memory for the framebuffer
+	 */
+	size = mode->xres * mode->yres * (fb_info->bits_per_pixel >> 3);
+	if (fb_info->fb_dev->size != 0) {
+		if (size > fb_info->fb_dev->size) {
+			pr_err("Cannot initialize video mode '%s': Its too large. "
+				"Required bytes are %u, available only %u\n",
+				mode->name, size, fb_info->fb_dev->size);
+			return -EINVAL;
+		}
+	} else
+		fb_info->fb_dev->size = size;
+
+	/*
+	 * if no framebuffer memory was specified yet, allocate one,
+	 * and allocate more memory, on user request
+	 */
+	if (fb_info->fb_dev->map_base == 0U)
+		fb_info->fb_dev->map_base = (resource_size_t)xzalloc(fb_info->fb_dev->size);
+
+	/* its useful to enable the unit's clock */
+	s3c244x_mod_clock(CLK_LCDC, 1);
+
+	/* ensure video output is _off_ */
+	writel(0x00000000, fbh->base + LCDCON1);
+
+	hclk = s3c24xx_get_hclk() / 1000U;	/* hclk in kHz */
+	div = hclk / PICOS2KHZ(mode->pixclock);
+	if (div < 3)
+		div  = 3;
+	/* pixel clock is: (hclk) / ((div + 1) * 2) */
+	div += 1;
+	div >>= 1;
+	div -= 1;
+
+	con1 = PNRMODE(3) | SET_CLKVAL(div);	/* PNRMODE=3 is TFT */
+
+	switch (fb_info->bits_per_pixel) {
+#if 0
+	/* TODO add CLUT based modes */
+	case 1:
+		con1 |= BPPMODE(8);
+		break;
+	case 2:
+		con1 |= BPPMODE(9);
+		break;
+	case 4:
+		con1 |= BPPMODE(10);
+		break;
+	case 8:
+		con1 |= BPPMODE(11);
+		break;
+#endif
+	case 16:
+		con1 |= BPPMODE(12);
+		con5 |= FRM565;
+		fb_info->red = def_rgb565[RED];
+		fb_info->green = def_rgb565[GREEN];
+		fb_info->blue = def_rgb565[BLUE];
+		fb_info->transp =  def_rgb565[TRANSP];
+		break;
+	case 24:
+		con1 |= BPPMODE(13);
+		con5 |= BPP24BL;	/* FIXME */
+		fb_info->red = def_rgb888[RED];
+		fb_info->green = def_rgb888[GREEN];
+		fb_info->blue = def_rgb888[BLUE];
+		fb_info->transp =  def_rgb888[TRANSP];
+		break;
+	default:
+		pr_err("Invalid bits per pixel value: %u\n", fb_info->bits_per_pixel);
+		return -EINVAL;
+	}
+
+	/* 'normal' in register description means positive logic */
+	if (!(mode->sync & FB_SYNC_HOR_HIGH_ACT))
+		con5 |= INV_HS;
+	if (!(mode->sync & FB_SYNC_VERT_HIGH_ACT))
+		con5 |= INV_VS;
+	if (!(mode->sync & FB_SYNC_DE_HIGH_ACT))
+		con5 |= INV_DE;
+	if (mode->sync & FB_SYNC_CLK_INVERT)
+		con5 |= INV_CLK;	/* display should latch at the rising edge */
+	if (mode->sync & FB_SYNC_SWAP_RGB)
+		con5 |= HWSWP;
+
+	/* TODO power enable config via platform data */
+
+	/* vertical timing */
+	con2 = SET_VBPD(mode->upper_margin - 1) |
+		SET_LINEVAL(mode->yres - 1) |
+		SET_VFPD(mode->lower_margin - 1) |
+		SET_VSPW(mode->vsync_len - 1);
+
+	/* horizontal timing */
+	con3 = SET_HBPD(mode->left_margin - 1) |
+		SET_HOZVAL(mode->xres - 1) |
+		SET_HFPD(mode->right_margin - 1);
+	con4 = SET_HSPW(mode->hsync_len - 1);
+
+	/* basic timing setup */
+	writel(con1, fbh->base + LCDCON1);
+	pr_debug(" Writing %08X into %08X (con1)\n", con1, fbh->base + LCDCON1);
+	writel(con2, fbh->base + LCDCON2);
+	pr_debug(" Writing %08X into %08X (con2)\n", con2, fbh->base + LCDCON2);
+	writel(con3, fbh->base + LCDCON3);
+	pr_debug(" Writing %08X into %08X (con3)\n", con3, fbh->base + LCDCON3);
+	writel(con4, fbh->base + LCDCON4);
+	pr_debug(" Writing %08X into %08X (con4)\n", con4, fbh->base + LCDCON4);
+	writel(con5, fbh->base + LCDCON5);
+	pr_debug(" Writing %08X into %08X (con5)\n", con5, fbh->base + LCDCON5);
+
+	pr_debug("Setting up the fb baseadress to %08X\n", fb_info->fb_dev->map_base);
+
+	/* framebuffer memory setup */
+	writel(fb_info->fb_dev->map_base >> 1, fbh->base + LCDSADDR1);
+	size = mode->xres * (fb_info->bits_per_pixel >> 3) * (mode->yres);
+	writel(SET_LCDBASEL((fb_info->fb_dev->map_base + size) >> 1), fbh->base + LCDSADDR2);
+
+	writel(SET_OFFSIZE(0) |
+		SET_PAGE_WIDTH((mode->xres * fb_info->bits_per_pixel) >> 4),
+		fbh->base + LCDSADDR3);
+
+	writel(FIWSEL, fbh->base + LCDINTMSK);
+
+	return 0;
+}
+
+/**
+ * Print some information about the current hardware state
+ * @param hw_dev S3C video device
+ */
+#ifdef CONFIG_VIDEO_INFO_VERBOSE
+static void s3cfb_info(struct device_d *hw_dev)
+{
+	uint32_t con1, addr1, addr2, addr3;
+
+	con1 = readl(hw_dev->map_base + LCDCON1);
+	addr1 = readl(hw_dev->map_base + LCDSADDR1);
+	addr2 = readl(hw_dev->map_base + LCDSADDR2);
+	addr3 = readl(hw_dev->map_base + LCDSADDR3);
+
+	printf(" Video hardware info:\n");
+	printf("  Video clock is running at %u Hz\n", s3c24xx_get_hclk() / ((GET_CLKVAL(con1) + 1) * 2));
+	printf("  Video memory bank starts at 0x%08X\n", GET_LCDBANK(addr1) << 22);
+	printf("  Video memory bank offset: 0x%08X\n", GET_LCDBASEU(addr1));
+	printf("  Video memory end: 0x%08X\n", GET_LCDBASEU(addr2));
+	printf("  Virtual screen offset size: %u half words\n", GET_OFFSIZE(addr3));
+	printf("  Virtual screen page width: %u half words\n", GET_PAGE_WIDTH(addr3));
+}
+#endif
+
+/*
+ * There is only one video hardware instance available.
+ * It makes no sense to dynamically allocate this data
+ */
+static struct s3cfb_host host_data = {
+	.fb_data.fb_mode = s3cfb_initialize_mode,
+	.fb_data.fb_enable = s3cfb_enable_controller,
+	.fb_data.fb_disable = s3cfb_disable_controller,
+};
+
+static int s3cfb_probe(struct device_d *hw_dev)
+{
+	struct s3c_fb_platform_data *pdata = hw_dev->platform_data;
+	struct device_d *fb_dev;
+
+	pr_debug("%s called\n", __func__);
+
+	if (pdata == NULL) {
+		pr_debug("Framebuffer driver missing platform data");
+		return -ENODEV;
+	}
+
+	/* enable unit's clock */
+	s3c244x_mod_clock(CLK_LCDC, 1);
+
+	writel(0, hw_dev->map_base + LCDCON1);
+	/* off may be means high level (FIXME itlp specific) */
+	writel(0, hw_dev->map_base + LCDCON5);
+
+	/* disable it again until user request */
+	s3c244x_mod_clock(CLK_LCDC, 0);
+
+	/* add runtime hardware info */
+	host_data.hw_dev = hw_dev;
+	host_data.base = (void*)hw_dev->map_base;
+
+	/* add runtime video info */
+	host_data.fb_data.mode = pdata->mode_list;
+	host_data.fb_data.mode_cnt = pdata->mode_cnt;
+
+	fb_dev = register_framebuffer(&host_data.fb_data, pdata->framebuffer, pdata->size);
+	if (fb_dev == NULL) {
+		pr_err("Failed to register framebuffer\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct driver_d s3cfb_driver = {
+	.name	= "s3cfb",
+	.probe	= s3cfb_probe,
+#ifdef CONFIG_VIDEO_INFO_VERBOSE
+	.info	= s3cfb_info,
+#endif
+};
+
+static int s3cfb_init(void)
+{
+	return register_driver(&s3cfb_driver);
+}
+
+device_initcall(s3cfb_init);
+
+/**
+ * The S3C244x LCD controller supports passive (CSTN/STN) and active (TFT) LC displays
+ *
+ * The driver itself currently supports only active TFT LC displays in the follwing manner:
+ *
+ *  * Palletized colours
+ *    - 1 bpp
+ *    - 2 bpp
+ *    - 4 bpp
+ *    - 8 bpp
+ *  * True colours
+ *    - 16 bpp
+ *    - 24 bpp
+ */
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 09/12] STM378x: Add video driver for this platform

[Juergen Beisert]

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 arch/arm/mach-stm/include/mach/fb.h |   38 +++
 drivers/video/Kconfig               |    7 +
 drivers/video/Makefile              |    1 +
 drivers/video/stm.c                 |  561 +++++++++++++++++++++++++++++++++++
 4 files changed, 607 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-stm/include/mach/fb.h
 create mode 100644 drivers/video/stm.c

diff --git a/arch/arm/mach-stm/include/mach/fb.h b/arch/arm/mach-stm/include/mach/fb.h
new file mode 100644
index 0000000..6596ef2
--- /dev/null
+++ b/arch/arm/mach-stm/include/mach/fb.h
@@ -0,0 +1,38 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#ifndef __MACH_FB_H
+# define __MACH_FB_H
+
+#include <fb.h>
+
+#define STMLCDIF_8BIT 1	/** pixel data bus to the display is of 8 bit width */
+#define STMLCDIF_16BIT 0 /** pixel data bus to the display is of 16 bit width */
+#define STMLCDIF_18BIT 2 /** pixel data bus to the display is of 18 bit width */
+#define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24 bit width */
+
+struct imx_fb_videomode {
+	struct fb_videomode *mode_list;
+	unsigned mode_count;
+	void *framebuffer;	/**< force fixed framebuffer address if != NULL */
+	unsigned size;		/**< force fixed size if != NULL */
+
+	unsigned dotclk_delay;	/**< refer manual HW_LCDIF_VDCTRL4 register */
+	unsigned ld_intf_width;	/**< refer STMLCDIF_* macros */
+};
+
+#endif /* __MACH_FB_H */
+
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 5a5edd2..6cda478 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -45,4 +45,11 @@ config S3C_VIDEO
 	help
 	  Add support for the S3C244x LCD controller.
 
+config DRIVER_VIDEO_STM
+	bool "i.MX23/28 framebuffer driver"
+	depends on ARCH_STM
+	help
+	  Say 'Y' here to enable framebuffer and spash screen support for
+	  i.MX23 and i.MX28 based systems.
+
 endif
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 4287fc8..0ddb81e 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_VIDEO) += fb.o
 
+obj-$(CONFIG_DRIVER_VIDEO_STM) += stm.o
 obj-$(CONFIG_DRIVER_VIDEO_IMX) += imx.o
 obj-$(CONFIG_DRIVER_VIDEO_IMX_IPU) += imx-ipu-fb.o
 obj-$(CONFIG_S3C_VIDEO) += s3c.o
diff --git a/drivers/video/stm.c b/drivers/video/stm.c
new file mode 100644
index 0000000..5f5d60a
--- /dev/null
+++ b/drivers/video/stm.c
@@ -0,0 +1,561 @@
+/*
+ * Copyright (C) 2010 Juergen Beisert, Pengutronix
+ *
+ * This code is based on:
+ * Author: Vitaly Wool <vital@embeddedalley.com>
+ *
+ * Copyright 2008-2009 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+/**
+ * @file
+ * @brief LCDIF driver for i.MX23 and i.MX28 (i.MX23 untested yet)
+ *
+ * The LCDIF support four modes of operation
+ * - MPU interface (to drive smart displays) -> not supported yet
+ * - VSYNC interface (like MPU interface plus Vsync) -> not supported yet
+ * - Dotclock interface (to drive LC displays with RGB data and sync signals)
+ * - DVI (to drive ITU-R BT656)  -> not supported yet
+ *
+ * This driver depends on a correct setup of the pins used for this purpose
+ * (platform specific).
+ *
+ * For the developer: Don't forget to set the data bus width to the display
+ * in the imx_fb_videomode structure. You will else end up with ugly colours.
+ * If you fight against jitter you can vary the clock delay. This is a feature
+ * of the i.MX28 and you can vary it between 2 ns ... 8 ns in 2 ns steps. Give
+ * the required value in the imx_fb_videomode structure.
+ */
+
+/* #define DEBUG */
+
+#include <common.h>
+#include <init.h>
+#include <driver.h>
+#include <malloc.h>
+#include <errno.h>
+#include <xfuncs.h>
+#include <asm/io.h>
+#include <mach/clock.h>
+#include <mach/fb.h>
+
+#define HW_LCDIF_CTRL 0x00
+# define CTRL_SFTRST (1 << 31)
+# define CTRL_CLKGATE (1 << 30)
+# define CTRL_BYPASS_COUNT (1 << 19)
+# define CTRL_VSYNC_MODE (1 << 18)
+# define CTRL_DOTCLK_MODE (1 << 17)
+# define CTRL_DATA_SELECT (1 << 16)
+# define SET_BUS_WIDTH(x) (((x) & 0x3) << 10)
+# define SET_WORD_LENGTH(x) (((x) & 0x3) << 8)
+# define GET_WORD_LENGTH(x) (((x) >> 8) & 0x3)
+# define CTRL_MASTER (1 << 5)
+# define CTRL_DF16 (1 << 3)
+# define CTRL_DF18 (1 << 2)
+# define CTRL_DF24 (1 << 1)
+# define CTRL_RUN (1 << 0)
+
+#define HW_LCDIF_CTRL1 0x10
+# define SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16)
+# define GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf)
+
+#define HW_LCDIF_CTRL2 0x20
+
+#define HW_LCDIF_TRANSFER_COUNT 0x30
+# define SET_VCOUNT(x) (((x) & 0xffff) << 16)
+# define SET_HCOUNT(x) ((x) & 0xffff)
+
+#define HW_LCDIF_CUR_BUF 0x40
+
+#define HW_LCDIF_NEXT_BUF 0x50
+
+#define HW_LCDIF_TIMING 0x60
+# define SET_CMD_HOLD(x) (((x) & 0xff) << 24)
+# define SET_CMD_SETUP(x) (((x) & 0xff) << 16)
+# define SET_DATA_HOLD(x) (((x) & 0xff) << 8)
+# define SET_DATA_SETUP(x) ((x) & 0xff))
+
+#define HW_LCDIF_VDCTRL0 0x70
+# define VDCTRL0_ENABLE_PRESENT (1 << 28)
+# define VDCTRL0_VSYNC_POL (1 << 27) /* 0 = low active, 1 = high active */
+# define VDCTRL0_HSYNC_POL (1 << 26) /* 0 = low active, 1 = high active */
+# define VDCTRL0_DOTCLK_POL (1 << 25) /* 0 = output at falling edge, capturing at rising edge */
+# define VDCTRL0_ENABLE_POL (1 << 24) /* 0 = low active, 1 = high active */
+# define VDCTRL0_VSYNC_PERIOD_UNIT (1 << 21)
+# define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT (1 << 20)
+# define VDCTRL0_HALF_LINE (1 << 19)
+# define VDCTRL0_HALF_LINE_MODE (1 << 18)
+# define SET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff)
+
+#define HW_LCDIF_VDCTRL1 0x80
+
+#define HW_LCDIF_VDCTRL2 0x90
+# define SET_HSYNC_PULSE_WIDTH(x) (((x) & 0x3fff) << 18)
+# define SET_HSYNC_PERIOD(x) ((x) & 0x3ffff)
+
+#define HW_LCDIF_VDCTRL3 0xa0
+# define VDCTRL3_MUX_SYNC_SIGNALS (1 << 29)
+# define VDCTRL3_VSYNC_ONLY (1 << 28)
+# define SET_HOR_WAIT_CNT(x) (((x) & 0xfff) << 16)
+# define SET_VERT_WAIT_CNT(x) ((x) & 0xffff)
+
+#define HW_LCDIF_VDCTRL4 0xb0
+# define SET_DOTCLK_DLY(x) (((x) & 0x7) << 29)
+# define VDCTRL4_SYNC_SIGNALS_ON (1 << 18)
+# define SET_DOTCLK_H_VALID_DATA_CNT(x) ((x) & 0x3ffff)
+
+#define HW_LCDIF_DVICTRL0 0xc0
+#define HW_LCDIF_DVICTRL1 0xd0
+#define HW_LCDIF_DVICTRL2 0xe0
+#define HW_LCDIF_DVICTRL3 0xf0
+#define HW_LCDIF_DVICTRL4 0x100
+#define HW_LCDIF_DATA 0x180
+
+#define HW_LCDIF_DEBUG0 0x1d0
+# define DEBUG_HSYNC (1 < 26)
+# define DEBUG_VSYNC (1 < 25)
+
+#define RED 0
+#define GREEN 1
+#define BLUE 2
+#define TRANSP 3
+
+struct imxfb_host {
+	struct fb_host fb_data;
+	struct device_d *hw_dev;
+	void __iomem *base;
+};
+
+#define fb_info_to_imxfb_host(x) ((struct imxfb_host*)((x)->host))
+
+/* the RGB565 true colour mode */
+static const struct fb_bitfield def_rgb565[] = {
+	[RED] = {
+		.offset = 11,
+		.length = 5,
+	},
+	[GREEN] = {
+		.offset = 5,
+		.length = 6,
+	},
+	[BLUE] = {
+		.offset = 0,
+		.length = 5,
+	},
+	[TRANSP] = {	/* no support for transparency */
+		.length = 0,
+	}
+};
+
+/* the RGB666 true colour mode */
+static const struct fb_bitfield def_rgb666[] = {
+	[RED] = {
+		.offset = 16,
+		.length = 6,
+	},
+	[GREEN] = {
+		.offset = 8,
+		.length = 6,
+	},
+	[BLUE] = {
+		.offset = 0,
+		.length = 6,
+	},
+	[TRANSP] = {	/* no support for transparency */
+		.length = 0,
+	}
+};
+
+/* the RGB888 true colour mode */
+static const struct fb_bitfield def_rgb888[] = {
+	[RED] = {
+		.offset = 16,
+		.length = 8,
+	},
+	[GREEN] = {
+		.offset = 8,
+		.length = 8,
+	},
+	[BLUE] = {
+		.offset = 0,
+		.length = 8,
+	},
+	[TRANSP] = {	/* no support for transparency */
+		.length = 0,
+	}
+};
+
+/**
+ * Just calculate the amount of required bytes per line
+ * @param ppl Used pixel per line
+ * @param bpp Bits per pixel
+ * @return Byte count
+ */
+static inline unsigned calc_line_length(unsigned ppl, unsigned bpp)
+{
+	return (ppl * bpp) >> 3;
+}
+
+/**
+ * Prepare the video hardware for a specified video mode
+ * @param fb_info Framebuffer information
+ * @param mode The video mode description to initialize
+ * @return 0 on success
+ *
+ * Dotclock mode:
+ * One line of pixels or one frame in the i.MX28 is defined to:
+ * @verbatim
+ * |<---------------------- one line period -------------------------------->|
+ * |<- HSync length ->|
+ * |<----- Start of line --->|
+ *                           |<-------- active line data ------>|
+ *
+ * |<------------------------ frame period --------------------------------->|
+ * |<- VSync length ->|
+ * |<--- Start of 1. line -->|
+ *                           |<---------- active lines -------->|
+ * @endverbatim
+ * Based on the values from struct fb_videomode:
+ * - "one line period" = left_margin + xres + right_margin + hsync_len
+ * - "HSync length" = hsync_len
+ * - "Start of line" = hsync_len + left_margin
+ * - "active line data" = xres
+ */
+static int stmfb_initialize_mode(struct fb_info *fb_info,
+		const struct fb_videomode *mode)
+{
+	struct imxfb_host *fbh = fb_info_to_imxfb_host(fb_info);
+	struct imx_fb_videomode *pdata = fbh->hw_dev->platform_data;
+	uint32_t reg;
+	unsigned size;
+
+	pr_debug("%s called\n", __func__);
+	/*
+	 * we need at least this amount of memory for the framebuffer
+	 */
+	size = calc_line_length(mode->xres, fb_info->bits_per_pixel) * mode->yres;
+	if (fb_info->fb_dev->size != 0) {
+		if (size > fb_info->fb_dev->size) {
+			pr_err("Cannot initialize video mode '%s': Its too large. "
+				"Required bytes are %u, available only %u\n",
+				mode->name, size, fb_info->fb_dev->size);
+			return -EINVAL;
+		}
+	} else
+		fb_info->fb_dev->size = size;
+
+	/*
+	 * if no framebuffer memory was specified yet, allocate one,
+	 * and allocate more memory, on user request
+	 */
+	if (fb_info->fb_dev->map_base == 0U)
+		fb_info->fb_dev->map_base = (resource_size_t)xzalloc(fb_info->fb_dev->size);
+
+	/* TODO HCLK must be active at this point of time! */
+
+	size = imx_set_lcdifclk(PICOS2KHZ(mode->pixclock));
+	if (size == 0) {
+		pr_debug("Unable to set a valid pixel clock\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * bring the controller out of reset and configure it into DOTCLOCK mode
+	 */
+	reg = CTRL_BYPASS_COUNT |	/* always in DOTCLOCK mode */
+		CTRL_DOTCLK_MODE;
+	writel(reg, fbh->base + HW_LCDIF_CTRL);
+
+	/* master mode only */
+	reg |= CTRL_MASTER;
+
+	/*
+	 * Configure videomode and interface mode
+	 */
+	reg |= SET_BUS_WIDTH(pdata->ld_intf_width);
+	switch (fb_info->bits_per_pixel) {
+	case 8:
+		reg |= SET_WORD_LENGTH(1);
+		/* TODO refer manual page 2046 */
+		pr_warning("8 bpp mode not supported yet\n");
+		break;
+	case 16:
+		pr_debug("Setting up an RGB565 mode\n");
+		reg |= SET_WORD_LENGTH(0) | CTRL_DF16; /* we assume RGB565 */
+		writel(SET_BYTE_PACKAGING(0xf), fbh->base + HW_LCDIF_CTRL1);
+		fb_info->red = def_rgb565[RED];
+		fb_info->green = def_rgb565[GREEN];
+		fb_info->blue = def_rgb565[BLUE];
+		fb_info->transp =  def_rgb565[TRANSP];
+		break;
+	case 24:
+	case 32:
+		pr_debug("Setting up an RGB888/666 mode\n");
+		reg |= SET_WORD_LENGTH(3);
+		switch (pdata->ld_intf_width) {
+		case STMLCDIF_8BIT:
+			pr_debug("Unsupported LCD bus width mapping\n");
+			break;
+		case STMLCDIF_16BIT:
+		case STMLCDIF_18BIT:
+			/* 24 bit to 18 bit mapping */
+			reg |= CTRL_DF24; /* ignore the upper 2 bits in each colour component */
+			fb_info->red = def_rgb666[RED];
+			fb_info->green = def_rgb666[GREEN];
+			fb_info->blue = def_rgb666[BLUE];
+			fb_info->transp =  def_rgb666[TRANSP];
+			break;
+		case STMLCDIF_24BIT:
+			/* real 24 bit */
+			fb_info->red = def_rgb888[RED];
+			fb_info->green = def_rgb888[GREEN];
+			fb_info->blue = def_rgb888[BLUE];
+			fb_info->transp =  def_rgb888[TRANSP];
+			break;
+		}
+		/* do not use packed pixels = one pixel per word instead */
+		writel(SET_BYTE_PACKAGING(0x7), fbh->base + HW_LCDIF_CTRL1);
+		break;
+	default:
+		pr_debug("Unhandled colour depth of %u\n", fb_info->bits_per_pixel);
+		return -EINVAL;
+	}
+	writel(reg, fbh->base + HW_LCDIF_CTRL);
+	pr_debug("Setting up CTRL to %08X\n", reg);
+
+	writel(SET_VCOUNT(mode->yres) |
+		SET_HCOUNT(mode->xres), fbh->base + HW_LCDIF_TRANSFER_COUNT);
+
+	reg = VDCTRL0_ENABLE_PRESENT |	/* always in DOTCLOCK mode */
+		VDCTRL0_VSYNC_PERIOD_UNIT |
+		VDCTRL0_VSYNC_PULSE_WIDTH_UNIT;
+	if (mode->sync & FB_SYNC_HOR_HIGH_ACT)
+		reg |= VDCTRL0_HSYNC_POL;
+	if (mode->sync & FB_SYNC_VERT_HIGH_ACT)
+		reg |= VDCTRL0_VSYNC_POL;
+	if (mode->sync & FB_SYNC_DE_HIGH_ACT)
+		reg |= VDCTRL0_ENABLE_POL;
+	if (mode->sync & FB_SYNC_CLK_INVERT)
+		reg |= VDCTRL0_DOTCLK_POL;
+
+	reg |= SET_VSYNC_PULSE_WIDTH(mode->vsync_len);
+	writel(reg, fbh->base + HW_LCDIF_VDCTRL0);
+	pr_debug("Setting up VDCTRL0 to %08X\n", reg);
+
+	/* frame length in lines */
+	writel(mode->upper_margin + mode->vsync_len + mode->lower_margin + mode->yres,
+		fbh->base + HW_LCDIF_VDCTRL1);
+
+	/* line length in units of clocks or pixels */
+	writel(SET_HSYNC_PULSE_WIDTH(mode->hsync_len) |
+		SET_HSYNC_PERIOD(mode->left_margin + mode->hsync_len + mode->right_margin + mode->xres),
+		fbh->base + HW_LCDIF_VDCTRL2);
+
+	writel(SET_HOR_WAIT_CNT(mode->left_margin + mode->hsync_len) |
+		SET_VERT_WAIT_CNT(mode->upper_margin + mode->vsync_len),
+		fbh->base + HW_LCDIF_VDCTRL3);
+
+	writel(SET_DOTCLK_DLY(pdata->dotclk_delay) |
+		SET_DOTCLK_H_VALID_DATA_CNT(mode->xres),
+		fbh->base + HW_LCDIF_VDCTRL4);
+
+	writel(fb_info->fb_dev->map_base, fbh->base + HW_LCDIF_CUR_BUF);
+	/* always show one framebuffer only */
+	writel(fb_info->fb_dev->map_base, fbh->base + HW_LCDIF_NEXT_BUF);
+
+	return 0;
+}
+
+/**
+ * @param fb_info Framebuffer information
+ */
+static void stmfb_enable_controller(struct fb_info *fb_info)
+{
+	struct imxfb_host *fbh = fb_info_to_imxfb_host(fb_info);
+	uint32_t reg, last_reg;
+	unsigned loop, edges;
+
+	pr_debug("%s called\n", __func__);
+
+	/* if it was disabled, re-enable the mode again */
+	reg = readl(fbh->base + HW_LCDIF_CTRL);
+	reg |= CTRL_DOTCLK_MODE;
+	writel(reg, fbh->base + HW_LCDIF_CTRL);
+
+	/* enable the SYNC signals first, then the DMA enginge */
+	reg = readl(fbh->base + HW_LCDIF_VDCTRL4);
+	reg |= VDCTRL4_SYNC_SIGNALS_ON;
+	writel(reg, fbh->base + HW_LCDIF_VDCTRL4);
+
+	/*
+	 * Give the attached LC display or monitor a chance to sync into
+	 * our signals.
+	 * Wait for at least 2 VSYNCs = four VSYNC edges
+	 */
+	edges = 4;
+
+	while (edges != 0) {
+		loop = 800;
+		last_reg = readl(fbh->base + HW_LCDIF_DEBUG0) & DEBUG_VSYNC;
+		do {
+			reg = readl(fbh->base + HW_LCDIF_DEBUG0) & DEBUG_VSYNC;
+			if (reg != last_reg);
+				break;
+			last_reg = reg;
+			loop--;
+		} while (loop != 0);
+		edges--;
+	}
+
+	reg = readl(fbh->base + HW_LCDIF_CTRL);
+	reg |= CTRL_RUN;
+	writel(reg, fbh->base + HW_LCDIF_CTRL);
+}
+
+/**
+ * @param fb_info Framebuffer information
+ */
+static void stmfb_disable_controller(struct fb_info *fb_info)
+{
+	struct imxfb_host *fbh = fb_info_to_imxfb_host(fb_info);
+	unsigned loop;
+	uint32_t reg;
+
+	pr_debug("%s called\n", __func__);
+	/*
+	 * Even if we disable the controller here, it will still continue
+	 * until its FIFOs are running out of data
+	 */
+	reg = readl(fbh->base + HW_LCDIF_CTRL);
+	reg &= ~CTRL_DOTCLK_MODE;
+	writel(reg, fbh->base + HW_LCDIF_CTRL);
+
+	loop = 1000;
+	while (loop) {
+		reg = readl(fbh->base + HW_LCDIF_CTRL);
+		if (!(reg & CTRL_RUN))
+			break;
+		loop--;
+	}
+
+	reg = readl(fbh->base + HW_LCDIF_VDCTRL4);
+	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
+	writel(reg, fbh->base + HW_LCDIF_VDCTRL4);
+}
+
+/**
+ * Print some information about the current hardware state
+ * @param hw_dev STM video device
+ */
+static void stmfb_info(struct device_d *hw_dev)
+{
+	uint32_t reg;
+
+	printf(" STM video hardware:\n");
+	printf("  Pixel clock: %u kHz\n", imx_get_lcdifclk());
+
+	reg = readl(hw_dev->map_base + HW_LCDIF_CTRL);
+	switch (GET_WORD_LENGTH(reg)) {
+	case 0:
+		printf("  16 bpp mode with %s colour scheme\n",
+			reg & CTRL_DF16 ? "RGB565" : "ARGB1555");
+		switch (GET_BYTE_PACKAGING(readl(hw_dev->map_base + HW_LCDIF_CTRL1))) {
+		case 0xf:
+			printf("  One pixel per halfword\n");
+			break;
+		case 0x3:
+			printf("  One pixel per word\n");
+			break;
+		default:
+			printf("Unknown pixel packaging: %u\n",
+				GET_BYTE_PACKAGING(readl(hw_dev->map_base + HW_LCDIF_CTRL1)));
+		}
+		break;
+	case 1:
+	case 2:
+		printf("Unsupported bpp mode yet!\n");
+		break;
+	case 3:
+		printf("  24 bpp mode with %s colour scheme\n",
+			reg & CTRL_DF24 ? "RGB888" : "RGB666");
+		switch (GET_BYTE_PACKAGING(readl(hw_dev->map_base + HW_LCDIF_CTRL1))) {
+		case 0x7:
+			printf("  One pixel per word (xRGB xRGB xRGB ...)\n");
+			break;
+		case 0xf:
+			printf("Packed pixel format per word (RGBR GBRG BRGB ...)\n");
+			break;
+		default:
+			printf("Unknown pixel packaging: %u\n",
+				GET_BYTE_PACKAGING(readl(hw_dev->map_base + HW_LCDIF_CTRL1)));
+		}
+		break;
+	}
+}
+
+/*
+ * There is only one video hardware instance available.
+ * It makes no sense to dynamically allocate this data
+ */
+static struct imxfb_host host_data = {
+	.fb_data.fb_mode = stmfb_initialize_mode,
+	.fb_data.fb_enable = stmfb_enable_controller,
+	.fb_data.fb_disable = stmfb_disable_controller,
+	.fb_data.bits_per_pixel = 16,
+};
+
+static int stmfb_probe(struct device_d *hw_dev)
+{
+	struct imx_fb_videomode *pdata = hw_dev->platform_data;
+	struct device_d *fb_dev;
+
+	if (pdata == NULL) {
+		pr_debug("STMFB: No platformdata. Giving up\n");
+		return -ENODEV;
+	}
+
+	/* add runtime hardware info */
+	host_data.hw_dev = hw_dev;
+	host_data.base = (void*)hw_dev->map_base;
+
+	/* add runtime video info */
+	host_data.fb_data.mode = pdata->mode_list;
+	host_data.fb_data.mode_cnt = pdata->mode_count;
+
+	fb_dev = register_framebuffer(&host_data.fb_data, pdata->framebuffer,
+					pdata->size);
+	if (fb_dev == NULL) {
+		pr_err("STMFB: Failed to register framebuffer\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct driver_d stmfb_driver = {
+	.name	= "stmfb",
+	.probe	= stmfb_probe,
+	.info	= stmfb_info,
+};
+
+static int stmfb_init(void)
+{
+	return register_driver(&stmfb_driver);
+}
+
+device_initcall(stmfb_init);
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 10/12] Remove variable size restrictions

[Juergen Beisert]

There is no really need for special variable types in these structures. Replace
them by standard C types with the same behaviour.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 include/fb.h |   41 ++++++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/include/fb.h b/include/fb.h
index c94c1d0..b42532e 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -43,19 +43,19 @@
 
 struct fb_videomode {
 	const char *name;	/* optional */
-	u32 refresh;		/* optional */
-	u32 xres;
-	u32 yres;
-	u32 pixclock;
-	u32 left_margin;
-	u32 right_margin;
-	u32 upper_margin;
-	u32 lower_margin;
-	u32 hsync_len;
-	u32 vsync_len;
-	u32 sync;
-	u32 vmode;
-	u32 flag;
+	unsigned refresh;		/* optional */
+	unsigned xres;
+	unsigned yres;
+	unsigned pixclock;
+	unsigned left_margin;
+	unsigned right_margin;
+	unsigned upper_margin;
+	unsigned lower_margin;
+	unsigned hsync_len;
+	unsigned vsync_len;
+	unsigned sync;
+	unsigned vmode;
+	unsigned flag;
 };
 
 /* Interpretation of offset for color fields: All offsets are from the right,
@@ -69,10 +69,9 @@ struct fb_videomode {
  * of available palette entries (i.e. # of entries = 1 << length).
  */
 struct fb_bitfield {
-	u32 offset;			/* beginning of bitfield	*/
-	u32 length;			/* length of bitfield		*/
-	u32 msb_right;			/* != 0 : Most significant bit is */ 
-					/* right */ 
+	unsigned offset;		/* beginning of bitfield	*/
+	unsigned length;		/* length of bitfield		*/
+	int msb_right;			/* != 0 : Most significant bit is right */
 };
 
 struct fb_info;
@@ -97,11 +96,11 @@ struct fb_info {
 	struct device_d *fb_dev;
 	const struct fb_videomode *active_mode;
 
-	u32 xres;			/* visible resolution		*/
-	u32 yres;
-	u32 bits_per_pixel;		/* guess what			*/
+	unsigned xres;			/* visible resolution		*/
+	unsigned yres;
+	unsigned bits_per_pixel;	/* guess what			*/
 
-	u32 grayscale;			/* != 0 Graylevels instead of colors */
+	int grayscale;			/* != 0 Graylevels instead of colors */
 
 	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
 	struct fb_bitfield green;	/* else only length is significant */
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 11/12] Add doxygen documentation to the framebfuffer code

[Juergen Beisert]

Add some (hopefully) helpful documentation to the source code.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 Documentation/developers_manual.dox |    1 +
 Documentation/users_manual.dox      |    1 +
 drivers/video/fb.c                  |  234 +++++++++++++++++++++++++++++++++++
 include/fb.h                        |   98 ++++++++++-----
 4 files changed, 301 insertions(+), 33 deletions(-)

diff --git a/Documentation/developers_manual.dox b/Documentation/developers_manual.dox
index 2f7d360..69a0872 100644
--- a/Documentation/developers_manual.dox
+++ b/Documentation/developers_manual.dox
@@ -20,5 +20,6 @@ This part of the documentation is intended for developers of @a barebox.
 @li @subpage barebox_simul
 @li @subpage io_access_functions
 @li @subpage mcfv4e_MCDlib
+@li @subpage fb_for_developers
 
 */
diff --git a/Documentation/users_manual.dox b/Documentation/users_manual.dox
index 0a0a13e..f434fc4 100644
--- a/Documentation/users_manual.dox
+++ b/Documentation/users_manual.dox
@@ -11,5 +11,6 @@ work easier.
 @li @subpage command_reference
 @li @subpage x86_bootloader
 @li @subpage net_netconsole
+@li @subpage fb_for_users
 
 */
diff --git a/drivers/video/fb.c b/drivers/video/fb.c
index cb66744..fb95316 100644
--- a/drivers/video/fb.c
+++ b/drivers/video/fb.c
@@ -32,6 +32,13 @@ static int fb_ioctl(struct cdev* cdev, int req, void *data)
 }
 
 #ifdef CONFIG_VIDEO_DELAY_INIT
+/**
+ * Check if the framebuffer is already initialized
+ * @param fb_dev The framebuffer device to check
+ * @return 0 if the framebuffer is still uninitialized, -EPERM if already initialized
+ *
+ * @note Currently video mode initializing is a "one time only" task.
+ */
 static int fb_check_if_already_initialized(struct device_d *fb_dev)
 {
 	struct cdev *cdev = fb_dev->priv;
@@ -45,6 +52,9 @@ static int fb_check_if_already_initialized(struct device_d *fb_dev)
 	return 0;
 }
 
+/**
+ * Change colour depth via device parameter
+ */
 static int fb_cdepth_set(struct device_d *fb_dev, struct param_d *param, const char *val)
 {
 	struct cdev *cdev = fb_dev->priv;
@@ -66,6 +76,9 @@ static int fb_cdepth_set(struct device_d *fb_dev, struct param_d *param, const c
 }
 #endif
 
+/**
+ * Enable/disable video output via device parameter
+ */
 static int fb_enable_set(struct device_d *fb_dev, struct param_d *param, const char *val)
 {
 	struct cdev *cdev = fb_dev->priv;
@@ -95,6 +108,10 @@ static int fb_enable_set(struct device_d *fb_dev, struct param_d *param, const c
 	return dev_param_set_generic(fb_dev, param, new);
 }
 
+/**
+ * Output the list of supported video modes in this framebuffer
+ * @param host Platformdata of the hardware video device
+ */
 static void fb_list_modes(struct fb_host *host)
 {
 	unsigned u;
@@ -107,6 +124,14 @@ static void fb_list_modes(struct fb_host *host)
 		printf("  '%s'\n", host->mode[u].name);
 }
 
+/**
+ * Call the video hardware driver to initialize the given video mode
+ * @param fb_dev Framebuffer device
+ * @param mode Mode description to initialize
+ * @return 0 on success
+ *
+ * @note This call does not imply enabling the video output device!
+ */
 static int fb_activate_mode(struct device_d *fb_dev, const struct fb_videomode *mode)
 {
 	struct cdev *cdev = fb_dev->priv;
@@ -131,6 +156,15 @@ static int fb_activate_mode(struct device_d *fb_dev, const struct fb_videomode *
 }
 
 #ifdef CONFIG_VIDEO_DELAY_INIT
+/**
+ * Setup the requested video mode via device parameter
+ * @param dev Device instance
+ * @param param FIXME
+ * @param name Video mode name to activate
+ * @return 0 on success
+ *
+ * @note One time setup only, changing video modes is currently not supported.
+ */
 static int fb_mode_set(struct device_d *fb_dev, struct param_d *param, const char *name)
 {
 	struct fb_host *host = fb_dev->platform_data;
@@ -207,6 +241,14 @@ static void fb_info(struct device_d *fb_dev)
 	fb_list_modes(fb_dev->platform_data);
 }
 
+/**
+ * Add parameter to the framebuffer device on demand
+ * @param dev Device instance
+ * @return 0 on success
+ *
+ * Some parameter are only available (or usefull) if the intialization or
+ * enabling the video hardware is delayed.
+ */
 static int add_fb_parameter(struct device_d *fb_dev)
 {
 #ifdef CONFIG_VIDEO_DELAY_INIT
@@ -289,6 +331,13 @@ static int framebuffer_init(void)
 
 device_initcall(framebuffer_init);
 
+/**
+ * Create a new framebuffer device (for convenience)
+ * @param pinfo Video device's platform data for this framebuffer device
+ * @param base force framebuffer's base address to given value, or NULL for dynamically allocation
+ * @param sze force framebuffer's size to this value, or 0 for dynamically allocation
+ * @return Pointer to the newly created device or NULL on failure
+ */
 struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned size)
 {
 	struct device_d *fb_dev;
@@ -312,3 +361,188 @@ struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned
 
 	return fb_dev;
 }
+
+/**
+@page fb_for_users Framebuffer handling for users
+
+@section regular_fb Regular framebuffer setup
+
+After registering the video device the driver initializes the video hardware,
+allocates framebuffer memory, clears this memory (set all to zero) but does not
+enable the video output hardware. This will keep the visible display dark/off
+Any shell script can now paint a splash screen image into the framebuffer memory
+(refer the 'bmp' command) and after it's done it can enable the video output via
+setting a framebuffer's parameter. This feature ensures a flicker free visual
+startup of the system.
+
+To enable the video output after painting a splash screen just run a
+@verbatim
+barebox:/ framebuffer0.enable=1
+@endverbatim
+in your shell code.
+
+@section delayed_fb Dynamic framebuffer setup
+
+If the platform supports more than one video output device, its possible to select
+one of the supported ones at runtime. Say 'y' to the "Delayed initialization" menu
+entry to activate this feature. This will add the parameter 'mode' to the framebuffer
+device and will delay the initialization of the video hardware until the video
+output device gets specified. But keep in mind that there will be also no
+framebuffer memory until the output video hardware and its videomode get
+specified. This is important to know, of you want to paint some nice splash
+screen.
+
+Running the @b devinfo command on this framebuffer0 device will output:
+@verbatim
+barebox:/ devinfo framebuffer0
+base  : 0x00000000
+size  : 0x00000000
+driver: framebuffer
+
+ Video/Mode info:
+  Video output not enabled
+ Current video mode:
+  No video mode selected yet
+ Supported video mode(s):
+  'QVGA'
+  'VGA'
+Parameters:
+          cdepth = 16
+            mode = <NULL>
+          enable = <NULL>
+@endverbatim
+
+@note As long @b devinfo reports a @b base or @b size of zero there is
+@b no framebuffer memory yet!
+
+This framebuffer device is not initialized yet. As shown in the list, it
+supports two video modes: QVGA and VGA.
+
+So, the user can first specifiy the video output device with (for example)
+@verbatim
+barebox:/ framebuffer0.mode="QVGA"
+@endverbatim
+
+After this the @b devinfo command's output changes to:
+@verbatim
+barebox:/ devinfo framebuffer0
+base  : 0x31fc0000
+size  : 0x00040000
+driver: framebuffer
+
+ Video/Mode info:
+  Video output not enabled
+ Current video mode:
+  Name: QVGA
+  Refresh rate: 60 Hz
+  Horizontal active pixel: 320
+  Vertical active lines: 240
+  Pixel clock: 6500 kHz
+  Left/Right margin (pixel): 20/20
+  Upper/Lower margin (lines): 10/10
+  HSYNC length in pixel: 10, polarity: high
+  VSYNC length in lines: 5, polarity: high
+  Colour depth: 16 bpp
+ Supported video mode(s):
+  'QVGA'
+  'VGA'
+Parameters:
+          cdepth = 16
+            mode = QVGA
+          enable = <NULL>
+@endverbatim
+As one can see, the framebuffer has a @b base, a @b size and a video mode
+configuration now.
+@note Take care if setting a video mode fails. In this case @b base and @b size
+will kept at zero!
+
+With this setting its possible to paint some kind of image into the framebuffer
+memory and enabling the video output as the final step at runtime
+@verbatim
+barebox:/ framebuffer0.enable=1
+@endverbatim
+The video output is fully enabled now:
+@verbatim
+barebox:/ devinfo framebuffer0
+base  : 0x31fc0000
+size  : 0x00040000
+driver: framebuffer
+
+ Video/Mode info:
+  Video output enabled
+ Current video mode:
+  Name: QVGA
+  Refresh rate: 60 Hz
+  Horizontal active pixel: 320
+  Vertical active lines: 240
+  Pixel clock: 6500 kHz
+  Left/Right margin (pixel): 20/20
+  Upper/Lower margin (lines): 10/10
+  HSYNC length in pixel: 10, polarity: high
+  VSYNC length in lines: 5, polarity: high
+  Colour depth: 16 bpp
+ Supported video mode(s):
+  'QVGA'
+  'VGA'
+Parameters:
+          cdepth = 16
+            mode = QVGA
+          enable = 1
+@endverbatim
+
+@section other_fb_params Other framebuffer parameter
+@verbatim
+framebuffer0.cdepth=[1 | 4 | 8 | 16 | 24 | 32]
+@endverbatim
+
+Only available if "Delayed initialization" is selected. Colour depth to be used with
+the framebuffer. Its unit is "bit per pixel" and the default value is 16 bits per
+pixel (means "RGB565" format). This value can only be changed prior specifying the
+video mode.
+
+@note The possible values from the list above are hardware dependend.
+
+@note The default colour depth value may also depend on the hardware
+*/
+
+/**
+@page fb_for_developers Framebuffer handling for developers
+
+@section fb_platform_dev For the platform developer
+
+When filling the platform specific video output device description you can still
+provide only one entry and you should setup the 'mode_cnt' entry with '0':
+Initialization of the video hardware will happen in accordance to the Kconfig
+menu settings.
+
+If you provide more than one video output device description use an array of
+this type. In this case the 'mode_cnt' entry must contain the count of existing
+array entries (> 1). Give each video output device description entry an unique
+name, because a user will select the required output device by this name
+at runtime.
+
+@section fb_driver_dev For the video hardware driver developer:
+
+Don't initialize a special video mode in your probe function (e.g. don't
+allocate any framebuffer memory and so on). The framework will call back your
+exported fb_mode() function to do so (immediately or delayed).
+
+Don't enable video output in your probe or exported fb_mode() function. Also
+do not switch on any LCD or backlight if any. The framework will call your
+exported fb_enable() function to do so.
+
+If your hardware cannot handle the default 16 bit colour depth, change the
+'bits_per_pixel' field prior registering your framebuffer.
+
+When your exported fb_mode() function is called, calculate the amount of memory
+you need for the requested video mode and colour depth, save this value to
+framebuffer's info struct in field 'fb_dev->size' and allocate the memory with
+this size for the framebuffer. Store the basepointer to this area into
+framebuffer's info struct in field 'fb_dev->map_base'.
+
+@note To support flickerless splash screen into the Linux kernel, your driver
+should support a fixed framebuffer memory. Fixed in location and size. The platform
+should hold the Linux kernel to not touch this memory in any way. Instead the
+kernel based video hardware driver should inherit the fixed settings.
+
+*/
diff --git a/include/fb.h b/include/fb.h
index b42532e..223b301 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -18,19 +18,27 @@
 					/* vtotal = 121d/242n/484i => NTSC */
 #define FB_SYNC_ON_GREEN	32	/* sync on green */
 /* LC display related settings */
+/** LC display uses active high data enable signal */
 #define FB_SYNC_DE_HIGH_ACT	(1 << 6)
+/** LC display will latch its data at clock's rising edge */
 #define FB_SYNC_CLK_INVERT	(1 << 7)
+/** output RGB data inverted */
 #define FB_SYNC_DATA_INVERT	(1 << 8)
+/** Stop clock if no data is sent (required for passive displays) */
 #define FB_SYNC_CLK_IDLE_EN	(1 << 9)
+/** swap RGB to BGR */
 #define FB_SYNC_SWAP_RGB	(1 << 10)
+/** FIXME */
 #define FB_SYNC_CLK_SEL_EN	(1 << 11)
+/** enable special signals for SHARP displays (_very_ hardware specific) */
 #define FB_SYNC_SHARP_MODE	(1 << 31)
 
-#define FB_VMODE_NONINTERLACED  0	/* non interlaced */
-#define FB_VMODE_INTERLACED	1	/* interlaced	*/
+#define FB_VMODE_NONINTERLACED  0	/** non interlaced */
+#define FB_VMODE_INTERLACED	1	/** interlaced	*/
 #define FB_VMODE_DOUBLE		2	/* double scan */
-#define FB_VMODE_ODD_FLD_FIRST	4	/* interlaced: top line first */
+#define FB_VMODE_ODD_FLD_FIRST	4	/** interlaced: top line first */
 /* LC display related settings */
+/** output two screen parts at once (required for passive displays) */
 #define FB_VMODE_DUAL_SCAN	8
 #define FB_VMODE_MASK		255
 
@@ -42,19 +50,24 @@
 #define KHZ2PICOS(a) (1000000000UL/(a))
 
 struct fb_videomode {
-	const char *name;	/* optional */
-	unsigned refresh;		/* optional */
-	unsigned xres;
-	unsigned yres;
-	unsigned pixclock;
-	unsigned left_margin;
-	unsigned right_margin;
-	unsigned upper_margin;
-	unsigned lower_margin;
-	unsigned hsync_len;
-	unsigned vsync_len;
-	unsigned sync;
-	unsigned vmode;
+	const char *name;	/**< always required and must be unique */
+	unsigned refresh;	/**< frame refresh rate in [Hz] (optional) */
+	unsigned xres;		/**< visible horizontal pixel */
+	unsigned yres;		/**< visible vertical pixel */
+	unsigned pixclock;	/**< pixel clock period in [ps]. Refer
+				     PICOS2KHZ/KHZ2PICOS macros */
+	unsigned left_margin;	/**< distance in pixels between ending active HSYNC
+				     and starting visible line content */
+	unsigned right_margin;	/**< distance in pixels between ending visible line
+				     content and starting active HSYNC */
+	unsigned upper_margin;	/**< distance in lines between ending active VSYNC
+				     and the first line with visible content */
+	unsigned lower_margin;	/**< distance in lines between last line with
+				     visible content and starting active VSYNC */
+	unsigned hsync_len;	/**< HSYNC's active length in pixels */
+	unsigned vsync_len;	/**< VSYNC's active lenght in lines */
+	unsigned sync;		/**< sync information, refer FB_SYNC_* macros */
+	unsigned vmode;		/**< video mode information, refer FB_VMODE_* macros */
 	unsigned flag;
 };
 
@@ -69,18 +82,33 @@ struct fb_videomode {
  * of available palette entries (i.e. # of entries = 1 << length).
  */
 struct fb_bitfield {
-	unsigned offset;		/* beginning of bitfield	*/
-	unsigned length;		/* length of bitfield		*/
-	int msb_right;			/* != 0 : Most significant bit is right */
+	unsigned offset;		/**< beginning of bitfield	*/
+	unsigned length;		/**< length of bitfield		*/
+	int msb_right;			/**< != 0 : Most significant bit is right */
 };
 
 struct fb_info;
 
+/**
+ * Framebuffer device's platform information
+ *
+ * The video hardware driver must set the following fields:
+ * - 'fb_mode' function to setup a specific video mode
+ * - 'fb_enable' function to activate the video output
+ * - 'fb_disable' function to deactivate the video output
+ * - 'fb_setcolreg' function to ???????? FIXME
+ *
+ * The video hardware driver can set default values for the following fields:
+ * - 'mode' if the driver supports only specific video modes.
+ * - 'mode_cnt' must be set, if 'mode_list' is given
+ * - 'bits_per_pixel' if the video hardware driver defaults to another bpp than 16
+ */
 struct fb_host {
-	const struct fb_videomode *mode;
-	unsigned mode_cnt;
+	/* information about possible video mode(s) */
+	const struct fb_videomode *mode;	/**< Array of modes */
+	unsigned mode_cnt;		/**< count of entries in 'mode'. */
 
-	struct device_d *hw_dev;
+	struct device_d *hw_dev;	/**< the host device */
 
 	/* callbacks into the video hardware driver */
 	int (*fb_setcolreg)(struct fb_info*, unsigned, unsigned, unsigned, unsigned, unsigned);
@@ -88,24 +116,28 @@ struct fb_host {
 	void (*fb_enable)(struct fb_info*);
 	void (*fb_disable)(struct fb_info*);
 
-	unsigned bits_per_pixel;
+	unsigned bits_per_pixel;	/**< default bpp, 0 = use framebuffer's default */
 };
 
+/**
+ * Framebuffer's runtime information
+ */
 struct fb_info {
-	struct fb_host *host;
-	struct device_d *fb_dev;
+	struct fb_host *host;		/**< host data this fb is based on */
+	struct device_d *fb_dev;	/**< the framebuffer device */
+	/* information about current video mode */
+	/** the currently active video mode if set. Can be NULL = no video mode set yet */
 	const struct fb_videomode *active_mode;
+	unsigned xres;			/**< visible horizontal pixel count */
+	unsigned yres;			/**< visible vertical line count */
+	unsigned bits_per_pixel;	/**< requested colour depth */
 
-	unsigned xres;			/* visible resolution		*/
-	unsigned yres;
-	unsigned bits_per_pixel;	/* guess what			*/
-
-	int grayscale;			/* != 0 Graylevels instead of colors */
+	int grayscale;			/**< != 0 Graylevels instead of colors */
 
-	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
-	struct fb_bitfield green;	/* else only length is significant */
+	struct fb_bitfield red;		/**< bitfield in fb mem if true color, */
+	struct fb_bitfield green;	/**< else only length is significant */
 	struct fb_bitfield blue;
-	struct fb_bitfield transp;	/* transparency			*/
+	struct fb_bitfield transp;	/**< transparency			*/
 
 	int enabled;
 };
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 12/12] Provide more driver specific data in a videomode

[Juergen Beisert]

In order to support more than one videomode in a binary barebox, some drivers
need more specific data to setup the requested video mode in a correct manner.
This patch adds the 'priv' field to the generic videomode description to give
any platform a chance to forward some video hardware specific information on
a per videomode base.

This is currently i.MX21/i.MX25/i.MX27 specific.

BTW: At least the 'pcr' value could be generated at runtime from the 'sync'
field in 'struct fb_videomode'.

Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
---
 arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c |   22 +++++------
 arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c |   21 +++++-----
 arch/arm/boards/guf-neso/board.c                  |   45 +++++++++++----------
 arch/arm/boards/imx21ads/imx21ads.c               |   24 ++++++------
 arch/arm/boards/pcm038/pcm038.c                   |   40 +++++++++---------
 arch/arm/mach-imx/include/mach/imxfb.h            |   20 +++++----
 drivers/video/imx.c                               |   19 +++++----
 include/fb.h                                      |    2 +
 8 files changed, 99 insertions(+), 94 deletions(-)

diff --git a/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c b/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
index 032897a..9a9021a 100644
--- a/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
+++ b/arch/arm/boards/eukrea_cpuimx25/eukrea_cpuimx25.c
@@ -120,7 +120,14 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
-static struct fb_videomode cmo_display = {
+static const struct imx_fb_driver_data driver_data = {
+	.pwmr		= 0x00A903FF,
+	.lscr1		= 0x00120300,
+	.dmacr		= 0x80040060,
+	.pcr		= 0xCAD08B80,
+};
+
+static const struct fb_videomode cmo_display = {
 	.name		= "CMO-QVGA",
 	.refresh	= 60,
 	.xres		= 320,
@@ -134,20 +141,11 @@ static struct fb_videomode cmo_display = {
 	.lower_margin	= 4,
 };
 
-static struct imx_fb_videomode imxfb_mode = {
-	.mode		= &cmo_display,
-	.pcr		= 0xCAD08B80,
-	.bpp		= 16,
-};
-
 static struct imx_fb_platform_data eukrea_cpuimx25_fb_data = {
-	.mode		= &imxfb_mode,
-	.pwmr		= 0x00A903FF,
-	.lscr1		= 0x00120300,
-	.dmacr		= 0x80040060,
+	.mode		= &cmo_display,
+	.mode_cnt	= 1,
 };
 
-
 static struct device_d imxfb_dev = {
 	.id		= -1,
 	.name		= "imxfb",
diff --git a/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c b/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
index 3ea2466..3696105 100644
--- a/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
+++ b/arch/arm/boards/eukrea_cpuimx27/eukrea_cpuimx27.c
@@ -186,7 +186,14 @@ static void eukrea_cpuimx27_mmu_init(void)
 #endif
 
 #ifdef CONFIG_DRIVER_VIDEO_IMX
-static struct fb_videomode cmo_display = {
+static const struct imx_fb_driver_data driver_data = {
+	.pwmr	= 0x00A903FF,
+	.lscr1	= 0x00120300,
+	.dmacr	= 0x00020010,
+	.pcr	= 0xFAD08B80,
+};
+
+static const struct fb_videomode cmo_display = {
 	.name		= "CMO-QVGA",
 	.refresh	= 60,
 	.xres		= 320,
@@ -198,18 +205,12 @@ static struct fb_videomode cmo_display = {
 	.vsync_len	= 3,
 	.upper_margin	= 15,
 	.lower_margin	= 4,
+	.priv		= &driver_data,
 };
 
-static struct imx_fb_videomode imxfb_mode = {
-	.mode		= &cmo_display,
-	.pcr		= 0xFAD08B80,
-	.bpp		= 16,};
-
 static struct imx_fb_platform_data eukrea_cpuimx27_fb_data = {
-	.mode	= &imxfb_mode,
-	.pwmr	= 0x00A903FF,
-	.lscr1	= 0x00120300,
-	.dmacr	= 0x00020010,
+	.mode	= &cmo_display,
+	.mode_cnt = 1,
 };
 
 static struct device_d imxfb_dev = {
diff --git a/arch/arm/boards/guf-neso/board.c b/arch/arm/boards/guf-neso/board.c
index 6949675..a4d246f 100644
--- a/arch/arm/boards/guf-neso/board.c
+++ b/arch/arm/boards/guf-neso/board.c
@@ -91,22 +91,11 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
-static struct fb_videomode cpt_display = {
-	.name		= "CPT CLAA070LC0JCT",
-	.refresh	= 60,
-	.xres		= 800,
-	.yres		= 480,
-	.pixclock	= KHZ2PICOS(27000),
-	.hsync_len	= 1,	/* DE only sync */
-	.left_margin	= 50,
-	.right_margin	= 50,
-	.vsync_len	= 1,	/* DE only sync */
-	.upper_margin	= 10,
-	.lower_margin	= 10,
-};
-
-static struct imx_fb_videomode imxfb_mode = {
-	.mode = &cpt_display,
+static const struct imx_fb_driver_data driver_data = {
+	.pwmr	= 0x00000000,	/* doesn't matter */
+	.lscr1	= 0x00120300,	/* doesn't matter */
+	/* dynamic mode -> using the reset values (as recommended in the datasheet) */
+	.dmacr	= (0 << 31) | (4 << 16) | 96,
 	/*
 	 * - TFT style panel
 	 * - clk enabled while idle
@@ -122,7 +111,21 @@ static struct imx_fb_videomode imxfb_mode = {
 		PCR_SCLK_SEL |
 		PCR_LPPOL |
 		PCR_FLMPOL,
-	.bpp = 16,	/* TODO 32 bit does not work: The 'green' component is lacking in this mode */
+};
+
+static struct fb_videomode cpt_display = {
+	.name		= "CPT CLAA070LC0JCT",
+	.refresh	= 60,
+	.xres		= 800,
+	.yres		= 480,
+	.pixclock	= KHZ2PICOS(27000),
+	.hsync_len	= 1,	/* DE only sync */
+	.left_margin	= 50,
+	.right_margin	= 50,
+	.vsync_len	= 1,	/* DE only sync */
+	.upper_margin	= 10,
+	.lower_margin	= 10,
+	.priv		= &driver_data,
 };
 
 static void neso_fb_enable(int enable)
@@ -132,12 +135,10 @@ static void neso_fb_enable(int enable)
 }
 
 static struct imx_fb_platform_data neso_fb_data = {
-	.mode	= &imxfb_mode,
-	.pwmr	= 0x00000000,	/* doesn't matter */
-	.lscr1	= 0x00120300,	/* doesn't matter */
-	/* dynamic mode -> using the reset values (as recommended in the datasheet) */
-	.dmacr	= (0 << 31) | (4 << 16) | 96,
+	.mode	= &cpt_display,
+	.mode_cnt = 1,
 	.enable	= neso_fb_enable,
+	.bpp = 16,	/* TODO 32 bit does not work: The 'green' component is lacking in this mode */
 	.framebuffer_ovl = (void *)0xa7f00000,
 };
 
diff --git a/arch/arm/boards/imx21ads/imx21ads.c b/arch/arm/boards/imx21ads/imx21ads.c
index 81006de..3b654ec 100644
--- a/arch/arm/boards/imx21ads/imx21ads.c
+++ b/arch/arm/boards/imx21ads/imx21ads.c
@@ -79,7 +79,14 @@ static struct device_d cs8900_dev = {
 	// IRQ is connected to UART3_RTS
 };
 
-static struct fb_videomode sharp_display = {
+static const struct imx_fb_driver_data driver_data = {
+	.pwmr           = 0x00a903ff,
+	.lscr1          = 0x00120300,
+	.dmacr          = 0x00020008,
+        .pcr            = 0xfb108bc7,
+};
+
+static const struct fb_videomode sharp_display = {
 	.name           = "Sharp-LQ035Q7",
 	.refresh        = 60,
 	.xres           = 240,
@@ -94,23 +101,16 @@ static struct fb_videomode sharp_display = {
 	.sync           = 0,
 	.vmode          = FB_VMODE_NONINTERLACED,
 	.flag           = 0,
-};
-
-/* Sharp LQ035Q7DB02 QVGA display */
-static struct imx_fb_videomode imx_fb_modedata = {
-        .mode           = &sharp_display,
-        .pcr            = 0xfb108bc7,
-        .bpp            = 16,
+	.priv		= &driver_data,
 };
 
 static struct imx_fb_platform_data imx_fb_data = {
-	.mode           = &imx_fb_modedata,
+/* Sharp LQ035Q7DB02 QVGA display */
+	.mode           = &sharp_display,
+	.mode_cnt	= 1,
 	.cmap_greyscale = 0,
 	.cmap_inverse   = 0,
 	.cmap_static    = 0,
-	.pwmr           = 0x00a903ff,
-	.lscr1          = 0x00120300,
-	.dmacr          = 0x00020008,
 };
 
 static struct device_d imxfb_dev = {
diff --git a/arch/arm/boards/pcm038/pcm038.c b/arch/arm/boards/pcm038/pcm038.c
index 026e9c0..6a5d938 100644
--- a/arch/arm/boards/pcm038/pcm038.c
+++ b/arch/arm/boards/pcm038/pcm038.c
@@ -127,7 +127,23 @@ static struct device_d nand_dev = {
 	.platform_data	= &nand_info,
 };
 
-static struct fb_videomode sharp_display = {
+static const struct imx_fb_driver_data driver_data = {
+	/*
+	 * - HSYNC active high
+	 * - VSYNC active high
+	 * - clk notenabled while idle
+	 * - clock not inverted
+	 * - data not inverted
+	 * - data enable low active
+	 * - enable sharp mode
+	 */
+	.pwmr	= 0x00A903FF,
+	.lscr1	= 0x00120300,
+	.dmacr	= 0x00020010,
+	.pcr	= 0xF00080C0,
+};
+
+static const struct fb_videomode sharp_display = {
 	.name		= "Sharp-LQ035Q7",
 	.refresh	= 60,
 	.xres		= 240,
@@ -139,28 +155,12 @@ static struct fb_videomode sharp_display = {
 	.vsync_len	= 1,
 	.upper_margin	= 7,
 	.lower_margin	= 9,
-};
-
-static struct imx_fb_videomode imxfb_mode = {
-	.mode		= &sharp_display,
-	/*
-	 * - HSYNC active high
-	 * - VSYNC active high
-	 * - clk notenabled while idle
-	 * - clock not inverted
-	 * - data not inverted
-	 * - data enable low active
-	 * - enable sharp mode
-	 */
-	.pcr		= 0xF00080C0,
-	.bpp		= 16,
+	.priv		= &driver_data,
 };
 
 static struct imx_fb_platform_data pcm038_fb_data = {
-	.mode	= &imxfb_mode,
-	.pwmr	= 0x00A903FF,
-	.lscr1	= 0x00120300,
-	.dmacr	= 0x00020010,
+	.mode	= &sharp_display,
+	.mode_cnt = 1,
 };
 
 static struct device_d imxfb_dev = {
diff --git a/arch/arm/mach-imx/include/mach/imxfb.h b/arch/arm/mach-imx/include/mach/imxfb.h
index c536119..dbd7c93 100644
--- a/arch/arm/mach-imx/include/mach/imxfb.h
+++ b/arch/arm/mach-imx/include/mach/imxfb.h
@@ -49,28 +49,30 @@
 #define DMACR_HM(x)	(((x) & 0xf) << 16)
 #define DMACR_TM(x)	((x) & 0xf)
 
-struct imx_fb_videomode {
-	struct fb_videomode *mode;
-	u32 pcr;
-	unsigned char	bpp;
+/**
+ * Videomode dependent, but driver specific data
+ */
+struct imx_fb_driver_data {
+	uint32_t	pwmr;	/**< refer datasheet: LPCCR register */
+	uint32_t	lscr1;	/**< refer datasheet: LSCR register */
+	uint32_t	dmacr;	/**< refer datasheet: LDCR register */
+	uint32_t	pcr;	/**< refer datasheet: LPCR register */
 };
 
 /**
  * Define relevant framebuffer information
  */
 struct imx_fb_platform_data {
-	struct imx_fb_videomode *mode;
+	const struct fb_videomode *mode;
 	unsigned mode_cnt;	/**< number of entries in 'mode' */
 
+	unsigned char	bpp;	/**< preferred colour depth for this device */
+
 	u_int		cmap_greyscale:1,
 			cmap_inverse:1,
 			cmap_static:1,
 			unused:29;
 
-	u_int		pwmr;
-	u_int		lscr1;
-	u_int		dmacr;
-
 	/** force a memory area to be used, else NULL for dynamic allocation */
 	void		*framebuffer;
 	/** force a memory area to be used, else NULL for dynamic allocation */
diff --git a/drivers/video/imx.c b/drivers/video/imx.c
index 07438e5..d935149 100644
--- a/drivers/video/imx.c
+++ b/drivers/video/imx.c
@@ -300,6 +300,7 @@ static int imxfb_initialize_mode(struct fb_info *info, const struct fb_videomode
 	unsigned long lcd_clk;
 	unsigned long long tmp;
 	struct imxfb_info *fbi = fb_info_to_imxfb_info(info);
+	const struct imx_fb_driver_data *drv_data = mode->priv;
 	u32 pcr;
 	unsigned size;
 
@@ -324,6 +325,11 @@ static int imxfb_initialize_mode(struct fb_info *info, const struct fb_videomode
 	if (info->fb_dev->map_base == 0U /* FIXME should be 'NULL'*/)
 		info->fb_dev->map_base = (unsigned long)xzalloc(info->fb_dev->size);
 
+	fbi->pcr = drv_data->pcr;
+	fbi->pwmr = drv_data->pwmr;
+	fbi->lscr1 = drv_data->lscr1;
+	fbi->dmacr = drv_data->dmacr;
+
 	/* physical screen start address	    */
 	writel(VPW_VPW(mode->xres * info->bits_per_pixel / 8 / 4),
 		fbi->regs + LCDC_VPW);
@@ -535,7 +541,7 @@ static int imxfb_register_overlay(struct imxfb_info *fbi, struct device_d *hw_de
 	overlay->fb_setcolreg = imxfb_overlay_setcolreg;
 
 	/* add runtime video info */
-	overlay->mode = pdata->mode->mode;
+	overlay->mode = pdata->mode;
 	overlay->mode_cnt = 1;	/* no choice */
 
 	overlay_dev = register_framebuffer(overlay, pdata->framebuffer_ovl, 0);
@@ -585,17 +591,12 @@ static int imxfb_probe(struct device_d *dev)
 	fbi->fb_host.fb_setcolreg = imxfb_setcolreg;
 
 	fbi->regs = (void*)dev->map_base;
-	fbi->pcr = pdata->mode->pcr;
-	fbi->pwmr = pdata->pwmr;
-	fbi->lscr1 = pdata->lscr1;
-	fbi->dmacr = pdata->dmacr;
 	fbi->enable = pdata->enable;
 
 	/* add runtime video info */
-	fbi->fb_host.mode = pdata->mode->mode;
-	/* to be backward compatible */
-	fbi->fb_host.mode_cnt = pdata->mode_cnt == 0 ? 1 : pdata->mode_cnt;
-	fbi->fb_host.bits_per_pixel = 16;	/* RGB565, the default */
+	fbi->fb_host.mode = pdata->mode;
+	fbi->fb_host.mode_cnt = pdata->mode_cnt;
+	fbi->fb_host.bits_per_pixel = pdata->bpp;
 
 	fb_dev = register_framebuffer(&fbi->fb_host, pdata->framebuffer, 0);
 	if (dev == NULL) {
diff --git a/include/fb.h b/include/fb.h
index 223b301..8ba4e5a 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -69,6 +69,8 @@ struct fb_videomode {
 	unsigned sync;		/**< sync information, refer FB_SYNC_* macros */
 	unsigned vmode;		/**< video mode information, refer FB_VMODE_* macros */
 	unsigned flag;
+
+	const void *priv;	/**< video driver related information */
 };
 
 /* Interpretation of offset for color fields: All offsets are from the right,
-- 
1.7.2.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Sascha Hauer]

Hi Jürgen,

On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
> Currently barebox uses a fixed videomode setup. Everything is compiled in.
> This change adds the possibility to select a videomode according to a
> connected display at runtime. The current behaviour is still present if not
> otherwise configured. If configured for runtime setup, initialization of the
> video hardware will be delayed until the required videomode will be selected
> from the shell code. If more than one videomode is supported by the platform,
> running the 'devinfo' command on the framebuffer device shows the supported
> videomode list. After selecting the videomode, the output can be enabled.
>

General remarks about this series:

- Please do not add code with '#if 0' and activate it later. This shows
  the series has the wrong order.
- Please refrain from basing your internal functions around 'struct
  device_d'. By doing so we completey lose type safety and at least in
  case of the mci framework where three different devices are involved
  this leads to unreadable and error prone code. The framebuffer
  code should be based around struct fb_info.
- Please keep the line lengths within sensible limits.
- Get rid of CONFIG_VIDEO_DELAY_INIT and make the mode runtime
  changeable. All this requires is a
  host->fb_disable(info); host->fb_mode(info, newmode); host->fb_enable(mode);

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Eric Bénard]

Hi Jürgen,

Le 01/11/2010 14:19, Sascha Hauer a écrit :
> General remarks about this series:
>
> - Please do not add code with '#if 0' and activate it later. This shows
>    the series has the wrong order.
> - Please refrain from basing your internal functions around 'struct
>    device_d'. By doing so we completey lose type safety and at least in
>    case of the mci framework where three different devices are involved
>    this leads to unreadable and error prone code. The framebuffer
>    code should be based around struct fb_info.
> - Please keep the line lengths within sensible limits.
> - Get rid of CONFIG_VIDEO_DELAY_INIT and make the mode runtime
>    changeable. All this requires is a
>    host->fb_disable(info); host->fb_mode(info, newmode); host->fb_enable(mode);
>
and pcr in imxfb_mode which describes polarity of signals and thus can change 
from a screen to another.

Eric

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/12] Bring in dynamic videomode selection at runtime

[Sascha Hauer]

On Tue, Oct 26, 2010 at 01:31:39PM +0200, Juergen Beisert wrote:
> This patch mostly rewrites all parts of /drivers/video/fb.c. As it changes
> the API to the drivers, it must be done in one step to keep the repository
> bisectable. But to do it in one step makes the patches itself unreadable.
> 
> So, I decided to do it in a few steps, only for the review. All patches marked
> with a "step n" should be merged, prior the final commit onto the repository.
> 
> This step brings in the required function for dynamic videomode selection at
> runtime but keep the old functions untouched (the new one are commented out).
> 
> This is patch 1 of 7 to keep the repository bisectable.
> 
> Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> ---
>  drivers/video/Kconfig |   13 +++
>  drivers/video/fb.c    |  243 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fb.h          |   40 ++++++++
>  3 files changed, 296 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> --- a/drivers/video/fb.c
> +++ b/drivers/video/fb.c
> @@ -1,3 +1,4 @@
> +#include <init.h>
>  #include <common.h>
>  #include <fb.h>
>  #include <errno.h>
> @@ -5,6 +6,8 @@
>  #include <getopt.h>
>  #include <fcntl.h>
>  #include <fs.h>
> +#include <malloc.h>
> +#include <xfuncs.h>
>  
>  static int fb_ioctl(struct cdev* cdev, int req, void *data)
>  {
> @@ -57,6 +60,139 @@ static int fb_enable_set(struct device_d *dev, struct param_d *param,
>  	return 0;
>  }
>  
> +#if 0
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +static int fb_check_if_already_initialized(struct device_d *fb_dev)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +
> +	if (info->active_mode != NULL) {
> +		pr_err("Video mode '%s' is already set. Cannot change colour depth anymore.\n", info->active_mode->name);

We have dev_err, also this line is really long.

> +
> +	if (enable) {
> +		(host->fb_enable)(info);

Please do not add these unnecessary braces around function pointers.

> +		new = "1";
> +	} else {
> +		(host->fb_disable)(info);
> +		new = "0";
> +	}
> +
> +	info->enabled = !!enable;
> +
> +	return dev_param_set_generic(fb_dev, param, new);
> +}
> +
> +static void fb_list_modes(struct fb_host *host)
> +{
> +	unsigned u;
> +
> +	if (host->mode_cnt == 0)
> +		return;

This is not needed.

> +
> +	printf(" Supported video mode(s):\n");
> +	for (u = 0; u < host->mode_cnt; u++)
> +		printf("  '%s'\n", host->mode[u].name);
> +}
> +
> +static int fb_activate_mode(struct device_d *fb_dev, const struct fb_videomode *mode)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	struct fb_host *host = fb_dev->platform_data;
> +	int rc;
> +
> +	rc = (host->fb_mode)(info, mode);
> +	if (rc != 0)
> +		return rc;
> +
> +	info->active_mode = mode;
> +	/*
> +	 * At this point of time we know the remaining information we need
> +	 * for the cdev and fb_info structure.
> +	 */
> +	cdev->size = fb_dev->size;
> +	info->xres = mode->xres;
> +	info->yres = mode->yres;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +static int fb_mode_set(struct device_d *fb_dev, struct param_d *param, const char *name)
> +{
> +	struct fb_host *host = fb_dev->platform_data;
> +	unsigned u;
> +	int rc;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	rc = fb_check_if_already_initialized(fb_dev);
> +	if (rc != 0)
> +		return rc;
> +
> +	/* Search for the requested video mode by name */
> +	for (u = 0; u < host->mode_cnt; u++) {
> +		if (!strcmp(host->mode[u].name, name))
> +			break;
> +	}
> +	if (u >= host->mode_cnt) {
> +		fb_list_modes(host);	/* no mode with 'name' found */
> +		return -ENODEV;
> +	} else {
> +		rc = fb_activate_mode(fb_dev, &host->mode[u]);
> +	}
> +
> +	if (rc == 0)
> +		dev_param_set_generic(fb_dev, param, name);
> +
> +	return rc;
> +}
> +#endif
> +#endif
> +
>  static struct file_operations fb_ops = {
>  	.read	= mem_read,
>  	.write	= mem_write,
> @@ -65,6 +201,113 @@ static struct file_operations fb_ops = {
>  	.ioctl	= fb_ioctl,
>  };
>  
> +#if 0
> +static int add_fb_parameter(struct device_d *fb_dev)
> +{
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	char cd[10];
> +
> +	/** @todo provide base address parameter for the user */
> +
> +	dev_add_param(fb_dev, "cdepth", fb_cdepth_set, NULL, 0);
> +	if (info->bits_per_pixel == 0) {
> +		dev_set_param(fb_dev, "cdepth", "16");
> +		info->bits_per_pixel = 16;
> +	} else {
> +		sprintf(cd, "%u", info->bits_per_pixel);
> +		dev_set_param(fb_dev, "cdepth", cd);
> +	}
> +	dev_add_param(fb_dev, "mode", fb_mode_set, NULL, 0);
> +	/* default is 'none' for delayed video mode setup */
> +#endif
> +	dev_add_param(fb_dev, "enable", fb_enable_set, NULL, 0);
> +	/* default is 'off' for delayed video output */
> +	return 0;
> +}
> +
> +struct framebuffer_desc {
> +	struct cdev cdev;
> +	struct fb_info info;
> +};
> +
> +static int fb_probe(struct device_d *fb_dev)
> +{
> +	int id = get_free_deviceid("fb");
> +	struct cdev *cdev;
> +	struct fb_info *info;
> +	struct fb_host *host = fb_dev->platform_data;
> +
> +	cdev = xzalloc(sizeof(struct framebuffer_desc));
> +	info = (struct fb_info*)&cdev[1];

Why this? struct fb_info contains a struct cdev which is probably
removed in a later patch in this series. The current implementation
is clearly easier to read than this &cdev[1] construct.

> +
> +	fb_dev->priv = cdev;	/* pointer forward */
> +	cdev->dev = fb_dev;	/* pointer backward */
> +
> +	cdev->ops = &fb_ops;
> +	cdev->name = asprintf("fb%d", id);
> +
> +	cdev->size = fb_dev->size;	/* use the default if any */
> +	cdev->priv = info;
> +
> +	info->host = host;
> +	info->fb_dev = fb_dev;
> +
> +	/* setup defaults */
> +	if (host->bits_per_pixel != 0)
> +		info->bits_per_pixel = host->bits_per_pixel;
> +	else
> +		info->bits_per_pixel = 16;	/* means RGB565 */

No, this does not mean RGB565. It could also mean BGR or even something
else.

> +
> +	/* add params on demand */

There is no information in this comment.

> +	add_fb_parameter(fb_dev);
> +
> +	devfs_create(cdev);
> +#ifndef CONFIG_VIDEO_DELAY_INIT
> +	/* initialize video mode immediately (the first one) */
> +	fb_activate_mode(fb_dev, &host->mode[0]);
> +#endif
> +	return 0;
> +}
> +
> +static struct driver_d fb_driver = {
> +	.name	= "framebuffer",
> +	.probe	= fb_probe,
> +};
> +
> +static int framebuffer_init(void)
> +{
> +	return register_driver(&fb_driver);
> +}
> +
> +device_initcall(framebuffer_init);
> +
> +struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned size)
> +{
> +	struct device_d *fb_dev;
> +	int rc;
> +
> +	fb_dev = xzalloc(sizeof(struct device_d));
> +
> +	strcpy(fb_dev->name, fb_driver.name);
> +	fb_dev->platform_data = (void*)host;
> +
> +	/* setup the defaults for this framebuffer if given */
> +	fb_dev->size = size;
> +	fb_dev->map_base = (unsigned long)base;
> +
> +	rc = register_device(fb_dev);
> +	if (rc != 0) {
> +		pr_debug("Cannot register framebuffer device\n");
> +		free(fb_dev);
> +		return NULL;
> +	}
> +
> +	return fb_dev;
> +}
> +#endif
> +
>  int register_framebuffer(struct fb_info *info)
>  {
>  	int id = get_free_deviceid("fb");
> diff --git a/include/fb.h b/include/fb.h
> index 218b244..96edc24 100644
> --- a/include/fb.h
> +++ b/include/fb.h
> @@ -85,6 +85,46 @@ struct fb_ops {
>  	void (*fb_disable)(struct fb_info *info);
>  };
>  
> +#if 0
> +struct fb_host {
> +	const struct fb_videomode *mode;
> +	unsigned mode_cnt;
> +
> +	struct device_d *hw_dev;
> +
> +	/* callbacks into the video hardware driver */
> +	int (*fb_setcolreg)(struct fb_info*, unsigned, unsigned, unsigned, unsigned, unsigned);
> +	int (*fb_mode)(struct fb_info*, const struct fb_videomode*);
> +	void (*fb_enable)(struct fb_info*);
> +	void (*fb_disable)(struct fb_info*);
> +
> +	unsigned bits_per_pixel;
> +};
> +
> +struct fb_info {
> +	struct fb_host *host;
> +	struct device_d *fb_dev;
> +	const struct fb_videomode *active_mode;
> +
> +	u32 xres;			/* visible resolution		*/
> +	u32 yres;
> +	u32 bits_per_pixel;		/* guess what			*/
> +
> +	u32 grayscale;			/* != 0 Graylevels instead of colors */
> +
> +	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
> +	struct fb_bitfield green;	/* else only length is significant */
> +	struct fb_bitfield blue;
> +	struct fb_bitfield transp;	/* transparency			*/

If you go the way of duplicating the code and removing the old code
afterwards I would assume that you add the code in the right way and
start to add doxygen from the beginning instead of fixing it later.

> +
> +#ifdef CONFIG_VIDEO_DELAY_ENABLING
> +	int enabled;
> +#endif
> +};

This is unused and removed in a later patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/12] Bring in dynamic videomode selection at runtime

[Sascha Hauer]

On Tue, Oct 26, 2010 at 01:31:39PM +0200, Juergen Beisert wrote:
> This patch mostly rewrites all parts of /drivers/video/fb.c. As it changes
> the API to the drivers, it must be done in one step to keep the repository
> bisectable. But to do it in one step makes the patches itself unreadable.
> 
> So, I decided to do it in a few steps, only for the review. All patches marked
> with a "step n" should be merged, prior the final commit onto the repository.
> 
> This step brings in the required function for dynamic videomode selection at
> runtime but keep the old functions untouched (the new one are commented out).
> 
> This is patch 1 of 7 to keep the repository bisectable.
> 
> Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> ---
>  drivers/video/Kconfig |   13 +++
>  drivers/video/fb.c    |  243 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fb.h          |   40 ++++++++
>  3 files changed, 296 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 7a89a3f..8138226 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -5,6 +5,19 @@ menuconfig VIDEO
>  
>  if VIDEO
>  
> +comment "runtime options"
> +
> +config VIDEO_DELAY_INIT
> +	bool "Delayed initialization"
> +	help
> +	  If the platform supports more than one video mode say 'y' her to delay
> +	  the initialization of the video device until any kind of barebox's
> +          shell code sets up the correct mode at runtime.
> +          This entry adds the "mode" parameter to the video device, to setup
> +          the desired videomode prior enabling it at runtime.
> +
> +comment "drivers"
> +
>  config DRIVER_VIDEO_IMX
>  	bool "i.MX framebuffer driver"
>  	depends on ARCH_IMX1 || ARCH_IMX21 || ARCH_IMX25 || ARCH_IMX27
> diff --git a/drivers/video/fb.c b/drivers/video/fb.c
> index f9a425e..c3c4761 100644
> --- a/drivers/video/fb.c
> +++ b/drivers/video/fb.c
> @@ -1,3 +1,4 @@
> +#include <init.h>
>  #include <common.h>
>  #include <fb.h>
>  #include <errno.h>
> @@ -5,6 +6,8 @@
>  #include <getopt.h>
>  #include <fcntl.h>
>  #include <fs.h>
> +#include <malloc.h>
> +#include <xfuncs.h>
>  
>  static int fb_ioctl(struct cdev* cdev, int req, void *data)
>  {
> @@ -57,6 +60,139 @@ static int fb_enable_set(struct device_d *dev, struct param_d *param,
>  	return 0;
>  }
>  
> +#if 0
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +static int fb_check_if_already_initialized(struct device_d *fb_dev)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +
> +	if (info->active_mode != NULL) {
> +		pr_err("Video mode '%s' is already set. Cannot change colour depth anymore.\n", info->active_mode->name);
> +		return -EPERM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fb_cdepth_set(struct device_d *fb_dev, struct param_d *param, const char *val)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	unsigned cdepth;
> +	int rc;
> +
> +	rc = fb_check_if_already_initialized(fb_dev);
> +	if (rc != 0)
> +		return rc;
> +
> +	cdepth = simple_strtoul(val, NULL, 0);
> +	if (cdepth != 0)
> +		info->bits_per_pixel = cdepth;
> +	else
> +		return -EINVAL;
> +
> +	return dev_param_set_generic(fb_dev, param, val);
> +}
> +#endif
> +
> +static int fb_enable_set(struct device_d *fb_dev, struct param_d *param, const char *val)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	struct fb_host *host = fb_dev->platform_data;
> +	int enable;
> +	char *new;
> +
> +	if (!val)
> +		return dev_param_set_generic(fb_dev, param, NULL);
> +
> +	enable = simple_strtoul(val, NULL, 0);
> +
> +	if (info->enabled == !!enable)
> +		return 0;
> +
> +	if (enable) {
> +		(host->fb_enable)(info);
> +		new = "1";
> +	} else {
> +		(host->fb_disable)(info);
> +		new = "0";
> +	}
> +
> +	info->enabled = !!enable;
> +
> +	return dev_param_set_generic(fb_dev, param, new);
> +}
> +
> +static void fb_list_modes(struct fb_host *host)
> +{
> +	unsigned u;
> +
> +	if (host->mode_cnt == 0)
> +		return;
> +
> +	printf(" Supported video mode(s):\n");
> +	for (u = 0; u < host->mode_cnt; u++)
> +		printf("  '%s'\n", host->mode[u].name);
> +}
> +
> +static int fb_activate_mode(struct device_d *fb_dev, const struct fb_videomode *mode)
> +{
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	struct fb_host *host = fb_dev->platform_data;
> +	int rc;
> +
> +	rc = (host->fb_mode)(info, mode);
> +	if (rc != 0)
> +		return rc;
> +
> +	info->active_mode = mode;
> +	/*
> +	 * At this point of time we know the remaining information we need
> +	 * for the cdev and fb_info structure.
> +	 */
> +	cdev->size = fb_dev->size;
> +	info->xres = mode->xres;
> +	info->yres = mode->yres;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +static int fb_mode_set(struct device_d *fb_dev, struct param_d *param, const char *name)
> +{
> +	struct fb_host *host = fb_dev->platform_data;
> +	unsigned u;
> +	int rc;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	rc = fb_check_if_already_initialized(fb_dev);
> +	if (rc != 0)
> +		return rc;
> +
> +	/* Search for the requested video mode by name */
> +	for (u = 0; u < host->mode_cnt; u++) {
> +		if (!strcmp(host->mode[u].name, name))
> +			break;
> +	}
> +	if (u >= host->mode_cnt) {
> +		fb_list_modes(host);	/* no mode with 'name' found */
> +		return -ENODEV;
> +	} else {
> +		rc = fb_activate_mode(fb_dev, &host->mode[u]);
> +	}
> +
> +	if (rc == 0)
> +		dev_param_set_generic(fb_dev, param, name);
> +
> +	return rc;
> +}
> +#endif
> +#endif
> +
>  static struct file_operations fb_ops = {
>  	.read	= mem_read,
>  	.write	= mem_write,
> @@ -65,6 +201,113 @@ static struct file_operations fb_ops = {
>  	.ioctl	= fb_ioctl,
>  };
>  
> +#if 0
> +static int add_fb_parameter(struct device_d *fb_dev)
> +{
> +#ifdef CONFIG_VIDEO_DELAY_INIT
> +	struct cdev *cdev = fb_dev->priv;
> +	struct fb_info *info = cdev->priv;
> +	char cd[10];
> +
> +	/** @todo provide base address parameter for the user */
> +
> +	dev_add_param(fb_dev, "cdepth", fb_cdepth_set, NULL, 0);
> +	if (info->bits_per_pixel == 0) {
> +		dev_set_param(fb_dev, "cdepth", "16");
> +		info->bits_per_pixel = 16;
> +	} else {
> +		sprintf(cd, "%u", info->bits_per_pixel);
> +		dev_set_param(fb_dev, "cdepth", cd);
> +	}
> +	dev_add_param(fb_dev, "mode", fb_mode_set, NULL, 0);
> +	/* default is 'none' for delayed video mode setup */
> +#endif
> +	dev_add_param(fb_dev, "enable", fb_enable_set, NULL, 0);
> +	/* default is 'off' for delayed video output */
> +	return 0;
> +}
> +
> +struct framebuffer_desc {
> +	struct cdev cdev;
> +	struct fb_info info;
> +};
> +
> +static int fb_probe(struct device_d *fb_dev)
> +{
> +	int id = get_free_deviceid("fb");
> +	struct cdev *cdev;
> +	struct fb_info *info;
> +	struct fb_host *host = fb_dev->platform_data;
> +
> +	cdev = xzalloc(sizeof(struct framebuffer_desc));
> +	info = (struct fb_info*)&cdev[1];
> +
> +	fb_dev->priv = cdev;	/* pointer forward */
> +	cdev->dev = fb_dev;	/* pointer backward */
> +
> +	cdev->ops = &fb_ops;
> +	cdev->name = asprintf("fb%d", id);
> +
> +	cdev->size = fb_dev->size;	/* use the default if any */
> +	cdev->priv = info;
> +
> +	info->host = host;
> +	info->fb_dev = fb_dev;
> +
> +	/* setup defaults */
> +	if (host->bits_per_pixel != 0)
> +		info->bits_per_pixel = host->bits_per_pixel;
> +	else
> +		info->bits_per_pixel = 16;	/* means RGB565 */
> +
> +	/* add params on demand */
> +	add_fb_parameter(fb_dev);
> +
> +	devfs_create(cdev);
> +#ifndef CONFIG_VIDEO_DELAY_INIT
> +	/* initialize video mode immediately (the first one) */
> +	fb_activate_mode(fb_dev, &host->mode[0]);
> +#endif
> +	return 0;
> +}
> +
> +static struct driver_d fb_driver = {
> +	.name	= "framebuffer",
> +	.probe	= fb_probe,
> +};
> +
> +static int framebuffer_init(void)
> +{
> +	return register_driver(&fb_driver);
> +}
> +
> +device_initcall(framebuffer_init);
> +
> +struct device_d *register_framebuffer(struct fb_host *host, void *base, unsigned size)
> +{

Why are base and size passed to register_framebuffer? The framebuffer
core should not be interested in this at this point.

> +	struct device_d *fb_dev;
> +	int rc;
> +
> +	fb_dev = xzalloc(sizeof(struct device_d));

Why is the device not a member of fb_info (or now fb_host) anymore like
it used to be? There was no malloc necessary for that.

> +
> +	strcpy(fb_dev->name, fb_driver.name);
> +	fb_dev->platform_data = (void*)host;

unnecessary cast.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Sascha Hauer]

Hi Eric,

On Mon, Nov 01, 2010 at 02:29:21PM +0100, Eric Bénard wrote:
> Hi Jürgen,
>
> Le 01/11/2010 14:19, Sascha Hauer a écrit :
>> General remarks about this series:
>>
>> - Please do not add code with '#if 0' and activate it later. This shows
>>    the series has the wrong order.
>> - Please refrain from basing your internal functions around 'struct
>>    device_d'. By doing so we completey lose type safety and at least in
>>    case of the mci framework where three different devices are involved
>>    this leads to unreadable and error prone code. The framebuffer
>>    code should be based around struct fb_info.
>> - Please keep the line lengths within sensible limits.
>> - Get rid of CONFIG_VIDEO_DELAY_INIT and make the mode runtime
>>    changeable. All this requires is a
>>    host->fb_disable(info); host->fb_mode(info, newmode); host->fb_enable(mode);
>>
> and pcr in imxfb_mode which describes polarity of signals and thus can 
> change from a screen to another.

This is handled in a later patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 08/12] Add a video driver for S3C2440 bases platforms

[Sascha Hauer]

On Tue, Oct 26, 2010 at 01:31:44PM +0200, Juergen Beisert wrote:
> From: Juergen Beisert <juergen@kreuzholzen.de>
> 
> Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>
> ---
>  arch/arm/mach-s3c24xx/include/mach/fb.h |   40 +++
>  drivers/video/Kconfig                   |    6 +
>  drivers/video/Makefile                  |    1 +
>  drivers/video/s3c.c                     |  452 +++++++++++++++++++++++++++++++
>  4 files changed, 499 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-s3c24xx/include/mach/fb.h
>  create mode 100644 drivers/video/s3c.c
> 
> diff --git a/arch/arm/mach-s3c24xx/include/mach/fb.h b/arch/arm/mach-s3c24xx/include/mach/fb.h
> new file mode 100644
> index 0000000..eec6164
> --- /dev/null
> +++ b/arch/arm/mach-s3c24xx/include/mach/fb.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2010 Juergen Beisert
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __MACH_FB_H_
> +# define __MACH_FB_H_
> +
> +#include <fb.h>
> +
> +struct s3c_fb_platform_data {
> +
> +	const struct fb_videomode *mode_list;
> +	unsigned mode_cnt;
> +
> +	int passive_display;	/**< enable support for STN or CSTN displays */
> +
> +	void *framebuffer;	/**< force framebuffer base address */
> +	unsigned size;		/**< force framebuffer size */
> +
> +	/** hook to enable backlight and stuff */
> +	void (*enable)(int);
> +};
> +
> +#endif /* __MACH_FB_H_ */
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index d7f3d01..5a5edd2 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -39,4 +39,10 @@ config DRIVER_VIDEO_IMX_IPU
>  	  Add support for the IPU framebuffer device found on
>  	  i.MX31 and i.MX35 CPUs.
>  
> +config S3C_VIDEO
> +	bool "S3C244x framebuffer driver"
> +	depends on ARCH_S3C24xx
> +	help
> +	  Add support for the S3C244x LCD controller.
> +
>  endif
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 179f0c4..4287fc8 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_VIDEO) += fb.o
>  
>  obj-$(CONFIG_DRIVER_VIDEO_IMX) += imx.o
>  obj-$(CONFIG_DRIVER_VIDEO_IMX_IPU) += imx-ipu-fb.o
> +obj-$(CONFIG_S3C_VIDEO) += s3c.o
> diff --git a/drivers/video/s3c.c b/drivers/video/s3c.c
> new file mode 100644
> index 0000000..8ae5785
> --- /dev/null
> +++ b/drivers/video/s3c.c
> @@ -0,0 +1,452 @@
> +/*
> + * Copyright (C) 2010 Juergen Beisert
> + *
> + * This driver is based on a patch found in the web:
> + * (C) Copyright 2006 by OpenMoko, Inc.
> + * Author: Harald Welte <laforge at openmoko.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + *
> + */
> +
> +/* #define DEBUG */
> +
> +#include <common.h>
> +#include <init.h>
> +#include <fb.h>
> +#include <driver.h>
> +#include <malloc.h>
> +#include <errno.h>
> +#include <asm/io.h>
> +#include <mach/gpio.h>
> +#include <mach/s3c24xx-generic.h>
> +#include <mach/s3c24x0-clocks.h>
> +#include <mach/fb.h>
> +
> +#define LCDCON1 0x00
> +# define PNRMODE(x) (((x) & 3) << 5)
> +# define BPPMODE(x) (((x) & 0xf) << 1)
> +# define SET_CLKVAL(x) (((x) & 0x3ff) << 8)
> +# define GET_CLKVAL(x) (((x) >> 8) & 0x3ff)
> +# define ENVID (1 << 0)
> +
> +#define LCDCON2 0x04
> +# define SET_VBPD(x) (((x) & 0xff) << 24)
> +# define SET_LINEVAL(x) (((x) & 0x3ff) << 14)
> +# define SET_VFPD(x) (((x) & 0xff) << 6)
> +# define SET_VSPW(x) ((x) & 0x3f)
> +
> +#define LCDCON3 0x08
> +# define SET_HBPD(x) (((x) & 0x7f) << 19)
> +# define SET_HOZVAL(x) (((x) & 0x7ff) << 8)
> +# define SET_HFPD(x) ((x) & 0xff)
> +
> +#define LCDCON4 0x0c
> +# define SET_HSPW(x) ((x) & 0xff)
> +
> +#define LCDCON5 0x10
> +# define BPP24BL (1 << 12)
> +# define FRM565 (1 << 11)
> +# define INV_CLK (1 << 10)
> +# define INV_HS (1 << 9)
> +# define INV_VS (1 << 8)
> +# define INV_DTA (1 << 7)
> +# define INV_DE (1 << 6)
> +# define INV_PWREN (1 << 5)
> +# define INV_LEND (1 << 4)	/* FIXME */
> +# define ENA_PWREN (1 << 3)
> +# define ENA_LEND (1 << 2)	/* FIXME */
> +# define BSWP (1 << 1)
> +# define HWSWP (1 << 0)
> +
> +#define LCDSADDR1 0x14
> +# define SET_LCDBANK(x) (((x) & 0x1ff) << 21)
> +# define GET_LCDBANK(x) (((x) >> 21) & 0x1ff)
> +# define SET_LCDBASEU(x) ((x) & 0x1fffff)
> +# define GET_LCDBASEU(x) ((x) & 0x1fffff)
> +
> +#define LCDSADDR2 0x18
> +# define SET_LCDBASEL(x) ((x) & 0x1fffff)
> +# define GET_LCDBASEL(x) ((x) & 0x1fffff)
> +
> +#define LCDSADDR3 0x1c
> +# define SET_OFFSIZE(x) (((x) & 0x7ff) << 11)
> +# define GET_OFFSIZE(x) (((x) >> 11) & 0x7ff)
> +# define SET_PAGE_WIDTH(x) ((x) & 0x3ff)
> +# define GET_PAGE_WIDTH(x) ((x) & 0x3ff)
> +
> +#define RED_LUT 0x20
> +#define GREEN_LUT 0x24
> +#define BLUE_LUT 0x28
> +
> +#define DITHMODE 0x4c
> +
> +#define TPAL 0x50
> +
> +#define LCDINTPND 0x54
> +
> +#define LCDSRCPND 0x58
> +
> +#define LCDINTMSK 0x5c
> +# define FIWSEL (1 << 2)
> +
> +#define TCONSEL 0x60
> +
> +#define RED 0
> +#define GREEN 1
> +#define BLUE 2
> +#define TRANSP 3
> +
> +struct s3cfb_host {
> +	struct fb_host fb_data;
> +	struct device_d *hw_dev;
> +	void __iomem *base;
> +};
> +
> +#define fb_info_to_s3cfb_host(x) ((struct s3cfb_host*)((x)->host))

> +#define s3cfb_host_to_s3c_fb_platform_data(x) ((struct s3c_fb_platform_data*)((x)->hw_dev->platform_data))

Please add a pointer to 3c_fb_platform_data to s3cfb_host and get rid of
this define.

> +
> +/* the RGB565 true colour mode */
> +static const struct fb_bitfield def_rgb565[] = {
> +	[RED] = {
> +		.offset = 11,
> +		.length = 5,
> +	},
> +	[GREEN] = {
> +		.offset = 5,
> +		.length = 6,
> +	},
> +	[BLUE] = {
> +		.offset = 0,
> +		.length = 5,
> +	},
> +	[TRANSP] = {	/* no support for transparency */
> +		.length = 0,
> +	}
> +};
> +
> +/* the RGB888 true colour mode */
> +static const struct fb_bitfield def_rgb888[] = {
> +	[RED] = {
> +		.offset = 16,
> +		.length = 8,
> +	},
> +	[GREEN] = {
> +		.offset = 8,
> +		.length = 8,
> +	},
> +	[BLUE] = {
> +		.offset = 0,
> +		.length = 8,
> +	},
> +	[TRANSP] = {	/* no support for transparency */
> +		.length = 0,
> +	}
> +};
> +
> +/**
> + * @param fb_info Framebuffer information
> + */
> +static void s3cfb_enable_controller(struct fb_info *fb_info)
> +{
> +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> +	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
> +	uint32_t con1;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	con1 = readl(fbh->base + LCDCON1);
> +
> +	con1 |= ENVID;
> +
> +	writel(con1, fbh->base + LCDCON1);
> +
> +	if (pdata->enable)
> +		(pdata->enable)(1);
> +}
> +
> +/**
> + * @param fb_info Framebuffer information
> + */
> +static void s3cfb_disable_controller(struct fb_info *fb_info)
> +{
> +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> +	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
> +	uint32_t con1;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	if (pdata->enable)
> +		(pdata->enable)(0);
> +
> +	con1 = readl(fbh->base + LCDCON1);
> +
> +	con1 &= ~ENVID;
> +
> +	writel(con1, fbh->base + LCDCON1);
> +}
> +
> +/**
> + * Prepare the video hardware for a specified video mode
> + * @param fb_info Framebuffer information
> + * @param mode The video mode description to initialize
> + * @return 0 on success
> + */
> +static int s3cfb_initialize_mode(struct fb_info *fb_info, const struct fb_videomode *mode)
> +{
> +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> +	struct s3c_fb_platform_data *pdata = s3cfb_host_to_s3c_fb_platform_data(fbh);
> +	unsigned size, hclk, div;
> +	uint32_t con1, con2, con3, con4, con5 = 0;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	if (pdata->passive_display != 0) {
> +		pr_err("Passive displays are currently not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * we need at least this amount of memory for the framebuffer
> +	 */
> +	size = mode->xres * mode->yres * (fb_info->bits_per_pixel >> 3);
> +	if (fb_info->fb_dev->size != 0) {
> +		if (size > fb_info->fb_dev->size) {
> +			pr_err("Cannot initialize video mode '%s': Its too large. "
> +				"Required bytes are %u, available only %u\n",
> +				mode->name, size, fb_info->fb_dev->size);
> +			return -EINVAL;
> +		}
> +	} else
> +		fb_info->fb_dev->size = size;
> +
> +	/*
> +	 * if no framebuffer memory was specified yet, allocate one,
> +	 * and allocate more memory, on user request
> +	 */
> +	if (fb_info->fb_dev->map_base == 0U)
> +		fb_info->fb_dev->map_base = (resource_size_t)xzalloc(fb_info->fb_dev->size);

Honestly I don't understand what the two blocks above do. I hope this
gets simpler once we remove the base/size arguments from
register_framebuffer.

> +
> +	/* its useful to enable the unit's clock */
> +	s3c244x_mod_clock(CLK_LCDC, 1);
> +
> +	/* ensure video output is _off_ */
> +	writel(0x00000000, fbh->base + LCDCON1);
> +
> +	hclk = s3c24xx_get_hclk() / 1000U;	/* hclk in kHz */
> +	div = hclk / PICOS2KHZ(mode->pixclock);
> +	if (div < 3)
> +		div  = 3;
> +	/* pixel clock is: (hclk) / ((div + 1) * 2) */
> +	div += 1;
> +	div >>= 1;
> +	div -= 1;
> +
> +	con1 = PNRMODE(3) | SET_CLKVAL(div);	/* PNRMODE=3 is TFT */
> +
> +	switch (fb_info->bits_per_pixel) {
> +#if 0
> +	/* TODO add CLUT based modes */
> +	case 1:
> +		con1 |= BPPMODE(8);
> +		break;
> +	case 2:
> +		con1 |= BPPMODE(9);
> +		break;
> +	case 4:
> +		con1 |= BPPMODE(10);
> +		break;
> +	case 8:
> +		con1 |= BPPMODE(11);
> +		break;
> +#endif
> +	case 16:
> +		con1 |= BPPMODE(12);
> +		con5 |= FRM565;
> +		fb_info->red = def_rgb565[RED];
> +		fb_info->green = def_rgb565[GREEN];
> +		fb_info->blue = def_rgb565[BLUE];
> +		fb_info->transp =  def_rgb565[TRANSP];
> +		break;
> +	case 24:
> +		con1 |= BPPMODE(13);
> +		con5 |= BPP24BL;	/* FIXME */

Please either remove the FIXME or explain what we have to fix here.

> +		fb_info->red = def_rgb888[RED];
> +		fb_info->green = def_rgb888[GREEN];
> +		fb_info->blue = def_rgb888[BLUE];
> +		fb_info->transp =  def_rgb888[TRANSP];
> +		break;
> +	default:
> +		pr_err("Invalid bits per pixel value: %u\n", fb_info->bits_per_pixel);
> +		return -EINVAL;
> +	}
> +
> +	/* 'normal' in register description means positive logic */
> +	if (!(mode->sync & FB_SYNC_HOR_HIGH_ACT))
> +		con5 |= INV_HS;
> +	if (!(mode->sync & FB_SYNC_VERT_HIGH_ACT))
> +		con5 |= INV_VS;
> +	if (!(mode->sync & FB_SYNC_DE_HIGH_ACT))
> +		con5 |= INV_DE;
> +	if (mode->sync & FB_SYNC_CLK_INVERT)
> +		con5 |= INV_CLK;	/* display should latch at the rising edge */
> +	if (mode->sync & FB_SYNC_SWAP_RGB)
> +		con5 |= HWSWP;
> +
> +	/* TODO power enable config via platform data */
> +
> +	/* vertical timing */
> +	con2 = SET_VBPD(mode->upper_margin - 1) |
> +		SET_LINEVAL(mode->yres - 1) |
> +		SET_VFPD(mode->lower_margin - 1) |
> +		SET_VSPW(mode->vsync_len - 1);
> +
> +	/* horizontal timing */
> +	con3 = SET_HBPD(mode->left_margin - 1) |
> +		SET_HOZVAL(mode->xres - 1) |
> +		SET_HFPD(mode->right_margin - 1);
> +	con4 = SET_HSPW(mode->hsync_len - 1);
> +
> +	/* basic timing setup */
> +	writel(con1, fbh->base + LCDCON1);
> +	pr_debug(" Writing %08X into %08X (con1)\n", con1, fbh->base + LCDCON1);
> +	writel(con2, fbh->base + LCDCON2);
> +	pr_debug(" Writing %08X into %08X (con2)\n", con2, fbh->base + LCDCON2);
> +	writel(con3, fbh->base + LCDCON3);
> +	pr_debug(" Writing %08X into %08X (con3)\n", con3, fbh->base + LCDCON3);
> +	writel(con4, fbh->base + LCDCON4);
> +	pr_debug(" Writing %08X into %08X (con4)\n", con4, fbh->base + LCDCON4);
> +	writel(con5, fbh->base + LCDCON5);
> +	pr_debug(" Writing %08X into %08X (con5)\n", con5, fbh->base + LCDCON5);
> +
> +	pr_debug("Setting up the fb baseadress to %08X\n", fb_info->fb_dev->map_base);
> +
> +	/* framebuffer memory setup */
> +	writel(fb_info->fb_dev->map_base >> 1, fbh->base + LCDSADDR1);
> +	size = mode->xres * (fb_info->bits_per_pixel >> 3) * (mode->yres);

You already calculated this value.

> +	writel(SET_LCDBASEL((fb_info->fb_dev->map_base + size) >> 1), fbh->base + LCDSADDR2);
> +
> +	writel(SET_OFFSIZE(0) |
> +		SET_PAGE_WIDTH((mode->xres * fb_info->bits_per_pixel) >> 4),
> +		fbh->base + LCDSADDR3);
> +
> +	writel(FIWSEL, fbh->base + LCDINTMSK);
> +
> +	return 0;
> +}
> +
> +/**
> + * Print some information about the current hardware state
> + * @param hw_dev S3C video device
> + */
> +#ifdef CONFIG_VIDEO_INFO_VERBOSE
> +static void s3cfb_info(struct device_d *hw_dev)
> +{
> +	uint32_t con1, addr1, addr2, addr3;
> +
> +	con1 = readl(hw_dev->map_base + LCDCON1);
> +	addr1 = readl(hw_dev->map_base + LCDSADDR1);
> +	addr2 = readl(hw_dev->map_base + LCDSADDR2);
> +	addr3 = readl(hw_dev->map_base + LCDSADDR3);
> +
> +	printf(" Video hardware info:\n");
> +	printf("  Video clock is running at %u Hz\n", s3c24xx_get_hclk() / ((GET_CLKVAL(con1) + 1) * 2));
> +	printf("  Video memory bank starts at 0x%08X\n", GET_LCDBANK(addr1) << 22);
> +	printf("  Video memory bank offset: 0x%08X\n", GET_LCDBASEU(addr1));
> +	printf("  Video memory end: 0x%08X\n", GET_LCDBASEU(addr2));
> +	printf("  Virtual screen offset size: %u half words\n", GET_OFFSIZE(addr3));
> +	printf("  Virtual screen page width: %u half words\n", GET_PAGE_WIDTH(addr3));
> +}
> +#endif
> +
> +/*
> + * There is only one video hardware instance available.
> + * It makes no sense to dynamically allocate this data
> + */
> +static struct s3cfb_host host_data = {
> +	.fb_data.fb_mode = s3cfb_initialize_mode,
> +	.fb_data.fb_enable = s3cfb_enable_controller,
> +	.fb_data.fb_disable = s3cfb_disable_controller,
> +};
> +
> +static int s3cfb_probe(struct device_d *hw_dev)
> +{
> +	struct s3c_fb_platform_data *pdata = hw_dev->platform_data;
> +	struct device_d *fb_dev;
> +
> +	pr_debug("%s called\n", __func__);
> +
> +	if (pdata == NULL) {
> +		pr_debug("Framebuffer driver missing platform data");
> +		return -ENODEV;
> +	}
> +
> +	/* enable unit's clock */
> +	s3c244x_mod_clock(CLK_LCDC, 1);
> +
> +	writel(0, hw_dev->map_base + LCDCON1);
> +	/* off may be means high level (FIXME itlp specific) */
> +	writel(0, hw_dev->map_base + LCDCON5);
> +
> +	/* disable it again until user request */
> +	s3c244x_mod_clock(CLK_LCDC, 0);
> +
> +	/* add runtime hardware info */
> +	host_data.hw_dev = hw_dev;
> +	host_data.base = (void*)hw_dev->map_base;
> +
> +	/* add runtime video info */
> +	host_data.fb_data.mode = pdata->mode_list;
> +	host_data.fb_data.mode_cnt = pdata->mode_cnt;
> +
> +	fb_dev = register_framebuffer(&host_data.fb_data, pdata->framebuffer, pdata->size);
> +	if (fb_dev == NULL) {
> +		pr_err("Failed to register framebuffer\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct driver_d s3cfb_driver = {
> +	.name	= "s3cfb",
> +	.probe	= s3cfb_probe,
> +#ifdef CONFIG_VIDEO_INFO_VERBOSE
> +	.info	= s3cfb_info,
> +#endif
> +};
> +
> +static int s3cfb_init(void)
> +{
> +	return register_driver(&s3cfb_driver);
> +}
> +
> +device_initcall(s3cfb_init);
> +
> +/**
> + * The S3C244x LCD controller supports passive (CSTN/STN) and active (TFT) LC displays
> + *
> + * The driver itself currently supports only active TFT LC displays in the follwing manner:
> + *
> + *  * Palletized colours
> + *    - 1 bpp
> + *    - 2 bpp
> + *    - 4 bpp
> + *    - 8 bpp
> + *  * True colours
> + *    - 16 bpp
> + *    - 24 bpp
> + */
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Juergen Beisert]

Hi Sascha,

Sascha Hauer wrote:
> On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
> > Currently barebox uses a fixed videomode setup. Everything is compiled
> > in. This change adds the possibility to select a videomode according to a
> > connected display at runtime. The current behaviour is still present if
> > not otherwise configured. If configured for runtime setup, initialization
> > of the video hardware will be delayed until the required videomode will
> > be selected from the shell code. If more than one videomode is supported
> > by the platform, running the 'devinfo' command on the framebuffer device
> > shows the supported videomode list. After selecting the videomode, the
> > output can be enabled.
>
> General remarks about this series:
>
> - Please do not add code with '#if 0' and activate it later. This shows
>   the series has the wrong order.

This was for review only. If I would change the code in one step, the patch is 
unreadable.

> - Please refrain from basing your internal functions around 'struct
>   device_d'. By doing so we completey lose type safety and at least in
>   case of the mci framework where three different devices are involved
>   this leads to unreadable and error prone code.

But IMHO in the case of the MCI there _are_ three devices!
 - The one that knows how to handle disk drives
 - The one that knows what a SD card is
 - the one that knows how to transfer data from an to an attached device.

Why this is unreadable or error prone? If you combine all these different 
functions into one I would say: Yes, the result is unreadable and error 
prone. And if you would say for a bootloader this separate approach is 
over-engineered, I would say: Maybe.

>   The framebuffer code should be based around struct fb_info.

I do not like this idea, but okay. In the next series I will do it in this 
way.

> - Please keep the line lengths within sensible limits.

Sorry, I checked it the last time, but some lines are slipped through.

> - Get rid of CONFIG_VIDEO_DELAY_INIT and make the mode runtime
>   changeable. All this requires is a
>   host->fb_disable(info); host->fb_mode(info, newmode);
> host->fb_enable(mode);

Hmm, you want to be able to change the videomode more than one times in 
barebox? So, I need more effort in memory management. Okay.

Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/12] Bring in dynamic videomode selection at runtime

[Juergen Beisert]

Sascha Hauer wrote:
> [...]
> > +	fb_dev->priv = cdev;	/* pointer forward */
> > +	cdev->dev = fb_dev;	/* pointer backward */
> > +
> > +	cdev->ops = &fb_ops;
> > +	cdev->name = asprintf("fb%d", id);
> > +
> > +	cdev->size = fb_dev->size;	/* use the default if any */
> > +	cdev->priv = info;
> > +
> > +	info->host = host;
> > +	info->fb_dev = fb_dev;
> > +
> > +	/* setup defaults */
> > +	if (host->bits_per_pixel != 0)
> > +		info->bits_per_pixel = host->bits_per_pixel;
> > +	else
> > +		info->bits_per_pixel = 16;	/* means RGB565 */
>
> No, this does not mean RGB565. It could also mean BGR or even something
> else.

You are right. But currently it means exactly what I wrote. All drivers 
currently using RGB565 for 16 bpp. To make it as you stated, we need more 
information about the bits per colour and their layout. Currently the 
graphics drivers do it in their own way. No way to intervent from the 
platform file.

> > +
> > +	/* add params on demand */
>
> There is no information in this comment.

Yes and no ;-) But when "CONFIG_VIDEO_DELAY_INIT" is gone also this feature is 
gone.

> > +	add_fb_parameter(fb_dev);
> > +
> > +	devfs_create(cdev);
> > +#ifndef CONFIG_VIDEO_DELAY_INIT
> > +	/* initialize video mode immediately (the first one) */
> > +	fb_activate_mode(fb_dev, &host->mode[0]);
> > +#endif
> > +	return 0;
> > +}
> > +
> > +static struct driver_d fb_driver = {
> > +	.name	= "framebuffer",
> > +	.probe	= fb_probe,
> > +};
> > +
> > +static int framebuffer_init(void)
> > +{
> > +	return register_driver(&fb_driver);
> > +}
> > +
> > +device_initcall(framebuffer_init);
> > +
> > +struct device_d *register_framebuffer(struct fb_host *host, void *base,
> > unsigned size) +{
> > +	struct device_d *fb_dev;
> > +	int rc;
> > +
> > +	fb_dev = xzalloc(sizeof(struct device_d));
> > +
> > +	strcpy(fb_dev->name, fb_driver.name);
> > +	fb_dev->platform_data = (void*)host;
> > +
> > +	/* setup the defaults for this framebuffer if given */
> > +	fb_dev->size = size;
> > +	fb_dev->map_base = (unsigned long)base;
> > +
> > +	rc = register_device(fb_dev);
> > +	if (rc != 0) {
> > +		pr_debug("Cannot register framebuffer device\n");
> > +		free(fb_dev);
> > +		return NULL;
> > +	}
> > +
> > +	return fb_dev;
> > +}
> > +#endif
> > +
> >  int register_framebuffer(struct fb_info *info)
> >  {
> >  	int id = get_free_deviceid("fb");
> > diff --git a/include/fb.h b/include/fb.h
> > index 218b244..96edc24 100644
> > --- a/include/fb.h
> > +++ b/include/fb.h
> > @@ -85,6 +85,46 @@ struct fb_ops {
> >  	void (*fb_disable)(struct fb_info *info);
> >  };
> >
> > +#if 0
> > +struct fb_host {
> > +	const struct fb_videomode *mode;
> > +	unsigned mode_cnt;
> > +
> > +	struct device_d *hw_dev;
> > +
> > +	/* callbacks into the video hardware driver */
> > +	int (*fb_setcolreg)(struct fb_info*, unsigned, unsigned, unsigned,
> > unsigned, unsigned); +	int (*fb_mode)(struct fb_info*, const struct
> > fb_videomode*);
> > +	void (*fb_enable)(struct fb_info*);
> > +	void (*fb_disable)(struct fb_info*);
> > +
> > +	unsigned bits_per_pixel;
> > +};
> > +
> > +struct fb_info {
> > +	struct fb_host *host;
> > +	struct device_d *fb_dev;
> > +	const struct fb_videomode *active_mode;
> > +
> > +	u32 xres;			/* visible resolution		*/
> > +	u32 yres;
> > +	u32 bits_per_pixel;		/* guess what			*/
> > +
> > +	u32 grayscale;			/* != 0 Graylevels instead of colors */
> > +
> > +	struct fb_bitfield red;		/* bitfield in fb mem if true color, */
> > +	struct fb_bitfield green;	/* else only length is significant */
> > +	struct fb_bitfield blue;
> > +	struct fb_bitfield transp;	/* transparency			*/
>
> If you go the way of duplicating the code and removing the old code
> afterwards I would assume that you add the code in the right way and
> start to add doxygen from the beginning instead of fixing it later.

I tried. But it makes the patch unreadable due to the fact it touches more 
lines than required for the code itself. And I know you don't like patches 
with too many '+' and '-' lines in...

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/12] Bring in dynamic videomode selection at runtime

[Juergen Beisert]

Hi Sascha,

Sascha Hauer wrote:
> [...]
> > +
> > +static int framebuffer_init(void)
> > +{
> > +	return register_driver(&fb_driver);
> > +}
> > +
> > +device_initcall(framebuffer_init);
> > +
> > +struct device_d *register_framebuffer(struct fb_host *host, void *base,
> > unsigned size) +{
>
> Why are base and size passed to register_framebuffer? The framebuffer
> core should not be interested in this at this point.

Intended for convenience only.

> > +	struct device_d *fb_dev;
> > +	int rc;
> > +
> > +	fb_dev = xzalloc(sizeof(struct device_d));
>
> Why is the device not a member of fb_info (or now fb_host) anymore like
> it used to be? There was no malloc necessary for that.

Glue things from different frameworks together is IMHO a bad idea. But as I 
stated in another mail, I will recombine them.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Belisko Marek]

Hi,

On Mon, Nov 15, 2010 at 10:57 AM, Juergen Beisert <jbe@pengutronix.de> wrote:
> Hi Sascha,
>
> Sascha Hauer wrote:
>> On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
>> > Currently barebox uses a fixed videomode setup. Everything is compiled
>> > in. This change adds the possibility to select a videomode according to a
>> > connected display at runtime. The current behaviour is still present if
>> > not otherwise configured. If configured for runtime setup, initialization
>> > of the video hardware will be delayed until the required videomode will
>> > be selected from the shell code. If more than one videomode is supported
>> > by the platform, running the 'devinfo' command on the framebuffer device
>> > shows the supported videomode list. After selecting the videomode, the
>> > output can be enabled.
>>
>> General remarks about this series:
>>
>> - Please do not add code with '#if 0' and activate it later. This shows
>>   the series has the wrong order.
>
> This was for review only. If I would change the code in one step, the patch is
> unreadable.
>
>> - Please refrain from basing your internal functions around 'struct
>>   device_d'. By doing so we completey lose type safety and at least in
>>   case of the mci framework where three different devices are involved
>>   this leads to unreadable and error prone code.
>
> But IMHO in the case of the MCI there _are_ three devices!
>  - The one that knows how to handle disk drives
>  - The one that knows what a SD card is
>  - the one that knows how to transfer data from an to an attached device.
>
> Why this is unreadable or error prone? If you combine all these different
> functions into one I would say: Yes, the result is unreadable and error
> prone. And if you would say for a bootloader this separate approach is
> over-engineered, I would say: Maybe.
>
>>   The framebuffer code should be based around struct fb_info.
>
> I do not like this idea, but okay. In the next series I will do it in this
> way.
>
>> - Please keep the line lengths within sensible limits.
>
> Sorry, I checked it the last time, but some lines are slipped through.
Couldn't be included in barebox scripts also checkpatch.pl script from kernel?
Would be nice to have proper patches with kernel coding style.
>
>> - Get rid of CONFIG_VIDEO_DELAY_INIT and make the mode runtime
>>   changeable. All this requires is a
>>   host->fb_disable(info); host->fb_mode(info, newmode);
>> host->fb_enable(mode);
>
> Hmm, you want to be able to change the videomode more than one times in
> barebox? So, I need more effort in memory management. Okay.
>
> Juergen
>
> --
> Pengutronix e.K.                              | Juergen Beisert             |
> Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
> Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |
>
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
>

thanks,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 08/12] Add a video driver for S3C2440 bases platforms

[Juergen Beisert]

Sascha Hauer wrote:
> On Tue, Oct 26, 2010 at 01:31:44PM +0200, Juergen Beisert wrote:
> > From: Juergen Beisert <juergen@kreuzholzen.de>
> >
> > Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>
> > ---
> >  arch/arm/mach-s3c24xx/include/mach/fb.h |   40 +++
> >  drivers/video/Kconfig                   |    6 +
> >  drivers/video/Makefile                  |    1 +
> >  drivers/video/s3c.c                     |  452
> > +++++++++++++++++++++++++++++++ 4 files changed, 499 insertions(+), 0
> > deletions(-)
> >  create mode 100644 arch/arm/mach-s3c24xx/include/mach/fb.h
> >  create mode 100644 drivers/video/s3c.c
> >
> > diff --git a/arch/arm/mach-s3c24xx/include/mach/fb.h
> > b/arch/arm/mach-s3c24xx/include/mach/fb.h new file mode 100644
> > index 0000000..eec6164
> > --- /dev/null
> > +++ b/arch/arm/mach-s3c24xx/include/mach/fb.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright (C) 2010 Juergen Beisert
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + *
> > + */
> > +
> > +#ifndef __MACH_FB_H_
> > +# define __MACH_FB_H_
> > +
> > +#include <fb.h>
> > +
> > +struct s3c_fb_platform_data {
> > +
> > +	const struct fb_videomode *mode_list;
> > +	unsigned mode_cnt;
> > +
> > +	int passive_display;	/**< enable support for STN or CSTN displays */
> > +
> > +	void *framebuffer;	/**< force framebuffer base address */
> > +	unsigned size;		/**< force framebuffer size */
> > +
> > +	/** hook to enable backlight and stuff */
> > +	void (*enable)(int);
> > +};
> > +
> > +#endif /* __MACH_FB_H_ */
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index d7f3d01..5a5edd2 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -39,4 +39,10 @@ config DRIVER_VIDEO_IMX_IPU
> >  	  Add support for the IPU framebuffer device found on
> >  	  i.MX31 and i.MX35 CPUs.
> >
> > +config S3C_VIDEO
> > +	bool "S3C244x framebuffer driver"
> > +	depends on ARCH_S3C24xx
> > +	help
> > +	  Add support for the S3C244x LCD controller.
> > +
> >  endif
> > diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> > index 179f0c4..4287fc8 100644
> > --- a/drivers/video/Makefile
> > +++ b/drivers/video/Makefile
> > @@ -2,3 +2,4 @@ obj-$(CONFIG_VIDEO) += fb.o
> >
> >  obj-$(CONFIG_DRIVER_VIDEO_IMX) += imx.o
> >  obj-$(CONFIG_DRIVER_VIDEO_IMX_IPU) += imx-ipu-fb.o
> > +obj-$(CONFIG_S3C_VIDEO) += s3c.o
> > diff --git a/drivers/video/s3c.c b/drivers/video/s3c.c
> > new file mode 100644
> > index 0000000..8ae5785
> > --- /dev/null
> > +++ b/drivers/video/s3c.c
> > @@ -0,0 +1,452 @@
> > +/*
> > + * Copyright (C) 2010 Juergen Beisert
> > + *
> > + * This driver is based on a patch found in the web:
> > + * (C) Copyright 2006 by OpenMoko, Inc.
> > + * Author: Harald Welte <laforge at openmoko.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + *
> > + */
> > +
> > +/* #define DEBUG */
> > +
> > +#include <common.h>
> > +#include <init.h>
> > +#include <fb.h>
> > +#include <driver.h>
> > +#include <malloc.h>
> > +#include <errno.h>
> > +#include <asm/io.h>
> > +#include <mach/gpio.h>
> > +#include <mach/s3c24xx-generic.h>
> > +#include <mach/s3c24x0-clocks.h>
> > +#include <mach/fb.h>
> > +
> > +#define LCDCON1 0x00
> > +# define PNRMODE(x) (((x) & 3) << 5)
> > +# define BPPMODE(x) (((x) & 0xf) << 1)
> > +# define SET_CLKVAL(x) (((x) & 0x3ff) << 8)
> > +# define GET_CLKVAL(x) (((x) >> 8) & 0x3ff)
> > +# define ENVID (1 << 0)
> > +
> > +#define LCDCON2 0x04
> > +# define SET_VBPD(x) (((x) & 0xff) << 24)
> > +# define SET_LINEVAL(x) (((x) & 0x3ff) << 14)
> > +# define SET_VFPD(x) (((x) & 0xff) << 6)
> > +# define SET_VSPW(x) ((x) & 0x3f)
> > +
> > +#define LCDCON3 0x08
> > +# define SET_HBPD(x) (((x) & 0x7f) << 19)
> > +# define SET_HOZVAL(x) (((x) & 0x7ff) << 8)
> > +# define SET_HFPD(x) ((x) & 0xff)
> > +
> > +#define LCDCON4 0x0c
> > +# define SET_HSPW(x) ((x) & 0xff)
> > +
> > +#define LCDCON5 0x10
> > +# define BPP24BL (1 << 12)
> > +# define FRM565 (1 << 11)
> > +# define INV_CLK (1 << 10)
> > +# define INV_HS (1 << 9)
> > +# define INV_VS (1 << 8)
> > +# define INV_DTA (1 << 7)
> > +# define INV_DE (1 << 6)
> > +# define INV_PWREN (1 << 5)
> > +# define INV_LEND (1 << 4)	/* FIXME */
> > +# define ENA_PWREN (1 << 3)
> > +# define ENA_LEND (1 << 2)	/* FIXME */
> > +# define BSWP (1 << 1)
> > +# define HWSWP (1 << 0)
> > +
> > +#define LCDSADDR1 0x14
> > +# define SET_LCDBANK(x) (((x) & 0x1ff) << 21)
> > +# define GET_LCDBANK(x) (((x) >> 21) & 0x1ff)
> > +# define SET_LCDBASEU(x) ((x) & 0x1fffff)
> > +# define GET_LCDBASEU(x) ((x) & 0x1fffff)
> > +
> > +#define LCDSADDR2 0x18
> > +# define SET_LCDBASEL(x) ((x) & 0x1fffff)
> > +# define GET_LCDBASEL(x) ((x) & 0x1fffff)
> > +
> > +#define LCDSADDR3 0x1c
> > +# define SET_OFFSIZE(x) (((x) & 0x7ff) << 11)
> > +# define GET_OFFSIZE(x) (((x) >> 11) & 0x7ff)
> > +# define SET_PAGE_WIDTH(x) ((x) & 0x3ff)
> > +# define GET_PAGE_WIDTH(x) ((x) & 0x3ff)
> > +
> > +#define RED_LUT 0x20
> > +#define GREEN_LUT 0x24
> > +#define BLUE_LUT 0x28
> > +
> > +#define DITHMODE 0x4c
> > +
> > +#define TPAL 0x50
> > +
> > +#define LCDINTPND 0x54
> > +
> > +#define LCDSRCPND 0x58
> > +
> > +#define LCDINTMSK 0x5c
> > +# define FIWSEL (1 << 2)
> > +
> > +#define TCONSEL 0x60
> > +
> > +#define RED 0
> > +#define GREEN 1
> > +#define BLUE 2
> > +#define TRANSP 3
> > +
> > +struct s3cfb_host {
> > +	struct fb_host fb_data;
> > +	struct device_d *hw_dev;
> > +	void __iomem *base;
> > +};
> > +
> > +#define fb_info_to_s3cfb_host(x) ((struct s3cfb_host*)((x)->host))
> >
> > +#define s3cfb_host_to_s3c_fb_platform_data(x) ((struct
> > s3c_fb_platform_data*)((x)->hw_dev->platform_data))
>
> Please add a pointer to 3c_fb_platform_data to s3cfb_host and get rid of
> this define.

But this pointer would only provide redundant information, as this pointer is 
already part of the hw_dev member.

> > +
> > +/* the RGB565 true colour mode */
> > +static const struct fb_bitfield def_rgb565[] = {
> > +	[RED] = {
> > +		.offset = 11,
> > +		.length = 5,
> > +	},
> > +	[GREEN] = {
> > +		.offset = 5,
> > +		.length = 6,
> > +	},
> > +	[BLUE] = {
> > +		.offset = 0,
> > +		.length = 5,
> > +	},
> > +	[TRANSP] = {	/* no support for transparency */
> > +		.length = 0,
> > +	}
> > +};
> > +
> > +/* the RGB888 true colour mode */
> > +static const struct fb_bitfield def_rgb888[] = {
> > +	[RED] = {
> > +		.offset = 16,
> > +		.length = 8,
> > +	},
> > +	[GREEN] = {
> > +		.offset = 8,
> > +		.length = 8,
> > +	},
> > +	[BLUE] = {
> > +		.offset = 0,
> > +		.length = 8,
> > +	},
> > +	[TRANSP] = {	/* no support for transparency */
> > +		.length = 0,
> > +	}
> > +};
> > +
> > +/**
> > + * @param fb_info Framebuffer information
> > + */
> > +static void s3cfb_enable_controller(struct fb_info *fb_info)
> > +{
> > +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> > +	struct s3c_fb_platform_data *pdata =
> > s3cfb_host_to_s3c_fb_platform_data(fbh); +	uint32_t con1;
> > +
> > +	pr_debug("%s called\n", __func__);
> > +
> > +	con1 = readl(fbh->base + LCDCON1);
> > +
> > +	con1 |= ENVID;
> > +
> > +	writel(con1, fbh->base + LCDCON1);
> > +
> > +	if (pdata->enable)
> > +		(pdata->enable)(1);
> > +}
> > +
> > +/**
> > + * @param fb_info Framebuffer information
> > + */
> > +static void s3cfb_disable_controller(struct fb_info *fb_info)
> > +{
> > +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> > +	struct s3c_fb_platform_data *pdata =
> > s3cfb_host_to_s3c_fb_platform_data(fbh); +	uint32_t con1;
> > +
> > +	pr_debug("%s called\n", __func__);
> > +
> > +	if (pdata->enable)
> > +		(pdata->enable)(0);
> > +
> > +	con1 = readl(fbh->base + LCDCON1);
> > +
> > +	con1 &= ~ENVID;
> > +
> > +	writel(con1, fbh->base + LCDCON1);
> > +}
> > +
> > +/**
> > + * Prepare the video hardware for a specified video mode
> > + * @param fb_info Framebuffer information
> > + * @param mode The video mode description to initialize
> > + * @return 0 on success
> > + */
> > +static int s3cfb_initialize_mode(struct fb_info *fb_info, const struct
> > fb_videomode *mode) +{
> > +	struct s3cfb_host *fbh = fb_info_to_s3cfb_host(fb_info);
> > +	struct s3c_fb_platform_data *pdata =
> > s3cfb_host_to_s3c_fb_platform_data(fbh); +	unsigned size, hclk, div;
> > +	uint32_t con1, con2, con3, con4, con5 = 0;
> > +
> > +	pr_debug("%s called\n", __func__);
> > +
> > +	if (pdata->passive_display != 0) {
> > +		pr_err("Passive displays are currently not supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * we need at least this amount of memory for the framebuffer
> > +	 */
> > +	size = mode->xres * mode->yres * (fb_info->bits_per_pixel >> 3);
> > +	if (fb_info->fb_dev->size != 0) {
> > +		if (size > fb_info->fb_dev->size) {
> > +			pr_err("Cannot initialize video mode '%s': Its too large. "
> > +				"Required bytes are %u, available only %u\n",
> > +				mode->name, size, fb_info->fb_dev->size);
> > +			return -EINVAL;
> > +		}
> > +	} else
> > +		fb_info->fb_dev->size = size;
> > +
> > +	/*
> > +	 * if no framebuffer memory was specified yet, allocate one,
> > +	 * and allocate more memory, on user request
> > +	 */
> > +	if (fb_info->fb_dev->map_base == 0U)
> > +		fb_info->fb_dev->map_base =
> > (resource_size_t)xzalloc(fb_info->fb_dev->size);
>
> Honestly I don't understand what the two blocks above do. I hope this
> gets simpler once we remove the base/size arguments from
> register_framebuffer.

These blocks distinguish between dynamic and a fixed framebuffer. And 
register_framebuffer's base/size parameters only allow to define a fixed 
framebuffer. So, how to define a fixed framebuffer when the base and size 
parameter are gone?

> > +
> > +	/* its useful to enable the unit's clock */
> > +	s3c244x_mod_clock(CLK_LCDC, 1);
> > +
> > +	/* ensure video output is _off_ */
> > +	writel(0x00000000, fbh->base + LCDCON1);
> > +
> > +	hclk = s3c24xx_get_hclk() / 1000U;	/* hclk in kHz */
> > +	div = hclk / PICOS2KHZ(mode->pixclock);
> > +	if (div < 3)
> > +		div  = 3;
> > +	/* pixel clock is: (hclk) / ((div + 1) * 2) */
> > +	div += 1;
> > +	div >>= 1;
> > +	div -= 1;
> > +
> > +	con1 = PNRMODE(3) | SET_CLKVAL(div);	/* PNRMODE=3 is TFT */
> > +
> > +	switch (fb_info->bits_per_pixel) {
> > +#if 0
> > +	/* TODO add CLUT based modes */
> > +	case 1:
> > +		con1 |= BPPMODE(8);
> > +		break;
> > +	case 2:
> > +		con1 |= BPPMODE(9);
> > +		break;
> > +	case 4:
> > +		con1 |= BPPMODE(10);
> > +		break;
> > +	case 8:
> > +		con1 |= BPPMODE(11);
> > +		break;
> > +#endif
> > +	case 16:
> > +		con1 |= BPPMODE(12);
> > +		con5 |= FRM565;
> > +		fb_info->red = def_rgb565[RED];
> > +		fb_info->green = def_rgb565[GREEN];
> > +		fb_info->blue = def_rgb565[BLUE];
> > +		fb_info->transp =  def_rgb565[TRANSP];
> > +		break;
> > +	case 24:
> > +		con1 |= BPPMODE(13);
> > +		con5 |= BPP24BL;	/* FIXME */
>
> Please either remove the FIXME or explain what we have to fix here.

Sure.

> > +		fb_info->red = def_rgb888[RED];
> > +		fb_info->green = def_rgb888[GREEN];
> > +		fb_info->blue = def_rgb888[BLUE];
> > +		fb_info->transp =  def_rgb888[TRANSP];
> > +		break;
> > +	default:
> > +		pr_err("Invalid bits per pixel value: %u\n", fb_info->bits_per_pixel);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* 'normal' in register description means positive logic */
> > +	if (!(mode->sync & FB_SYNC_HOR_HIGH_ACT))
> > +		con5 |= INV_HS;
> > +	if (!(mode->sync & FB_SYNC_VERT_HIGH_ACT))
> > +		con5 |= INV_VS;
> > +	if (!(mode->sync & FB_SYNC_DE_HIGH_ACT))
> > +		con5 |= INV_DE;
> > +	if (mode->sync & FB_SYNC_CLK_INVERT)
> > +		con5 |= INV_CLK;	/* display should latch at the rising edge */
> > +	if (mode->sync & FB_SYNC_SWAP_RGB)
> > +		con5 |= HWSWP;
> > +
> > +	/* TODO power enable config via platform data */
> > +
> > +	/* vertical timing */
> > +	con2 = SET_VBPD(mode->upper_margin - 1) |
> > +		SET_LINEVAL(mode->yres - 1) |
> > +		SET_VFPD(mode->lower_margin - 1) |
> > +		SET_VSPW(mode->vsync_len - 1);
> > +
> > +	/* horizontal timing */
> > +	con3 = SET_HBPD(mode->left_margin - 1) |
> > +		SET_HOZVAL(mode->xres - 1) |
> > +		SET_HFPD(mode->right_margin - 1);
> > +	con4 = SET_HSPW(mode->hsync_len - 1);
> > +
> > +	/* basic timing setup */
> > +	writel(con1, fbh->base + LCDCON1);
> > +	pr_debug(" Writing %08X into %08X (con1)\n", con1, fbh->base +
> > LCDCON1); +	writel(con2, fbh->base + LCDCON2);
> > +	pr_debug(" Writing %08X into %08X (con2)\n", con2, fbh->base +
> > LCDCON2); +	writel(con3, fbh->base + LCDCON3);
> > +	pr_debug(" Writing %08X into %08X (con3)\n", con3, fbh->base +
> > LCDCON3); +	writel(con4, fbh->base + LCDCON4);
> > +	pr_debug(" Writing %08X into %08X (con4)\n", con4, fbh->base +
> > LCDCON4); +	writel(con5, fbh->base + LCDCON5);
> > +	pr_debug(" Writing %08X into %08X (con5)\n", con5, fbh->base +
> > LCDCON5); +
> > +	pr_debug("Setting up the fb baseadress to %08X\n",
> > fb_info->fb_dev->map_base); +
> > +	/* framebuffer memory setup */
> > +	writel(fb_info->fb_dev->map_base >> 1, fbh->base + LCDSADDR1);
> > +	size = mode->xres * (fb_info->bits_per_pixel >> 3) * (mode->yres);
>
> You already calculated this value.

Ups, yes. Thanks.

> [...]

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/12] Bring in dynamic videomode selection at runtime

[Sascha Hauer]

On Mon, Nov 15, 2010 at 11:04:52AM +0100, Juergen Beisert wrote:
> Sascha Hauer wrote:
> > [...]
> > > +	fb_dev->priv = cdev;	/* pointer forward */
> > > +	cdev->dev = fb_dev;	/* pointer backward */
> > > +
> > > +	cdev->ops = &fb_ops;
> > > +	cdev->name = asprintf("fb%d", id);
> > > +
> > > +	cdev->size = fb_dev->size;	/* use the default if any */
> > > +	cdev->priv = info;
> > > +
> > > +	info->host = host;
> > > +	info->fb_dev = fb_dev;
> > > +
> > > +	/* setup defaults */
> > > +	if (host->bits_per_pixel != 0)
> > > +		info->bits_per_pixel = host->bits_per_pixel;
> > > +	else
> > > +		info->bits_per_pixel = 16;	/* means RGB565 */
> >
> > No, this does not mean RGB565. It could also mean BGR or even something
> > else.
> 
> You are right. But currently it means exactly what I wrote. All drivers 
> currently using RGB565 for 16 bpp. To make it as you stated, we need more 
> information about the bits per colour and their layout. Currently the 
> graphics drivers do it in their own way. No way to intervent from the 
> platform file.

No, the drivers are free to pass an arbitrary layout to the framework
using fb_bitfields. So you can't state in the framework that 16bit is
RGB565.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 08/12] Add a video driver for S3C2440 bases platforms

[Sascha Hauer]

On Mon, Nov 15, 2010 at 12:35:09PM +0100, Juergen Beisert wrote:
> Sascha Hauer wrote:
> > On Tue, Oct 26, 2010 at 01:31:44PM +0200, Juergen Beisert wrote:
> > > From: Juergen Beisert <juergen@kreuzholzen.de>
> > >
> > > Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>
> > > ---
> > >  arch/arm/mach-s3c24xx/include/mach/fb.h |   40 +++
> > >  drivers/video/Kconfig                   |    6 +
> > >  drivers/video/Makefile                  |    1 +
> > >  drivers/video/s3c.c                     |  452
> > > +++++++++++++++++++++++++++++++ 4 files changed, 499 insertions(+), 0
> > > deletions(-)
> > >  create mode 100644 arch/arm/mach-s3c24xx/include/mach/fb.h
> > >  create mode 100644 drivers/video/s3c.c
> > >
> > > diff --git a/arch/arm/mach-s3c24xx/include/mach/fb.h
> > > +
> > > +#define fb_info_to_s3cfb_host(x) ((struct s3cfb_host*)((x)->host))
> > >
> > > +#define s3cfb_host_to_s3c_fb_platform_data(x) ((struct
> > > s3c_fb_platform_data*)((x)->hw_dev->platform_data))
> >
> > Please add a pointer to 3c_fb_platform_data to s3cfb_host and get rid of
> > this define.
> 
> But this pointer would only provide redundant information, as this pointer is 
> already part of the hw_dev member.

As said before it's a bad idea to dereference this platform_data pointer
in too many functions. We loose type safety and when multiple devices
are involved it gets to easy to pick the wrong device. Also, it's
unreadable.

> > > +
> > > +	/*
> > > +	 * if no framebuffer memory was specified yet, allocate one,
> > > +	 * and allocate more memory, on user request
> > > +	 */
> > > +	if (fb_info->fb_dev->map_base == 0U)
> > > +		fb_info->fb_dev->map_base =
> > > (resource_size_t)xzalloc(fb_info->fb_dev->size);
> >
> > Honestly I don't understand what the two blocks above do. I hope this
> > gets simpler once we remove the base/size arguments from
> > register_framebuffer.
> 
> These blocks distinguish between dynamic and a fixed framebuffer. And 
> register_framebuffer's base/size parameters only allow to define a fixed 
> framebuffer. So, how to define a fixed framebuffer when the base and size 
> parameter are gone?

At some point you either allocate a framebuffer or use the memory
provided by the platform data. There is no need to pass the memory
through the framework beforehand.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Sascha Hauer]

On Mon, Nov 15, 2010 at 10:57:01AM +0100, Juergen Beisert wrote:
> Hi Sascha,
> 
> Sascha Hauer wrote:
> > On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
> > > Currently barebox uses a fixed videomode setup. Everything is compiled
> > > in. This change adds the possibility to select a videomode according to a
> > > connected display at runtime. The current behaviour is still present if
> > > not otherwise configured. If configured for runtime setup, initialization
> > > of the video hardware will be delayed until the required videomode will
> > > be selected from the shell code. If more than one videomode is supported
> > > by the platform, running the 'devinfo' command on the framebuffer device
> > > shows the supported videomode list. After selecting the videomode, the
> > > output can be enabled.
> >
> > General remarks about this series:
> >
> > - Please do not add code with '#if 0' and activate it later. This shows
> >   the series has the wrong order.
> 
> This was for review only. If I would change the code in one step, the patch is 
> unreadable.
> 
> > - Please refrain from basing your internal functions around 'struct
> >   device_d'. By doing so we completey lose type safety and at least in
> >   case of the mci framework where three different devices are involved
> >   this leads to unreadable and error prone code.
> 
> But IMHO in the case of the MCI there _are_ three devices!
>  - The one that knows how to handle disk drives
>  - The one that knows what a SD card is
>  - the one that knows how to transfer data from an to an attached device.
> 
> Why this is unreadable or error prone? If you combine all these different 
> functions into one I would say: Yes, the result is unreadable and error 
> prone. And if you would say for a bootloader this separate approach is 
> over-engineered, I would say: Maybe.

I'm not at all against the presence of three devices. It's only a bad
idea to use the struct device_d * as a reference between functions. In
the MCI framework all functions take some device and then the
platform_data is derefenced to three different struct types. How do I
know which of the three devices is passed there? I have to look at the
calling function figure this out. We can simply let the compiler barf
when somebody passes a wrong pointer type when we do it like we always
did: Just pass a struct mci_host or whatever around.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Sascha Hauer]

On Mon, Nov 15, 2010 at 11:25:53AM +0100, Belisko Marek wrote:
> Hi,
> 
> On Mon, Nov 15, 2010 at 10:57 AM, Juergen Beisert <jbe@pengutronix.de> wrote:
> > Hi Sascha,
> >
> > Sascha Hauer wrote:
> >> On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
> >> > Currently barebox uses a fixed videomode setup. Everything is compiled
> >> > in. This change adds the possibility to select a videomode according to a
> >> > connected display at runtime. The current behaviour is still present if
> >> > not otherwise configured. If configured for runtime setup, initialization
> >> > of the video hardware will be delayed until the required videomode will
> >> > be selected from the shell code. If more than one videomode is supported
> >> > by the platform, running the 'devinfo' command on the framebuffer device
> >> > shows the supported videomode list. After selecting the videomode, the
> >> > output can be enabled.
> >>
> >> General remarks about this series:
> >>
> >> - Please do not add code with '#if 0' and activate it later. This shows
> >>   the series has the wrong order.
> >
> > This was for review only. If I would change the code in one step, the patch is
> > unreadable.
> >
> >> - Please refrain from basing your internal functions around 'struct
> >>   device_d'. By doing so we completey lose type safety and at least in
> >>   case of the mci framework where three different devices are involved
> >>   this leads to unreadable and error prone code.
> >
> > But IMHO in the case of the MCI there _are_ three devices!
> >  - The one that knows how to handle disk drives
> >  - The one that knows what a SD card is
> >  - the one that knows how to transfer data from an to an attached device.
> >
> > Why this is unreadable or error prone? If you combine all these different
> > functions into one I would say: Yes, the result is unreadable and error
> > prone. And if you would say for a bootloader this separate approach is
> > over-engineered, I would say: Maybe.
> >
> >>   The framebuffer code should be based around struct fb_info.
> >
> > I do not like this idea, but okay. In the next series I will do it in this
> > way.
> >
> >> - Please keep the line lengths within sensible limits.
> >
> > Sorry, I checked it the last time, but some lines are slipped through.
> Couldn't be included in barebox scripts also checkpatch.pl script from kernel?
> Would be nice to have proper patches with kernel coding style.

Sure, send a patch ;)

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Belisko Marek]

Hi,

On Wed, Nov 17, 2010 at 9:44 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Nov 15, 2010 at 11:25:53AM +0100, Belisko Marek wrote:
>> Hi,
>>
>> On Mon, Nov 15, 2010 at 10:57 AM, Juergen Beisert <jbe@pengutronix.de> wrote:
>> > Hi Sascha,
>> >
>> > Sascha Hauer wrote:
>> >> On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
>> >> > Currently barebox uses a fixed videomode setup. Everything is compiled
>> >> > in. This change adds the possibility to select a videomode according to a
>> >> > connected display at runtime. The current behaviour is still present if
>> >> > not otherwise configured. If configured for runtime setup, initialization
>> >> > of the video hardware will be delayed until the required videomode will
>> >> > be selected from the shell code. If more than one videomode is supported
>> >> > by the platform, running the 'devinfo' command on the framebuffer device
>> >> > shows the supported videomode list. After selecting the videomode, the
>> >> > output can be enabled.
>> >>
>> >> General remarks about this series:
>> >>
>> >> - Please do not add code with '#if 0' and activate it later. This shows
>> >>   the series has the wrong order.
>> >
>> > This was for review only. If I would change the code in one step, the patch is
>> > unreadable.
>> >
>> >> - Please refrain from basing your internal functions around 'struct
>> >>   device_d'. By doing so we completey lose type safety and at least in
>> >>   case of the mci framework where three different devices are involved
>> >>   this leads to unreadable and error prone code.
>> >
>> > But IMHO in the case of the MCI there _are_ three devices!
>> >  - The one that knows how to handle disk drives
>> >  - The one that knows what a SD card is
>> >  - the one that knows how to transfer data from an to an attached device.
>> >
>> > Why this is unreadable or error prone? If you combine all these different
>> > functions into one I would say: Yes, the result is unreadable and error
>> > prone. And if you would say for a bootloader this separate approach is
>> > over-engineered, I would say: Maybe.
>> >
>> >>   The framebuffer code should be based around struct fb_info.
>> >
>> > I do not like this idea, but okay. In the next series I will do it in this
>> > way.
>> >
>> >> - Please keep the line lengths within sensible limits.
>> >
>> > Sorry, I checked it the last time, but some lines are slipped through.
>> Couldn't be included in barebox scripts also checkpatch.pl script from kernel?
>> Would be nice to have proper patches with kernel coding style.
>
> Sure, send a patch ;)
Just checkpatch.pl from kernel or invent some less restrictive tool ;)
in e.g .python?
>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCHv2] Add dynamic video initialization to barebox

[Sascha Hauer]

On Thu, Nov 18, 2010 at 09:18:36AM +0100, Belisko Marek wrote:
> Hi,
> 
> On Wed, Nov 17, 2010 at 9:44 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Nov 15, 2010 at 11:25:53AM +0100, Belisko Marek wrote:
> >> Hi,
> >>
> >> On Mon, Nov 15, 2010 at 10:57 AM, Juergen Beisert <jbe@pengutronix.de> wrote:
> >> > Hi Sascha,
> >> >
> >> > Sascha Hauer wrote:
> >> >> On Tue, Oct 26, 2010 at 01:31:36PM +0200, Juergen Beisert wrote:
> >> >> > Currently barebox uses a fixed videomode setup. Everything is compiled
> >> >> > in. This change adds the possibility to select a videomode according to a
> >> >> > connected display at runtime. The current behaviour is still present if
> >> >> > not otherwise configured. If configured for runtime setup, initialization
> >> >> > of the video hardware will be delayed until the required videomode will
> >> >> > be selected from the shell code. If more than one videomode is supported
> >> >> > by the platform, running the 'devinfo' command on the framebuffer device
> >> >> > shows the supported videomode list. After selecting the videomode, the
> >> >> > output can be enabled.
> >> >>
> >> >> General remarks about this series:
> >> >>
> >> >> - Please do not add code with '#if 0' and activate it later. This shows
> >> >>   the series has the wrong order.
> >> >
> >> > This was for review only. If I would change the code in one step, the patch is
> >> > unreadable.
> >> >
> >> >> - Please refrain from basing your internal functions around 'struct
> >> >>   device_d'. By doing so we completey lose type safety and at least in
> >> >>   case of the mci framework where three different devices are involved
> >> >>   this leads to unreadable and error prone code.
> >> >
> >> > But IMHO in the case of the MCI there _are_ three devices!
> >> >  - The one that knows how to handle disk drives
> >> >  - The one that knows what a SD card is
> >> >  - the one that knows how to transfer data from an to an attached device.
> >> >
> >> > Why this is unreadable or error prone? If you combine all these different
> >> > functions into one I would say: Yes, the result is unreadable and error
> >> > prone. And if you would say for a bootloader this separate approach is
> >> > over-engineered, I would say: Maybe.
> >> >
> >> >>   The framebuffer code should be based around struct fb_info.
> >> >
> >> > I do not like this idea, but okay. In the next series I will do it in this
> >> > way.
> >> >
> >> >> - Please keep the line lengths within sensible limits.
> >> >
> >> > Sorry, I checked it the last time, but some lines are slipped through.
> >> Couldn't be included in barebox scripts also checkpatch.pl script from kernel?
> >> Would be nice to have proper patches with kernel coding style.
> >
> > Sure, send a patch ;)
> Just checkpatch.pl from kernel or invent some less restrictive tool ;)
> in e.g .python?

checkpatch.pl should be fine. I won't require a patch to be checkpatch
clean, but sometimes it's nice to be able to tell people 'go fix these
checkpatch warnings' when there are obvious style problems in it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox