Re: [Linux-fbdev-devel] [PATCH] atmel_lcdfb: AT91/AT32 LCD Controller framebuffer driver

On Mon, 2007-05-07 at 16:11 +0200, Nicolas Ferre wrote:
From: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>

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 <nicolas.ferre@xxxxxxxxxxxxx>
---

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/