On Sat, Jan 22, 2011 at 06:29:45PM +0800, Guan Xuetao wrote:
> diff --git a/drivers/video/puv3_fb.c b/drivers/video/puv3_fb.c
> new file mode 100644
> index 0000000..d938a73
> --- /dev/null
> +++ b/drivers/video/puv3_fb.c
[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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
