Re: [PATCH] fbdev: sh_mipi_dsi: add extra settings method for platform

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tuesday 24 January 2012 01:48:55 Kuninori Morimoto wrote:
> Hi Laurent
> Cc Magnus, Guennadi
> 
> Thank you for checking patch

You're welcome.

> > > +	void (*set_dcs)(int handle,
> > > +			int (*mipi_dcs)(int handle, u8 cmd),
> > > +			int (*mipi_dcs_param)(int handle, u8 cmd, u8 param));
> > 
> > I don't think this is a good idea. First of all, we should reduce the
> > number of board code callbacks to make transition to the device tree
> > easier. Then, passing two functions to board code to read and write any
> > device register without the driver having any knowledge about that is a
> > clear violation of the device model, and will result in problems sooner
> > or later.
> > 
> > What MIPI settings do platforms need to modify ? Can those settings be
> > expressed as data in the sh_mipi_dsi_info structure instead ?
> 
> Hmm... now I'm in dilemma.
> Actually this is v2 patch for mipi display.
> (v1 patch was dropped since merge window issue)
> 
> The v1 patch was data-table style for mipi display initialization.
> (if platform had init data-table, driver used it)
> 
> But someone reviewed it and teach me that it is not-good idea.
> So, I created this style in v2.
> 
> Now platform needs "backlight ON" and "memory write ON" command,
> but I could not find these command on include/video/mipi_display.h.
> So, I thought it is platform specific command.

Why does the platform need that ? Please correct me if I'm wrong, but it seems 
to me that what you're trying to do is configure and control the display 
panel. I don't think that's platform-specific, it should be panel-specific 
instead. It looks to me like the right solution would be to write a display 
panel driver, similar to what is done for the OMAP2+ platform 
(drivers/video/omap2/displays).

-- 
Regards,

Laurent Pinchart