[linux-cirrus] Re: monochrome framebuffer driver
- From: Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
- To: linux-cirrus@xxxxxxxxxxxxx
- Date: Thu, 27 Jul 2006 09:53:37 +0200
On Thu, Jul 27, 2006 at 08:42:10AM +0200, Michael Burian wrote:
> The driver could need an initial review on kernel-mentors and
> linux-fbdev-devel afterwards.
From an initial quick scan:
- Like the entire Cirrus code base, the fb driver suffers from overuse
of config symbols. There's no excuse for having to specify, or even
being able to specify, a default depth in .config, nor to have
something like CONFIG_FB_LCD_EP93XX (why?)
- Don't use printascii()
- You have two of these:
59 int __init ep93xxfb_init(void);
[...]
64 int __init ep93xxfb_init(void);
- There's "TBD" comments here and there, those need to be addressed.
- These descriptions need to be changed (I wonder if you need the
descriptions at all, since that means replicating them in every
single fb driver.)
* xxxfb_setcolreg - Optional function. Sets a color register.
- Crap like this has to go. If you need a per-board hook, make the fb
driver a platform device, or something along those lines. (It also
has to go because the ADS Sphere code isn't in mainline, and we don't
generally merge hooks for stuff that isn't in mainline.)
268 #ifdef CONFIG_MACH_ADSSPHERE
269 void adssphere_blank(int blank_mode);
270
271 adssphere_blank(blank_mode);
272 #endif
- The comment above ep9312fb_map_video_memory() is wrong -- AFAIK the
ep93xx raster unit doesn't keep palette entries in RAM?
- The function ep9312fb_map_video_memory() is misnamed, as this driver
also works on the ep9315.
- Why are we using a non-cached mapping for video memory?
- RasterSetLocked has to be renamed to something else.
- Don't use writel() and friends, they are for PCI accesses only.
- Fix this:
748 /* hard code, since we don't support modedb & friends yet */
749 ep93xxfb_set_var(&info.var, -1, &info);
- Aren't we leaking the cmap and the video memory allocation in the fail
path here?
741 /* Allocate and map framebuffer memory in system DRAM */
742 if (ep9312fb_map_video_memory() != 0)
743 return -ENOMEM;
744
745 /* This has to been done !!! */
746 fb_alloc_cmap(&info.cmap, MAX_PALETTE_NUM_ENTRIES, 0);
747
748 /* hard code, since we don't support modedb & friends yet */
749 ep93xxfb_set_var(&info.var, -1, &info);
750
751 if (register_framebuffer(&info) < 0)
752 return -EINVAL;
- Argh. This is just a total mess.
62 #if defined (CONFIG_FB_LCD_EP93XX)
63 #define DEFAULT_MODE 1
64 #elif defined (CONFIG_FB_LCD_EP93XX_SHARP_LQ64D343)
65 #define DEFAULT_MODE 2
66 #elif defined (CONFIG_FB_CX25871)
67 #define DEFAULT_MODE 3
68 #elif defined (CONFIG_FB_CRT_EP93XX)
69 #define DEFAULT_MODE 0
70 #elif defined (CONFIG_FB_LCD_EP93XX_SHARP)
71 #define DEFAULT_MODE 5
72 #define CONFIG_FB_LCD_EP93XX 1 /*hack!!! FIXME: put whole
display support into different files */
73 #elif defined (CONFIG_FB_LCD_TX09D50VM1CCA)
74 #define DEFAULT_MODE 6
75 #else
76 #error What Display Setting was that!!!
77 #endif
- The PLL/VCLK divider related stuff isn't handled at all, heh. The
clock generation is what makes dealing with the ep93xx raster unit,
uhm, fun. Instead, the current code just hardcodes VDIV to depend on
PLL1, which means that it just won't work on boards that run PLL1 at
a different rate, nor will it work when we implement processor clock
scaling.
My general impression is that the driver was written to work on one
specific board with one specific resolution and one specific display
only, and that the author didn't really have extensibility in mind.
While that is okay, it doesn't warrant being merged as 'the ep93xx
framebuffer driver', since in its current state, it seems useful only
in a small subset of ep93xx configurations.
cheers,
Lennert
Other related posts:
