Re: [Linux-fbdev-devel] [PATCH 02/20] omapfb: Add support for MIPI-DCS compatible LCDs

[Imre Deak <imre.deak@nokia.com>]

Hi,

On Mon, Jun 08, 2009 at 12:43:24AM +0200, ext Krzysztof Helt wrote:
> On Thu,  4 Jun 2009 20:52:27 +0300
> Imre Deak <imre.deak@nokia.com> wrote:
>[...]
> > +
> > +#define to_mipid_device(p)		container_of(p, struct mipid_device, \
> > +						panel)
> > +struct mipid_device {
> > +	int		enabled;
> > +	int		model;
> 
> This one is only set and never read. A name is probably enough.

Ok, I'll remove model.

> 
> > +	int		revision;
> > +	u8		display_id[3];
> 
> This one should be a local variable.

Ok, I'll move it to the func where it's used.

> 
> > +	unsigned int	saved_bklight_level;
> > +	unsigned long	hw_guard_end;		/* next value of jiffies
> > +						   when we can issue the
> > +						   next sleep in/out command */
> > +	unsigned long	hw_guard_wait;		/* max guard time in jiffies */
> > +
> > +	struct omapfb_device	*fbdev;
> > +	struct spi_device	*spi;
> > +	struct mutex		mutex;
> > +	struct lcd_panel	panel;
> 
> How does it differ from fbdev->panel? Is it duplicated field?

fbdev->panel is a pointer to this device instance specific data. It's embedded here
so that we can get to struct mipid_device with the container_of macro when fbdev->panel
is passed to us.

> 
> I am sorry but I had not enough time to review the rest.

Thanks for the review, if there is nothing else I can post a new version with the
above changes.

--Imre