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

From: York Sun <yorksun@freescale.com>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: wd@denx.de, dzu@denx.de, John Rigby <jrigby@gmail.com>,
	linuxppc-dev@ozlabs.org, Anatolij Gustschin <agust@denx.de>
Subject: Re: [PATCH v3 09/11] powerpc/mpc5121: shared DIU framebuffer support
Date: Thu, 18 Feb 2010 10:15:45 -0600	[thread overview]
Message-ID: <1266509745.25353.16.camel@oslab-l1> (raw)
In-Reply-To: <fa686aa41002161006l3b301febt94fe990df4bddfe9@mail.gmail.com>

On Tue, 2010-02-16 at 11:06 -0700, Grant Likely wrote:
> [...]
> > 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