[PATCH] video: Add GRVGA framebuffer device driver

Subsystems: framebuffer layer, the rest

STALE5529d

6 messages, 3 authors, 2011-06-16 · open the first message on its own page

[PATCH] video: Add GRVGA framebuffer device driver

From: Kristoffer Glembo <hidden>
Date: 2011-06-15 08:56:20

This patch adds support for the GRVGA framebuffer IP core from Aeroflex Gaisler.
The device is used in LEON SPARCV8 based System on Chips. Documentation can
be found here: www.gaisler.com/products/grlib/grip.pdf.

Signed-off-by: Kristoffer Glembo <redacted>
---
 drivers/video/Kconfig  |   10 +
 drivers/video/Makefile |    1 +
 drivers/video/grvga.c  |  559 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/grvga.c
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 549b960..18ee201 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -259,6 +259,16 @@ config FB_TILEBLITTING
 comment "Frame buffer hardware drivers"
 	depends on FB
 
+config FB_GRVGA
+	tristate "Aeroflex Gaisler framebuffer support"
+	depends on FB && SPARC
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	---help---
+	This enables support for the SVGACTRL framebuffer in the GRLIB IP library from Aeroflex Gaisler.
+
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (ZORRO || PCI)
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 8b83129..4cff5ec 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_FB_DEFERRED_IO)   += fb_defio.o
 obj-$(CONFIG_FB_WMT_GE_ROPS)   += wmt_ge_rops.o
 
 # Hardware specific drivers go first
+obj-$(CONFIG_FB_GRVGA)            += grvga.o
 obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
 obj-$(CONFIG_FB_ARC)              += arcfb.o
 obj-$(CONFIG_FB_CLPS711X)         += clps711xfb.o
diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
new file mode 100644
index 0000000..e1514f9
--- /dev/null
+++ b/drivers/video/grvga.c
@@ -0,0 +1,559 @@
+/*
+ * Driver for Aeroflex Gaisler SVGACTRL framebuffer device.
+ *
+ * 2011 (c) Aeroflex Gaisler AB
+ *
+ * Full documentation of the core can be found here:
+ * http://www.gaisler.com/products/grlib/grip.pdf
+ *
+ * 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.
+ *
+ * Contributors: Kristoffer Glembo <kristoffer@gaisler.com>
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/mm.h>
+#include <linux/fb.h>
+#include <linux/io.h>
+
+#define GRVGA_REGLOAD(a)	    (__raw_readl(&(a)))
+#define GRVGA_REGSAVE(a, v)         (__raw_writel(v, &(a)))
+#define GRVGA_REGORIN(a, v)         (GRVGA_REGSAVE(a, (GRVGA_REGLOAD(a) | (v))))
+#define GRVGA_REGANDIN(a, v)        (GRVGA_REGSAVE(a, (GRVGA_REGLOAD(a) & (v))))
+
+#define	GRVGA_FBINFO_DEFAULT	(FBINFO_DEFAULT \
+				 | FBINFO_PARTIAL_PAN_OK \
+				 | FBINFO_HWACCEL_YPAN)
+
+struct grvga_regs {
+	u32 status; 		/* 0x00 */
+	u32 video_length; 	/* 0x04 */
+	u32 front_porch;	/* 0x08 */
+	u32 sync_length;	/* 0x0C */
+	u32 line_length;	/* 0x10 */
+	u32 fb_pos;		/* 0x14 */
+	u32 clk_vector[4];	/* 0x18 */
+	u32 clut;	        /* 0x20 */
+};
+
+struct grvga_par {
+	struct grvga_regs *regs;
+	u32 color_palette[16];  /* 16 entry pseudo palette used by fbcon in true color mode */
+	int clk_sel;
+};
+
+
+static const struct fb_videomode grvga_modedb[] = {
+    {
+	/* 640x480 @ 60 Hz */
+	NULL, 60, 640, 480, 40000, 48, 16, 39, 11, 96, 2,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 800x600 @ 60 Hz */
+	NULL, 60, 800, 600, 25000, 88, 40, 23, 1, 128, 4,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 800x600 @ 72 Hz */
+	NULL, 72, 800, 600, 20000, 64, 56, 23, 37, 120, 6,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 1024x768 @ 60 Hz */
+	NULL, 60, 1024, 768, 15385, 160, 24, 29, 3, 136, 6,
+	0, FB_VMODE_NONINTERLACED
+    }
+ };
+
+static struct fb_fix_screeninfo grvga_fix __initdata = {
+	.id =		"AG SVGACTRL",
+	.type =		FB_TYPE_PACKED_PIXELS,
+	.visual =       FB_VISUAL_PSEUDOCOLOR,
+	.xpanstep =	0,
+	.ypanstep =	1,
+	.ywrapstep =	0,
+	.accel =	FB_ACCEL_NONE,
+};
+
+
+static int grvga_check_var(struct fb_var_screeninfo *var,
+			   struct fb_info *info);
+static int grvga_set_par(struct fb_info *info);
+static int grvga_setcolreg(unsigned regno,
+			   unsigned red, unsigned green, unsigned blue, unsigned transp,
+			   struct fb_info *info);
+static int grvga_pan_display(struct fb_var_screeninfo *var, struct fb_info *info);
+
+static struct fb_ops grvga_ops = {
+	.owner          = THIS_MODULE,
+	.fb_check_var   = grvga_check_var,
+	.fb_set_par	= grvga_set_par,
+	.fb_setcolreg   = grvga_setcolreg,
+	.fb_pan_display = grvga_pan_display,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit
+};
+
+static int grvga_check_var(struct fb_var_screeninfo *var,
+			   struct fb_info *info)
+{
+	struct grvga_par *par = info->par;
+	int i;
+
+	if (!var->xres)
+		var->xres = 1;
+	if (!var->yres)
+		var->yres = 1;
+	if (var->bits_per_pixel <= 8)
+		var->bits_per_pixel = 8;
+	else if (var->bits_per_pixel <= 16)
+		var->bits_per_pixel = 16;
+	else if (var->bits_per_pixel <= 24)
+		var->bits_per_pixel = 24;
+	else if (var->bits_per_pixel <= 32)
+		var->bits_per_pixel = 32;
+	else
+		return -EINVAL;
+
+	var->xres_virtual = var->xres;
+	var->yres_virtual = 2*var->yres;
+
+	if (info->fix.smem_len) {
+		if ((var->yres_virtual*var->xres_virtual*var->bits_per_pixel/8) > info->fix.smem_len)
+			return -ENOMEM;
+	}
+
+	/* Which clocks that are available can be read out in these registers */
+	for (i = 0; i <= 3 ; i++) {
+		if (var->pixclock = par->regs->clk_vector[i])
+			break;
+	}
+	if (i != -1)
+		par->clk_sel = i;
+	else
+		return -EINVAL;
+
+	switch (info->var.bits_per_pixel) {
+	case 8:
+		var->red   = (struct fb_bitfield) {0, 8, 0};      /* offset, length, msb-right */
+		var->green = (struct fb_bitfield) {0, 8, 0};
+		var->blue  = (struct fb_bitfield) {0, 8, 0};
+		var->transp = (struct fb_bitfield) {0, 0, 0};
+		break;
+	case 16:
+		var->red   = (struct fb_bitfield) {11, 5, 0};
+		var->green = (struct fb_bitfield) {5, 6, 0};
+		var->blue  = (struct fb_bitfield) {0, 5, 0};
+		var->transp = (struct fb_bitfield) {0, 0, 0};
+		break;
+	case 24:
+	case 32:
+		var->red   = (struct fb_bitfield) {16, 8, 0};
+		var->green = (struct fb_bitfield) {8, 8, 0};
+		var->blue  = (struct fb_bitfield) {0, 8, 0};
+		var->transp = (struct fb_bitfield) {24, 8, 0};
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int grvga_set_par(struct fb_info *info)
+{
+
+	int func = 0;
+	struct grvga_par *par = info->par;
+
+	GRVGA_REGSAVE(par->regs->video_length,
+		      ((info->var.yres - 1)   << 16) | (info->var.xres - 1));
+	GRVGA_REGSAVE(par->regs->front_porch,
+		      (info->var.lower_margin << 16) | (info->var.right_margin));
+	GRVGA_REGSAVE(par->regs->sync_length,
+		      (info->var.vsync_len    << 16) | (info->var.hsync_len));
+	GRVGA_REGSAVE(par->regs->line_length,
+		      ((info->var.yres + info->var.lower_margin + info->var.upper_margin + info->var.vsync_len - 1) << 16) |
+		      (info->var.xres + info->var.right_margin + info->var.left_margin + info->var.hsync_len - 1));
+
+	switch (info->var.bits_per_pixel) {
+	case 8:
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+		func = 1;
+		break;
+	case 16:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		func = 2;
+		break;
+	case 24:
+	case 32:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		func = 3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	GRVGA_REGSAVE(par->regs->status, ((par->clk_sel << 6) | (func << 4)) | 1);
+
+	info->fix.line_length = (info->var.xres_virtual*info->var.bits_per_pixel)/8;
+	return 0;
+}
+
+static int grvga_setcolreg(unsigned regno, unsigned red, unsigned green, unsigned blue, unsigned transp, struct fb_info *info)
+{
+	struct grvga_par *par;
+	par = info->par;
+
+	if (regno >= 256)	/* Size of CLUT */
+		return -EINVAL;
+
+	if (info->var.grayscale) {
+		/* grayscale = 0.30*R + 0.59*G + 0.11*B */
+		red = green = blue = (red * 77 + green * 151 + blue * 28) >> 8;
+	}
+
+
+
+#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
+
+	red    = CNVT_TOHW(red,   info->var.red.length);
+	green  = CNVT_TOHW(green, info->var.green.length);
+	blue   = CNVT_TOHW(blue,  info->var.blue.length);
+	transp = CNVT_TOHW(transp, info->var.transp.length);
+
+#undef CNVT_TOHW
+
+	/* In PSEUDOCOLOR we use the hardware CLUT */
+	if (info->fix.visual = FB_VISUAL_PSEUDOCOLOR)
+		GRVGA_REGSAVE(par->regs->clut,
+			      (regno << 24) | (red << 16) | (green << 8) | blue);
+
+	/* Truecolor uses the pseudo palette */
+	else if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+		u32 v;
+		if (regno >= 16)
+			return -EINVAL;
+
+
+		v =     (red    << info->var.red.offset)   |
+			(green  << info->var.green.offset) |
+			(blue   << info->var.blue.offset)  |
+			(transp << info->var.transp.offset);
+
+		((u32 *) (info->pseudo_palette))[regno] = v;
+	}
+	return 0;
+}
+
+static int grvga_pan_display(struct fb_var_screeninfo *var,
+			     struct fb_info *info)
+{
+	struct grvga_par *par = info->par;
+	struct fb_fix_screeninfo *fix = &info->fix;
+	u32 base_addr;
+
+	if (var->xoffset != 0)
+		return -EINVAL;
+
+	base_addr = fix->smem_start + (var->yoffset * fix->line_length);
+	base_addr &= ~3UL;
+
+	/* Set framebuffer base address  */
+	GRVGA_REGSAVE(par->regs->fb_pos, base_addr);
+
+	return 0;
+}
+
+static int __init grvga_parse_custom(char *options,
+				     struct fb_var_screeninfo *screendata)
+{
+	char *this_opt;
+	int count = 0;
+	if (!options || !*options)
+		return -1;
+
+	while ((this_opt = strsep(&options, " ")) != NULL) {
+		if (!*this_opt)
+			continue;
+
+		switch (count) {
+		case 0:
+			screendata->pixclock = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 1:
+			screendata->xres = screendata->xres_virtual = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 2:
+			screendata->right_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 3:
+			screendata->hsync_len = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 4:
+			screendata->left_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 5:
+			screendata->yres = screendata->yres_virtual = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 6:
+			screendata->lower_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 7:
+			screendata->vsync_len = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 8:
+			screendata->upper_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 9:
+			screendata->bits_per_pixel = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		}
+	}
+	screendata->activate  = FB_ACTIVATE_NOW;
+	screendata->vmode     = FB_VMODE_NONINTERLACED;
+	return 0;
+}
+
+static int __devinit grvga_probe(struct platform_device *dev)
+{
+	struct fb_info *info;
+	int retval = -ENOMEM;
+	unsigned long virtual_start;
+	unsigned long grvga_fix_addr = 0;
+	unsigned long physical_start = 0;
+	unsigned long grvga_mem_size = 0;
+	struct grvga_par *par;
+	char *options = NULL, *mode_opt = NULL;
+
+	info = framebuffer_alloc(sizeof(struct grvga_par), &dev->dev);
+	if (!info) {
+		dev_err(&dev->dev, "framebuffer_alloc failed\n");
+		return -ENOMEM;
+	}
+
+	/* Expecting: "grvga: modestring, [addr:<framebuffer physical address>], [size:<framebuffer size>]
+	 *
+	 * If modestring is custom:<custom mode string> we parse the string which then contains all videoparameters
+	 * If address is left out, we allocate memory,
+	 * if size is left out we only allocate enough to support the given mode.
+	 */
+	if (fb_get_options("grvga", &options)) {
+		retval = -ENODEV;
+		goto err;
+	}
+
+	if (!options || !*options)
+		options =  "640x480-8@60";
+
+	while (1) {
+		char *this_opt = strsep(&options, ",");
+
+		if (!this_opt)
+			break;
+
+		if (!strncmp(this_opt, "custom", 6))
+			grvga_parse_custom(this_opt, &info->var);
+		else if (!strncmp(this_opt, "addr:", 5))
+			grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
+		else if (!strncmp(this_opt, "size:", 5))
+			grvga_mem_size = simple_strtoul(this_opt + 5, NULL, 0);
+		else
+			mode_opt = this_opt;
+	}
+
+	par = info->par;
+	info->fbops = &grvga_ops;
+	info->fix = grvga_fix;
+	info->pseudo_palette = par->color_palette;
+	info->flags = GRVGA_FBINFO_DEFAULT;
+	info->fix.smem_len = grvga_mem_size;
+
+	par->regs = of_ioremap(&dev->resource[0], 0,
+			       resource_size(&dev->resource[0]),
+			       "grlib-svgactrl regs");
+
+	retval = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (retval < 0) {
+		dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
+		retval = -ENOMEM;
+		goto err1;
+	}
+
+	if (mode_opt) {
+		retval = fb_find_mode(&info->var, info, mode_opt,
+				      grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
+		if (!retval || retval = 4) {
+			retval = -EINVAL;
+			goto err2;
+		}
+	}
+
+	if (!grvga_mem_size)
+		grvga_mem_size = info->var.xres_virtual * info->var.yres_virtual * info->var.bits_per_pixel/8;
+
+	if (grvga_fix_addr) {
+		/* Got framebuffer base address from argument list */
+
+		physical_start = grvga_fix_addr;
+
+		if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
+			dev_err(&dev->dev, "request mem region failed\n");
+			retval = -ENOMEM;
+			goto err2;
+		}
+
+		virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
+
+		if (!virtual_start) {
+			dev_err(&dev->dev, "error mapping memory\n");
+			retval = -ENOMEM;
+			goto err3;
+		}
+	} else {	/* Allocate frambuffer memory */
+
+		unsigned long page;
+
+		virtual_start = (unsigned long) __get_free_pages(GFP_ATOMIC | GFP_DMA,
+								 get_order(grvga_mem_size));
+		if (!virtual_start) {
+			dev_err(&dev->dev,
+				"unable to allocate framebuffer memory (%lu bytes)\n",
+				grvga_mem_size);
+			retval = -ENOMEM;
+			goto err2;
+		}
+
+
+		physical_start = __pa(virtual_start);
+
+		/* Set page reserved so that mmap will work. This is necessary
+		 * since we'll be remapping normal memory.
+		 */
+		for (page = virtual_start;
+		     page < PAGE_ALIGN(virtual_start + grvga_mem_size);
+		     page += PAGE_SIZE) {
+			SetPageReserved(virt_to_page(page));
+		}
+	}
+
+	memset((unsigned long *) virtual_start, 0, grvga_mem_size);
+
+	info->screen_base = (char __iomem *) virtual_start;
+	info->fix.smem_start = physical_start;
+	info->fix.smem_len   = grvga_mem_size;
+
+	dev_set_drvdata(&dev->dev, info);
+
+	dev_info(&dev->dev,
+		 "Aeroflex Gaisler framebuffer device (fb%d), %dx%d-%d, using %luK of video memory @ 0x%x\n",
+		 info->node, info->var.xres, info->var.yres, info->var.bits_per_pixel,
+		 grvga_mem_size >> 10, (unsigned int) info->screen_base);
+
+	retval = register_framebuffer(info);
+	if (retval < 0) {
+		dev_err(&dev->dev, "failed to register framebuffer\n");
+		if (grvga_fix_addr)
+			goto err3;
+		else {
+			kfree((void *)virtual_start);
+			goto err2;
+		}
+	}
+
+	GRVGA_REGSAVE(par->regs->fb_pos, physical_start);
+	GRVGA_REGORIN(par->regs->status, 1); /* Enable framebuffer */
+
+	return 0;
+
+err3:
+	release_mem_region(physical_start, grvga_mem_size);
+err2:
+	fb_dealloc_cmap(&info->cmap);
+err1:
+	of_iounmap(&dev->resource[0], par->regs,
+		   resource_size(&dev->resource[0]));
+err:
+	framebuffer_release(info);
+
+	return retval;
+}
+
+static int __devexit grvga_remove(struct platform_device *device)
+{
+	struct fb_info *info = dev_get_drvdata(&device->dev);
+	struct grvga_par *par = info->par;
+
+	if (info) {
+		unregister_framebuffer(info);
+		fb_dealloc_cmap(&info->cmap);
+		of_iounmap(&device->resource[0], par->regs,
+			   resource_size(&device->resource[0]));
+		framebuffer_release(info);
+		dev_set_drvdata(&device->dev, NULL);
+	}
+
+	return 0;
+}
+
+static struct of_device_id svgactrl_of_match[] = {
+	{
+		.name = "GAISLER_SVGACTRL",
+	},
+	{
+		.name = "01_063",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, svgactrl_of_match);
+
+static struct platform_driver grvga_driver = {
+	.driver = {
+		.name = "grlib-svgactrl",
+		.owner = THIS_MODULE,
+		.of_match_table = svgactrl_of_match,
+	},
+	.probe		= grvga_probe,
+	.remove		= __devexit_p(grvga_remove),
+};
+
+
+int __init grvga_init(void)
+{
+	return platform_driver_register(&grvga_driver);
+}
+
+static void __exit grvga_exit(void)
+{
+	platform_driver_unregister(&grvga_driver);
+}
+
+module_init(grvga_init);
+module_exit(grvga_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Aeroflex Gaisler");
+MODULE_DESCRIPTION("Aeroflex Gaisler framebuffer device driver");
-- 
1.6.4.1

Re: [PATCH] video: Add GRVGA framebuffer device driver

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2011-06-15 11:32:56

On Wed, Jun 15, 2011 at 10:56, Kristoffer Glembo [off-list ref] wrote:
quoted hunk
This patch adds support for the GRVGA framebuffer IP core from Aeroflex Gaisler.
The device is used in LEON SPARCV8 based System on Chips. Documentation can
be found here: www.gaisler.com/products/grlib/grip.pdf.

Signed-off-by: Kristoffer Glembo <redacted>
---
 drivers/video/Kconfig  |   10 +
 drivers/video/Makefile |    1 +
 drivers/video/grvga.c  |  559 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/grvga.c
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 549b960..18ee201 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -259,6 +259,16 @@ config FB_TILEBLITTING
 comment "Frame buffer hardware drivers"
       depends on FB

+config FB_GRVGA
+       tristate "Aeroflex Gaisler framebuffer support"
+       depends on FB && SPARC
At first sight, nothing in this driver seems to be SPARC-specific, so
perhaps this can be relaxed
to e.g. depends on OF_DEVICE?
+       select FB_CFB_FILLRECT
+       select FB_CFB_COPYAREA
+       select FB_CFB_IMAGEBLIT
+       ---help---
+       This enables support for the SVGACTRL framebuffer in the GRLIB IP library from Aeroflex Gaisler.
+
+
 config FB_CIRRUS
       tristate "Cirrus Logic support"
       depends on FB && (ZORRO || PCI)
quoted hunk
--- /dev/null
+++ b/drivers/video/grvga.c
+       dev_info(&dev->dev,
+                "Aeroflex Gaisler framebuffer device (fb%d), %dx%d-%d, using %luK of video memory @ 0x%x\n",
+                info->node, info->var.xres, info->var.yres, info->var.bits_per_pixel,
+                grvga_mem_size >> 10, (unsigned int) info->screen_base);
                                ^^^^^^^^^^^^^^
Please remove the cast and use %p to format the address.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] video: Add GRVGA framebuffer device driver

From: Kristoffer Glembo <hidden>
Date: 2011-06-15 11:52:47

Hi,

Geert Uytterhoeven wrote:
At first sight, nothing in this driver seems to be SPARC-specific, so
perhaps this can be relaxed
to e.g. depends on OF_DEVICE?

It uses of_ioremap/unmap which are SPARC-specific. I have been bitten by things like this before 
when thinking I was portable so this time I just depended on SPARC. The core is not used on any
other platforms and is not very likely to be used either.


Please remove the cast and use %p to format the address.
Will do.

Thanks,
Kristoffer Glembo

Re: [PATCH] video: Add GRVGA framebuffer device driver

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2011-06-15 12:09:25

On Wed, Jun 15, 2011 at 13:52, Kristoffer Glembo [off-list ref] wrote:
Geert Uytterhoeven wrote:
quoted
At first sight, nothing in this driver seems to be SPARC-specific, so
perhaps this can be relaxed
to e.g. depends on OF_DEVICE?
It uses of_ioremap/unmap which are SPARC-specific. I have been bitten by things like this before
when thinking I was portable so this time I just depended on SPARC. The core is not used on any
other platforms and is not very likely to be used either.
Ah, your're right. I just thought all OF-capable platforms had of_ioremap().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] video: Add GRVGA framebuffer device driver

From: Konrad Rzeszutek Wilk <hidden>
Date: 2011-06-15 14:56:20

On Wed, Jun 15, 2011 at 10:56:20AM +0200, Kristoffer Glembo wrote:
This patch adds support for the GRVGA framebuffer IP core from Aeroflex Gaisler.
The device is used in LEON SPARCV8 based System on Chips. Documentation can
be found here: www.gaisler.com/products/grlib/grip.pdf.
See comments below.
quoted hunk
Signed-off-by: Kristoffer Glembo <redacted>
---
 drivers/video/Kconfig  |   10 +
 drivers/video/Makefile |    1 +
 drivers/video/grvga.c  |  559 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/grvga.c
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 549b960..18ee201 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -259,6 +259,16 @@ config FB_TILEBLITTING
 comment "Frame buffer hardware drivers"
 	depends on FB
 
+config FB_GRVGA
+	tristate "Aeroflex Gaisler framebuffer support"
+	depends on FB && SPARC
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	---help---
+	This enables support for the SVGACTRL framebuffer in the GRLIB IP library from Aeroflex Gaisler.
+
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (ZORRO || PCI)
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 8b83129..4cff5ec 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_FB_DEFERRED_IO)   += fb_defio.o
 obj-$(CONFIG_FB_WMT_GE_ROPS)   += wmt_ge_rops.o
 
 # Hardware specific drivers go first
+obj-$(CONFIG_FB_GRVGA)            += grvga.o
 obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
 obj-$(CONFIG_FB_ARC)              += arcfb.o
 obj-$(CONFIG_FB_CLPS711X)         += clps711xfb.o
diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
new file mode 100644
index 0000000..e1514f9
--- /dev/null
+++ b/drivers/video/grvga.c
@@ -0,0 +1,559 @@
+/*
+ * Driver for Aeroflex Gaisler SVGACTRL framebuffer device.
+ *
+ * 2011 (c) Aeroflex Gaisler AB
+ *
+ * Full documentation of the core can be found here:
+ * http://www.gaisler.com/products/grlib/grip.pdf
+ *
+ * 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.
+ *
+ * Contributors: Kristoffer Glembo <kristoffer@gaisler.com>
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/mm.h>
+#include <linux/fb.h>
+#include <linux/io.h>
+
+#define GRVGA_REGLOAD(a)	    (__raw_readl(&(a)))
+#define GRVGA_REGSAVE(a, v)         (__raw_writel(v, &(a)))
writel?
+#define GRVGA_REGORIN(a, v)         (GRVGA_REGSAVE(a, (GRVGA_REGLOAD(a) | (v))))
+#define GRVGA_REGANDIN(a, v)        (GRVGA_REGSAVE(a, (GRVGA_REGLOAD(a) & (v))))
+
+#define	GRVGA_FBINFO_DEFAULT	(FBINFO_DEFAULT \
+				 | FBINFO_PARTIAL_PAN_OK \
+				 | FBINFO_HWACCEL_YPAN)
+
+struct grvga_regs {
+	u32 status; 		/* 0x00 */
+	u32 video_length; 	/* 0x04 */
+	u32 front_porch;	/* 0x08 */
+	u32 sync_length;	/* 0x0C */
+	u32 line_length;	/* 0x10 */
+	u32 fb_pos;		/* 0x14 */
+	u32 clk_vector[4];	/* 0x18 */
+	u32 clut;	        /* 0x20 */
+};
Would it make sense to add __packed__ here? It seems that you
really depend on this ordering.
+
+struct grvga_par {
+	struct grvga_regs *regs;
+	u32 color_palette[16];  /* 16 entry pseudo palette used by fbcon in true color mode */
+	int clk_sel;
+};
+
+
+static const struct fb_videomode grvga_modedb[] = {
+    {
+	/* 640x480 @ 60 Hz */
+	NULL, 60, 640, 480, 40000, 48, 16, 39, 11, 96, 2,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 800x600 @ 60 Hz */
+	NULL, 60, 800, 600, 25000, 88, 40, 23, 1, 128, 4,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 800x600 @ 72 Hz */
+	NULL, 72, 800, 600, 20000, 64, 56, 23, 37, 120, 6,
+	0, FB_VMODE_NONINTERLACED
+    }, {
+	/* 1024x768 @ 60 Hz */
+	NULL, 60, 1024, 768, 15385, 160, 24, 29, 3, 136, 6,
+	0, FB_VMODE_NONINTERLACED
+    }
+ };
+
+static struct fb_fix_screeninfo grvga_fix __initdata = {
+	.id =		"AG SVGACTRL",
+	.type =		FB_TYPE_PACKED_PIXELS,
+	.visual =       FB_VISUAL_PSEUDOCOLOR,
+	.xpanstep =	0,
+	.ypanstep =	1,
+	.ywrapstep =	0,
+	.accel =	FB_ACCEL_NONE,
+};
+
+
+static int grvga_check_var(struct fb_var_screeninfo *var,
+			   struct fb_info *info);
+static int grvga_set_par(struct fb_info *info);
+static int grvga_setcolreg(unsigned regno,
+			   unsigned red, unsigned green, unsigned blue, unsigned transp,
+			   struct fb_info *info);
+static int grvga_pan_display(struct fb_var_screeninfo *var, struct fb_info *info);
+
+static struct fb_ops grvga_ops = {
+	.owner          = THIS_MODULE,
+	.fb_check_var   = grvga_check_var,
+	.fb_set_par	= grvga_set_par,
+	.fb_setcolreg   = grvga_setcolreg,
+	.fb_pan_display = grvga_pan_display,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit
+};
Could you move this struct down and remove the declarations earlier?
+
+static int grvga_check_var(struct fb_var_screeninfo *var,
+			   struct fb_info *info)
+{
+	struct grvga_par *par = info->par;
+	int i;
+
+	if (!var->xres)
+		var->xres = 1;
+	if (!var->yres)
+		var->yres = 1;
+	if (var->bits_per_pixel <= 8)
+		var->bits_per_pixel = 8;
+	else if (var->bits_per_pixel <= 16)
+		var->bits_per_pixel = 16;
+	else if (var->bits_per_pixel <= 24)
+		var->bits_per_pixel = 24;
+	else if (var->bits_per_pixel <= 32)
+		var->bits_per_pixel = 32;
+	else
+		return -EINVAL;
+
+	var->xres_virtual = var->xres;
+	var->yres_virtual = 2*var->yres;
+
+	if (info->fix.smem_len) {
+		if ((var->yres_virtual*var->xres_virtual*var->bits_per_pixel/8) > info->fix.smem_len)
+			return -ENOMEM;
+	}
+
+	/* Which clocks that are available can be read out in these registers */
+	for (i = 0; i <= 3 ; i++) {
+		if (var->pixclock = par->regs->clk_vector[i])
+			break;
+	}
+	if (i != -1)
+		par->clk_sel = i;
How would i possibly become -1?
+	else
+		return -EINVAL;
+
+	switch (info->var.bits_per_pixel) {
+	case 8:
+		var->red   = (struct fb_bitfield) {0, 8, 0};      /* offset, length, msb-right */
+		var->green = (struct fb_bitfield) {0, 8, 0};
+		var->blue  = (struct fb_bitfield) {0, 8, 0};
+		var->transp = (struct fb_bitfield) {0, 0, 0};
+		break;
+	case 16:
+		var->red   = (struct fb_bitfield) {11, 5, 0};
+		var->green = (struct fb_bitfield) {5, 6, 0};
+		var->blue  = (struct fb_bitfield) {0, 5, 0};
+		var->transp = (struct fb_bitfield) {0, 0, 0};
+		break;
+	case 24:
+	case 32:
+		var->red   = (struct fb_bitfield) {16, 8, 0};
+		var->green = (struct fb_bitfield) {8, 8, 0};
+		var->blue  = (struct fb_bitfield) {0, 8, 0};
+		var->transp = (struct fb_bitfield) {24, 8, 0};
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int grvga_set_par(struct fb_info *info)
+{
+
+	int func = 0;
You probably want that to be u32.
+	struct grvga_par *par = info->par;
+
+	GRVGA_REGSAVE(par->regs->video_length,
+		      ((info->var.yres - 1)   << 16) | (info->var.xres - 1));
+	GRVGA_REGSAVE(par->regs->front_porch,
+		      (info->var.lower_margin << 16) | (info->var.right_margin));
+	GRVGA_REGSAVE(par->regs->sync_length,
+		      (info->var.vsync_len    << 16) | (info->var.hsync_len));
+	GRVGA_REGSAVE(par->regs->line_length,
+		      ((info->var.yres + info->var.lower_margin + info->var.upper_margin + info->var.vsync_len - 1) << 16) |
+		      (info->var.xres + info->var.right_margin + info->var.left_margin + info->var.hsync_len - 1));
+
+	switch (info->var.bits_per_pixel) {
+	case 8:
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+		func = 1;
+		break;
+	case 16:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		func = 2;
+		break;
+	case 24:
+	case 32:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		func = 3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	GRVGA_REGSAVE(par->regs->status, ((par->clk_sel << 6) | (func << 4)) | 1);
Just in case in the future you modify the code and bitshift func more than you thought
and end up with a signed value in an unsigned register.
+
+	info->fix.line_length = (info->var.xres_virtual*info->var.bits_per_pixel)/8;
+	return 0;
+}
+
+static int grvga_setcolreg(unsigned regno, unsigned red, unsigned green, unsigned blue, unsigned transp, struct fb_info *info)
+{
+	struct grvga_par *par;
+	par = info->par;
+
+	if (regno >= 256)	/* Size of CLUT */
+		return -EINVAL;
+
+	if (info->var.grayscale) {
+		/* grayscale = 0.30*R + 0.59*G + 0.11*B */
+		red = green = blue = (red * 77 + green * 151 + blue * 28) >> 8;
+	}
+
+
+
+#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
+
+	red    = CNVT_TOHW(red,   info->var.red.length);
+	green  = CNVT_TOHW(green, info->var.green.length);
+	blue   = CNVT_TOHW(blue,  info->var.blue.length);
+	transp = CNVT_TOHW(transp, info->var.transp.length);
+
+#undef CNVT_TOHW
+
+	/* In PSEUDOCOLOR we use the hardware CLUT */
+	if (info->fix.visual = FB_VISUAL_PSEUDOCOLOR)
+		GRVGA_REGSAVE(par->regs->clut,
+			      (regno << 24) | (red << 16) | (green << 8) | blue);
+
+	/* Truecolor uses the pseudo palette */
+	else if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+		u32 v;
+		if (regno >= 16)
+			return -EINVAL;
+
+
+		v =     (red    << info->var.red.offset)   |
+			(green  << info->var.green.offset) |
+			(blue   << info->var.blue.offset)  |
+			(transp << info->var.transp.offset);
+
+		((u32 *) (info->pseudo_palette))[regno] = v;
+	}
+	return 0;
+}
+
+static int grvga_pan_display(struct fb_var_screeninfo *var,
+			     struct fb_info *info)
+{
+	struct grvga_par *par = info->par;
+	struct fb_fix_screeninfo *fix = &info->fix;
+	u32 base_addr;
+
+	if (var->xoffset != 0)
+		return -EINVAL;
+
+	base_addr = fix->smem_start + (var->yoffset * fix->line_length);
+	base_addr &= ~3UL;
+
+	/* Set framebuffer base address  */
+	GRVGA_REGSAVE(par->regs->fb_pos, base_addr);
+
+	return 0;
+}
+
+static int __init grvga_parse_custom(char *options,
+				     struct fb_var_screeninfo *screendata)
+{
+	char *this_opt;
+	int count = 0;
+	if (!options || !*options)
+		return -1;
+
+	while ((this_opt = strsep(&options, " ")) != NULL) {
+		if (!*this_opt)
+			continue;
+
+		switch (count) {
+		case 0:
+			screendata->pixclock = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 1:
+			screendata->xres = screendata->xres_virtual = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 2:
+			screendata->right_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 3:
+			screendata->hsync_len = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 4:
+			screendata->left_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 5:
+			screendata->yres = screendata->yres_virtual = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 6:
+			screendata->lower_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 7:
+			screendata->vsync_len = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 8:
+			screendata->upper_margin = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
+		case 9:
+			screendata->bits_per_pixel = simple_strtoul(this_opt, NULL, 0);
+			count++;
+			break;
No default case?
+		}
+	}
+	screendata->activate  = FB_ACTIVATE_NOW;
+	screendata->vmode     = FB_VMODE_NONINTERLACED;
+	return 0;
+}
+
+static int __devinit grvga_probe(struct platform_device *dev)
+{
+	struct fb_info *info;
+	int retval = -ENOMEM;
+	unsigned long virtual_start;
+	unsigned long grvga_fix_addr = 0;
+	unsigned long physical_start = 0;
+	unsigned long grvga_mem_size = 0;
+	struct grvga_par *par;
+	char *options = NULL, *mode_opt = NULL;
+
+	info = framebuffer_alloc(sizeof(struct grvga_par), &dev->dev);
+	if (!info) {
+		dev_err(&dev->dev, "framebuffer_alloc failed\n");
+		return -ENOMEM;
+	}
+
+	/* Expecting: "grvga: modestring, [addr:<framebuffer physical address>], [size:<framebuffer size>]
+	 *
+	 * If modestring is custom:<custom mode string> we parse the string which then contains all videoparameters
+	 * If address is left out, we allocate memory,
+	 * if size is left out we only allocate enough to support the given mode.
+	 */
+	if (fb_get_options("grvga", &options)) {
+		retval = -ENODEV;
+		goto err;
+	}
+
+	if (!options || !*options)
+		options =  "640x480-8@60";
+
+	while (1) {
+		char *this_opt = strsep(&options, ",");
+
+		if (!this_opt)
+			break;
+
+		if (!strncmp(this_opt, "custom", 6))
+			grvga_parse_custom(this_opt, &info->var);
+		else if (!strncmp(this_opt, "addr:", 5))
+			grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
+		else if (!strncmp(this_opt, "size:", 5))
+			grvga_mem_size = simple_strtoul(this_opt + 5, NULL, 0);
+		else
+			mode_opt = this_opt;
+	}
+
+	par = info->par;
+	info->fbops = &grvga_ops;
+	info->fix = grvga_fix;
+	info->pseudo_palette = par->color_palette;
+	info->flags = GRVGA_FBINFO_DEFAULT;
+	info->fix.smem_len = grvga_mem_size;
+
+	par->regs = of_ioremap(&dev->resource[0], 0,
+			       resource_size(&dev->resource[0]),
+			       "grlib-svgactrl regs");
+
+	retval = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (retval < 0) {
+		dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
+		retval = -ENOMEM;
+		goto err1;
+	}
+
+	if (mode_opt) {
+		retval = fb_find_mode(&info->var, info, mode_opt,
+				      grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
+		if (!retval || retval = 4) {
+			retval = -EINVAL;
+			goto err2;
+		}
+	}
+
+	if (!grvga_mem_size)
+		grvga_mem_size = info->var.xres_virtual * info->var.yres_virtual * info->var.bits_per_pixel/8;
Should you update the info->fix.smem_len ?
+
+	if (grvga_fix_addr) {
+		/* Got framebuffer base address from argument list */
+
+		physical_start = grvga_fix_addr;
+
+		if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
+			dev_err(&dev->dev, "request mem region failed\n");
requested..
+			retval = -ENOMEM;
+			goto err2;
+		}
+
+		virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
+
+		if (!virtual_start) {
+			dev_err(&dev->dev, "error mapping memory\n");
+			retval = -ENOMEM;
+			goto err3;
+		}
+	} else {	/* Allocate frambuffer memory */
+
+		unsigned long page;
+
+		virtual_start = (unsigned long) __get_free_pages(GFP_ATOMIC | GFP_DMA,
+								 get_order(grvga_mem_size));
+		if (!virtual_start) {
+			dev_err(&dev->dev,
+				"unable to allocate framebuffer memory (%lu bytes)\n",
+				grvga_mem_size);
+			retval = -ENOMEM;
+			goto err2;
+		}
+
+
+		physical_start = __pa(virtual_start);
Yikes. That is one big assumption. You don't want to use the PCI/DMA API
to map it? I thought that on SPARCs the Bus addresses are different
from the physical addresses?
+
+		/* Set page reserved so that mmap will work. This is necessary
+		 * since we'll be remapping normal memory.
+		 */
+		for (page = virtual_start;
+		     page < PAGE_ALIGN(virtual_start + grvga_mem_size);
+		     page += PAGE_SIZE) {
+			SetPageReserved(virt_to_page(page));
+		}
+	}
+
+	memset((unsigned long *) virtual_start, 0, grvga_mem_size);
+
+	info->screen_base = (char __iomem *) virtual_start;
+	info->fix.smem_start = physical_start;
+	info->fix.smem_len   = grvga_mem_size;
Ah, you update it here. OK.
+
+	dev_set_drvdata(&dev->dev, info);
+
+	dev_info(&dev->dev,
+		 "Aeroflex Gaisler framebuffer device (fb%d), %dx%d-%d, using %luK of video memory @ 0x%x\n",
+		 info->node, info->var.xres, info->var.yres, info->var.bits_per_pixel,
+		 grvga_mem_size >> 10, (unsigned int) info->screen_base);
+
+	retval = register_framebuffer(info);
+	if (retval < 0) {
+		dev_err(&dev->dev, "failed to register framebuffer\n");
+		if (grvga_fix_addr)
+			goto err3;
+		else {
+			kfree((void *)virtual_start);
+			goto err2;
+		}
+	}
+
+	GRVGA_REGSAVE(par->regs->fb_pos, physical_start);
+	GRVGA_REGORIN(par->regs->status, 1); /* Enable framebuffer */
+
+	return 0;
+
+err3:
+	release_mem_region(physical_start, grvga_mem_size);
+err2:
+	fb_dealloc_cmap(&info->cmap);
+err1:
+	of_iounmap(&dev->resource[0], par->regs,
+		   resource_size(&dev->resource[0]));
+err:
+	framebuffer_release(info);
+
+	return retval;
+}
+
+static int __devexit grvga_remove(struct platform_device *device)
+{
+	struct fb_info *info = dev_get_drvdata(&device->dev);
+	struct grvga_par *par = info->par;
+
+	if (info) {
+		unregister_framebuffer(info);
+		fb_dealloc_cmap(&info->cmap);
+		of_iounmap(&device->resource[0], par->regs,
+			   resource_size(&device->resource[0]));
+		framebuffer_release(info);
+		dev_set_drvdata(&device->dev, NULL);
Shouldn't that be much earlier? Like right after unregister_framebuffer?
+	}
+
+	return 0;
+}
+
+static struct of_device_id svgactrl_of_match[] = {
+	{
+		.name = "GAISLER_SVGACTRL",
+	},
+	{
+		.name = "01_063",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, svgactrl_of_match);
+
+static struct platform_driver grvga_driver = {
+	.driver = {
+		.name = "grlib-svgactrl",
+		.owner = THIS_MODULE,
+		.of_match_table = svgactrl_of_match,
+	},
+	.probe		= grvga_probe,
+	.remove		= __devexit_p(grvga_remove),
+};
+
+
+int __init grvga_init(void)
+{
+	return platform_driver_register(&grvga_driver);
+}
+
+static void __exit grvga_exit(void)
+{
+	platform_driver_unregister(&grvga_driver);
+}
+
+module_init(grvga_init);
+module_exit(grvga_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Aeroflex Gaisler");
+MODULE_DESCRIPTION("Aeroflex Gaisler framebuffer device driver");
-- 
1.6.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video: Add GRVGA framebuffer device driver

From: Kristoffer Glembo <hidden>
Date: 2011-06-16 07:20:33

Hi Konrad,


Konrad Rzeszutek Wilk wrote:
quoted
+#define GRVGA_REGLOAD(a)	    (__raw_readl(&(a)))
+#define GRVGA_REGSAVE(a, v)         (__raw_writel(v, &(a)))
writel?

I use __raw since I want native endianess. On a big endian system
writel/readl byte swap since they are for little endian (PCI).

quoted
+struct grvga_regs {
+	u32 status; 		/* 0x00 */
+	u32 video_length; 	/* 0x04 */
+	u32 front_porch;	/* 0x08 */
+	u32 sync_length;	/* 0x0C */
+	u32 line_length;	/* 0x10 */
+	u32 fb_pos;		/* 0x14 */
+	u32 clk_vector[4];	/* 0x18 */
+	u32 clut;	        /* 0x20 */
+};
Would it make sense to add __packed__ here? It seems that you
really depend on this ordering.
I depend on the ordering, but that is guaranteed by the compiler without __packed__. 
Attribute __packed__ is dangerous here since it tells the compiler that it is ok
to do byte accesses.

quoted
+static struct fb_ops grvga_ops = {
+	.owner          = THIS_MODULE,
+	.fb_check_var   = grvga_check_var,
+	.fb_set_par	= grvga_set_par,
+	.fb_setcolreg   = grvga_setcolreg,
+	.fb_pan_display = grvga_pan_display,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit
+};
Could you move this struct down and remove the declarations earlier?
Yes sure.

quoted
+	}
+	if (i != -1)
+		par->clk_sel = i;
How would i possibly become -1?

Refactoring error .. thanks!

quoted
+static int grvga_set_par(struct fb_info *info)
+{
+
+	int func = 0;
You probably want that to be u32.
Sure why not.

No default case?
Will add ...

quoted
+		physical_start = __pa(virtual_start);
Yikes. That is one big assumption. You don't want to use the PCI/DMA API
to map it? I thought that on SPARCs the Bus addresses are different
from the physical addresses?
Yeah I should map it for cleanliness sake. It does not matter on this architecture (LEON) though.

quoted
+
+	if (info) {
+		unregister_framebuffer(info);
+		fb_dealloc_cmap(&info->cmap);
+		of_iounmap(&device->resource[0], par->regs,
+			   resource_size(&device->resource[0]));
+		framebuffer_release(info);
+		dev_set_drvdata(&device->dev, NULL);
Shouldn't that be much earlier? Like right after unregister_framebuffer?
You mean I should set driver_data = NULL ASAP after unregistering the framebuffer?
Does it really matter? Please enlighten me.


Thanks a lot for your feedback.

Best regards,
Kristoffer Glembo
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help