[PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for platform

[PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for platform

[Kuninori Morimoto]

some platform needs extra MIPI dcs to use.
this patch add support it.

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

diff --git a/drivers/video/sh_mipi_dsi.c b/drivers/video/sh_mipi_dsi.c
index 096c76a..bdf687d 100644
--- a/drivers/video/sh_mipi_dsi.c
+++ b/drivers/video/sh_mipi_dsi.c
@@ -46,6 +46,13 @@
 /* E.g., sh7372 has 2 MIPI-DSIs - one for each LCDC */
 #define MAX_SH_MIPI_DSI 2
 
+/*
+ * for extra array
+ * see sh_mipi_dsi.h
+ */
+#define GET_EXCMD(p)	((p >> 8) & 0xFF)
+#define GET_EXPARAM(p)	(p & 0xFF)
+
 struct sh_mipi {
 	void __iomem	*base;
 	void __iomem	*linkbase;
@@ -150,8 +157,9 @@ static int __init sh_mipi_setup(struct sh_mipi *mipi,
 {
 	void __iomem *base = mipi->base;
 	struct sh_mobile_lcdc_chan_cfg *ch = pdata->lcd_chan;
-	u32 pctype, datatype, pixfmt, linelength, vmctr2 = 0x00e00000;
+	u32 pctype, datatype, pixfmt, linelength, vmctr2 = 0x00e00000, excmd;
 	bool yuv;
+	int i;
 
 	/*
 	 * Select data format. MIPI DSI is not hot-pluggable, so, we just use
@@ -355,6 +363,20 @@ static int __init sh_mipi_setup(struct sh_mipi *mipi,
 			  pixfmt << 4);
 	sh_mipi_dcs(ch->chan, MIPI_DCS_SET_DISPLAY_ON);
 
+	/*
+	 * FIXME
+	 *
+	 * extra dcs settings for platform
+	 */
+	for (i = 0; i < pdata->extra_array_len; i++) {
+		excmd = pdata->extra_array[i];
+		if (0x10000000 & excmd)
+			sh_mipi_dcs(ch->chan, GET_EXCMD(excmd));
+		else
+			sh_mipi_dcs_param(ch->chan, GET_EXCMD(excmd),
+						    GET_EXPARAM(excmd));
+	}
+
 	return 0;
 }
 
diff --git a/include/video/sh_mipi_dsi.h b/include/video/sh_mipi_dsi.h
index 58b78f8..e52097a 100644
--- a/include/video/sh_mipi_dsi.h
+++ b/include/video/sh_mipi_dsi.h
@@ -32,12 +32,25 @@ struct sh_mobile_lcdc_chan_cfg;
 #define SH_MIPI_DSI_HFPBM	(1 << 2)
 #define SH_MIPI_DSI_BL2E	(1 << 3)
 
+/*
+ * x000ccpp
+ *
+ * x : 1 use sh_mipi_dcs()
+ *     0 use sh_mipi_dcs_param()
+ * cc : cmd
+ * pp : param
+ */
+#define SH_MIPI_DCS(cmd)		(0x10000000 | (cmd & 0xFF) << 8)
+#define SH_MIPI_DCS_PARAM(cmd, param)	((cmd & 0xFF) << 8 | (param & 0xFF))
+
 struct sh_mipi_dsi_info {
 	enum sh_mipi_dsi_data_fmt	data_format;
 	struct sh_mobile_lcdc_chan_cfg	*lcd_chan;
 	unsigned long			flags;
 	u32				clksrc;
 	unsigned int			vsynw_offset;
+	u32				*extra_array;
+	int				extra_array_len;
 };
 
 #endif
-- 
1.7.4.1

Re: [PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for platform

[Magnus Damm]

On Thu, Aug 25, 2011 at 1:05 PM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> some platform needs extra MIPI dcs to use.
> this patch add support it.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/video/sh_mipi_dsi.c |   24 +++++++++++++++++++++++-
>  include/video/sh_mipi_dsi.h |   13 +++++++++++++
>  2 files changed, 36 insertions(+), 1 deletions(-)

[snip]

> --- a/include/video/sh_mipi_dsi.h
> +++ b/include/video/sh_mipi_dsi.h
> @@ -32,12 +32,25 @@ struct sh_mobile_lcdc_chan_cfg;
>  #define SH_MIPI_DSI_HFPBM      (1 << 2)
>  #define SH_MIPI_DSI_BL2E       (1 << 3)
>
> +/*
> + * x000ccpp
> + *
> + * x : 1 use sh_mipi_dcs()
> + *     0 use sh_mipi_dcs_param()
> + * cc : cmd
> + * pp : param
> + */
> +#define SH_MIPI_DCS(cmd)               (0x10000000 | (cmd & 0xFF) << 8)
> +#define SH_MIPI_DCS_PARAM(cmd, param)  ((cmd & 0xFF) << 8 | (param & 0xFF))
> +
>  struct sh_mipi_dsi_info {
>        enum sh_mipi_dsi_data_fmt       data_format;
>        struct sh_mobile_lcdc_chan_cfg  *lcd_chan;
>        unsigned long                   flags;
>        u32                             clksrc;
>        unsigned int                    vsynw_offset;
> +       u32                             *extra_array;
> +       int                             extra_array_len;
>  };
>
>  #endif

Hi Morimoto-san,

Thanks for adding support for MIPI-DSI on the Kota2 board. If I
understand correctly then this patch allows us to use board-specific
code to setup the MIPI-DSI panel. And this is needed on the Kota2
board.

I do however wonder if this array is the best approach to adding board
specific code. My personal take would be to use something like the SYS
bus support for the LCDC, see callbacks in struct
sh_mobile_lcdc_sys_bus_ops located in
include/video/sh_mobile_lcdc.h and board/panel code like
arch/sh/boards/mach-migor/lcd_qvga.c.

Also, if your board/panel specific code is dealing with backlight on
MIPI-DSI, perhaps there is a chance this is generic enough to be
broken out and reused somehow.

Maybe Paul and/or Guennadi has some ideas about this too.

Thanks,

/ magnus

Re: [PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for

[Guennadi Liakhovetski]

On Mon, 29 Aug 2011, Magnus Damm wrote:

> On Thu, Aug 25, 2011 at 1:05 PM, Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> > some platform needs extra MIPI dcs to use.
> > this patch add support it.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> >  drivers/video/sh_mipi_dsi.c |   24 +++++++++++++++++++++++-
> >  include/video/sh_mipi_dsi.h |   13 +++++++++++++
> >  2 files changed, 36 insertions(+), 1 deletions(-)
> 
> [snip]
> 
> > --- a/include/video/sh_mipi_dsi.h
> > +++ b/include/video/sh_mipi_dsi.h
> > @@ -32,12 +32,25 @@ struct sh_mobile_lcdc_chan_cfg;
> >  #define SH_MIPI_DSI_HFPBM      (1 << 2)
> >  #define SH_MIPI_DSI_BL2E       (1 << 3)
> >
> > +/*
> > + * x000ccpp
> > + *
> > + * x : 1 use sh_mipi_dcs()
> > + *     0 use sh_mipi_dcs_param()
> > + * cc : cmd
> > + * pp : param
> > + */
> > +#define SH_MIPI_DCS(cmd)               (0x10000000 | (cmd & 0xFF) << 8)
> > +#define SH_MIPI_DCS_PARAM(cmd, param)  ((cmd & 0xFF) << 8 | (param & 0xFF))
> > +
> >  struct sh_mipi_dsi_info {
> >        enum sh_mipi_dsi_data_fmt       data_format;
> >        struct sh_mobile_lcdc_chan_cfg  *lcd_chan;
> >        unsigned long                   flags;
> >        u32                             clksrc;
> >        unsigned int                    vsynw_offset;
> > +       u32                             *extra_array;
> > +       int                             extra_array_len;
> >  };
> >
> >  #endif
> 
> Hi Morimoto-san,
> 
> Thanks for adding support for MIPI-DSI on the Kota2 board. If I
> understand correctly then this patch allows us to use board-specific
> code to setup the MIPI-DSI panel. And this is needed on the Kota2
> board.
> 
> I do however wonder if this array is the best approach to adding board
> specific code. My personal take would be to use something like the SYS
> bus support for the LCDC, see callbacks in struct
> sh_mobile_lcdc_sys_bus_ops located in
> include/video/sh_mobile_lcdc.h and board/panel code like
> arch/sh/boards/mach-migor/lcd_qvga.c.

I don't like that array either, but if we want to do this _right_, 
callbacks wouldn't be my preference either. I'm not familiar with the set 
up: are those values wpecific to the LCD panel or to the board? If it's 
the LCD panel, then ideally one would want a display driver for them. 
Backlight controlling would want a proper driver too.

> Also, if your board/panel specific code is dealing with backlight on
> MIPI-DSI, perhaps there is a chance this is generic enough to be
> broken out and reused somehow.

Exactly, however, lately I'me coming to an unfortunate conclusion, that 
due to a vast variety of embedded systems and their components, many 
hardware blocks will ever be only used on a single embedded platform, 
running Linux, and only be exposed to a handful of users. So, creating and 
maintaining a proper driver for each such block has a good chance to be 
considered an overkill...

> Maybe Paul and/or Guennadi has some ideas about this too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for platform

[Paul Mundt]

On Mon, Aug 29, 2011 at 10:45:37AM +0200, Guennadi Liakhovetski wrote:
> On Mon, 29 Aug 2011, Magnus Damm wrote:
> > I do however wonder if this array is the best approach to adding board
> > specific code. My personal take would be to use something like the SYS
> > bus support for the LCDC, see callbacks in struct
> > sh_mobile_lcdc_sys_bus_ops located in
> > include/video/sh_mobile_lcdc.h and board/panel code like
> > arch/sh/boards/mach-migor/lcd_qvga.c.
> 
> I don't like that array either, but if we want to do this _right_, 
> callbacks wouldn't be my preference either. I'm not familiar with the set 
> up: are those values wpecific to the LCD panel or to the board? If it's 
> the LCD panel, then ideally one would want a display driver for them. 
> Backlight controlling would want a proper driver too.
> 
> > Also, if your board/panel specific code is dealing with backlight on
> > MIPI-DSI, perhaps there is a chance this is generic enough to be
> > broken out and reused somehow.
> 
> Exactly, however, lately I'me coming to an unfortunate conclusion, that 
> due to a vast variety of embedded systems and their components, many 
> hardware blocks will ever be only used on a single embedded platform, 
> running Linux, and only be exposed to a handful of users. So, creating and 
> maintaining a proper driver for each such block has a good chance to be 
> considered an overkill...
> 
That's true, but I'm not sure I see how that matters for the case at
hand. MIPI is hardly SH-Mobile specific, it just happens to be the place
where a lot of the work is being done for the moment (OMAP DSS being
another). There is already a lot of duplication between the different
MIPI users in-tree today, and it's reasonable to expect that sooner or
later there will be some consolidation effort.

Given that all of the SH-Mobile parts are also using it going forward (at
least for the moment) it's certainly worth trying to get right. Throwing
on some opaque "extras" pointer at the end of the structure is just
asking for abuse, particularly if the only purpose it's serving is more
or less common functionality we can expect to see on upcoming platforms.

While the SYS bus bits were pretty much stillborn, this wasn't so much a
problem of the abstraction (or lack thereof -- and I'm certainly not
endorsing the SYS bus abstraction, either) as general timing. At the time
most of that work happened it was still playing catch up to support a
platform that would be obsolete by the time support was sufficiently far
enough for anyone to make use of it. The present line of SH-Mobile CPUs
don't really suffer from the same sort of knee-jerk IP block scizophrenia
usually associated with the device team (even if it seems like we're
sometimes suffering from CPU or board of the week syndrome), so I would
certainly lean towards doing it right, even if it's going to take a bit
longer to hash out the abstraction.

Your comments about a proper backlight driver and so on are accurate as
well, and this is something we were also discussing with the SYS bus
bits, it simply wasn't bothered with since the platform was basically
abandoned before anyone really cared.