Re: [PATCH v3 09/11] powerpc/mpc5121: shared DIU framebuffer support

On Tue, 2010-02-16 at 11:06 -0700, Grant Likely wrote:

[...]

quoted

diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index 72d68b3..29c7f31 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c

@@ -34,7 +34,7 @@
 #include <linux/of_platform.h>

 #include <sysdev/fsl_soc.h>
-#include "fsl-diu-fb.h"
+#include <linux/fsl-diu-fb.h>

 /*
 * These parameters give default parameters

@@ -178,6 +178,21 @@ static struct fb_videomode __devinitdata fsl_diu_mode_db[] = {
               .sync           = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
               .vmode          = FB_VMODE_NONINTERLACED
       },
+       {
+               .name           = "800x480-60",
+               .refresh        = 60,
+               .xres           = 800,
+               .yres           = 480,
+               .pixclock       = 31250,
+               .left_margin    = 86,
+               .right_margin   = 42,
+               .upper_margin   = 33,
+               .lower_margin   = 10,
+               .hsync_len      = 128,
+               .vsync_len      = 2,
+               .sync           = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
+               .vmode          = FB_VMODE_NONINTERLACED
+       },
 };

This hunk bothers me.  It looks like the type of data that belongs
either in some common shared .c file, or encoded into the device tree.
 It seems to be data about the display panel, instead of data about
the framebuffer driver.  I know that the driver already uses this
pattern, but before I merge this patch and further rely on that
pattern, I think it is worth discussing.

Kumar, York, thoughts?

g.

It is a hardware related configuration. It is only used during booting
before other configuration comes available.

I am OK to move it to anywhere as long as it doesn't get confused or
lost.

York