On Mon, 2007-05-07 at 16:11 +0200, Nicolas Ferre wrote:

From: Nicolas Ferre <redacted>

Adds a framebuffer driver to ATMEL AT91SAM9x and AT32 
aka AVR32 platforms. Those chips share quite the same
IP and this code is suitable for both architectures.

Signed-off-by: Nicolas Ferre <redacted>
---

+#if defined(CONFIG_ARCH_AT91)
+#define	ATMEL_LCDFB_FBINFO_DEFAULT	FBINFO_DEFAULT
+
+static inline void atmel_lcdfb_update_dma2d(struct atmel_lcdfb_info *sinfo,
+					struct fb_var_screeninfo *var)
+{
+
+}

Or

#define atmel_lcdfb_update(...) do {} while (0)

?

+
+static struct fb_fix_screeninfo atmel_lcdfb_fix __initdata = {
+	.type		= FB_TYPE_PACKED_PIXELS,
+	.visual		= FB_VISUAL_TRUECOLOR,
+	.xpanstep	= 0,
+	.ypanstep	= 0,
+	.ywrapstep	= 0,
+	.accel		= FB_ACCEL_NONE,
+};
+

This driver has fb_pan_display() which I assume works. However, you also
need to set ypanstep and/or xpanstep and/or ywrapstep to a nonzero
value, depending on the hardware/drive capability.

+static u32 pseudo_palette[16] = {
+	0x000000,
+	0xaa0000,
+	0x00aa00,
+	0xaa5500,
+	0x0000aa,
+	0xaa00aa,
+	0x00aaaa,
+	0xaaaaaa,
+	0x555555,
+	0xff5555,
+	0x55ff55,
+	0xffff55,
+	0x5555ff,
+	0xff55ff,
+	0x55ffff,
+	0xffffff
+};
+

Do you really need to pre-fill pseudo_palette[]? The contents are going
to be overwritten by the console anyway (The pseudo_palette is for
fbcon's use only).

You can also include pseudo_palette[] as part of struct
atmel_lcdfb_info, then in your probe routine:

info->pseudo_palette = par->pseudo_palette;

to reduce the size of the kernel image. 

+
+static inline u_int chan_to_field(u_int chan, const struct fb_bitfield *bf)

Might as well change u_int to u32 or unsigned int, for consistency.

+{
+	chan &= 0xffff;
+	chan >>= 16 - bf->length;
+	return chan << bf->offset;
+}
+
+
+
+
+static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo)
+{
+	struct fb_info *info = sinfo->info;
+	int ret = 0;
+
+	memset(info->screen_base, 0, info->fix.smem_len);

memset_io?

+	info->var.activate |= FB_ACTIVATE_FORCE | FB_ACTIVATE_NOW;
+
+	dev_info(info->device,
+	       "%luKiB frame buffer at %08lx (mapped at %p)\n",
+	       (unsigned long)info->fix.smem_len / 1024,
+	       (unsigned long)info->fix.smem_start,
+	       info->screen_base);
+
+	/* Allocate colormap */
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret < 0)
+		dev_err(info->device, "Alloc color map failed\n");
+
+	return ret;
+}
+

+
+
+static int __init atmel_lcdfb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fb_info *info;
+	struct atmel_lcdfb_info *sinfo;
+	struct atmel_lcdfb_info *pdata_sinfo;
+	struct resource *regs = NULL;
+	struct resource *map = NULL;
+	int ret;
+
+	dev_dbg(dev, "%s BEGIN\n", __func__);
+
+	ret = -ENOMEM;
+	info = framebuffer_alloc(sizeof(struct atmel_lcdfb_info), dev);
+	if (!info) {
+		dev_err(dev, "cannot allocate memory\n");
+		goto out;
+	}
+
+	sinfo = info->par;
+
+	if (dev->platform_data) {
+		pdata_sinfo = (struct atmel_lcdfb_info *)dev->platform_data;
+		sinfo->default_bpp = pdata_sinfo->default_bpp;
+		sinfo->default_dmacon = pdata_sinfo->default_dmacon;
+		sinfo->default_lcdcon2 = pdata_sinfo->default_lcdcon2;
+		sinfo->default_monspecs = pdata_sinfo->default_monspecs;
+		sinfo->atmel_lcdfb_power_control = pdata_sinfo->atmel_lcdfb_power_control;
+		sinfo->guard_time = pdata_sinfo->guard_time;
+	} else {
+		dev_err(dev, "cannot get default configuration\n");
+		goto out;

Wrong goto? Should be goto free_info?

+
+free_cmap:
+	fb_dealloc_cmap(&info->cmap);
+unregister_irqs:
+	free_irq(sinfo->irq_base, info);
+unmap_mmio:
+	iounmap(sinfo->mmio);
+release_mem:
+ 	release_mem_region(info->fix.mmio_start, info->fix.mmio_len);
+free_fb:
+	if (map) {
+		iounmap(info->screen_base);
+	} else {
+		atmel_lcdfb_free_video_memory(sinfo);
+	}
+
+release_intmem:
+	if (map) {
+		release_mem_region(info->fix.smem_start, info->fix.smem_len);
+	}

Unnecessary curly braces

+
+static struct platform_driver atmel_lcdfb_driver = {
+	.remove		= __exit_p(atmel_lcdfb_remove),
+	.driver		= {
+		.name	= "atmel_lcdfb",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init atmel_lcdfb_init(void)
+{
+	return platform_driver_probe(&atmel_lcdfb_driver, atmel_lcdfb_probe);
+}

Is this intentional?  Why not platform_driver_register()?

+
+static void __exit atmel_lcdfb_exit(void)
+{
+	platform_driver_unregister(&atmel_lcdfb_driver);
+}
+

Tony