Re: [PATCH] unicore32: add framebuffer driver for unigfx engine in PKUnity SoC

On Sat, Jan 22, 2011 at 06:29:45PM +0800, Guan Xuetao wrote:

[snip]

+static unsigned long unifb_regs[10];
+

This is probably something that you will want to move in to platform
data.

+#define VIDEOMEMSIZE	(SZ_4M)		/* 4 MB for 1024*768*32b */
+
+static void *videomemory;
+static u_long videomemorysize = VIDEOMEMSIZE;
+module_param(videomemorysize, ulong, 0);
+

Along with these, given that they're not dynamically acquired by the
driver.

+static int unifb_enable __initdata = 1;	/* enable by default */
+module_param(unifb_enable, bool, 0);
+

No need for this, you can use fb_get_options() and -ENODEV out in the
init path then simply use the generic "off" parsing done by the fbmem
code.

+#ifdef CONFIG_PUV3_UNIGFX_HWACCEL
+static void unifb_sync(void)
+{
+	/* TODO: may, this can be replaced by interrupt */
+	int cnt;
+
+	for (cnt = 0; cnt < 0x10000000; cnt++) {
+		if (UGE_COMMAND & 0x1000000)
+			return;
+	}
+
+	if (cnt > 0x8000000)
+		printk(KERN_WARNING "Warning: UniGFX GE time out ...\n");
+}
+

In addition to switching to platform data for the I/O offsets, you can
also use dev_warn() here on the device pointer (fb_info->device).

+static struct fb_ops unifb_ops = {
+	.fb_read        = fb_sys_read,
+	.fb_write       = fb_sys_write,
+	.fb_check_var	= unifb_check_var,
+	.fb_set_par	= unifb_set_par,
+	.fb_setcolreg	= unifb_setcolreg,
+	.fb_pan_display	= unifb_pan_display,
+#ifdef CONFIG_PUV3_UNIGFX_HWACCEL
+	.fb_fillrect	= unifb_fillrect,
+	.fb_copyarea	= unifb_copyarea,
+	.fb_imageblit   = unifb_imageblit,
+#else
+	.fb_fillrect	= sys_fillrect,
+	.fb_copyarea	= sys_copyarea,
+	.fb_imageblit	= sys_imageblit,
+#endif
+	.fb_mmap	= unifb_mmap,
+};
+

There's no need for the ifdef here, all of your accel routines manually
check if acceleration is disabled or not and fall back on the sys_xxx()
paths, so it's safe to always reference them.

+static int unifb_set_par(struct fb_info *info)
+{
+	int hTotal, vTotal, hSyncStart, hSyncEnd, vSyncStart, vSyncEnd;
+	int format;
+
+#ifdef CONFIG_PUV3_PM
+	struct clk *clk_vga;
+	u32 pixclk = 0;
+	int i;
+
+	for (i = 0; i <= 10; i++) {
+		if    (info->var.xres         = unifb_modes[i].xres
+		    && info->var.yres         = unifb_modes[i].yres
+		    && info->var.upper_margin = unifb_modes[i].upper_margin
+		    && info->var.lower_margin = unifb_modes[i].lower_margin
+		    && info->var.left_margin  = unifb_modes[i].left_margin
+		    && info->var.right_margin = unifb_modes[i].right_margin
+		    && info->var.hsync_len    = unifb_modes[i].hsync_len
+		    && info->var.vsync_len    = unifb_modes[i].vsync_len) {
+			pixclk = unifb_modes[i].pixclock;
+			break;
+		}
+	}
+
+	/* set clock rate */
+	clk_vga = clk_get(NULL, "VGA_CLK");
+	i = 0;

You can pass info->device to clk_get() instead of NULL. This will make it
easier to match specific clocks and lookups per device in the future.

You'll presumably also want to check if clk_get() returned an error and
handle that accordingly.

+	if (pixclk != 0) {
+		if (clk_set_rate(clk_vga, pixclk) = 0)
+			i = 1;
+	}
+
+	if (i = 0) {           /* set clock failed */
+		info->fix = unifb_fix;
+		info->var = unifb_default;
+		clk_set_rate(clk_vga, unifb_default.pixclock);
+	}

If clk_set_rate() can fail the first time, it can fail the second time
too. You'll want more error handling here.

+	/* Truecolor has hardware independent palette */
+	if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+		u32 v;
+
+		if (regno >= 16)
+			return 1;
+
+		v = (red << info->var.red.offset) |
+		    (green << info->var.green.offset) |
+		    (blue << info->var.blue.offset) |
+		    (transp << info->var.transp.offset);
+		switch (info->var.bits_per_pixel) {
+		case 8:
+			break;
+		case 16:
+			((u32 *) (info->pseudo_palette))[regno] = v;
+			break;
+		case 24:
+		case 32:
+			((u32 *) (info->pseudo_palette))[regno] = v;
+			break;
+		}

The 16 case is no different than the 24/32 case.

The default case should also return 1 to indicate an error.

+#ifndef MODULE
+/*
+ * The virtual framebuffer driver is only enabled if explicitly
+ * requested by passing 'video=unifb:' (or any actual options).
+ */
+static int __init unifb_setup(char *options)
+{
+	char *this_opt;
+
+	unifb_enable = 0;
+
+	if (!options)
+		return 1;
+
+	unifb_enable = 1;
+
+	if (!*options)
+		return 1;
+
+	while ((this_opt = strsep(&options, ",")) != NULL) {
+		if (!*this_opt)
+			continue;
+		/* Test disable for backwards compatibility */
+		if (!strcmp(this_opt, "disable"))
+			unifb_enable = 0;
+	}
+	return 1;
+}
+#endif  /*  MODULE  */
+

Just kill all of this and use the generic "off" handling.

+static int unifb_probe(struct platform_device *dev)
+{
+	struct fb_info *info;
+	int retval = -ENOMEM;
+
+	/*
+	 * For real video cards we use ioremap.
+	 */
+	videomemory = (void *)KUSER_UNIGFX_BASE;
+
+	/*
+	 * VFB could clear memory to prevent kernel info
+	 * leakage into userspace and set black screen
+	 * VGA-based drivers MUST NOT clear memory if
+	 * they want to be able to take over vgacon
+	 */
+	/* memset(videomemory, 0, videomemorysize); */
+

This is a remnant from a vmalloc acquired framebuffer base, so can be
killed off.

+	info = framebuffer_alloc(sizeof(u32)*256, &dev->dev);
+	if (!info)
+		goto err;
+
+	info->screen_base = (char __iomem *)videomemory;

+	unifb_fix.smem_start = PKUNITY_UNIGFX_MMAP_BASE;
+	unifb_fix.smem_len = videomemorysize;
+	unifb_fix.mmio_start = PKUNITY_UNIGFX_BASE;

All of these should be passed in via your platform device resources, with
necessary remapping.

+	info->fix = unifb_fix;
+	info->pseudo_palette = info->par;
+	info->par = NULL;
+	info->flags = FBINFO_FLAG_DEFAULT;
+#ifdef CONFIG_PUV3_UNIGFX_HWACCEL
+	info->fix.accel = FB_ACCEL_PUV3_UNIGFX;
+	info->flags |= FBINFO_HWACCEL_COPYAREA | FBINFO_HWACCEL_FILLRECT;
+#else
+	info->flags |= FBINFO_HWACCEL_DISABLED;
+#endif
+

The accel flag is deprecated, so just drop it completely.

What happened to FBINFO_HWACCEL_IMAGEBLIT?

+#ifdef CONFIG_PM
+static int unifb_resume(struct platform_device *dev)
+{
+	int rc = 0;
+
+	if (dev->dev.power.power_state.event = PM_EVENT_ON)
+		return 0;
+
+	acquire_console_sem();
+
+	if (dev->dev.power.power_state.event = PM_EVENT_SUSPEND) {
+		UDE_FSA = unifb_regs[0];
+		UDE_LS  = unifb_regs[1];
+		UDE_PS  = unifb_regs[2];
+		UDE_HAT = unifb_regs[3];
+		UDE_HBT = unifb_regs[4];
+		UDE_HST = unifb_regs[5];
+		UDE_VAT = unifb_regs[6];
+		UDE_VBT = unifb_regs[7];
+		UDE_VST = unifb_regs[8];
+		UDE_CFG = unifb_regs[9];
+	}

This is probably worth abstracting through platform data.

+
+static int __init unifb_init(void)
+{
+	int ret = 0;
+
+#ifndef MODULE
+	if (fb_get_options("unifb", &unifb_option))
+		return -ENODEV;
+	unifb_setup(unifb_option);
+#endif
+
+	if (!unifb_enable)
+		return -ENXIO;
+

All of this can just be replaced with:

	if (fb_get_options("unifb", NULL))
		return -ENODEV;

+	ret = platform_driver_register(&unifb_driver);
+
+	if (!ret) {
+		unifb_device = platform_device_alloc("unifb", 0);
+
+		if (unifb_device)
+			ret = platform_device_add(unifb_device);
+		else
+			ret = -ENOMEM;
+
+		if (ret) {
+			platform_device_put(unifb_device);
+			platform_driver_unregister(&unifb_driver);
+		}
+	}
+
+	return ret;
+}
+

And if your architecture will then register a platform device with the
platform data filled out, all of this can be turned in to just a simple
platform_driver_register().

+module_init(unifb_init);
+
+#ifdef MODULE
+static void __exit unifb_exit(void)
+{
+	platform_device_unregister(unifb_device);
+	platform_driver_unregister(&unifb_driver);
+}
+
+module_exit(unifb_exit);
+
+MODULE_LICENSE("GPL v2");
+#endif

No need for the ifdef here, either.