[PATCH] video: Add GRVGA framebuffer device driver

[PATCH] video: Add GRVGA framebuffer device driver

[Kristoffer Glembo]

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 <kristoffer@gaisler.com>
---
 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

[Geert Uytterhoeven]

On Wed, Jun 15, 2011 at 10:56, Kristoffer Glembo <kristoffer@gaisler.com> 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.
>
> Signed-off-by: Kristoffer Glembo <kristoffer@gaisler.com>
> ---
>  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)

> --- /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

[Kristoffer Glembo]

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

[Geert Uytterhoeven]

On Wed, Jun 15, 2011 at 13:52, Kristoffer Glembo <kristoffer@gaisler.com> wrote:
> 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.

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

[Konrad Rzeszutek Wilk]

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.
> 
> Signed-off-by: Kristoffer Glembo <kristoffer@gaisler.com>
> ---
>  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

[Kristoffer Glembo]

Hi Konrad,


Konrad Rzeszutek Wilk wrote:

>> +#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).


>> +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.


>> +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.


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


Refactoring error .. thanks!


>> +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 ...


>> +		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.


>> +
>> +	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