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

[Grant Likely <grant.likely@secretlab.ca>]

On Fri, Feb 5, 2010 at 6:42 AM, Anatolij Gustschin <agust@denx.de> wrote:
> MPC5121 DIU configuration/setup as initialized by the boot
> loader currently will get lost while booting Linux. As a
> result displaying the boot splash is not possible through
> the boot process.
>
> To prevent this we reserve configured DIU frame buffer
> address range while booting and preserve AOI descriptor
> and gamma table so that DIU continues displaying through
> the whole boot process. On first open from user space
> DIU frame buffer driver releases the reserved frame
> buffer area and continues to operate as usual.
>
> The patch also moves drivers/video/fsl-diu-fb.h file to
> include/linux as we use some DIU structures in platform
> code.
>
> 'diu_ops' callbacks in platform code borrowed from John's
> DIU code.
>
> Signed-off-by: John Rigby <jrigby@gmail.com>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
[...]
> 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 @@
> =A0#include <linux/of_platform.h>
>
> =A0#include <sysdev/fsl_soc.h>
> -#include "fsl-diu-fb.h"
> +#include <linux/fsl-diu-fb.h>
>
> =A0/*
> =A0* These parameters give default parameters
> @@ -178,6 +178,21 @@ static struct fb_videomode __devinitdata fsl_diu_mod=
e_db[] =3D {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.sync =A0 =A0 =A0 =A0 =A0 =3D FB_SYNC_COMP=
_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.vmode =A0 =A0 =A0 =A0 =A0=3D FB_VMODE_NON=
INTERLACED
> =A0 =A0 =A0 =A0},
> + =A0 =A0 =A0 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .name =A0 =A0 =A0 =A0 =A0 =3D "800x480-60",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .refresh =A0 =A0 =A0 =A0=3D 60,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .xres =A0 =A0 =A0 =A0 =A0 =3D 800,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .yres =A0 =A0 =A0 =A0 =A0 =3D 480,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .pixclock =A0 =A0 =A0 =3D 31250,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .left_margin =A0 =A0=3D 86,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .right_margin =A0 =3D 42,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .upper_margin =A0 =3D 33,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .lower_margin =A0 =3D 10,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .hsync_len =A0 =A0 =A0=3D 128,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .vsync_len =A0 =A0 =A0=3D 2,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .sync =A0 =A0 =A0 =A0 =A0 =3D FB_SYNC_COMP_=
HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .vmode =A0 =A0 =A0 =A0 =A0=3D FB_VMODE_NONI=
NTERLACED
> + =A0 =A0 =A0 },
> =A0};

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.