| Message ID | 87vctmt9t1.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)