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

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

[Kuninori Morimoto]

Some platform needs extra MIPI settings.
This patch add support it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/sh_mipi_dsi.c |    8 ++++++++
 include/video/sh_mipi_dsi.h |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/video/sh_mipi_dsi.c b/drivers/video/sh_mipi_dsi.c
index 05151b8..1532894 100644
--- a/drivers/video/sh_mipi_dsi.c
+++ b/drivers/video/sh_mipi_dsi.c
@@ -386,6 +386,14 @@ static int __init sh_mipi_setup(struct sh_mipi *mipi,
 			  pixfmt << 4);
 	sh_mipi_dcs(ch->chan, MIPI_DCS_SET_DISPLAY_ON);
 
+	/*
+	 * extra dcs settings for platform
+	 */
+	if (pdata->set_dcs)
+		pdata->set_dcs(ch->chan,
+			       sh_mipi_dcs,
+			       sh_mipi_dcs_param);
+
 	/* Enable timeout counters */
 	iowrite32(0x00000f00, base + DSICTRL);
 
diff --git a/include/video/sh_mipi_dsi.h b/include/video/sh_mipi_dsi.h
index 434d56b..6e1f7bc 100644
--- a/include/video/sh_mipi_dsi.h
+++ b/include/video/sh_mipi_dsi.h
@@ -52,6 +52,9 @@ struct sh_mipi_dsi_info {
 	unsigned long			flags;
 	u32				clksrc;
 	unsigned int			vsynw_offset;
+	void (*set_dcs)(int handle,
+			int (*mipi_dcs)(int handle, u8 cmd),
+			int (*mipi_dcs_param)(int handle, u8 cmd, u8 param));
 	int	(*set_dot_clock)(struct platform_device *pdev,
 				 void __iomem *base,
 				 int enable);
-- 
1.7.5.4

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

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Monday 23 January 2012 06:08:34 Kuninori Morimoto wrote:
> Some platform needs extra MIPI settings.
> This patch add support it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/video/sh_mipi_dsi.c |    8 ++++++++
>  include/video/sh_mipi_dsi.h |    3 +++
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/sh_mipi_dsi.c b/drivers/video/sh_mipi_dsi.c
> index 05151b8..1532894 100644
> --- a/drivers/video/sh_mipi_dsi.c
> +++ b/drivers/video/sh_mipi_dsi.c
> @@ -386,6 +386,14 @@ static int __init sh_mipi_setup(struct sh_mipi *mipi,
>  			  pixfmt << 4);
>  	sh_mipi_dcs(ch->chan, MIPI_DCS_SET_DISPLAY_ON);
> 
> +	/*
> +	 * extra dcs settings for platform
> +	 */
> +	if (pdata->set_dcs)
> +		pdata->set_dcs(ch->chan,
> +			       sh_mipi_dcs,
> +			       sh_mipi_dcs_param);
> +
>  	/* Enable timeout counters */
>  	iowrite32(0x00000f00, base + DSICTRL);
> 
> diff --git a/include/video/sh_mipi_dsi.h b/include/video/sh_mipi_dsi.h
> index 434d56b..6e1f7bc 100644
> --- a/include/video/sh_mipi_dsi.h
> +++ b/include/video/sh_mipi_dsi.h
> @@ -52,6 +52,9 @@ struct sh_mipi_dsi_info {
>  	unsigned long			flags;
>  	u32				clksrc;
>  	unsigned int			vsynw_offset;
> +	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 ?

>  	int	(*set_dot_clock)(struct platform_device *pdev,
>  				 void __iomem *base,
>  				 int enable);

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

Hi Laurent
Cc Magnus, Guennadi

Thank you for checking patch

> > +	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.

Best regards
---
Kuninori Morimoto

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

[Laurent Pinchart]

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

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

[Magnus Damm]

On Tue, Jan 24, 2012 at 7:09 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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).

I think that it is a very good recommendation to solve this in theory,
but in practice we've never really seen anyone use the same LCD module
and/or on-glass LCD controller for another board. What I've done so
far is to write the part number of the LCD module together with LCD
controller part number and hopefully also LCD panel configuration
together with the register settings so it is possible to search and
consolidate at some point when we have multiple users.

I'm not sure about this particular case, but I've seen quite much code
that just has a set of LCD controller register settings without
documentation, and it's kind of hard to turn that into a proper
driver. Especially since these register settings are both related to
the on-module LCD controller and LCD panel that together form a LCD
module. We usually don't know which setting is for what part. And we
probably would like to abstract the module into separate pieces of
code for different parts. I've seen some data sheet of a on-module LCD
controller and it was about 800 pages I believe, so it's not exactly
something you can cook up in an afternoon. And add closed MIPI specs
and all sorts of fun and this can take forever. =)

That aside, it would be really nice to see some proper drivers for LCD modules.

Cheers,

/ magnus

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

[Laurent Pinchart]

Hi Magnus,

On Tuesday 24 January 2012 11:44:13 Magnus Damm wrote:
> On Tue, Jan 24, 2012 at 7:09 PM, Laurent Pinchart wrote:
> > 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).
> 
> I think that it is a very good recommendation to solve this in theory,
> but in practice we've never really seen anyone use the same LCD module
> and/or on-glass LCD controller for another board. What I've done so
> far is to write the part number of the LCD module together with LCD
> controller part number and hopefully also LCD panel configuration
> together with the register settings so it is possible to search and
> consolidate at some point when we have multiple users.
> 
> I'm not sure about this particular case, but I've seen quite much code
> that just has a set of LCD controller register settings without
> documentation, and it's kind of hard to turn that into a proper
> driver. Especially since these register settings are both related to
> the on-module LCD controller and LCD panel that together form a LCD
> module. We usually don't know which setting is for what part. And we
> probably would like to abstract the module into separate pieces of
> code for different parts. I've seen some data sheet of a on-module LCD
> controller and it was about 800 pages I believe, so it's not exactly
> something you can cook up in an afternoon. And add closed MIPI specs
> and all sorts of fun and this can take forever. =)

How does TI handle that for the OMAP platform ? I don't think they configure 
the LCD module/panel in board code. Maybe we should ask them if they have a 
solution, or even plans to handle the problem ? Tomi Valkeinen is probably the 
right person to ask.

> That aside, it would be really nice to see some proper drivers for LCD
> modules.

-- 
Regards,

Laurent Pinchart

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

[Kuninori Morimoto]

Hi

> How does TI handle that for the OMAP platform ? I don't think they configure 
> the LCD module/panel in board code. Maybe we should ask them if they have a 
> solution, or even plans to handle the problem ? Tomi Valkeinen is probably the 
> right person to ask.

I asked it to Tomi (off-list), and I got one solution.
I will create SuperH panel driver for it.

Thank you Laurent, Tomi.

Best regards
---
Kuninori Morimoto