[PATCH] video: SuperH Mobile LCDC Driver

[PATCH] video: SuperH Mobile LCDC Driver

[Magnus Damm]

This patch adds frame buffer support for the LCDC block found in
SuperH Mobile processors. The hardware supports up to two LCD panels
per LCDC block, and both RGB and SYS interfaces can be used to hook
up LCD panels/modules.

The device driver is a regular platform driver, so LCD configuration
and board specific hooks are passed to the driver using platform data.
LCD modules using SYS interface often require special configuration
using the SYS bus, and to solve this cleanly the driver provides SYS
interface operations to the board code.

Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
WVGA panels using RGB16 and RGB18 interfaces.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 drivers/video/Kconfig            |   10 
 drivers/video/Makefile           |    2 
 drivers/video/sh_mobile_lcdcfb.c |  742 ++++++++++++++++++++++++++++++++++++++
 include/asm-sh/sh_mobile_lcdc.h  |   66 +++
 4 files changed, 820 insertions(+)

--- 0001/drivers/video/Kconfig
+++ work/drivers/video/Kconfig	2008-05-29 16:06:54.000000000 +0900
@@ -1839,6 +1839,16 @@ config FB_W100
 
 	  If unsure, say N.
 
+config FB_SH_MOBILE_LCDC
+	tristate "SuperH Mobile LCDC framebuffer support"
+	depends on FB && SUPERH
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	default m
+	---help---
+	  Frame buffer driver for the on-chip SH-Mobile LCD controller.
+
 config FB_S3C2410
 	tristate "S3C2410 LCD framebuffer support"
 	depends on FB && ARCH_S3C2410
--- 0001/drivers/video/Makefile
+++ work/drivers/video/Makefile	2008-05-29 16:06:54.000000000 +0900
@@ -115,6 +115,8 @@ obj-$(CONFIG_FB_IBM_GXT4500)	  += gxt450
 obj-$(CONFIG_FB_PS3)		  += ps3fb.o
 obj-$(CONFIG_FB_SM501)            += sm501fb.o
 obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
+obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
+obj-$(CONFIG_FB_SH7343VOU)	  += sh7343_voufb.o
 obj-$(CONFIG_FB_OMAP)             += omap/
 obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
 
--- /dev/null
+++ work/drivers/video/sh_mobile_lcdcfb.c	2008-05-29 17:21:48.000000000 +0900
@@ -0,0 +1,742 @@
+/*
+ * SuperH Mobile LCDC Framebuffer
+ *
+ * Copyright (c) 2008 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/mm.h>
+#include <linux/fb.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <asm/sh_mobile_lcdc.h>
+
+#define PALETTE_NR 16
+
+struct sh_mobile_lcdc_priv;
+struct sh_mobile_lcdc_chan {
+	struct sh_mobile_lcdc_priv *lcdc;
+	unsigned long *reg_offs;
+	unsigned long ldmt1r_value;
+	unsigned long enabled; /* ME and SE in LDCNT2R */
+	struct sh_mobile_lcdc_chan_cfg cfg;
+	u32 pseudo_palette[PALETTE_NR];
+	struct fb_info info;
+	dma_addr_t dma_handle;
+};
+
+struct sh_mobile_lcdc_priv {
+	void __iomem *base;
+	struct clk *clk;
+	unsigned long lddckr;
+	struct sh_mobile_lcdc_chan ch[2];
+};
+
+/* shared registers */
+
+#define _LDDCKR 0x410
+#define _LDDCKSTPR 0x414
+#define _LDINTR 0x468
+#define _LDSR 0x46c
+#define _LDCNT1R 0x470
+#define _LDCNT2R 0x474
+#define _LDDDSR 0x47c
+#define _LDDWD0R 0x800
+#define _LDDRDR 0x840
+#define _LDDWAR 0x900
+#define _LDDRAR 0x904
+
+/* per-channel registers */
+
+enum { LDDCKPAT1R, LDDCKPAT2R, LDMT1R, LDMT2R, LDMT3R, LDDFR, LDSM1R,
+       LDSA1R, LDMLSR, LDHCNR, LDHSYNR, LDVLNR, LDVSYNR, LDPMR };
+
+static unsigned long lcdc_offs_mainlcd[] = {
+	[LDDCKPAT1R] = 0x400,
+	[LDDCKPAT2R] = 0x404,
+	[LDMT1R] = 0x418,
+	[LDMT2R] = 0x41c,
+	[LDMT3R] = 0x420,
+	[LDDFR] = 0x424,
+	[LDSM1R] = 0x428,
+	[LDSA1R] = 0x430,
+	[LDMLSR] = 0x438,
+	[LDHCNR] = 0x448,
+	[LDHSYNR] = 0x44c,
+	[LDVLNR] = 0x450,
+	[LDVSYNR] = 0x454,
+	[LDPMR] = 0x460,
+};
+
+static unsigned long lcdc_offs_sublcd[] = {
+	[LDDCKPAT1R] = 0x408,
+	[LDDCKPAT2R] = 0x40c,
+	[LDMT1R] = 0x600,
+	[LDMT2R] = 0x604,
+	[LDMT3R] = 0x608,
+	[LDDFR] = 0x60c,
+	[LDSM1R] = 0x610,
+	[LDSA1R] = 0x618,
+	[LDMLSR] = 0x620,
+	[LDHCNR] = 0x624,
+	[LDHSYNR] = 0x628,
+	[LDVLNR] = 0x62c,
+	[LDVSYNR] = 0x630,
+	[LDPMR] = 0x63c,
+};
+
+#define START_LCDC	0x00000001
+#define LCDC_RESET	0x00000100
+#define DISPLAY_BEU	0x00000008
+#define LCDC_ENABLE	0x00000001
+
+static void lcdc_write_chan(struct sh_mobile_lcdc_chan *chan,
+			    int reg_nr, unsigned long data)
+{
+	iowrite32(data, chan->lcdc->base + chan->reg_offs[reg_nr]);
+}
+
+static unsigned long lcdc_read_chan(struct sh_mobile_lcdc_chan *chan,
+				    int reg_nr)
+{
+	return ioread32(chan->lcdc->base + chan->reg_offs[reg_nr]);
+}
+
+static void lcdc_write(struct sh_mobile_lcdc_priv *priv,
+		       int reg_offs, unsigned long data)
+{
+	iowrite32(data, priv->base + reg_offs);
+}
+
+static unsigned long lcdc_read(struct sh_mobile_lcdc_priv *priv,
+			       unsigned long reg_offs)
+{
+	return ioread32(priv->base + reg_offs);
+}
+
+static void lcdc_sys_write_index(void *handle, unsigned long data)
+{
+	struct sh_mobile_lcdc_chan *ch = handle;
+
+	lcdc_write(ch->lcdc, _LDDWD0R, data | 0x10000000);
+
+	while (lcdc_read(ch->lcdc, _LDSR) & 2)
+	  ;
+
+	if (ch->cfg.chan == LCDC_CHAN_SUBLCD)
+		lcdc_write(ch->lcdc, _LDDWAR, 3);
+	else
+		lcdc_write(ch->lcdc, _LDDWAR, 1);
+}
+
+static void lcdc_sys_write_data(void *handle, unsigned long data)
+{
+	struct sh_mobile_lcdc_chan *ch = handle;
+
+	lcdc_write(ch->lcdc, _LDDWD0R, data | 0x11000000);
+
+	while (lcdc_read(ch->lcdc, _LDSR) & 2)
+	  ;
+
+	if (ch->cfg.chan == LCDC_CHAN_SUBLCD)
+		lcdc_write(ch->lcdc, _LDDWAR, 3);
+	else
+		lcdc_write(ch->lcdc, _LDDWAR, 1);
+}
+
+static unsigned long lcdc_sys_read_data(void *handle)
+{
+	struct sh_mobile_lcdc_chan *ch = handle;
+
+	lcdc_write(ch->lcdc, _LDDRDR, 0x01000000);
+
+	while (lcdc_read(ch->lcdc, _LDSR) & 2)
+	  ;
+
+	if (ch->cfg.chan == LCDC_CHAN_SUBLCD)
+		lcdc_write(ch->lcdc, _LDDRAR, 3);
+	else
+		lcdc_write(ch->lcdc, _LDDRAR, 1);
+
+	udelay(1);
+
+	return lcdc_read(ch->lcdc, _LDDRDR) & 0xffff;
+}
+
+struct sh_mobile_lcdc_sys_bus_ops sh_mobile_lcdc_sys_bus_ops = {
+	lcdc_sys_write_index,
+	lcdc_sys_write_data,
+	lcdc_sys_read_data,
+};
+
+static int sh_mobile_lcdc_start_hw(struct sh_mobile_lcdc_priv *priv)
+{
+	struct sh_mobile_lcdc_chan *ch;
+	struct fb_videomode *lcd_cfg;
+	struct sh_mobile_lcdc_board_cfg	*board_cfg;
+	unsigned long tmp;
+	int k, m;
+	int ret = 0;
+
+	/* reset */
+	lcdc_write(priv, _LDCNT2R, lcdc_read(priv, _LDCNT2R) | LCDC_RESET);
+	while (lcdc_read(priv, _LDCNT2R) & LCDC_RESET)
+		;
+
+	/* enable LCDC channels */
+	tmp = lcdc_read(priv, _LDCNT2R);
+	tmp |= priv->ch[0].enabled;
+	tmp |= priv->ch[1].enabled;
+	lcdc_write(priv, _LDCNT2R, tmp);
+
+	/* read data from external memory, avoid using the BEU for now */
+	lcdc_write(priv, _LDCNT2R, lcdc_read(priv, _LDCNT2R) & ~DISPLAY_BEU);
+
+	/* stops the lcdc operation */
+	lcdc_write(priv, _LDCNT2R, lcdc_read(priv, _LDCNT2R) & ~START_LCDC);
+
+	/* wait until power is stopped on all channels */
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++)
+		if (lcdc_read(priv, _LDCNT2R) & priv->ch[k].enabled)
+			while (lcdc_read_chan(&priv->ch[k], LDPMR) & 3)
+				;
+
+	/* stop dotclock */
+	lcdc_write(priv, _LDDCKSTPR, 1);
+
+	/* configure clocks */
+	tmp = priv->lddckr;
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
+		ch = &priv->ch[k];
+
+		if (!priv->ch[k].enabled)
+			continue;
+
+		m = ch->cfg.clock_divider;
+		if (!m)
+			continue;
+
+		if (m == 1)
+			m = 1 << 6;
+		tmp |= m << ((ch->cfg.chan == LCDC_CHAN_SUBLCD) ? 8 : 0);
+
+		lcdc_write_chan(ch, LDDCKPAT1R, 0x00000000);
+		lcdc_write_chan(ch, LDDCKPAT2R, (1 << (m/2)) - 1);
+	}
+
+	lcdc_write(priv, _LDDCKR, tmp);
+
+	/* start dotclock again */
+	lcdc_write(priv, _LDDCKSTPR, 0);
+
+	while (lcdc_read(priv, _LDDCKSTPR))
+		;
+
+	/* interrupts are disabled */
+	lcdc_write(priv, _LDINTR, 0);
+
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
+		ch = &priv->ch[k];
+		lcd_cfg = &ch->cfg.lcd_cfg;
+
+		if (!ch->enabled)
+			continue;
+
+		tmp = ch->ldmt1r_value;
+		tmp |= (lcd_cfg->sync & FB_SYNC_VERT_HIGH_ACT) ? 0 : 1 << 28;
+		tmp |= (lcd_cfg->sync & FB_SYNC_HOR_HIGH_ACT) ? 0 : 1 << 27;
+		lcdc_write_chan(ch, LDMT1R, tmp);
+
+		/* setup SYS bus */
+		lcdc_write_chan(ch, LDMT2R, ch->cfg.sys_bus_cfg.ldmt2r);
+		lcdc_write_chan(ch, LDMT3R, ch->cfg.sys_bus_cfg.ldmt3r);
+
+		/* horizontal configuration */
+		tmp = lcd_cfg->xres + lcd_cfg->hsync_len;
+		tmp += lcd_cfg->left_margin;
+		tmp += lcd_cfg->right_margin;
+		tmp /= 8; /* HTCN */
+		tmp |= (lcd_cfg->xres / 8) << 16; /* HDCN */
+		lcdc_write_chan(ch, LDHCNR, tmp);
+
+		tmp = lcd_cfg->xres;
+		tmp += lcd_cfg->right_margin;
+		tmp /= 8; /* HSYNP */
+		tmp |= (lcd_cfg->hsync_len / 8) << 16; /* HSYNW */
+		lcdc_write_chan(ch, LDHSYNR, tmp);
+
+		/* power supply */
+		lcdc_write_chan(ch, LDPMR, 0);
+
+		/* vertical configuration */
+		tmp = lcd_cfg->yres + lcd_cfg->vsync_len;
+		tmp += lcd_cfg->upper_margin;
+		tmp += lcd_cfg->lower_margin; /* VTLN */
+		tmp |= lcd_cfg->yres << 16; /* VDLN */
+		lcdc_write_chan(ch, LDVLNR, tmp);
+
+		tmp = lcd_cfg->yres;
+		tmp += lcd_cfg->lower_margin; /* VSYNP */
+		tmp |= lcd_cfg->vsync_len << 16; /* VSYNW */
+		lcdc_write_chan(ch, LDVSYNR, tmp);
+
+		board_cfg = &ch->cfg.board_cfg;
+		if (board_cfg->setup_sys)
+			ret = board_cfg->setup_sys(board_cfg->board_data, ch,
+						   &sh_mobile_lcdc_sys_bus_ops);
+		if (ret)
+			return ret;
+	}
+
+	/* --- display_lcdc_data() --- */
+	lcdc_write(priv, _LDINTR, 0x00000f00);
+
+	/* word and long word swap */
+	lcdc_write(priv, _LDDDSR, lcdc_read(priv, _LDDDSR) | 6);
+
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
+		ch = &priv->ch[k];
+
+		if (!priv->ch[k].enabled)
+			continue;
+
+		/* set bpp format in PKF[4:0] */
+		tmp = lcdc_read_chan(ch, LDDFR);
+		tmp &= ~(0x0001001f);
+		tmp |= (priv->ch[k].info.var.bits_per_pixel == 16) ? 3 : 0;
+		lcdc_write_chan(ch, LDDFR, tmp);
+
+		/* point out our frame buffer */
+		lcdc_write_chan(ch, LDSA1R, ch->info.fix.smem_start);
+
+		/* set line size */
+		lcdc_write_chan(ch, LDMLSR, ch->info.fix.line_length);
+
+		/* continuous read mode */
+		lcdc_write_chan(ch, LDSM1R, 0);
+	}
+
+	/* display output */
+	lcdc_write(priv, _LDCNT1R, LCDC_ENABLE);
+
+	/* start the lcdc operation */
+	lcdc_write(priv, _LDCNT2R, lcdc_read(priv, _LDCNT2R) | START_LCDC);
+
+	/* wait until power is applied to all channels */
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++)
+		if (lcdc_read(priv, _LDCNT2R) & priv->ch[k].enabled)
+			while (!(lcdc_read_chan(&priv->ch[k], LDPMR) & 3))
+				;
+
+	/* tell the board code to enable the panel */
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
+		ch = &priv->ch[k];
+		board_cfg = &ch->cfg.board_cfg;
+		if (board_cfg->display_on)
+			board_cfg->display_on(board_cfg->board_data);
+	}
+
+	return 0;
+}
+
+static void sh_mobile_lcdc_stop_hw(struct sh_mobile_lcdc_priv *priv)
+{
+	struct sh_mobile_lcdc_chan *ch;
+	struct sh_mobile_lcdc_board_cfg	*board_cfg;
+	int k;
+
+	/* tell the board code to disable the panel */
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
+		ch = &priv->ch[k];
+		board_cfg = &ch->cfg.board_cfg;
+		if (board_cfg->display_off)
+			board_cfg->display_off(board_cfg->board_data);
+	}
+
+	/* stops the lcdc operation */
+	lcdc_write(priv, _LDCNT2R, lcdc_read(priv, _LDCNT2R) & ~START_LCDC);
+
+	/* wait until power is stopped on all channels */
+	for (k = 0; k < ARRAY_SIZE(priv->ch); k++)
+		if (lcdc_read(priv, _LDCNT2R) & priv->ch[k].enabled)
+			while (lcdc_read_chan(&priv->ch[k], LDPMR) & 3)
+				;
+
+	/* stop dotclock */
+	lcdc_write(priv, _LDDCKSTPR, 1);
+
+}
+
+static int sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan *ch)
+{
+	int ifm, miftyp;
+
+	switch (ch->cfg.interface_type) {
+	case RGB8: ifm = 0; miftyp = 0; break;
+	case RGB9: ifm = 0; miftyp = 4; break;
+	case RGB12A: ifm = 0; miftyp = 5; break;
+	case RGB12B: ifm = 0; miftyp = 6; break;
+	case RGB16: ifm = 0; miftyp = 7; break;
+	case RGB18: ifm = 0; miftyp = 10; break;
+	case RGB24: ifm = 0; miftyp = 11; break;
+	case SYS8A: ifm = 1; miftyp = 0; break;
+	case SYS8B: ifm = 1; miftyp = 1; break;
+	case SYS8C: ifm = 1; miftyp = 2; break;
+	case SYS8D: ifm = 1; miftyp = 3; break;
+	case SYS9: ifm = 1; miftyp = 4; break;
+	case SYS12: ifm = 1; miftyp = 5; break;
+	case SYS16A: ifm = 1; miftyp = 7; break;
+	case SYS16B: ifm = 1; miftyp = 8; break;
+	case SYS16C: ifm = 1; miftyp = 9; break;
+	case SYS18: ifm = 1; miftyp = 10; break;
+	case SYS24: ifm = 1; miftyp = 11; break;
+	default: goto bad;
+	}
+
+	/* SUBLCD only supports SYS interface */
+	if (ch->cfg.chan == LCDC_CHAN_SUBLCD) {
+		if (ifm == 0)
+			goto bad;
+		else
+			ifm = 0;
+	}
+
+	ch->ldmt1r_value = (ifm << 12) | miftyp;
+	return 0;
+ bad:
+	return -EINVAL;
+}
+
+static int sh_mobile_lcdc_setup_clocks(struct device *dev, int clock_source,
+				       struct sh_mobile_lcdc_priv *priv)
+{
+	char *str;
+	int icksel;
+
+	switch (clock_source) {
+	case LCDC_CLK_BUS: str = "bus_clk"; icksel = 0; break;
+	case LCDC_CLK_PERIPHERAL: str = "peripheral_clk"; icksel = 1; break;
+	case LCDC_CLK_EXTERNAL: str = NULL; icksel = 2; break;
+	default:
+		return -EINVAL;
+	}
+
+	priv->lddckr = icksel << 16;
+
+	if (str) {
+		priv->clk = clk_get(dev, str);
+		if (IS_ERR(priv->clk)) {
+			dev_err(dev, "cannot get clock %s\n", str);
+			return PTR_ERR(priv->clk);
+		}
+
+		clk_enable(priv->clk);
+	}
+
+	return 0;
+}
+
+static int sh_mobile_lcdc_setcolreg(u_int regno,
+				    u_int red, u_int green, u_int blue,
+				    u_int transp, struct fb_info *info)
+{
+	u32 *palette = info->pseudo_palette;
+
+	if (regno >= PALETTE_NR)
+		return -EINVAL;
+
+	/* only FB_VISUAL_TRUECOLOR supported */
+
+	red >>= 16 - info->var.red.length;
+	green >>= 16 - info->var.green.length;
+	blue >>= 16 - info->var.blue.length;
+	transp >>= 16 - info->var.transp.length;
+
+	palette[regno] = (red << info->var.red.offset) |
+	  (green << info->var.green.offset) |
+	  (blue << info->var.blue.offset) |
+	  (transp << info->var.transp.offset);
+
+	return 0;
+}
+
+static struct fb_fix_screeninfo sh_mobile_lcdc_fix  = {
+	.id =		"SH Mobile LCDC",
+	.type =		FB_TYPE_PACKED_PIXELS,
+	.visual =	FB_VISUAL_TRUECOLOR,
+	.accel =	FB_ACCEL_NONE,
+};
+
+static struct fb_ops sh_mobile_lcdc_ops = {
+	.fb_setcolreg	= sh_mobile_lcdc_setcolreg,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit,
+};
+
+static int sh_mobile_lcdc_set_bpp(struct fb_var_screeninfo *var, int bpp)
+{
+	switch (bpp) {
+	case 16: /* PKF[4:0] = 00011 - RGB 565 */
+		var->red.offset = 11;
+		var->red.length = 5;
+		var->green.offset = 5;
+		var->green.length = 6;
+		var->blue.offset = 0;
+		var->blue.length = 5;
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		break;
+
+	case 32: /* PKF[4:0] = 00000 - RGB 888
+		  * sh7722 pdf says 00RRGGBB but reality is GGBB00RR
+		  * this may be because LDDDSR has word swap enabled..
+		  */
+		var->red.offset = 0;
+		var->red.length = 8;
+		var->green.offset = 24;
+		var->green.length = 8;
+		var->blue.offset = 16;
+		var->blue.length = 8;
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	var->bits_per_pixel = bpp;
+	var->red.msb_right = 0;
+	var->green.msb_right = 0;
+	var->blue.msb_right = 0;
+	var->transp.msb_right = 0;
+	return 0;
+}
+
+static int sh_mobile_lcdc_remove(struct platform_device *pdev);
+
+static int __init sh_mobile_lcdc_probe(struct platform_device *pdev)
+{
+	struct fb_info *info;
+	struct sh_mobile_lcdc_priv *priv;
+	struct sh_mobile_lcdc_info *pdata;
+	struct sh_mobile_lcdc_chan_cfg *cfg;
+	struct resource *res;
+	int error;
+	void *buf;
+	int i, j;
+
+	if (!pdev->dev.platform_data) {
+		dev_err(&pdev->dev, "no platform data defined\n");
+		error = -EINVAL;
+		goto err0;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		dev_err(&pdev->dev, "cannot find IO resource\n");
+		error = -ENOENT;
+		goto err0;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&pdev->dev, "cannot allocate device data\n");
+		error = -ENOMEM;
+		goto err0;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	pdata = pdev->dev.platform_data;
+
+	j = 0;
+	for (i = 0; i < ARRAY_SIZE(pdata->ch); i++) {
+		priv->ch[j].lcdc = priv;
+		memcpy(&priv->ch[j].cfg, &pdata->ch[i], sizeof(pdata->ch[i]));
+
+		error = sh_mobile_lcdc_check_interface(&priv->ch[i]);
+		if (error) {
+			dev_err(&pdev->dev, "unsupported interface type\n");
+			goto err1;
+		}
+
+		switch (pdata->ch[i].chan) {
+		case LCDC_CHAN_MAINLCD:
+			priv->ch[j].enabled = 1 << 1;
+			priv->ch[j].reg_offs = lcdc_offs_mainlcd;
+			j++;
+			break;
+		case LCDC_CHAN_SUBLCD:
+			priv->ch[j].enabled = 1 << 2;
+			priv->ch[j].reg_offs = lcdc_offs_sublcd;
+			j++;
+			break;
+		}
+	}
+
+	if (!j) {
+		dev_err(&pdev->dev, "no channels defined\n");
+		error = -EINVAL;
+		goto err1;
+	}
+
+	error = sh_mobile_lcdc_setup_clocks(&pdev->dev,
+					    pdata->clock_source, priv);
+	if (error) {
+		dev_err(&pdev->dev, "unable to setup clocks\n");
+		goto err1;
+	}
+
+	priv->lddckr = pdata->lddckr;
+	priv->base = ioremap_nocache(res->start, (res->end - res->start) + 1);
+
+	for (i = 0; i < j; i++) {
+		info = &priv->ch[i].info;
+		cfg = &priv->ch[i].cfg;
+
+		info->fbops = &sh_mobile_lcdc_ops;
+		info->var.xres = info->var.xres_virtual = cfg->lcd_cfg.xres;
+		info->var.yres = info->var.yres_virtual = cfg->lcd_cfg.yres;
+		info->var.activate = FB_ACTIVATE_NOW;
+		error = sh_mobile_lcdc_set_bpp(&info->var, cfg->bpp);
+		if (error)
+			break;
+
+		info->fix = sh_mobile_lcdc_fix;
+		info->fix.line_length = cfg->lcd_cfg.xres * (cfg->bpp / 8);
+		info->fix.smem_len = info->fix.line_length * cfg->lcd_cfg.yres;
+
+		buf = dma_alloc_coherent(&pdev->dev, info->fix.smem_len,
+					 &priv->ch[i].dma_handle, GFP_KERNEL);
+		if (!buf) {
+			dev_err(&pdev->dev, "unable to allocate buffer\n");
+			error = -ENOMEM;
+			break;
+		}
+
+		info->pseudo_palette = &priv->ch[i].pseudo_palette;
+		info->flags = FBINFO_FLAG_DEFAULT;
+
+		error = fb_alloc_cmap(&info->cmap, PALETTE_NR, 0);
+		if (error < 0) {
+			dev_err(&pdev->dev, "unable to allocate cmap\n");
+			dma_free_coherent(&pdev->dev, info->fix.smem_len,
+					  buf, priv->ch[i].dma_handle);
+			break;
+		}
+
+		memset(buf, 0, info->fix.smem_len);
+		info->fix.smem_start = priv->ch[i].dma_handle;
+		info->screen_base = buf;
+		info->device = &pdev->dev;
+	}
+
+	if (error)
+		goto err1;
+
+	error = sh_mobile_lcdc_start_hw(priv);
+	if (error) {
+		dev_err(&pdev->dev, "unable to start hardware\n");
+		goto err1;
+	}
+
+	for (i = 0; i < j; i++) {
+		error = register_framebuffer(&priv->ch[i].info);
+		if (error < 0)
+			goto err1;
+	}
+
+	for (i = 0; i < j; i++) {
+		info = &priv->ch[i].info;
+		dev_info(info->dev,
+			 "registered %s/%s as %dx%d %dbpp.\n",
+			 pdev->name,
+			 (priv->ch[i].cfg.chan == LCDC_CHAN_MAINLCD) ?
+			 "mainlcd" : "sublcd",
+			 (int) priv->ch[i].cfg.lcd_cfg.xres,
+			 (int) priv->ch[i].cfg.lcd_cfg.yres,
+			 priv->ch[i].cfg.bpp);
+	}
+
+	return 0;
+ err1:
+	sh_mobile_lcdc_remove(pdev);
+ err0:
+	return error;
+}
+
+static int sh_mobile_lcdc_remove(struct platform_device *pdev)
+{
+	struct sh_mobile_lcdc_priv *priv = platform_get_drvdata(pdev);
+	struct fb_info *info;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->ch); i++)
+		if (priv->ch[i].info.dev)
+			unregister_framebuffer(&priv->ch[i].info);
+
+	sh_mobile_lcdc_stop_hw(priv);
+
+	for (i = 0; i < ARRAY_SIZE(priv->ch); i++) {
+		info = &priv->ch[i].info;
+
+		if (!info->device)
+			continue;
+
+		dma_free_coherent(&pdev->dev, info->fix.smem_len,
+				  info->screen_base, priv->ch[i].dma_handle);
+		fb_dealloc_cmap(&info->cmap);
+	}
+
+	if (priv->clk) {
+		clk_disable(priv->clk);
+		clk_put(priv->clk);
+	}
+
+	if (priv->base)
+		iounmap(priv->base);
+
+	kfree(priv);
+	return 0;
+}
+
+static struct platform_driver sh_mobile_lcdc_driver = {
+	.driver		= {
+		.name		= "sh_mobile_lcdc_fb",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= sh_mobile_lcdc_probe,
+	.remove		= sh_mobile_lcdc_remove,
+};
+
+static int __init sh_mobile_lcdc_init(void)
+{
+	return platform_driver_register(&sh_mobile_lcdc_driver);
+}
+
+static void __exit sh_mobile_lcdc_exit(void)
+{
+	platform_driver_unregister(&sh_mobile_lcdc_driver);
+}
+
+module_init(sh_mobile_lcdc_init);
+module_exit(sh_mobile_lcdc_exit);
+
+MODULE_DESCRIPTION("SuperH Mobile LCDC Framebuffer driver");
+MODULE_AUTHOR("Magnus Damm <damm@opensource.se>");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/asm-sh/sh_mobile_lcdc.h	2008-05-29 17:18:47.000000000 +0900
@@ -0,0 +1,66 @@
+#ifndef __ASM_SH_MOBILE_LCDC_H__
+#define __ASM_SH_MOBILE_LCDC_H__
+
+#include <linux/fb.h>
+
+enum { RGB8,   /* 24bpp, 8:8:8 */
+       RGB9,   /* 18bpp, 9:9 */
+       RGB12A, /* 24bpp, 12:12 */
+       RGB12B, /* 12bpp */
+       RGB16,  /* 16bpp */
+       RGB18,  /* 18bpp */
+       RGB24,  /* 24bpp */
+       SYS8A,  /* 24bpp, 8:8:8 */
+       SYS8B,  /* 18bpp, 8:8:2 */
+       SYS8C,  /* 18bpp, 2:8:8 */
+       SYS8D,  /* 16bpp, 8:8 */
+       SYS9,   /* 18bpp, 9:9 */
+       SYS12,  /* 24bpp, 12:12 */
+       SYS16A, /* 16bpp */
+       SYS16B, /* 18bpp, 16:2 */
+       SYS16C, /* 18bpp, 2:16 */
+       SYS18,  /* 18bpp */
+       SYS24 };/* 24bpp */
+
+enum { LCDC_CHAN_DISABLED = 0,
+       LCDC_CHAN_MAINLCD,
+       LCDC_CHAN_SUBLCD };
+
+enum { LCDC_CLK_BUS, LCDC_CLK_PERIPHERAL, LCDC_CLK_EXTERNAL };
+
+struct sh_mobile_lcdc_sys_bus_cfg {
+	unsigned long ldmt2r;
+	unsigned long ldmt3r;
+};
+
+struct sh_mobile_lcdc_sys_bus_ops {
+	void (*write_index)(void *handle, unsigned long data);
+	void (*write_data)(void *handle, unsigned long data);
+	unsigned long (*read_data)(void *handle);
+};
+
+struct sh_mobile_lcdc_board_cfg {
+	void *board_data;
+	int (*setup_sys)(void *board_data, void *sys_ops_handle,
+			 struct sh_mobile_lcdc_sys_bus_ops *sys_ops);
+	void (*display_on)(void *board_data);
+	void (*display_off)(void *board_data);
+};
+
+struct sh_mobile_lcdc_chan_cfg {
+	int chan;
+	int bpp;
+	int interface_type; /* selects RGBn or SYSn I/F, see above */
+	int clock_divider;
+	struct fb_videomode lcd_cfg;
+	struct sh_mobile_lcdc_board_cfg board_cfg;
+	struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn I/F */
+};
+
+struct sh_mobile_lcdc_info {
+	unsigned long lddckr;
+	int clock_source;
+	struct sh_mobile_lcdc_chan_cfg ch[2];
+};
+
+#endif /* __ASM_SH_MOBILE_LCDC_H__ */

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Andrew Morton]

On Thu, 29 May 2008 18:24:51 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:

> This patch adds frame buffer support for the LCDC block found in
> SuperH Mobile processors. The hardware supports up to two LCD panels
> per LCDC block, and both RGB and SYS interfaces can be used to hook
> up LCD panels/modules.
> 
> The device driver is a regular platform driver, so LCD configuration
> and board specific hooks are passed to the driver using platform data.
> LCD modules using SYS interface often require special configuration
> using the SYS bus, and to solve this cleanly the driver provides SYS
> interface operations to the board code.
> 
> Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
> WVGA panels using RGB16 and RGB18 interfaces.
> 
> ...
>
> +	while (lcdc_read(ch->lcdc, _LDSR) & 2)
> +	  ;

Indenting is odd and inconsistent with most of the other zillions of
busywait loops here (multiple instances)

It might make sense to write a single function to do this busywaiting. 
That function can implement a timeout and can avoid lockups and provide
useful diagnostics.

Is sh's cpu_relax() a no-op?  Or are we assuming that ioread is a
suitable substitute?

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Magnus Damm]

On Thu, May 29, 2008 at 6:40 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 29 May 2008 18:24:51 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> This patch adds frame buffer support for the LCDC block found in
>> SuperH Mobile processors. The hardware supports up to two LCD panels
>> per LCDC block, and both RGB and SYS interfaces can be used to hook
>> up LCD panels/modules.
>>
>> The device driver is a regular platform driver, so LCD configuration
>> and board specific hooks are passed to the driver using platform data.
>> LCD modules using SYS interface often require special configuration
>> using the SYS bus, and to solve this cleanly the driver provides SYS
>> interface operations to the board code.
>>
>> Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
>> WVGA panels using RGB16 and RGB18 interfaces.
>>
>> ...
>>
>> +     while (lcdc_read(ch->lcdc, _LDSR) & 2)
>> +       ;
>
> Indenting is odd and inconsistent with most of the other zillions of
> busywait loops here (multiple instances)
>
> It might make sense to write a single function to do this busywaiting.
> That function can implement a timeout and can avoid lockups and provide
> useful diagnostics.

Breaking out to a single function and implementing timeout makes
sense. I'll see what I can do.

> Is sh's cpu_relax() a no-op?  Or are we assuming that ioread is a
> suitable substitute?

I simply forgot to use cpu_relax(). =)

I'll fix up the code and post a V2. Thank you for your review!

/ magnus

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [Linux-fbdev-devel] [PATCH] video: SuperH Mobile LCDC Driver

[Krzysztof Helt]

Hi Magnus,

Thank you for a new driver. I have reviewed your driver.
It is quite clean. Please resolve issues below and I will ACK it.

On Thu, 29 May 2008 18:24:51 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> --- /dev/null
> +++ work/drivers/video/sh_mobile_lcdcfb.c	2008-05-29 17:21:48.000000000 +0900
> @@ -0,0 +1,742 @@
> +/*
> + * SuperH Mobile LCDC Framebuffer
> + *
> + * Copyright (c) 2008 Magnus Damm
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License
> + *

English is not my native language but the last line sounds suspicious (either what?)

(...)
> +
> +struct sh_mobile_lcdc_priv;
> +struct sh_mobile_lcdc_chan {
> +	struct sh_mobile_lcdc_priv *lcdc;
> +	unsigned long *reg_offs;
> +	unsigned long ldmt1r_value;
> +	unsigned long enabled; /* ME and SE in LDCNT2R */
> +	struct sh_mobile_lcdc_chan_cfg cfg;
> +	u32 pseudo_palette[PALETTE_NR];
> +	struct fb_info info;
> +	dma_addr_t dma_handle;
> +};
> +
> +struct sh_mobile_lcdc_priv {
> +	void __iomem *base;
> +	struct clk *clk;
> +	unsigned long lddckr;
> +	struct sh_mobile_lcdc_chan ch[2];
> +};
> +

You have created a circular link here 
lcdc_priv -> lcdc_chan -> lcdc_priv

you can easily break it by removing the lcdc
field from the lcdc_chan structure.

> +
> +static void lcdc_write_chan(struct sh_mobile_lcdc_chan *chan,
> +			    int reg_nr, unsigned long data)
> +{
> +	iowrite32(data, chan->lcdc->base + chan->reg_offs[reg_nr]);
> +}
> +
> +static unsigned long lcdc_read_chan(struct sh_mobile_lcdc_chan *chan,
> +				    int reg_nr)
> +{
> +	return ioread32(chan->lcdc->base + chan->reg_offs[reg_nr]);
> +}
> +

put lcdc_priv and channel number (or base address and and channel pointer) 
as arguments to break the circular link. 
You can make these functions inline as well.

> +static void lcdc_write(struct sh_mobile_lcdc_priv *priv,
> +		       int reg_offs, unsigned long data)
> +{
> +	iowrite32(data, priv->base + reg_offs);
> +}
> +
> +static unsigned long lcdc_read(struct sh_mobile_lcdc_priv *priv,
> +			       unsigned long reg_offs)
> +{
> +	return ioread32(priv->base + reg_offs);
> +}
> +

These two functions can be inline.

> +static void lcdc_sys_write_index(void *handle, unsigned long data)
> +{
> +	struct sh_mobile_lcdc_chan *ch = handle;
> +
> +	lcdc_write(ch->lcdc, _LDDWD0R, data | 0x10000000);
> +
> +	while (lcdc_read(ch->lcdc, _LDSR) & 2)
> +	  ;
> +

As Andrew Morton put it it would be better to put
cpu_relax() inside this loop (or do some other
cpu releasing). There are more busy loops
in the driver's code.

That's it. A few things for a whole new driver.

Regards,
Krzysztof

----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>> http://link.interia.pl/f1e13  

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Paul Mundt]

On Thu, May 29, 2008 at 08:09:51PM +0200, Krzysztof Helt wrote:
> > --- /dev/null
> > +++ work/drivers/video/sh_mobile_lcdcfb.c	2008-05-29 17:21:48.000000000 +0900
> > @@ -0,0 +1,742 @@
> > +/*
> > + * SuperH Mobile LCDC Framebuffer
> > + *
> > + * Copyright (c) 2008 Magnus Damm
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License
> > + *
> 
> English is not my native language but the last line sounds suspicious (either what?)
> 
Yes, there's overzealous trimming here. This should be amended to refer
to GPLv2 only, though as the MODULE_LICENSE() its already get this right,
it's mostly a cosmetic change.

> put lcdc_priv and channel number (or base address and and channel pointer) 
> as arguments to break the circular link. 
> You can make these functions inline as well.
> 
As the 'circular' link is being used in a pre-defined way, it's not clear
that there's a problem with it existing in the first place. It's
obviously a matter of convenience.

> > +static void lcdc_write(struct sh_mobile_lcdc_priv *priv,
> > +		       int reg_offs, unsigned long data)
> > +{
> > +	iowrite32(data, priv->base + reg_offs);
> > +}
> > +
> > +static unsigned long lcdc_read(struct sh_mobile_lcdc_priv *priv,
> > +			       unsigned long reg_offs)
> > +{
> > +	return ioread32(priv->base + reg_offs);
> > +}
> > +
> 
> These two functions can be inline.
> 
The compiler will do this automatically, there's no need to mark it
explicity.

> > +static void lcdc_sys_write_index(void *handle, unsigned long data)
> > +{
> > +	struct sh_mobile_lcdc_chan *ch = handle;
> > +
> > +	lcdc_write(ch->lcdc, _LDDWD0R, data | 0x10000000);
> > +
> > +	while (lcdc_read(ch->lcdc, _LDSR) & 2)
> > +	  ;
> > +
> 
> As Andrew Morton put it it would be better to put
> cpu_relax() inside this loop (or do some other
> cpu releasing). There are more busy loops
> in the driver's code.
> 
cpu_relax() doesn't really matter here. It's good to include for general
coherency, since we don't have the same barriers in the ioread() path,
but it's hardly critical.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [Linux-fbdev-devel] [PATCH] video: SuperH Mobile LCDC Driver

[Krzysztof Helt]

On Fri, 30 May 2008 06:07:36 +0900
Paul Mundt <lethal@linux-sh.org> wrote:

> On Thu, May 29, 2008 at 08:09:51PM +0200, Krzysztof Helt wrote:
> > > --- /dev/null
> > > +++ work/drivers/video/sh_mobile_lcdcfb.c	2008-05-29 17:21:48.000000000 +0900
> > > @@ -0,0 +1,742 @@
> > > +/*
> > > + * SuperH Mobile LCDC Framebuffer
> > > + *
> > > + * Copyright (c) 2008 Magnus Damm
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License
> > > + *
> > 
> > English is not my native language but the last line sounds suspicious (either what?)
> > 
> Yes, there's overzealous trimming here. This should be amended to refer
> to GPLv2 only, though as the MODULE_LICENSE() its already get this right,
> it's mostly a cosmetic change.
> 

I am not a lawyer and I simply afraid errors in the legal statements.
I just pointed a mistake here.

> > put lcdc_priv and channel number (or base address and and channel pointer) 
> > as arguments to break the circular link. 
> > You can make these functions inline as well.
> > 
> As the 'circular' link is being used in a pre-defined way, it's not clear
> that there's a problem with it existing in the first place. It's
> obviously a matter of convenience.
> 

It is not that big problem so if it is required to simplify the driver it is fine.
It seems it is  access to only the lcdc_priv->base field is needed
from the channel structure. Just take a second look at it if it can be done
differently without adding to much complexity to the driver.

> > > +static void lcdc_write(struct sh_mobile_lcdc_priv *priv,
> > > +		       int reg_offs, unsigned long data)
> > > +{
> > > +	iowrite32(data, priv->base + reg_offs);
> > > +}
> > > +
> > > +static unsigned long lcdc_read(struct sh_mobile_lcdc_priv *priv,
> > > +			       unsigned long reg_offs)
> > > +{
> > > +	return ioread32(priv->base + reg_offs);
> > > +}
> > > +
> > 
> > These two functions can be inline.
> > 
> The compiler will do this automatically, there's no need to mark it
> explicity.
> 

I don't know the SuperH compiler but I saw things like not inlined functions
like this on PC (checked by disassembling the module object):

static void t_outb(struct tridentfb_par *p, u8 val, u16 reg)
{
        fb_writeb(val, p->io_virt + reg);
}

I didn't know why, so safer was just to mark it inline then 
the function was inlined. That was x86 gcc 3.4.6 compiler.
Before the check I believed that compiler did the right thing all the time.
You can check the objdump of the compiled module.

> > > +static void lcdc_sys_write_index(void *handle, unsigned long data)
> > > +{
> > > +	struct sh_mobile_lcdc_chan *ch = handle;
> > > +
> > > +	lcdc_write(ch->lcdc, _LDDWD0R, data | 0x10000000);
> > > +
> > > +	while (lcdc_read(ch->lcdc, _LDSR) & 2)
> > > +	  ;
> > > +
> > 
> > As Andrew Morton put it it would be better to put
> > cpu_relax() inside this loop (or do some other
> > cpu releasing). There are more busy loops
> > in the driver's code.
> > 
> cpu_relax() doesn't really matter here. It's good to include for general
> coherency, since we don't have the same barriers in the ioread() path,
> but it's hardly critical.
> 

No, it is not critical. It is improvement.

The driver code is very clean. All above are just hints for possible 
improvements.

Regards,
Krzysztof


----------------------------------------------------------------------
Mucha powraca!
Sprawdz >>> http://link.interia.pl/f1e0d

Re: [Linux-fbdev-devel] [PATCH] video: SuperH Mobile LCDC Driver

[Magnus Damm]

Hi Krzysztof,

On Fri, May 30, 2008 at 3:09 AM, Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:
> Hi Magnus,
>
> Thank you for a new driver. I have reviewed your driver.
> It is quite clean. Please resolve issues below and I will ACK it.
>
> On Thu, 29 May 2008 18:24:51 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> --- /dev/null
>> +++ work/drivers/video/sh_mobile_lcdcfb.c     2008-05-29 17:21:48.000000000 +0900
>> @@ -0,0 +1,742 @@
>> +/*
>> + * SuperH Mobile LCDC Framebuffer
>> + *
>> + * Copyright (c) 2008 Magnus Damm
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License
>> + *
>
> English is not my native language but the last line sounds suspicious (either what?)

Yep, same here and I agree.

> You have created a circular link here
> lcdc_priv -> lcdc_chan -> lcdc_priv
>
> you can easily break it by removing the lcdc
> field from the lcdc_chan structure.

Sure, but the field is there for convenience.

>> +static void lcdc_write_chan(struct sh_mobile_lcdc_chan *chan,
>> +                         int reg_nr, unsigned long data)
>> +{
>> +     iowrite32(data, chan->lcdc->base + chan->reg_offs[reg_nr]);
>> +}
>> +
>> +static unsigned long lcdc_read_chan(struct sh_mobile_lcdc_chan *chan,
>> +                                 int reg_nr)
>> +{
>> +     return ioread32(chan->lcdc->base + chan->reg_offs[reg_nr]);
>> +}
>> +
>
> put lcdc_priv and channel number (or base address and and channel pointer)
> as arguments to break the circular link.

That's possible, and maybe it makes sense. It didn't when I originally
wrote the code - quite some time ago - abstracting the LCDs into
channels and passing around a single pointer just seemed more natural
to me.

I'll have a second look at the code though. Maybe it makes sense to
shuffle things around a bit.

> That's it. A few things for a whole new driver.

Excellent. Thanks for your review!

/ magnus

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Nobuhiro Iwamatsu]

Hi, Magnus.

On Thu, 29 May 2008 18:24:51 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> This patch adds frame buffer support for the LCDC block found in
> SuperH Mobile processors. The hardware supports up to two LCD panels
> per LCDC block, and both RGB and SYS interfaces can be used to hook
> up LCD panels/modules.
> 
> The device driver is a regular platform driver, so LCD configuration
> and board specific hooks are passed to the driver using platform data.
> LCD modules using SYS interface often require special configuration
> using the SYS bus, and to solve this cleanly the driver provides SYS
> interface operations to the board code.
> 
> Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
> WVGA panels using RGB16 and RGB18 interfaces.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  drivers/video/Kconfig            |   10 
>  drivers/video/Makefile           |    2 
>  drivers/video/sh_mobile_lcdcfb.c |  742 ++++++++++++++++++++++++++++++++++++++
>  include/asm-sh/sh_mobile_lcdc.h  |   66 +++

This is a proposal. 
I am coding sh7763's frame buffer driver.
I think that the number of FB drivers of SH will increase in the future. 
I think that I want to make the driver/video/sh directory, and to bring 
SH together in this directory for the future.
Do you think about it?

Best regards,
 Nobuhiro
 
-- 
Nobuhiro Iwamatsu

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Manuel Lauss]

Hi Folks,

On Fri, May 30, 2008 at 01:59:28PM +0900, Nobuhiro Iwamatsu wrote:
> Hi, Magnus.
> 
> On Thu, 29 May 2008 18:24:51 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
> 
> > This patch adds frame buffer support for the LCDC block found in
> > SuperH Mobile processors. The hardware supports up to two LCD panels
> > per LCDC block, and both RGB and SYS interfaces can be used to hook
> > up LCD panels/modules.
> > 
> > The device driver is a regular platform driver, so LCD configuration
> > and board specific hooks are passed to the driver using platform data.
> > LCD modules using SYS interface often require special configuration
> > using the SYS bus, and to solve this cleanly the driver provides SYS
> > interface operations to the board code.
> > 
> > Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
> > WVGA panels using RGB16 and RGB18 interfaces.
> > 
> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > ---
> > 
> >  drivers/video/Kconfig            |   10 
> >  drivers/video/Makefile           |    2 
> >  drivers/video/sh_mobile_lcdcfb.c |  742 ++++++++++++++++++++++++++++++++++++++
> >  include/asm-sh/sh_mobile_lcdc.h  |   66 +++
> 
> This is a proposal. 
> I am coding sh7763's frame buffer driver.

 I wrote a driver for the SH7760 LCDC 2 years ago. From a quick glance at
the respective datasheets it seems both 7760 and 7763 have identical
register layout.  Looking at Magnus' driver, the 7722/7723 LCDC seems to
be an extended version of the 7760/63 LCDC. Maybe we can unify them somehow?


	Manuel Lauss

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Magnus Damm]

Hi Manuel,

Thanks for your email.

On Fri, May 30, 2008 at 2:15 PM, Manuel Lauss
<mano@roarinelk.homelinux.net> wrote:
> On Fri, May 30, 2008 at 01:59:28PM +0900, Nobuhiro Iwamatsu wrote:
>> On Thu, 29 May 2008 18:24:51 +0900
>> Magnus Damm <magnus.damm@gmail.com> wrote:
>>
>> > This patch adds frame buffer support for the LCDC block found in
>> > SuperH Mobile processors. The hardware supports up to two LCD panels
>> > per LCDC block, and both RGB and SYS interfaces can be used to hook
>> > up LCD panels/modules.
>> >
>> > The device driver is a regular platform driver, so LCD configuration
>> > and board specific hooks are passed to the driver using platform data.
>> > LCD modules using SYS interface often require special configuration
>> > using the SYS bus, and to solve this cleanly the driver provides SYS
>> > interface operations to the board code.
>> >
>> > Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
>> > WVGA panels using RGB16 and RGB18 interfaces.
>> >
>> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> > ---

>> I am coding sh7763's frame buffer driver.
>
>  I wrote a driver for the SH7760 LCDC 2 years ago. From a quick glance at
> the respective datasheets it seems both 7760 and 7763 have identical
> register layout.  Looking at Magnus' driver, the 7722/7723 LCDC seems to
> be an extended version of the 7760/63 LCDC. Maybe we can unify them somehow?

As for the SuperH Mobile LCDC block, the sh7723 is register compatible
with sh7722, but it only supports a single channel. I suspect that
sh7343 is register compatible with sh7722 as well, but I have no docs.
Both SYS and RGB interfaces are supported. Up to 24bpp. Both RGB and
YUV pixel formats are supported by the hardware. Clock configuration
and SYS bus stuff is shared between the channels.

The sh7760 LCDC supports a single channel using RGB interface up to
16bpp. No YUV. Just a handful of registers seem to have the same
layout as the SuperH Mobile LCDC block.

My gut feeling is that the complexity of a driver for one type of LCDC
block is enough. Think about the number of combinations of board type,
processor type, LCD panel, bus type, bus configuration and potentially
external LCD controller chip. And now we're talking about an extra
dimension on top of that... =)

If we can unify drivers and keep the complexity relatively low then
that's great, but before that can happen we sort of need something to
unify. So please write simple framebuffer drivers for now, and work on
getting them merged upstream. And when that's done _then_ we can unify
drivers and/or break out common code. In my mind, that's the right
order to do things.

/ magnus

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Nobuhiro Iwamatsu]

Hi, Manuel.

On Fri, 30 May 2008 07:15:54 +0200
Manuel Lauss <mano@roarinelk.homelinux.net> wrote:

> > This is a proposal. 
> > I am coding sh7763's frame buffer driver.
> 
>  I wrote a driver for the SH7760 LCDC 2 years ago. From a quick glance at
> the respective datasheets it seems both 7760 and 7763 have identical
> register layout.  Looking at Magnus' driver, the 7722/7723 LCDC seems to
> be an extended version of the 7760/63 LCDC. Maybe we can unify them somehow?

Thank you for your comments.

I comment on SH7760 and SH7763 because Magnus is writing for SH7722 and SH7723. 

I checked these data sheets. Most registers are the same. 
I think that I can merge it. 

Can I check the driver of SH7760 that you wrote where?

And I need found SH7760 LCDC suport board .....

Best regards,
 Nobuhiro

-- 
Nobuhiro Iwamatsu

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Magnus Damm]

Hi Iwamatsu-san!

On Fri, May 30, 2008 at 1:59 PM, Nobuhiro Iwamatsu <iwamatsu@nigauri.org> wrote:
> Hi, Magnus.
>
> On Thu, 29 May 2008 18:24:51 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> This patch adds frame buffer support for the LCDC block found in
>> SuperH Mobile processors. The hardware supports up to two LCD panels
>> per LCDC block, and both RGB and SYS interfaces can be used to hook
>> up LCD panels/modules.
>>
>> The device driver is a regular platform driver, so LCD configuration
>> and board specific hooks are passed to the driver using platform data.
>> LCD modules using SYS interface often require special configuration
>> using the SYS bus, and to solve this cleanly the driver provides SYS
>> interface operations to the board code.
>>
>> Tested on sh7723 and sh7722 processors with a SYS16A QVGA panel and
>> WVGA panels using RGB16 and RGB18 interfaces.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>>  drivers/video/Kconfig            |   10
>>  drivers/video/Makefile           |    2
>>  drivers/video/sh_mobile_lcdcfb.c |  742 ++++++++++++++++++++++++++++++++++++++
>>  include/asm-sh/sh_mobile_lcdc.h  |   66 +++
>
> This is a proposal.
> I am coding sh7763's frame buffer driver.

That's great news!

> I think that the number of FB drivers of SH will increase in the future.
> I think that I want to make the driver/video/sh directory, and to bring
> SH together in this directory for the future.
> Do you think about it?

I think grouping drivers together by architecture is a good
suggestion, but I personally prefer a step by step approach. So I
think we should deal with it later when we actually have more drivers
upstream.

I'm aiming for having this driver included in 2.6.27. Your driver is
targeted for some release later on, right?

Thanks!

/ magnus

Re: [PATCH] video: SuperH Mobile LCDC Driver

[Nobuhiro Iwamatsu]

Hi, Magnus.

On Fri, 30 May 2008 14:24:26 +0900
"Magnus Damm" <magnus.damm@gmail.com> wrote:

> I think grouping drivers together by architecture is a good
> suggestion, but I personally prefer a step by step approach. So I
> think we should deal with it later when we actually have more drivers
> upstream.
> 
OK.
I also have the driver first released. 

> I'm aiming for having this driver included in 2.6.27. Your driver is
> targeted for some release later on, right?
I also am working aiming at 2.6.27.

Best regards,
 Nobuhiro
-- 
Nobuhiro Iwamatsu