[RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Dmitry Baryshkov]

Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
chips.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
Cc: Ian Molton <spyro@f2s.com>
Cc: Samuel Ortiz <sameo@openedhand.com>
---
 drivers/video/Kconfig    |   22 +
 drivers/video/Makefile   |    1 +
 drivers/video/tmiofb.c   | 1036 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tmio.h |   15 +
 4 files changed, 1074 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/tmiofb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 70d135e..cf71db4 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -1876,6 +1876,28 @@ config FB_SH_MOBILE_LCDC
 	---help---
 	  Frame buffer driver for the on-chip SH-Mobile LCD controller.
 
+config FB_TMIO
+	tristate "Toshiba Mobice IO FrameBuffer support"
+	depends on FB && MFD_CORE
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	---help---
+	  Frame buffer driver for the Toshiba Mobile IO integrated as found
+	  on the Sharp SL-6000 series
+
+	  This driver is also available as a module ( = code which can be
+	  inserted and removed from the running kernel whenever you want). The
+	  module will be called tmiofb. If you want to compile it as a module,
+	  say M here and read <file:Documentation/kbuild/modules.txt>.
+
+	  If unsure, say N.
+
+config FB_TMIO_ACCELL
+	bool "tmiofb acceleration"
+	depends on FB_TMIO
+	default y
+
 config FB_S3C2410
 	tristate "S3C2410 LCD framebuffer support"
 	depends on FB && ARCH_S3C2410
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index a6b5529..32ca1f9 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_FB_CIRRUS)		  += cirrusfb.o
 obj-$(CONFIG_FB_ASILIANT)	  += asiliantfb.o
 obj-$(CONFIG_FB_PXA)		  += pxafb.o
 obj-$(CONFIG_FB_W100)		  += w100fb.o
+obj-$(CONFIG_FB_TMIO)		  += tmiofb.o
 obj-$(CONFIG_FB_AU1100)		  += au1100fb.o
 obj-$(CONFIG_FB_AU1200)		  += au1200fb.o
 obj-$(CONFIG_FB_PMAG_AA)	  += pmag-aa-fb.o
diff --git a/drivers/video/tmiofb.c b/drivers/video/tmiofb.c
new file mode 100644
index 0000000..6c18f89
--- /dev/null
+++ b/drivers/video/tmiofb.c
@@ -0,0 +1,1036 @@
+/*
+ * Frame Buffer Device for Toshiba Mobile IO(TMIO) controller
+ *
+ * Copyright(C) 2005-2006 Chris Humbert
+ * Copyright(C) 2005 Dirk Opfer
+ * Copytight(C) 2007,2008 Dmitry Baryshkov
+ *
+ * Based on:
+ *	drivers/video/w100fb.c
+ *	code written by Sharp/Lineo for 2.4 kernels
+ *
+ * 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, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/fb.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+/* Why should fb driver call console functions? because acquire_console_sem() */
+#include <linux/console.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tmio.h>
+#include <linux/uaccess.h>
+
+/*
+ * accelerator commands
+ */
+#define TMIOFB_ACC_CSADR(x)	(0x00000000 | ((x) & 0x001ffffe))
+#define TMIOFB_ACC_CHPIX(x)	(0x01000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_CVPIX(x)	(0x02000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_PSADR(x)	(0x03000000 | ((x) & 0x00fffffe))
+#define TMIOFB_ACC_PHPIX(x)	(0x04000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_PVPIX(x)	(0x05000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_PHOFS(x)	(0x06000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_PVOFS(x)	(0x07000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_POADR(x)	(0x08000000 | ((x) & 0x00fffffe))
+#define TMIOFB_ACC_RSTR(x)	(0x09000000 | ((x) & 0x000000ff))
+#define TMIOFB_ACC_TCLOR(x)	(0x0A000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_FILL(x)	(0x0B000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_DSADR(x)	(0x0C000000 | ((x) & 0x00fffffe))
+#define TMIOFB_ACC_SSADR(x)	(0x0D000000 | ((x) & 0x00fffffe))
+#define TMIOFB_ACC_DHPIX(x)	(0x0E000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_DVPIX(x)	(0x0F000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_SHPIX(x)	(0x10000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_SVPIX(x)	(0x11000000 | ((x) & 0x000003ff))
+#define TMIOFB_ACC_LBINI(x)	(0x12000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_LBK2(x)	(0x13000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_SHBINI(x)	(0x14000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_SHBK2(x)	(0x15000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_SVBINI(x)	(0x16000000 | ((x) & 0x0000ffff))
+#define TMIOFB_ACC_SVBK2(x)	(0x17000000 | ((x) & 0x0000ffff))
+
+#define	TMIOFB_ACC_CMGO		0x20000000
+#define	TMIOFB_ACC_CMGO_CEND	0x00000001
+#define	TMIOFB_ACC_CMGO_INT	0x00000002
+#define	TMIOFB_ACC_CMGO_CMOD	0x00000010
+#define	TMIOFB_ACC_CMGO_CDVRV	0x00000020
+#define	TMIOFB_ACC_CMGO_CDHRV	0x00000040
+#define	TMIOFB_ACC_CMGO_RUND	0x00008000
+#define	TMIOFB_ACC_SCGO		0x21000000
+#define	TMIOFB_ACC_SCGO_CEND	0x00000001
+#define	TMIOFB_ACC_SCGO_INT	0x00000002
+#define	TMIOFB_ACC_SCGO_ROP3	0x00000004
+#define	TMIOFB_ACC_SCGO_TRNS	0x00000008
+#define	TMIOFB_ACC_SCGO_DVRV	0x00000010
+#define	TMIOFB_ACC_SCGO_DHRV	0x00000020
+#define	TMIOFB_ACC_SCGO_SVRV	0x00000040
+#define	TMIOFB_ACC_SCGO_SHRV	0x00000080
+#define	TMIOFB_ACC_SCGO_DSTXY	0x00008000
+#define	TMIOFB_ACC_SBGO		0x22000000
+#define	TMIOFB_ACC_SBGO_CEND	0x00000001
+#define	TMIOFB_ACC_SBGO_INT	0x00000002
+#define	TMIOFB_ACC_SBGO_DVRV	0x00000010
+#define	TMIOFB_ACC_SBGO_DHRV	0x00000020
+#define	TMIOFB_ACC_SBGO_SVRV	0x00000040
+#define	TMIOFB_ACC_SBGO_SHRV	0x00000080
+#define	TMIOFB_ACC_SBGO_SBMD	0x00000100
+#define	TMIOFB_ACC_FLGO		0x23000000
+#define	TMIOFB_ACC_FLGO_CEND	0x00000001
+#define	TMIOFB_ACC_FLGO_INT	0x00000002
+#define	TMIOFB_ACC_FLGO_ROP3	0x00000004
+#define	TMIOFB_ACC_LDGO		0x24000000
+#define	TMIOFB_ACC_LDGO_CEND	0x00000001
+#define	TMIOFB_ACC_LDGO_INT	0x00000002
+#define	TMIOFB_ACC_LDGO_ROP3	0x00000004
+#define	TMIOFB_ACC_LDGO_ENDPX	0x00000008
+#define	TMIOFB_ACC_LDGO_LVRV	0x00000010
+#define	TMIOFB_ACC_LDGO_LHRV	0x00000020
+#define	TMIOFB_ACC_LDGO_LDMOD	0x00000040
+
+/* a FIFO is always allocated, even if acceleration is not used */
+#define TMIOFB_FIFO_SIZE	512
+
+/*
+ * LCD Host Controller Configuration Register
+ *
+ * This iomem area supports only 16-bit IO.
+ */
+#define CCR_CMD			0x04 /* Command				*/
+#define CCR_REVID		0x08 /* Revision ID			*/
+#define CCR_BASEL		0x10 /* LCD Control Reg Base Addr Low	*/
+#define CCR_BASEH		0x12 /* LCD Control Reg Base Addr High	*/
+#define CCR_UGCC		0x40 /* Unified Gated Clock Control	*/
+#define CCR_GCC			0x42 /* Gated Clock Control		*/
+#define CCR_USC			0x50 /* Unified Software Clear		*/
+#define CCR_VRAMRTC		0x60 /* VRAM Timing Control		*/
+				/* 0x61 VRAM Refresh Control		*/
+#define CCR_VRAMSAC		0x62 /* VRAM Access Control		*/
+				/* 0x63	VRAM Status			*/
+#define CCR_VRAMBC		0x64 /* VRAM Block Control		*/
+
+/*
+ * LCD Control Register
+ *
+ * This iomem area supports only 16-bit IO.
+ */
+#define LCR_UIS			0x000 /* Unified Interrupt Status		*/
+#define LCR_VHPN		0x008 /* VRAM Horizontal Pixel Number		*/
+#define LCR_CFSAL		0x00a /* Command FIFO Start Address Low		*/
+#define LCR_CFSAH		0x00c /* Command FIFO Start Address High	*/
+#define LCR_CFS			0x00e /* Command FIFO Size			*/
+#define LCR_CFWS		0x010 /* Command FIFO Writeable Size		*/
+#define LCR_BBIE		0x012 /* BitBLT Interrupt Enable		*/
+#define LCR_BBISC		0x014 /* BitBLT Interrupt Status and Clear	*/
+#define LCR_CCS			0x016 /* Command Count Status			*/
+#define LCR_BBES		0x018 /* BitBLT Execution Status		*/
+#define LCR_CMDL		0x01c /* Command Low				*/
+#define LCR_CMDH		0x01e /* Command High				*/
+#define LCR_CFC			0x022 /* Command FIFO Clear			*/
+#define LCR_CCIFC		0x024 /* CMOS Camera IF Control			*/
+#define LCR_HWT			0x026 /* Hardware Test				*/
+#define LCR_LCDCCRC		0x100 /* LCDC Clock and Reset Control		*/
+#define LCR_LCDCC		0x102 /* LCDC Control				*/
+#define LCR_LCDCOPC		0x104 /* LCDC Output Pin Control		*/
+#define LCR_LCDIS		0x108 /* LCD Interrupt Status			*/
+#define LCR_LCDIM		0x10a /* LCD Interrupt Mask			*/
+#define LCR_LCDIE		0x10c /* LCD Interrupt Enable			*/
+#define LCR_GDSAL		0x122 /* Graphics Display Start Address Low	*/
+#define LCR_GDSAH		0x124 /* Graphics Display Start Address High	*/
+#define LCR_VHPCL		0x12a /* VRAM Horizontal Pixel Count Low	*/
+#define LCR_VHPCH		0x12c /* VRAM Horizontal Pixel Count High	*/
+#define LCR_GM			0x12e /* Graphic Mode(VRAM access enable)	*/
+#define LCR_HT			0x140 /* Horizontal Total			*/
+#define LCR_HDS			0x142 /* Horizontal Display Start		*/
+#define LCR_HSS			0x144 /* H-Sync Start				*/
+#define LCR_HSE			0x146 /* H-Sync End				*/
+#define LCR_HNP			0x14c /* Horizontal Number of Pixels		*/
+#define LCR_VT			0x150 /* Vertical Total				*/
+#define LCR_VDS			0x152 /* Vertical Display Start			*/
+#define LCR_VSS			0x154 /* V-Sync Start				*/
+#define LCR_VSE			0x156 /* V-Sync End				*/
+#define LCR_CDLN		0x160 /* Current Display Line Number		*/
+#define LCR_ILN			0x162 /* Interrupt Line Number			*/
+#define LCR_SP			0x164 /* Sync Polarity				*/
+#define LCR_MISC		0x166 /* MISC(RGB565 mode)			*/
+#define LCR_VIHSS		0x16a /* Video Interface H-Sync Start		*/
+#define LCR_VIVS		0x16c /* Video Interface Vertical Start		*/
+#define LCR_VIVE		0x16e /* Video Interface Vertical End		*/
+#define LCR_VIVSS		0x170 /* Video Interface V-Sync Start		*/
+#define LCR_VCCIS		0x17e /* Video / CMOS Camera Interface Select	*/
+#define LCR_VIDWSAL		0x180 /* VI Data Write Start Address Low	*/
+#define LCR_VIDWSAH		0x182 /* VI Data Write Start Address High	*/
+#define LCR_VIDRSAL		0x184 /* VI Data Read Start Address Low		*/
+#define LCR_VIDRSAH		0x186 /* VI Data Read Start Address High	*/
+#define LCR_VIPDDST		0x188 /* VI Picture Data Display Start Timing	*/
+#define LCR_VIPDDET		0x186 /* VI Picture Data Display End Timing	*/
+#define LCR_VIE			0x18c /* Video Interface Enable			*/
+#define LCR_VCS			0x18e /* Video/Camera Select			*/
+#define LCR_VPHWC		0x194 /* Video Picture Horizontal Wait Count	*/
+#define LCR_VPHS		0x196 /* Video Picture Horizontal Size		*/
+#define LCR_VPVWC		0x198 /* Video Picture Vertical Wait Count	*/
+#define LCR_VPVS		0x19a /* Video Picture Vertical Size		*/
+#define LCR_PLHPIX		0x1a0 /* PLHPIX					*/
+#define LCR_XS			0x1a2 /* XStart					*/
+#define LCR_XCKHW		0x1a4 /* XCK High Width				*/
+#define LCR_STHS		0x1a8 /* STH Start				*/
+#define LCR_VT2			0x1aa /* Vertical Total				*/
+#define LCR_YCKSW		0x1ac /* YCK Start Wait				*/
+#define LCR_YSTS		0x1ae /* YST Start				*/
+#define LCR_PPOLS		0x1b0 /* #PPOL Start				*/
+#define LCR_PRECW		0x1b2 /* PREC Width				*/
+#define LCR_VCLKHW		0x1b4 /* VCLK High Width			*/
+#define LCR_OC			0x1b6 /* Output Control				*/
+
+static char *mode_option __devinitdata;
+
+struct tmiofb_par {
+	u32				pseudo_palette[16];
+
+#ifdef CONFIG_FB_TMIO_ACCELL
+	wait_queue_head_t		wait_acc;
+	bool				use_polling;
+#endif
+
+	void __iomem			*ccr;
+	void __iomem			*lcr;
+	void __iomem			*vram;
+};
+
+/*--------------------------------------------------------------------------*/
+
+static irqreturn_t tmiofb_irq(int irq, void *__info);
+
+/*--------------------------------------------------------------------------*/
+
+
+/*
+ * Turns off the LCD controller and LCD host controller.
+ */
+static int tmiofb_hw_stop(struct platform_device *dev)
+{
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	struct tmio_fb_data		*data	= cell->driver_data;
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct tmiofb_par		*par	= info->par;
+
+	tmio_iowrite16(0, par->ccr + CCR_UGCC);
+	tmio_iowrite16(0, par->lcr + LCR_GM);
+	data->lcd_set_power(dev, 0);
+	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
+
+	return 0;
+}
+
+/*
+ * Initializes the LCD host controller.
+ */
+static int tmiofb_hw_init(struct platform_device *dev)
+{
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	struct tmio_fb_data		*data	= cell->driver_data;
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct tmiofb_par		*par	= info->par;
+	const struct resource		*nlcr	= &cell->resources[0];
+	const struct resource		*vram	= &cell->resources[2];
+	unsigned long			base;
+
+	if (nlcr == NULL || vram == NULL)
+		return -EINVAL;
+
+	base = nlcr->start;
+
+	if (info->mode == NULL) {
+		printk(KERN_ERR "tmio-fb: null info->mode\n");
+		info->mode = data->modes;
+	}
+
+	data->lcd_mode(dev, info->mode);
+
+	tmio_iowrite16(0x003a, par->ccr + CCR_UGCC);
+	tmio_iowrite16(0x003a, par->ccr + CCR_GCC);
+	tmio_iowrite16(0x3f00, par->ccr + CCR_USC);
+
+	data->lcd_set_power(dev, 1);
+	mdelay(2);
+
+	tmio_iowrite16(0x0000, par->ccr + CCR_USC);
+	tmio_iowrite16(base >> 16, par->ccr + CCR_BASEH);
+	tmio_iowrite16(base, par->ccr + CCR_BASEL);
+	tmio_iowrite16(0x0002, par->ccr + CCR_CMD);	/* base address enable	*/
+	tmio_iowrite16(0x40a8, par->ccr + CCR_VRAMRTC);	/* VRAMRC, VRAMTC	*/
+	tmio_iowrite16(0x0018, par->ccr + CCR_VRAMSAC);	/* VRAMSTS, VRAMAC	*/
+	tmio_iowrite16(0x0002, par->ccr + CCR_VRAMBC);
+	mdelay(2);
+	tmio_iowrite16(0x000b, par->ccr + CCR_VRAMBC);
+
+	base = vram->start + info->screen_size;
+	tmio_iowrite16(base >> 16, par->lcr + LCR_CFSAH);
+	tmio_iowrite16(base, par->lcr + LCR_CFSAL);
+	tmio_iowrite16(TMIOFB_FIFO_SIZE - 1, par->lcr + LCR_CFS);
+	tmio_iowrite16(1, par->lcr + LCR_CFC);
+	tmio_iowrite16(1, par->lcr + LCR_BBIE);
+	tmio_iowrite16(0, par->lcr + LCR_CFWS);
+
+	return 0;
+}
+
+/*
+ * Sets the LCD controller's output resolution and pixel clock
+ */
+static void tmiofb_hw_mode(struct platform_device *dev)
+{
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	struct tmio_fb_data		*data	= cell->driver_data;
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct fb_videomode		*mode	= info->mode;
+	struct tmiofb_par		*par	= info->par;
+	unsigned int			i;
+
+	tmio_iowrite16(0, par->lcr + LCR_GM);
+	data->lcd_set_power(dev, 0);
+	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
+	data->lcd_mode(dev, mode);
+	data->lcd_set_power(dev, 1);
+
+	tmio_iowrite16(i = mode->xres * 2, par->lcr + LCR_VHPN);
+	tmio_iowrite16(0, par->lcr + LCR_GDSAH);
+	tmio_iowrite16(0, par->lcr + LCR_GDSAL);
+	tmio_iowrite16(i >> 16, par->lcr + LCR_VHPCH);
+	tmio_iowrite16(i, par->lcr + LCR_VHPCL);
+	tmio_iowrite16(i = 0, par->lcr + LCR_HSS);
+	tmio_iowrite16(i += mode->hsync_len, par->lcr + LCR_HSE);
+	tmio_iowrite16(i += mode->left_margin, par->lcr + LCR_HDS);
+	tmio_iowrite16(i += mode->xres + mode->right_margin, par->lcr + LCR_HT);
+	tmio_iowrite16(mode->xres, par->lcr + LCR_HNP);
+	tmio_iowrite16(i = 0, par->lcr + LCR_VSS);
+	tmio_iowrite16(i += mode->vsync_len, par->lcr + LCR_VSE);
+	tmio_iowrite16(i += mode->upper_margin, par->lcr + LCR_VDS);
+	tmio_iowrite16(i += mode->yres, par->lcr + LCR_ILN);
+	tmio_iowrite16(i += mode->lower_margin, par->lcr + LCR_VT);
+	tmio_iowrite16(3, par->lcr + LCR_MISC);	/* RGB565 mode */
+	tmio_iowrite16(1, par->lcr + LCR_GM);	/* VRAM enable */
+	tmio_iowrite16(0x4007, par->lcr + LCR_LCDCC);
+	tmio_iowrite16(3, par->lcr + LCR_SP);	 /* sync polarity */
+
+	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
+	mdelay(5);
+	tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC);	/* STOP_CKP */
+	mdelay(5);
+	tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC);	/* STOP_CKP | SOFT_RESET */
+	tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);
+}
+
+/*--------------------------------------------------------------------------*/
+
+#ifdef CONFIG_FB_TMIO_ACCELL
+static int __must_check
+tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
+{
+	struct tmiofb_par		*par	= info->par;
+	if (in_atomic() || par->use_polling) {
+		int i = 0;
+		while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
+			udelay(1);
+			i++;
+			if (i > 10000) {
+				printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
+				return -ETIMEDOUT;
+			}
+			tmiofb_irq(-1, info);
+		}
+	} else {
+		if (!wait_event_interruptible_timeout(par->wait_acc,
+				tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
+			printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Writes an accelerator command to the accelerator's FIFO.
+ */
+static int
+tmiofb_acc_write(struct fb_info *info, const u32 *cmd, unsigned int count)
+{
+	struct tmiofb_par		*par	= info->par;
+	int ret;
+
+	ret = tmiofb_acc_wait(info, TMIOFB_FIFO_SIZE - count);
+	if (ret)
+		return ret;
+
+	for (; count; count--, cmd++) {
+		tmio_iowrite16(*cmd >> 16, par->lcr + LCR_CMDH);
+		tmio_iowrite16(*cmd, par->lcr + LCR_CMDL);
+	}
+
+	return ret;
+}
+
+/*
+ * Wait for the accelerator to finish its operations before writing
+ * to the framebuffer for consistent display output.
+ */
+static int tmiofb_sync(struct fb_info *fbi)
+{
+	struct tmiofb_par		*par	= fbi->par;
+
+	int ret;
+	int i = 0;
+
+	ret = tmiofb_acc_wait(fbi, 0);
+
+	while (tmio_ioread16(par->lcr + LCR_BBES) & 2) {	/* blit active */
+		udelay(1);
+		i++ ;
+		if (i > 10000) {
+			printk(KERN_ERR "timeout waiting for blit to end!\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return ret;
+}
+
+static void
+tmiofb_fillrect(struct fb_info *fbi, const struct fb_fillrect *rect)
+{
+	const u32 cmd [] = {
+		TMIOFB_ACC_DSADR((rect->dy * fbi->mode->xres + rect->dx) * 2),
+		TMIOFB_ACC_DHPIX(rect->width	- 1),
+		TMIOFB_ACC_DVPIX(rect->height	- 1),
+		TMIOFB_ACC_FILL(rect->color),
+		TMIOFB_ACC_FLGO,
+	};
+
+	if (fbi->state != FBINFO_STATE_RUNNING ||
+	    fbi->flags & FBINFO_HWACCEL_DISABLED) {
+		cfb_fillrect(fbi, rect);
+		return;
+	}
+
+	tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
+}
+
+static void
+tmiofb_copyarea(struct fb_info *fbi, const struct fb_copyarea *area)
+{
+	const u32 cmd [] = {
+		TMIOFB_ACC_DSADR((area->dy * fbi->mode->xres + area->dx) * 2),
+		TMIOFB_ACC_DHPIX(area->width	- 1),
+		TMIOFB_ACC_DVPIX(area->height	- 1),
+		TMIOFB_ACC_SSADR((area->sy * fbi->mode->xres + area->sx) * 2),
+		TMIOFB_ACC_SCGO,
+	};
+
+	if (fbi->state != FBINFO_STATE_RUNNING ||
+	    fbi->flags & FBINFO_HWACCEL_DISABLED) {
+		cfb_copyarea(fbi, area);
+		return;
+	}
+
+	tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
+}
+#endif
+
+static void tmiofb_clearscreen(struct fb_info *info)
+{
+	const struct fb_fillrect rect = {
+		.dx	= 0,
+		.dy	= 0,
+		.width	= info->mode->xres,
+		.height	= info->mode->yres,
+		.color	= 0,
+	};
+
+	info->fbops->fb_fillrect(info, &rect);
+}
+
+static int tmiofb_vblank(struct fb_info *fbi, struct fb_vblank *vblank)
+{
+	struct tmiofb_par	*par	= fbi->par;
+	struct fb_videomode	*mode	= fbi->mode;
+	unsigned int		vcount	= tmio_ioread16(par->lcr + LCR_CDLN);
+	unsigned int		vds	= mode->vsync_len + mode->upper_margin;
+
+	vblank->vcount	= vcount;
+	vblank->flags	= FB_VBLANK_HAVE_VBLANK	| FB_VBLANK_HAVE_VCOUNT
+						| FB_VBLANK_HAVE_VSYNC;
+
+	if (vcount < mode->vsync_len)
+		vblank->flags |= FB_VBLANK_VSYNCING;
+
+	if (vcount < vds || vcount > vds + mode->yres)
+		vblank->flags |= FB_VBLANK_VBLANKING;
+
+	return 0;
+}
+
+
+static int tmiofb_ioctl(struct fb_info *fbi,
+		unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case FBIOGET_VBLANK: {
+		struct fb_vblank	vblank	= {0};
+		void __user		*argp	= (void __user *) arg;
+
+		tmiofb_vblank(fbi, &vblank);
+		if (copy_to_user(argp, &vblank, sizeof vblank))
+				return -EFAULT;
+		return 0;
+	}
+
+#ifdef CONFIG_FB_TMIO_ACCELL
+	case FBIO_TMIO_ACC_SYNC:
+		tmiofb_sync(fbi);
+		return 0;
+
+	case FBIO_TMIO_ACC_WRITE: {
+		u32 __user	*argp	= (void __user *) arg;
+		u32		len;
+		u32		acc [16];
+
+		if (copy_from_user(&len, argp, sizeof(u32)))
+			return -EFAULT;
+		if (len > ARRAY_SIZE(acc))
+			return -EINVAL;
+		if (copy_from_user(acc, argp + 1, sizeof(u32) * len))
+			return -EFAULT;
+
+		return tmiofb_acc_write(fbi, acc, len);
+	}
+#endif
+	}
+
+	return -EINVAL;
+}
+
+/*--------------------------------------------------------------------------*/
+
+/* Select the smallest mode that allows the desired resolution to be
+ * displayed.  If desired, the x and y parameters can be rounded up to
+ * match the selected mode.
+ */
+static struct fb_videomode*
+tmiofb_find_mode(struct fb_info *info, struct fb_var_screeninfo *var)
+{
+	struct mfd_cell			*cell	= to_platform_device(info->device)->dev.platform_data;
+	struct tmio_fb_data		*data	= cell->driver_data;
+	struct fb_videomode		*best	= NULL;
+	int				i;
+
+	for (i = 0; i < data->num_modes; i++) {
+		struct fb_videomode *mode = data->modes + i;
+
+		if (mode->xres >= var->xres && mode->yres >= var->yres
+				&& (!best || (mode->xres < best->xres
+					   && mode->yres < best->yres)))
+			best = mode;
+	}
+
+	return best;
+}
+
+static int tmiofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+
+	struct fb_videomode	*mode;
+
+	mode = tmiofb_find_mode(info, var);
+	if (!mode || var->bits_per_pixel > 16)
+		return -EINVAL;
+
+	fb_videomode_to_var(var, mode);
+
+	var->xres_virtual	= mode->xres;
+	var->yres_virtual	= info->screen_size / (mode->xres * 2);
+	var->xoffset		= 0;
+	var->yoffset		= 0;
+	var->bits_per_pixel	= 16;
+	var->grayscale		= 0;
+	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;
+	var->nonstd		= 0;
+	var->height		= 82;	/* mm */
+	var->width		= 60;	/* mm */
+	var->rotate		= 0;
+	return 0;
+}
+
+static int tmiofb_set_par(struct fb_info *info)
+{
+	struct fb_var_screeninfo	*var	= &info->var;
+	struct fb_videomode		*mode;
+
+	mode = tmiofb_find_mode(info, var);
+	if (!mode)
+		return -EINVAL;
+
+/*	if (info->mode == mode)
+		return 0;*/
+
+	info->mode		= mode;
+	info->fix.line_length	= info->mode->xres * 2;
+
+	tmiofb_hw_mode(to_platform_device(info->device));
+	tmiofb_clearscreen(info);
+	return 0;
+}
+
+static int tmiofb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			   unsigned blue, unsigned transp,
+			   struct fb_info *info)
+{
+	struct tmiofb_par	*par	= info->par;
+
+	if (regno < ARRAY_SIZE(par->pseudo_palette)) {
+		par->pseudo_palette [regno] =
+			((red	& 0xf800))		|
+			((green	& 0xfc00) >>  5)	|
+			((blue	& 0xf800) >> 11);
+		return 0;
+	}
+
+	return 1;
+}
+
+static int tmiofb_blank(int blank, struct fb_info *info)
+{
+	/*
+	 * everything is done in lcd/bl drivers.
+	 * this is purely to make sysfs happy and work.
+	 */
+	return 0;
+}
+
+static struct fb_ops tmiofb_ops = {
+	.owner		= THIS_MODULE,
+
+	.fb_ioctl	= tmiofb_ioctl,
+	.fb_check_var	= tmiofb_check_var,
+	.fb_set_par	= tmiofb_set_par,
+	.fb_setcolreg	= tmiofb_setcolreg,
+	.fb_blank	= tmiofb_blank,
+	.fb_imageblit	= cfb_imageblit,
+#ifdef CONFIG_FB_TMIO_ACCELL
+	.fb_sync	= tmiofb_sync,
+	.fb_fillrect	= tmiofb_fillrect,
+	.fb_copyarea	= tmiofb_copyarea,
+#else
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+#endif
+};
+
+/*--------------------------------------------------------------------------*/
+
+/*
+ * reasons for an interrupt:
+ *	uis	bbisc	lcdis
+ *	0100	0001		accelerator command completed
+ * 	2000		0001	vsync start
+ * 	2000		0002	display start
+ * 	2000		0004	line number match(0x1ff mask???)
+ */
+static irqreturn_t tmiofb_irq(int irq, void *__info)
+{
+	struct fb_info			*info	= __info;
+	struct tmiofb_par		*par	= info->par;
+	unsigned int			bbisc	= tmio_ioread16(par->lcr + LCR_BBISC);
+
+
+	if (unlikely(par->use_polling && irq != -1)) {
+		printk(KERN_INFO "tmiofb: switching to waitq\n");
+		par->use_polling = false;
+	}
+
+	tmio_iowrite16(bbisc, par->lcr + LCR_BBISC);
+
+#ifdef CONFIG_FB_TMIO_ACCELL
+	if (bbisc & 1)
+		wake_up(&par->wait_acc);
+#endif
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit tmiofb_probe(struct platform_device *dev)
+{
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	struct tmio_fb_data		*data	= cell->driver_data;
+	struct resource			*ccr	= platform_get_resource(dev, IORESOURCE_MEM, 1);
+	struct resource			*lcr	= platform_get_resource(dev, IORESOURCE_MEM, 0);
+	struct resource			*vram	= platform_get_resource(dev, IORESOURCE_MEM, 2);
+	int				irq	= platform_get_irq(dev, 0);
+	struct fb_info			*info;
+	struct tmiofb_par		*par;
+	int				retval;
+
+	if (data == NULL) {
+		dev_err(&dev->dev, "NULL platform data!\n");
+		return -EINVAL;
+	}
+
+	info = framebuffer_alloc(sizeof(struct tmiofb_par), &dev->dev);
+
+	if (!info) {
+		retval = -ENOMEM;
+		goto err_framebuffer_alloc;
+	}
+
+	par = info->par;
+	platform_set_drvdata(dev, info);
+
+#ifdef CONFIG_FB_TMIO_ACCELL
+	init_waitqueue_head(&par->wait_acc);
+
+	par->use_polling	= true;
+
+	info->flags		= FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA
+						 | FBINFO_HWACCEL_FILLRECT;
+#else
+	info->flags		= FBINFO_DEFAULT;
+#endif
+
+	info->fbops		= &tmiofb_ops;
+
+	strcpy(info->fix.id, "tmio-fb");
+	info->fix.smem_start	= vram->start;
+	info->fix.smem_len	= vram->end - vram->start + 1;
+	info->fix.type		= FB_TYPE_PACKED_PIXELS;
+	info->fix.visual	= FB_VISUAL_TRUECOLOR;
+	info->fix.mmio_start	= lcr->start;
+	info->fix.mmio_len	= lcr->end - lcr->start + 1;
+	info->fix.accel		= FB_ACCEL_NONE;
+	info->screen_size	= info->fix.smem_len - (4 * TMIOFB_FIFO_SIZE);
+	info->pseudo_palette	= par->pseudo_palette;
+
+	par->ccr = ioremap(ccr->start, ccr->end - ccr->start + 1);
+	if (!par->ccr) {
+		retval = -ENOMEM;
+		goto err_ioremap_ccr;
+	}
+
+	par->lcr = ioremap(info->fix.mmio_start, info->fix.mmio_len);
+	if (!par->lcr) {
+		retval = -ENOMEM;
+		goto err_ioremap_lcr;
+	}
+
+	par->vram = ioremap(info->fix.smem_start, info->fix.smem_len);
+	if (!par->vram) {
+		retval = -ENOMEM;
+		goto err_ioremap_vram;
+	}
+	info->screen_base = par->vram;
+
+	retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
+					dev->dev.bus_id, info);
+
+	if (retval)
+		goto err_request_irq;
+
+	retval = fb_find_mode(&info->var, info, mode_option,
+			data->modes, data->num_modes,
+			data->modes, 16);
+	if (!retval) {
+		retval = -EINVAL;
+		goto err_find_mode;
+	}
+
+	if (cell->enable) {
+		retval = cell->enable(dev);
+		if (retval)
+			goto err_enable;
+	}
+
+	retval = tmiofb_hw_init(dev);
+	if (retval)
+		goto err_hw_init;
+
+/*	retval = tmiofb_set_par(info);
+	if (retval)
+		goto err_set_par;*/
+
+	fb_videomode_to_modelist(data->modes, data->num_modes,
+				 &info->modelist);
+
+	retval = register_framebuffer(info);
+	if (retval < 0)
+		goto err_register_framebuffer;
+
+	printk(KERN_INFO "fb%d: %s frame buffer device\n",
+				info->node, info->fix.id);
+
+	return 0;
+
+err_register_framebuffer:
+/*err_set_par:*/
+	tmiofb_hw_stop(dev);
+err_hw_init:
+	if (cell->disable)
+		cell->disable(dev);
+err_enable:
+err_find_mode:
+	free_irq(irq, info);
+err_request_irq:
+	iounmap(par->vram);
+err_ioremap_vram:
+	iounmap(par->lcr);
+err_ioremap_lcr:
+	iounmap(par->ccr);
+err_ioremap_ccr:
+	platform_set_drvdata(dev, NULL);
+	framebuffer_release(info);
+err_framebuffer_alloc:
+	return retval;
+}
+
+static int __devexit tmiofb_remove(struct platform_device *dev)
+{
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	int				irq	= platform_get_irq(dev, 0);
+	struct tmiofb_par		*par;
+
+	if (info) {
+		par = info->par;
+		unregister_framebuffer(info);
+
+		tmiofb_hw_stop(dev);
+
+		if (cell->disable)
+			cell->disable(dev);
+
+		free_irq(irq, info);
+
+		iounmap(par->vram);
+		iounmap(par->lcr);
+		iounmap(par->ccr);
+
+		framebuffer_release(info);
+		platform_set_drvdata(dev, NULL);
+	}
+
+	return 0;
+}
+
+#ifdef DEBUG
+static void tmiofb_dump_regs(struct platform_device *dev)
+{
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct tmiofb_par		*par	= info->par;
+
+	printk("lhccr:\n");
+#define CCR_PR(n)	printk("\t" #n " = \t%04x\n", ioread16(par->ccr + CCR_ ## n));
+	CCR_PR(CMD);
+	CCR_PR(REVID);
+	CCR_PR(BASEL);
+	CCR_PR(BASEH);
+	CCR_PR(UGCC);
+	CCR_PR(GCC);
+	CCR_PR(USC);
+	CCR_PR(VRAMRTC);
+	CCR_PR(VRAMSAC);
+	CCR_PR(VRAMBC);
+#undef CCR_PR
+
+	printk("lcr: \n");
+#define LCR_PR(n)	printk("\t" #n " = \t%04x\n", tmio_ioread16(par->lcr + LCR_ ## n));
+	LCR_PR(UIS);
+	LCR_PR(VHPN);
+	LCR_PR(CFSAL);
+	LCR_PR(CFSAH);
+	LCR_PR(CFS);
+	LCR_PR(CFWS);
+	LCR_PR(BBIE);
+	LCR_PR(BBISC);
+	LCR_PR(CCS);
+	LCR_PR(BBES);
+	LCR_PR(CMDL);
+	LCR_PR(CMDH);
+	LCR_PR(CFC);
+	LCR_PR(CCIFC);
+	LCR_PR(HWT);
+	LCR_PR(LCDCCRC);
+	LCR_PR(LCDCC);
+	LCR_PR(LCDCOPC);
+	LCR_PR(LCDIS);
+	LCR_PR(LCDIM);
+	LCR_PR(LCDIE);
+	LCR_PR(GDSAL);
+	LCR_PR(GDSAH);
+	LCR_PR(VHPCL);
+	LCR_PR(VHPCH);
+	LCR_PR(GM);
+	LCR_PR(HT);
+	LCR_PR(HDS);
+	LCR_PR(HSS);
+	LCR_PR(HSE);
+	LCR_PR(HNP);
+	LCR_PR(VT);
+	LCR_PR(VDS);
+	LCR_PR(VSS);
+	LCR_PR(VSE);
+	LCR_PR(CDLN);
+	LCR_PR(ILN);
+	LCR_PR(SP);
+	LCR_PR(MISC);
+	LCR_PR(VIHSS);
+	LCR_PR(VIVS);
+	LCR_PR(VIVE);
+	LCR_PR(VIVSS);
+	LCR_PR(VCCIS);
+	LCR_PR(VIDWSAL);
+	LCR_PR(VIDWSAH);
+	LCR_PR(VIDRSAL);
+	LCR_PR(VIDRSAH);
+	LCR_PR(VIPDDST);
+	LCR_PR(VIPDDET);
+	LCR_PR(VIE);
+	LCR_PR(VCS);
+	LCR_PR(VPHWC);
+	LCR_PR(VPHS);
+	LCR_PR(VPVWC);
+	LCR_PR(VPVS);
+	LCR_PR(PLHPIX);
+	LCR_PR(XS);
+	LCR_PR(XCKHW);
+	LCR_PR(STHS);
+	LCR_PR(VT2);
+	LCR_PR(YCKSW);
+	LCR_PR(YSTS);
+	LCR_PR(PPOLS);
+	LCR_PR(PRECW);
+	LCR_PR(VCLKHW);
+	LCR_PR(OC);
+#undef LCR_PR
+}
+#endif
+
+#ifdef CONFIG_PM
+static int tmiofb_suspend(struct platform_device *dev, pm_message_t state)
+{
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct tmiofb_par		*par	= info->par;
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	int				retval	= 0;
+
+	acquire_console_sem();
+
+	fb_set_suspend(info, 1);
+
+	if (info->fbops->fb_sync)
+		info->fbops->fb_sync(info);
+
+
+	printk(KERN_INFO "tmiofb: switching to polling\n");
+	par->use_polling = true;
+	tmiofb_hw_stop(dev);
+
+	if (cell->suspend)
+		retval = cell->suspend(dev);
+
+	release_console_sem();
+
+	return retval;
+}
+
+static int tmiofb_resume(struct platform_device *dev)
+{
+	struct fb_info			*info	= platform_get_drvdata(dev);
+	struct mfd_cell			*cell	= dev->dev.platform_data;
+	int				retval;
+
+	acquire_console_sem();
+
+	if (cell->resume) {
+		retval = cell->resume(dev);
+		if (retval)
+			goto out;
+	}
+
+	tmiofb_irq(-1, info);
+
+	tmiofb_hw_init(dev);
+
+	tmiofb_hw_mode(dev);
+
+	fb_set_suspend(info, 0);
+out:
+	release_console_sem();
+	return retval;
+}
+#else
+#define tmiofb_suspend	NULL
+#define tmiofb_resume	NULL
+#endif
+
+static struct platform_driver tmiofb_driver = {
+	.driver.name	= "tmio-fb",
+	.driver.owner	= THIS_MODULE,
+	.probe		= tmiofb_probe,
+	.remove		= __devexit_p(tmiofb_remove),
+	.suspend	= tmiofb_suspend,
+	.resume		= tmiofb_resume,
+};
+
+/*--------------------------------------------------------------------------*/
+
+#ifndef MODULE
+static void __init tmiofb_setup(char *options)
+{
+	char *this_opt;
+
+	if (!options || !*options)
+		return;
+
+	while ((this_opt = strsep(&options, ",")) != NULL) {
+		if (!*this_opt) continue;
+		/*
+		 * FIXME
+		 */
+	}
+}
+#endif
+
+static int __init tmiofb_init(void)
+{
+#ifndef MODULE
+	char *option = NULL;
+
+	if (fb_get_options("tmiofb", &option))
+		return -ENODEV;
+	tmiofb_setup(option);
+#endif
+	return platform_driver_register(&tmiofb_driver);
+}
+
+static void __exit tmiofb_cleanup(void)
+{
+	platform_driver_unregister(&tmiofb_driver);
+}
+
+module_init(tmiofb_init);
+module_exit(tmiofb_cleanup);
+
+MODULE_DESCRIPTION("TMIO framebuffer driver");
+MODULE_AUTHOR("Chris Humbert, Dirk Opfer, Dmitry Baryshkov");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index ec612e6..311d3ea 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -1,6 +1,8 @@
 #ifndef MFD_TMIO_H
 #define MFD_TMIO_H
 
+#include <linux/fb.h>
+
 #define tmio_ioread8(addr) readb(addr)
 #define tmio_ioread16(addr) readw(addr)
 #define tmio_ioread16_rep(r, b, l) readsw(r, b, l)
@@ -25,4 +27,17 @@ struct tmio_nand_data {
 	unsigned int		num_partitions;
 };
 
+#define FBIO_TMIO_ACC_WRITE	0x7C639300
+#define FBIO_TMIO_ACC_SYNC	0x7C639301
+
+struct tmio_fb_data {
+	int			(*lcd_set_power)(struct platform_device *fb_dev,
+								bool on);
+	int			(*lcd_mode)(struct platform_device *fb_dev,
+						const struct fb_videomode *mode);
+	int			num_modes;
+	struct fb_videomode	*modes;
+};
+
+
 #endif
-- 
1.5.6.5


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb

[Dmitry Baryshkov]

Add support for tmiofb cell found in tc6393xb chip.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
Cc: Ian Molton <spyro@f2s.com>
Cc: Samuel Ortiz <sameo@openedhand.com>
---
 drivers/mfd/tc6393xb.c       |  113 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tc6393xb.h |    6 ++
 2 files changed, 119 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index e4c1c78..fc11e65 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -113,6 +113,7 @@ struct tc6393xb {
 enum {
 	TC6393XB_CELL_NAND,
 	TC6393XB_CELL_MMC,
+	TC6393XB_CELL_FB,
 };
 
 /*--------------------------------------------------------------------------*/
@@ -170,6 +171,104 @@ static struct resource __devinitdata tc6393xb_mmc_resources[] = {
 	},
 };
 
+static struct resource __devinitdata tc6393xb_fb_resources[] = {
+	{
+		.start	= 0x5000,
+		.end	= 0x51ff,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= 0x0500,
+		.end	= 0x05ff,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= 0x100000,
+		.end	= 0x1fffff,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= IRQ_TC6393_FB,
+		.end	= IRQ_TC6393_FB,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static int tc6393xb_fb_enable(struct platform_device *dev)
+{
+	struct tc6393xb *tc6393xb = dev_get_drvdata(dev->dev.parent);
+	unsigned long flags;
+	u16 ccr;
+
+	spin_lock_irqsave(&tc6393xb->lock, flags);
+
+	ccr = tmio_ioread16(tc6393xb->scr + SCR_CCR);
+	ccr &= ~SCR_CCR_MCLK_MASK;
+	ccr |= SCR_CCR_MCLK_48;
+	tmio_iowrite16(ccr, tc6393xb->scr + SCR_CCR);
+
+	spin_unlock_irqrestore(&tc6393xb->lock, flags);
+
+	return 0;
+}
+
+static int tc6393xb_fb_disable(struct platform_device *dev)
+{
+	struct tc6393xb *tc6393xb = dev_get_drvdata(dev->dev.parent);
+	unsigned long flags;
+	u16 ccr;
+
+	spin_lock_irqsave(&tc6393xb->lock, flags);
+
+	ccr = tmio_ioread16(tc6393xb->scr + SCR_CCR);
+	ccr &= ~SCR_CCR_MCLK_MASK;
+	ccr |= SCR_CCR_MCLK_OFF;
+	tmio_iowrite16(ccr, tc6393xb->scr + SCR_CCR);
+
+	spin_unlock_irqrestore(&tc6393xb->lock, flags);
+
+	return 0;
+}
+
+int tc6393xb_lcd_set_power(struct platform_device *fb, bool on)
+{
+	struct platform_device *dev = to_platform_device(fb->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+	u8 fer;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tc6393xb->lock, flags);
+
+	fer = ioread8(tc6393xb->scr + SCR_FER);
+	if (on)
+		fer |= SCR_FER_SLCDEN;
+	else
+		fer &= ~SCR_FER_SLCDEN;
+	iowrite8(fer, tc6393xb->scr + SCR_FER);
+
+	spin_unlock_irqrestore(&tc6393xb->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(tc6393xb_lcd_set_power);
+
+int tc6393xb_lcd_mode(struct platform_device *fb,
+					const struct fb_videomode *mode) {
+	struct platform_device *dev = to_platform_device(fb->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tc6393xb->lock, flags);
+
+	iowrite16(mode->pixclock, tc6393xb->scr + SCR_PLL1CR + 0);
+	iowrite16(mode->pixclock >> 16, tc6393xb->scr + SCR_PLL1CR + 2);
+
+	spin_unlock_irqrestore(&tc6393xb->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(tc6393xb_lcd_mode);
+
 static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	[TC6393XB_CELL_NAND] = {
 		.name = "tmio-nand",
@@ -182,6 +281,15 @@ static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 		.num_resources = ARRAY_SIZE(tc6393xb_mmc_resources),
 		.resources = tc6393xb_mmc_resources,
 	},
+	[TC6393XB_CELL_FB] = {
+		.name = "tmio-fb",
+		.num_resources = ARRAY_SIZE(tc6393xb_fb_resources),
+		.resources = tc6393xb_fb_resources,
+		.enable = tc6393xb_fb_enable,
+		.suspend = tc6393xb_fb_disable,
+		.resume = tc6393xb_fb_enable,
+		.disable = tc6393xb_fb_disable,
+	},
 };
 
 /*--------------------------------------------------------------------------*/
@@ -498,6 +606,11 @@ static int __devinit tc6393xb_probe(struct platform_device *dev)
 	tc6393xb_cells[TC6393XB_CELL_MMC].data_size =
 		sizeof(tc6393xb_cells[TC6393XB_CELL_MMC]);
 
+	tc6393xb_cells[TC6393XB_CELL_FB].driver_data = tcpd->fb_data;
+	tc6393xb_cells[TC6393XB_CELL_FB].platform_data =
+		&tc6393xb_cells[TC6393XB_CELL_FB];
+	tc6393xb_cells[TC6393XB_CELL_FB].data_size =
+		sizeof(tc6393xb_cells[TC6393XB_CELL_FB]);
 
 	ret = mfd_add_devices(&dev->dev, dev->id,
 			tc6393xb_cells, ARRAY_SIZE(tc6393xb_cells),
diff --git a/include/linux/mfd/tc6393xb.h b/include/linux/mfd/tc6393xb.h
index fec7b3f..28a80a2 100644
--- a/include/linux/mfd/tc6393xb.h
+++ b/include/linux/mfd/tc6393xb.h
@@ -33,13 +33,19 @@ struct tc6393xb_platform_data {
 	int	gpio_base;
 
 	struct tmio_nand_data	*nand_data;
+	struct tmio_fb_data	*fb_data;
 };
 
+extern int tc6393xb_lcd_mode(struct platform_device *fb,
+					const struct fb_videomode *mode);
+extern int tc6393xb_lcd_set_power(struct platform_device *fb, bool on);
+
 /*
  * Relative to irq_base
  */
 #define	IRQ_TC6393_NAND		0
 #define	IRQ_TC6393_MMC		1
+#define	IRQ_TC6393_FB		4
 
 #define	TC6393XB_NR_IRQS	8
 
-- 
1.5.6.5


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Geert Uytterhoeven]

On Tue, 30 Sep 2008, Dmitry Baryshkov wrote:

Going for the low hanging fruits...

> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -1876,6 +1876,28 @@ config FB_SH_MOBILE_LCDC
>  	---help---
>  	  Frame buffer driver for the on-chip SH-Mobile LCD controller.
>  
> +config FB_TMIO
> +	tristate "Toshiba Mobice IO FrameBuffer support"
                              ^
			      l

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Ian Molton]

Dmitry Baryshkov wrote:
> Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
> chips.
> 
> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>

On the grounds that this wont hurt any current e-series platforms, Go 
for it. I havent checked the code, but Im ok with it.

Acked-by: Ian Molton <spyro@f2s.com>

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Krzysztof Helt]

On Tue, 30 Sep 2008 12:38:29 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
> chips.
> 
> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> Cc: Ian Molton <spyro@f2s.com>
> Cc: Samuel Ortiz <sameo@openedhand.com>
> ---

A more detailed review now:


>  drivers/video/Kconfig    |   22 +
>  drivers/video/Makefile   |    1 +
>  drivers/video/tmiofb.c   | 1036 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tmio.h |   15 +
>  4 files changed, 1074 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/tmiofb.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 70d135e..cf71db4 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -1876,6 +1876,28 @@ config FB_SH_MOBILE_LCDC
>  	---help---
>  	  Frame buffer driver for the on-chip SH-Mobile LCD controller.
>  
> +config FB_TMIO
> +	tristate "Toshiba Mobice IO FrameBuffer support"
> +	depends on FB && MFD_CORE
> +	select FB_CFB_FILLRECT
> +	select FB_CFB_COPYAREA
> +	select FB_CFB_IMAGEBLIT
> +	---help---
> +	  Frame buffer driver for the Toshiba Mobile IO integrated as found
> +	  on the Sharp SL-6000 series
> +
> +	  This driver is also available as a module ( = code which can be
> +	  inserted and removed from the running kernel whenever you want). The
> +	  module will be called tmiofb. If you want to compile it as a module,
> +	  say M here and read <file:Documentation/kbuild/modules.txt>.
> +
> +	  If unsure, say N.
> +
> +config FB_TMIO_ACCELL
> +	bool "tmiofb acceleration"
> +	depends on FB_TMIO
> +	default y
> +
>  config FB_S3C2410
>  	tristate "S3C2410 LCD framebuffer support"
>  	depends on FB && ARCH_S3C2410
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index a6b5529..32ca1f9 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_FB_CIRRUS)		  += cirrusfb.o
>  obj-$(CONFIG_FB_ASILIANT)	  += asiliantfb.o
>  obj-$(CONFIG_FB_PXA)		  += pxafb.o
>  obj-$(CONFIG_FB_W100)		  += w100fb.o
> +obj-$(CONFIG_FB_TMIO)		  += tmiofb.o
>  obj-$(CONFIG_FB_AU1100)		  += au1100fb.o
>  obj-$(CONFIG_FB_AU1200)		  += au1200fb.o
>  obj-$(CONFIG_FB_PMAG_AA)	  += pmag-aa-fb.o
> diff --git a/drivers/video/tmiofb.c b/drivers/video/tmiofb.c
> new file mode 100644
> index 0000000..6c18f89
> --- /dev/null
> +++ b/drivers/video/tmiofb.c
> @@ -0,0 +1,1036 @@
> +/*
> + * Frame Buffer Device for Toshiba Mobile IO(TMIO) controller
> + *
> + * Copyright(C) 2005-2006 Chris Humbert
> + * Copyright(C) 2005 Dirk Opfer
> + * Copytight(C) 2007,2008 Dmitry Baryshkov
> + *
> + * Based on:
> + *	drivers/video/w100fb.c
> + *	code written by Sharp/Lineo for 2.4 kernels
> + *
> + * 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, or
> + * (at your option) any later version.
> + *

I don't know if the clause "or (at your option) any later version" is widely
accepted for kernel sources. See Linus post here:

http://lkml.org/lkml/2006/1/25/273

A safer clause is:

 * This file is subject to the terms and conditions of the GNU General Public
 * License. See the file COPYING in the main directory of this archive for
 * more details.

> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/fb.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +/* Why should fb driver call console functions? because acquire_console_sem() */
> +#include <linux/console.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/tmio.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * accelerator commands
> + */
> +#define TMIOFB_ACC_CSADR(x)	(0x00000000 | ((x) & 0x001ffffe))
> +#define TMIOFB_ACC_CHPIX(x)	(0x01000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_CVPIX(x)	(0x02000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_PSADR(x)	(0x03000000 | ((x) & 0x00fffffe))
> +#define TMIOFB_ACC_PHPIX(x)	(0x04000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_PVPIX(x)	(0x05000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_PHOFS(x)	(0x06000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_PVOFS(x)	(0x07000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_POADR(x)	(0x08000000 | ((x) & 0x00fffffe))
> +#define TMIOFB_ACC_RSTR(x)	(0x09000000 | ((x) & 0x000000ff))
> +#define TMIOFB_ACC_TCLOR(x)	(0x0A000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_FILL(x)	(0x0B000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_DSADR(x)	(0x0C000000 | ((x) & 0x00fffffe))
> +#define TMIOFB_ACC_SSADR(x)	(0x0D000000 | ((x) & 0x00fffffe))
> +#define TMIOFB_ACC_DHPIX(x)	(0x0E000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_DVPIX(x)	(0x0F000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_SHPIX(x)	(0x10000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_SVPIX(x)	(0x11000000 | ((x) & 0x000003ff))
> +#define TMIOFB_ACC_LBINI(x)	(0x12000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_LBK2(x)	(0x13000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_SHBINI(x)	(0x14000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_SHBK2(x)	(0x15000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_SVBINI(x)	(0x16000000 | ((x) & 0x0000ffff))
> +#define TMIOFB_ACC_SVBK2(x)	(0x17000000 | ((x) & 0x0000ffff))
> +
> +#define	TMIOFB_ACC_CMGO		0x20000000
> +#define	TMIOFB_ACC_CMGO_CEND	0x00000001
> +#define	TMIOFB_ACC_CMGO_INT	0x00000002
> +#define	TMIOFB_ACC_CMGO_CMOD	0x00000010
> +#define	TMIOFB_ACC_CMGO_CDVRV	0x00000020
> +#define	TMIOFB_ACC_CMGO_CDHRV	0x00000040
> +#define	TMIOFB_ACC_CMGO_RUND	0x00008000
> +#define	TMIOFB_ACC_SCGO		0x21000000
> +#define	TMIOFB_ACC_SCGO_CEND	0x00000001
> +#define	TMIOFB_ACC_SCGO_INT	0x00000002
> +#define	TMIOFB_ACC_SCGO_ROP3	0x00000004
> +#define	TMIOFB_ACC_SCGO_TRNS	0x00000008
> +#define	TMIOFB_ACC_SCGO_DVRV	0x00000010
> +#define	TMIOFB_ACC_SCGO_DHRV	0x00000020
> +#define	TMIOFB_ACC_SCGO_SVRV	0x00000040
> +#define	TMIOFB_ACC_SCGO_SHRV	0x00000080
> +#define	TMIOFB_ACC_SCGO_DSTXY	0x00008000
> +#define	TMIOFB_ACC_SBGO		0x22000000
> +#define	TMIOFB_ACC_SBGO_CEND	0x00000001
> +#define	TMIOFB_ACC_SBGO_INT	0x00000002
> +#define	TMIOFB_ACC_SBGO_DVRV	0x00000010
> +#define	TMIOFB_ACC_SBGO_DHRV	0x00000020
> +#define	TMIOFB_ACC_SBGO_SVRV	0x00000040
> +#define	TMIOFB_ACC_SBGO_SHRV	0x00000080
> +#define	TMIOFB_ACC_SBGO_SBMD	0x00000100
> +#define	TMIOFB_ACC_FLGO		0x23000000
> +#define	TMIOFB_ACC_FLGO_CEND	0x00000001
> +#define	TMIOFB_ACC_FLGO_INT	0x00000002
> +#define	TMIOFB_ACC_FLGO_ROP3	0x00000004
> +#define	TMIOFB_ACC_LDGO		0x24000000
> +#define	TMIOFB_ACC_LDGO_CEND	0x00000001
> +#define	TMIOFB_ACC_LDGO_INT	0x00000002
> +#define	TMIOFB_ACC_LDGO_ROP3	0x00000004
> +#define	TMIOFB_ACC_LDGO_ENDPX	0x00000008
> +#define	TMIOFB_ACC_LDGO_LVRV	0x00000010
> +#define	TMIOFB_ACC_LDGO_LHRV	0x00000020
> +#define	TMIOFB_ACC_LDGO_LDMOD	0x00000040
> +
> +/* a FIFO is always allocated, even if acceleration is not used */
> +#define TMIOFB_FIFO_SIZE	512
> +
> +/*
> + * LCD Host Controller Configuration Register
> + *
> + * This iomem area supports only 16-bit IO.
> + */
> +#define CCR_CMD			0x04 /* Command				*/
> +#define CCR_REVID		0x08 /* Revision ID			*/
> +#define CCR_BASEL		0x10 /* LCD Control Reg Base Addr Low	*/
> +#define CCR_BASEH		0x12 /* LCD Control Reg Base Addr High	*/
> +#define CCR_UGCC		0x40 /* Unified Gated Clock Control	*/
> +#define CCR_GCC			0x42 /* Gated Clock Control		*/
> +#define CCR_USC			0x50 /* Unified Software Clear		*/
> +#define CCR_VRAMRTC		0x60 /* VRAM Timing Control		*/
> +				/* 0x61 VRAM Refresh Control		*/
> +#define CCR_VRAMSAC		0x62 /* VRAM Access Control		*/
> +				/* 0x63	VRAM Status			*/
> +#define CCR_VRAMBC		0x64 /* VRAM Block Control		*/
> +
> +/*
> + * LCD Control Register
> + *
> + * This iomem area supports only 16-bit IO.
> + */

If you can cut one tab before values you can fit the section below within 80-column limit.

> +#define LCR_UIS			0x000 /* Unified Interrupt Status		*/
> +#define LCR_VHPN		0x008 /* VRAM Horizontal Pixel Number		*/
> +#define LCR_CFSAL		0x00a /* Command FIFO Start Address Low		*/
> +#define LCR_CFSAH		0x00c /* Command FIFO Start Address High	*/
> +#define LCR_CFS			0x00e /* Command FIFO Size			*/
> +#define LCR_CFWS		0x010 /* Command FIFO Writeable Size		*/
> +#define LCR_BBIE		0x012 /* BitBLT Interrupt Enable		*/
> +#define LCR_BBISC		0x014 /* BitBLT Interrupt Status and Clear	*/
> +#define LCR_CCS			0x016 /* Command Count Status			*/
> +#define LCR_BBES		0x018 /* BitBLT Execution Status		*/
> +#define LCR_CMDL		0x01c /* Command Low				*/
> +#define LCR_CMDH		0x01e /* Command High				*/
> +#define LCR_CFC			0x022 /* Command FIFO Clear			*/
> +#define LCR_CCIFC		0x024 /* CMOS Camera IF Control			*/
> +#define LCR_HWT			0x026 /* Hardware Test				*/
> +#define LCR_LCDCCRC		0x100 /* LCDC Clock and Reset Control		*/
> +#define LCR_LCDCC		0x102 /* LCDC Control				*/
> +#define LCR_LCDCOPC		0x104 /* LCDC Output Pin Control		*/
> +#define LCR_LCDIS		0x108 /* LCD Interrupt Status			*/
> +#define LCR_LCDIM		0x10a /* LCD Interrupt Mask			*/
> +#define LCR_LCDIE		0x10c /* LCD Interrupt Enable			*/
> +#define LCR_GDSAL		0x122 /* Graphics Display Start Address Low	*/
> +#define LCR_GDSAH		0x124 /* Graphics Display Start Address High	*/
> +#define LCR_VHPCL		0x12a /* VRAM Horizontal Pixel Count Low	*/
> +#define LCR_VHPCH		0x12c /* VRAM Horizontal Pixel Count High	*/
> +#define LCR_GM			0x12e /* Graphic Mode(VRAM access enable)	*/
> +#define LCR_HT			0x140 /* Horizontal Total			*/
> +#define LCR_HDS			0x142 /* Horizontal Display Start		*/
> +#define LCR_HSS			0x144 /* H-Sync Start				*/
> +#define LCR_HSE			0x146 /* H-Sync End				*/
> +#define LCR_HNP			0x14c /* Horizontal Number of Pixels		*/
> +#define LCR_VT			0x150 /* Vertical Total				*/
> +#define LCR_VDS			0x152 /* Vertical Display Start			*/
> +#define LCR_VSS			0x154 /* V-Sync Start				*/
> +#define LCR_VSE			0x156 /* V-Sync End				*/
> +#define LCR_CDLN		0x160 /* Current Display Line Number		*/
> +#define LCR_ILN			0x162 /* Interrupt Line Number			*/
> +#define LCR_SP			0x164 /* Sync Polarity				*/
> +#define LCR_MISC		0x166 /* MISC(RGB565 mode)			*/
> +#define LCR_VIHSS		0x16a /* Video Interface H-Sync Start		*/
> +#define LCR_VIVS		0x16c /* Video Interface Vertical Start		*/
> +#define LCR_VIVE		0x16e /* Video Interface Vertical End		*/
> +#define LCR_VIVSS		0x170 /* Video Interface V-Sync Start		*/
> +#define LCR_VCCIS		0x17e /* Video / CMOS Camera Interface Select	*/
> +#define LCR_VIDWSAL		0x180 /* VI Data Write Start Address Low	*/
> +#define LCR_VIDWSAH		0x182 /* VI Data Write Start Address High	*/
> +#define LCR_VIDRSAL		0x184 /* VI Data Read Start Address Low		*/
> +#define LCR_VIDRSAH		0x186 /* VI Data Read Start Address High	*/
> +#define LCR_VIPDDST		0x188 /* VI Picture Data Display Start Timing	*/
> +#define LCR_VIPDDET		0x186 /* VI Picture Data Display End Timing	*/
> +#define LCR_VIE			0x18c /* Video Interface Enable			*/
> +#define LCR_VCS			0x18e /* Video/Camera Select			*/
> +#define LCR_VPHWC		0x194 /* Video Picture Horizontal Wait Count	*/
> +#define LCR_VPHS		0x196 /* Video Picture Horizontal Size		*/
> +#define LCR_VPVWC		0x198 /* Video Picture Vertical Wait Count	*/
> +#define LCR_VPVS		0x19a /* Video Picture Vertical Size		*/
> +#define LCR_PLHPIX		0x1a0 /* PLHPIX					*/
> +#define LCR_XS			0x1a2 /* XStart					*/
> +#define LCR_XCKHW		0x1a4 /* XCK High Width				*/
> +#define LCR_STHS		0x1a8 /* STH Start				*/
> +#define LCR_VT2			0x1aa /* Vertical Total				*/
> +#define LCR_YCKSW		0x1ac /* YCK Start Wait				*/
> +#define LCR_YSTS		0x1ae /* YST Start				*/
> +#define LCR_PPOLS		0x1b0 /* #PPOL Start				*/
> +#define LCR_PRECW		0x1b2 /* PREC Width				*/
> +#define LCR_VCLKHW		0x1b4 /* VCLK High Width			*/
> +#define LCR_OC			0x1b6 /* Output Control				*/
> +
> +static char *mode_option __devinitdata;
> +
> +struct tmiofb_par {
> +	u32				pseudo_palette[16];
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	wait_queue_head_t		wait_acc;
> +	bool				use_polling;
> +#endif
> +
> +	void __iomem			*ccr;
> +	void __iomem			*lcr;
> +	void __iomem			*vram;

The vram field is not needed as it is always equal to fb_info->screen_base.

> +};
> +
> +/*--------------------------------------------------------------------------*/
> +
> +static irqreturn_t tmiofb_irq(int irq, void *__info);

You can move the trmiofb_irq earlier and just drop the forward
declaration.

> +
> +/*--------------------------------------------------------------------------*/
> +
> +
> +/*
> + * Turns off the LCD controller and LCD host controller.
> + */
> +static int tmiofb_hw_stop(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct tmiofb_par		*par	= info->par;
> +
> +	tmio_iowrite16(0, par->ccr + CCR_UGCC);
> +	tmio_iowrite16(0, par->lcr + LCR_GM);
> +	data->lcd_set_power(dev, 0);
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +
> +	return 0;
> +}
> +
> +/*
> + * Initializes the LCD host controller.
> + */
> +static int tmiofb_hw_init(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct tmiofb_par		*par	= info->par;
> +	const struct resource		*nlcr	= &cell->resources[0];
> +	const struct resource		*vram	= &cell->resources[2];
> +	unsigned long			base;
> +
> +	if (nlcr == NULL || vram == NULL)
> +		return -EINVAL;
> +
> +	base = nlcr->start;
> +
> +	if (info->mode == NULL) {
> +		printk(KERN_ERR "tmio-fb: null info->mode\n");
> +		info->mode = data->modes;
> +	}
> +
> +	data->lcd_mode(dev, info->mode);
> +
> +	tmio_iowrite16(0x003a, par->ccr + CCR_UGCC);
> +	tmio_iowrite16(0x003a, par->ccr + CCR_GCC);
> +	tmio_iowrite16(0x3f00, par->ccr + CCR_USC);
> +
> +	data->lcd_set_power(dev, 1);
> +	mdelay(2);

msleep() is preferred over mdelay() here and below.

> +
> +	tmio_iowrite16(0x0000, par->ccr + CCR_USC);
> +	tmio_iowrite16(base >> 16, par->ccr + CCR_BASEH);
> +	tmio_iowrite16(base, par->ccr + CCR_BASEL);
> +	tmio_iowrite16(0x0002, par->ccr + CCR_CMD);	/* base address enable	*/
> +	tmio_iowrite16(0x40a8, par->ccr + CCR_VRAMRTC);	/* VRAMRC, VRAMTC	*/
> +	tmio_iowrite16(0x0018, par->ccr + CCR_VRAMSAC);	/* VRAMSTS, VRAMAC	*/
> +	tmio_iowrite16(0x0002, par->ccr + CCR_VRAMBC);
> +	mdelay(2);
> +	tmio_iowrite16(0x000b, par->ccr + CCR_VRAMBC);
> +
> +	base = vram->start + info->screen_size;
> +	tmio_iowrite16(base >> 16, par->lcr + LCR_CFSAH);
> +	tmio_iowrite16(base, par->lcr + LCR_CFSAL);
> +	tmio_iowrite16(TMIOFB_FIFO_SIZE - 1, par->lcr + LCR_CFS);
> +	tmio_iowrite16(1, par->lcr + LCR_CFC);
> +	tmio_iowrite16(1, par->lcr + LCR_BBIE);
> +	tmio_iowrite16(0, par->lcr + LCR_CFWS);
> +
> +	return 0;
> +}
> +
> +/*
> + * Sets the LCD controller's output resolution and pixel clock
> + */
> +static void tmiofb_hw_mode(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct fb_videomode		*mode	= info->mode;
> +	struct tmiofb_par		*par	= info->par;
> +	unsigned int			i;
> +
> +	tmio_iowrite16(0, par->lcr + LCR_GM);
> +	data->lcd_set_power(dev, 0);
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +	data->lcd_mode(dev, mode);
> +	data->lcd_set_power(dev, 1);
> +
> +	tmio_iowrite16(i = mode->xres * 2, par->lcr + LCR_VHPN);
> +	tmio_iowrite16(0, par->lcr + LCR_GDSAH);
> +	tmio_iowrite16(0, par->lcr + LCR_GDSAL);
> +	tmio_iowrite16(i >> 16, par->lcr + LCR_VHPCH);
> +	tmio_iowrite16(i, par->lcr + LCR_VHPCL);
> +	tmio_iowrite16(i = 0, par->lcr + LCR_HSS);
> +	tmio_iowrite16(i += mode->hsync_len, par->lcr + LCR_HSE);
> +	tmio_iowrite16(i += mode->left_margin, par->lcr + LCR_HDS);
> +	tmio_iowrite16(i += mode->xres + mode->right_margin, par->lcr + LCR_HT);
> +	tmio_iowrite16(mode->xres, par->lcr + LCR_HNP);
> +	tmio_iowrite16(i = 0, par->lcr + LCR_VSS);
> +	tmio_iowrite16(i += mode->vsync_len, par->lcr + LCR_VSE);
> +	tmio_iowrite16(i += mode->upper_margin, par->lcr + LCR_VDS);
> +	tmio_iowrite16(i += mode->yres, par->lcr + LCR_ILN);
> +	tmio_iowrite16(i += mode->lower_margin, par->lcr + LCR_VT);
> +	tmio_iowrite16(3, par->lcr + LCR_MISC);	/* RGB565 mode */
> +	tmio_iowrite16(1, par->lcr + LCR_GM);	/* VRAM enable */
> +	tmio_iowrite16(0x4007, par->lcr + LCR_LCDCC);
> +	tmio_iowrite16(3, par->lcr + LCR_SP);	 /* sync polarity */
> +
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +	mdelay(5);
> +	tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC);	/* STOP_CKP */
> +	mdelay(5);
> +	tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC);	/* STOP_CKP | SOFT_RESET */
> +	tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);
> +}
> +
> +/*--------------------------------------------------------------------------*/
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +static int __must_check
> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
> +{
> +	struct tmiofb_par		*par	= info->par;

Such a wide formatting does not look good here and in few functions below.

> +	if (in_atomic() || par->use_polling) {
> +		int i = 0;
> +		while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
> +			udelay(1);
> +			i++;
> +			if (i > 10000) {
> +				printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> +				return -ETIMEDOUT;
> +			}
> +			tmiofb_irq(-1, info);
> +		}
> +	} else {
> +		if (!wait_event_interruptible_timeout(par->wait_acc,
> +				tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
> +			printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Writes an accelerator command to the accelerator's FIFO.
> + */
> +static int
> +tmiofb_acc_write(struct fb_info *info, const u32 *cmd, unsigned int count)
> +{
> +	struct tmiofb_par		*par	= info->par;
> +	int ret;
> +
> +	ret = tmiofb_acc_wait(info, TMIOFB_FIFO_SIZE - count);
> +	if (ret)
> +		return ret;
> +
> +	for (; count; count--, cmd++) {
> +		tmio_iowrite16(*cmd >> 16, par->lcr + LCR_CMDH);
> +		tmio_iowrite16(*cmd, par->lcr + LCR_CMDL);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Wait for the accelerator to finish its operations before writing
> + * to the framebuffer for consistent display output.
> + */
> +static int tmiofb_sync(struct fb_info *fbi)
> +{
> +	struct tmiofb_par		*par	= fbi->par;
> +
> +	int ret;
> +	int i = 0;
> +
> +	ret = tmiofb_acc_wait(fbi, 0);
> +
> +	while (tmio_ioread16(par->lcr + LCR_BBES) & 2) {	/* blit active */
> +		udelay(1);
> +		i++ ;
> +		if (i > 10000) {
> +			printk(KERN_ERR "timeout waiting for blit to end!\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void
> +tmiofb_fillrect(struct fb_info *fbi, const struct fb_fillrect *rect)
> +{
> +	const u32 cmd [] = {
> +		TMIOFB_ACC_DSADR((rect->dy * fbi->mode->xres + rect->dx) * 2),
> +		TMIOFB_ACC_DHPIX(rect->width	- 1),
> +		TMIOFB_ACC_DVPIX(rect->height	- 1),
> +		TMIOFB_ACC_FILL(rect->color),
> +		TMIOFB_ACC_FLGO,
> +	};
> +
> +	if (fbi->state != FBINFO_STATE_RUNNING ||
> +	    fbi->flags & FBINFO_HWACCEL_DISABLED) {
> +		cfb_fillrect(fbi, rect);
> +		return;
> +	}
> +
> +	tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
> +}
> +
> +static void
> +tmiofb_copyarea(struct fb_info *fbi, const struct fb_copyarea *area)
> +{
> +	const u32 cmd [] = {
> +		TMIOFB_ACC_DSADR((area->dy * fbi->mode->xres + area->dx) * 2),
> +		TMIOFB_ACC_DHPIX(area->width	- 1),
> +		TMIOFB_ACC_DVPIX(area->height	- 1),
> +		TMIOFB_ACC_SSADR((area->sy * fbi->mode->xres + area->sx) * 2),
> +		TMIOFB_ACC_SCGO,
> +	};
> +
> +	if (fbi->state != FBINFO_STATE_RUNNING ||
> +	    fbi->flags & FBINFO_HWACCEL_DISABLED) {
> +		cfb_copyarea(fbi, area);
> +		return;
> +	}
> +
> +	tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
> +}
> +#endif
> +
> +static void tmiofb_clearscreen(struct fb_info *info)
> +{
> +	const struct fb_fillrect rect = {
> +		.dx	= 0,
> +		.dy	= 0,
> +		.width	= info->mode->xres,
> +		.height	= info->mode->yres,
> +		.color	= 0,
> +	};
> +
> +	info->fbops->fb_fillrect(info, &rect);
> +}
> +

This function is probably redundant as it is used only inside
the set_par() function. My experience shows that the screen is
blanked every time set_par() changes parameters so clearing
it again does nothing. Please test if this function is needed.

> +static int tmiofb_vblank(struct fb_info *fbi, struct fb_vblank *vblank)
> +{
> +	struct tmiofb_par	*par	= fbi->par;
> +	struct fb_videomode	*mode	= fbi->mode;
> +	unsigned int		vcount	= tmio_ioread16(par->lcr + LCR_CDLN);
> +	unsigned int		vds	= mode->vsync_len + mode->upper_margin;
> +
> +	vblank->vcount	= vcount;
> +	vblank->flags	= FB_VBLANK_HAVE_VBLANK	| FB_VBLANK_HAVE_VCOUNT
> +						| FB_VBLANK_HAVE_VSYNC;
> +
> +	if (vcount < mode->vsync_len)
> +		vblank->flags |= FB_VBLANK_VSYNCING;
> +
> +	if (vcount < vds || vcount > vds + mode->yres)
> +		vblank->flags |= FB_VBLANK_VBLANKING;
> +
> +	return 0;
> +}
> +
> +
> +static int tmiofb_ioctl(struct fb_info *fbi,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case FBIOGET_VBLANK: {
> +		struct fb_vblank	vblank	= {0};
> +		void __user		*argp	= (void __user *) arg;
> +
> +		tmiofb_vblank(fbi, &vblank);
> +		if (copy_to_user(argp, &vblank, sizeof vblank))
> +				return -EFAULT;
> +		return 0;
> +	}
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	case FBIO_TMIO_ACC_SYNC:
> +		tmiofb_sync(fbi);
> +		return 0;
> +
> +	case FBIO_TMIO_ACC_WRITE: {
> +		u32 __user	*argp	= (void __user *) arg;
> +		u32		len;
> +		u32		acc [16];
> +
> +		if (copy_from_user(&len, argp, sizeof(u32)))
> +			return -EFAULT;
> +		if (len > ARRAY_SIZE(acc))
> +			return -EINVAL;
> +		if (copy_from_user(acc, argp + 1, sizeof(u32) * len))
> +			return -EFAULT;
> +
> +		return tmiofb_acc_write(fbi, acc, len);
> +	}
> +#endif
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/*--------------------------------------------------------------------------*/
> +
> +/* Select the smallest mode that allows the desired resolution to be
> + * displayed.  If desired, the x and y parameters can be rounded up to
> + * match the selected mode.
> + */
> +static struct fb_videomode*
> +tmiofb_find_mode(struct fb_info *info, struct fb_var_screeninfo *var)
> +{
> +	struct mfd_cell			*cell	= to_platform_device(info->device)->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_videomode		*best	= NULL;
> +	int				i;
> +
> +	for (i = 0; i < data->num_modes; i++) {
> +		struct fb_videomode *mode = data->modes + i;
> +
> +		if (mode->xres >= var->xres && mode->yres >= var->yres
> +				&& (!best || (mode->xres < best->xres
> +					   && mode->yres < best->yres)))
> +			best = mode;
> +	}
> +
> +	return best;
> +}
> +
> +static int tmiofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> +
> +	struct fb_videomode	*mode;
> +
> +	mode = tmiofb_find_mode(info, var);
> +	if (!mode || var->bits_per_pixel > 16)
> +		return -EINVAL;
> +
> +	fb_videomode_to_var(var, mode);
> +
> +	var->xres_virtual	= mode->xres;
> +	var->yres_virtual	= info->screen_size / (mode->xres * 2);

This could be a mistake as the yres_virtual is half size of the possible
memory. There is no check if the yres > yres_virtual nor that 
(screen_size >= 2 * xres).

> +	var->xoffset		= 0;
> +	var->yoffset		= 0;
> +	var->bits_per_pixel	= 16;
> +	var->grayscale		= 0;
> +	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;
> +	var->nonstd		= 0;
> +	var->height		= 82;	/* mm */
> +	var->width		= 60;	/* mm */

Shouldn't the height and width be in the platform_data
as the resolution?

> +	var->rotate		= 0;
> +	return 0;
> +}
> +
> +static int tmiofb_set_par(struct fb_info *info)
> +{
> +	struct fb_var_screeninfo	*var	= &info->var;
> +	struct fb_videomode		*mode;
> +
> +	mode = tmiofb_find_mode(info, var);
> +	if (!mode)
> +		return -EINVAL;
> +
> +/*	if (info->mode == mode)
> +		return 0;*/
> +
> +	info->mode		= mode;
> +	info->fix.line_length	= info->mode->xres * 2;

I would go for (var->bpp / 8) instead of 2 here (or a comment).

> +
> +	tmiofb_hw_mode(to_platform_device(info->device));
> +	tmiofb_clearscreen(info);
> +	return 0;
> +}
> +
> +static int tmiofb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	struct tmiofb_par	*par	= info->par;
> +
> +	if (regno < ARRAY_SIZE(par->pseudo_palette)) {
> +		par->pseudo_palette [regno] =
> +			((red	& 0xf800))		|
> +			((green	& 0xfc00) >>  5)	|
> +			((blue	& 0xf800) >> 11);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int tmiofb_blank(int blank, struct fb_info *info)
> +{
> +	/*
> +	 * everything is done in lcd/bl drivers.
> +	 * this is purely to make sysfs happy and work.
> +	 */
> +	return 0;
> +}
> +
> +static struct fb_ops tmiofb_ops = {
> +	.owner		= THIS_MODULE,
> +
> +	.fb_ioctl	= tmiofb_ioctl,
> +	.fb_check_var	= tmiofb_check_var,
> +	.fb_set_par	= tmiofb_set_par,
> +	.fb_setcolreg	= tmiofb_setcolreg,
> +	.fb_blank	= tmiofb_blank,
> +	.fb_imageblit	= cfb_imageblit,
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	.fb_sync	= tmiofb_sync,
> +	.fb_fillrect	= tmiofb_fillrect,
> +	.fb_copyarea	= tmiofb_copyarea,
> +#else
> +	.fb_fillrect	= cfb_fillrect,
> +	.fb_copyarea	= cfb_copyarea,
> +#endif
> +};
> +
> +/*--------------------------------------------------------------------------*/
> +
> +/*
> + * reasons for an interrupt:
> + *	uis	bbisc	lcdis
> + *	0100	0001		accelerator command completed
> + * 	2000		0001	vsync start
> + * 	2000		0002	display start
> + * 	2000		0004	line number match(0x1ff mask???)
> + */
> +static irqreturn_t tmiofb_irq(int irq, void *__info)
> +{
> +	struct fb_info			*info	= __info;
> +	struct tmiofb_par		*par	= info->par;
> +	unsigned int			bbisc	= tmio_ioread16(par->lcr + LCR_BBISC);
> +
> +
> +	if (unlikely(par->use_polling && irq != -1)) {
> +		printk(KERN_INFO "tmiofb: switching to waitq\n");
> +		par->use_polling = false;
> +	}
> +
> +	tmio_iowrite16(bbisc, par->lcr + LCR_BBISC);
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	if (bbisc & 1)
> +		wake_up(&par->wait_acc);
> +#endif
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit tmiofb_probe(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct resource			*ccr	= platform_get_resource(dev, IORESOURCE_MEM, 1);
> +	struct resource			*lcr	= platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	struct resource			*vram	= platform_get_resource(dev, IORESOURCE_MEM, 2);
> +	int				irq	= platform_get_irq(dev, 0);
> +	struct fb_info			*info;
> +	struct tmiofb_par		*par;
> +	int				retval;
> +
> +	if (data == NULL) {
> +		dev_err(&dev->dev, "NULL platform data!\n");
> +		return -EINVAL;
> +	}
> +
> +	info = framebuffer_alloc(sizeof(struct tmiofb_par), &dev->dev);
> +
> +	if (!info) {
> +		retval = -ENOMEM;
> +		goto err_framebuffer_alloc;

You can do just return -ENOMEM here (as in the condition above).

> +	}
> +
> +	par = info->par;
> +	platform_set_drvdata(dev, info);
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	init_waitqueue_head(&par->wait_acc);
> +
> +	par->use_polling	= true;
> +
> +	info->flags		= FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA
> +						 | FBINFO_HWACCEL_FILLRECT;
> +#else
> +	info->flags		= FBINFO_DEFAULT;
> +#endif
> +
> +	info->fbops		= &tmiofb_ops;
> +
> +	strcpy(info->fix.id, "tmio-fb");
> +	info->fix.smem_start	= vram->start;
> +	info->fix.smem_len	= vram->end - vram->start + 1;

resource_size() function is handy here.

> +	info->fix.type		= FB_TYPE_PACKED_PIXELS;
> +	info->fix.visual	= FB_VISUAL_TRUECOLOR;
> +	info->fix.mmio_start	= lcr->start;
> +	info->fix.mmio_len	= lcr->end - lcr->start + 1;

resource_size() function is handy here and below in this function.

> +	info->fix.accel		= FB_ACCEL_NONE;
> +	info->screen_size	= info->fix.smem_len - (4 * TMIOFB_FIFO_SIZE);
> +	info->pseudo_palette	= par->pseudo_palette;
> +
> +	par->ccr = ioremap(ccr->start, ccr->end - ccr->start + 1);
> +	if (!par->ccr) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_ccr;
> +	}
> +
> +	par->lcr = ioremap(info->fix.mmio_start, info->fix.mmio_len);
> +	if (!par->lcr) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_lcr;
> +	}
> +
> +	par->vram = ioremap(info->fix.smem_start, info->fix.smem_len);
> +	if (!par->vram) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_vram;
> +	}
> +	info->screen_base = par->vram;
> +
> +	retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
> +					dev->dev.bus_id, info);
> +
> +	if (retval)
> +		goto err_request_irq;
> +
> +	retval = fb_find_mode(&info->var, info, mode_option,
> +			data->modes, data->num_modes,
> +			data->modes, 16);
> +	if (!retval) {
> +		retval = -EINVAL;
> +		goto err_find_mode;
> +	}
> +
> +	if (cell->enable) {
> +		retval = cell->enable(dev);
> +		if (retval)
> +			goto err_enable;
> +	}
> +
> +	retval = tmiofb_hw_init(dev);
> +	if (retval)
> +		goto err_hw_init;
> +
> +/*	retval = tmiofb_set_par(info);
> +	if (retval)
> +		goto err_set_par;*/
> +

Please kill or enable the code above.

> +	fb_videomode_to_modelist(data->modes, data->num_modes,
> +				 &info->modelist);
> +
> +	retval = register_framebuffer(info);
> +	if (retval < 0)
> +		goto err_register_framebuffer;
> +
> +	printk(KERN_INFO "fb%d: %s frame buffer device\n",
> +				info->node, info->fix.id);
> +
> +	return 0;
> +
> +err_register_framebuffer:
> +/*err_set_par:*/
> +	tmiofb_hw_stop(dev);
> +err_hw_init:
> +	if (cell->disable)
> +		cell->disable(dev);
> +err_enable:
> +err_find_mode:
> +	free_irq(irq, info);
> +err_request_irq:
> +	iounmap(par->vram);
> +err_ioremap_vram:
> +	iounmap(par->lcr);
> +err_ioremap_lcr:
> +	iounmap(par->ccr);
> +err_ioremap_ccr:
> +	platform_set_drvdata(dev, NULL);
> +	framebuffer_release(info);
> +err_framebuffer_alloc:
> +	return retval;
> +}
> +
> +static int __devexit tmiofb_remove(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	int				irq	= platform_get_irq(dev, 0);
> +	struct tmiofb_par		*par;
> +
> +	if (info) {
> +		par = info->par;
> +		unregister_framebuffer(info);
> +
> +		tmiofb_hw_stop(dev);
> +
> +		if (cell->disable)
> +			cell->disable(dev);
> +
> +		free_irq(irq, info);
> +
> +		iounmap(par->vram);
> +		iounmap(par->lcr);
> +		iounmap(par->ccr);
> +
> +		framebuffer_release(info);
> +		platform_set_drvdata(dev, NULL);

Exchange these two lines above.

The rest of the driver is fine. 

Please run checkpatch.pl script and try to keep the code in 80-column format (many of longer lines can be easily converted to something shorter).

Regards,
Krzysztof

----------------------------------------------------------------------
Dzwon taniej na zagraniczne komorki!
Sprawdz >> http://link.interia.pl/f1f26 


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Andrew Morton]

On Tue, 30 Sep 2008 12:38:29 +0400 Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
> chips.
> 

Please feed the diff through scripts/checkpatch.pl and consider the
resulting report.  afacit all the warnings could/should be repaired,
except for the 80-col ones.  And even some of the latter could be
addressed just by being a bit more careful with the tab key.

>
> ...
>
> +config FB_TMIO
> +	tristate "Toshiba Mobice IO FrameBuffer support"
> +	depends on FB && MFD_CORE
> +	select FB_CFB_FILLRECT
> +	select FB_CFB_COPYAREA
> +	select FB_CFB_IMAGEBLIT
> +	---help---
> +	  Frame buffer driver for the Toshiba Mobile IO integrated as found
> +	  on the Sharp SL-6000 series
> +
> +	  This driver is also available as a module ( = code which can be
> +	  inserted and removed from the running kernel whenever you want). The
> +	  module will be called tmiofb. If you want to compile it as a module,
> +	  say M here and read <file:Documentation/kbuild/modules.txt>.
> +
> +	  If unsure, say N.
> +
> +config FB_TMIO_ACCELL
> +	bool "tmiofb acceleration"
> +	depends on FB_TMIO
> +	default y

Why does FB_TMIO_ACCELL exist?  Can we remove it and make that code
unconditional?

>
> ...
>
> +/*
> + * Turns off the LCD controller and LCD host controller.
> + */
> +static int tmiofb_hw_stop(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct tmiofb_par		*par	= info->par;

The tab frenzy doesn't do much for me.  Probably because I'm used to
looking at kernel code which rarely does this...

> +	tmio_iowrite16(0, par->ccr + CCR_UGCC);
> +	tmio_iowrite16(0, par->lcr + LCR_GM);
> +	data->lcd_set_power(dev, 0);
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +
> +	return 0;
> +}
> +
> +/*
> + * Initializes the LCD host controller.
> + */
> +static int tmiofb_hw_init(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct tmiofb_par		*par	= info->par;
> +	const struct resource		*nlcr	= &cell->resources[0];
> +	const struct resource		*vram	= &cell->resources[2];
> +	unsigned long			base;
> +
> +	if (nlcr == NULL || vram == NULL)
> +		return -EINVAL;
> +
> +	base = nlcr->start;
> +
> +	if (info->mode == NULL) {
> +		printk(KERN_ERR "tmio-fb: null info->mode\n");

Can this happen?

> +		info->mode = data->modes;
> +	}
> +
> +	data->lcd_mode(dev, info->mode);
> +
> +	tmio_iowrite16(0x003a, par->ccr + CCR_UGCC);
> +	tmio_iowrite16(0x003a, par->ccr + CCR_GCC);
> +	tmio_iowrite16(0x3f00, par->ccr + CCR_USC);
> +
> +	data->lcd_set_power(dev, 1);
> +	mdelay(2);
> +
> +	tmio_iowrite16(0x0000, par->ccr + CCR_USC);
> +	tmio_iowrite16(base >> 16, par->ccr + CCR_BASEH);
> +	tmio_iowrite16(base, par->ccr + CCR_BASEL);
> +	tmio_iowrite16(0x0002, par->ccr + CCR_CMD);	/* base address enable	*/
> +	tmio_iowrite16(0x40a8, par->ccr + CCR_VRAMRTC);	/* VRAMRC, VRAMTC	*/
> +	tmio_iowrite16(0x0018, par->ccr + CCR_VRAMSAC);	/* VRAMSTS, VRAMAC	*/
> +	tmio_iowrite16(0x0002, par->ccr + CCR_VRAMBC);
> +	mdelay(2);
> +	tmio_iowrite16(0x000b, par->ccr + CCR_VRAMBC);
> +
> +	base = vram->start + info->screen_size;
> +	tmio_iowrite16(base >> 16, par->lcr + LCR_CFSAH);
> +	tmio_iowrite16(base, par->lcr + LCR_CFSAL);
> +	tmio_iowrite16(TMIOFB_FIFO_SIZE - 1, par->lcr + LCR_CFS);
> +	tmio_iowrite16(1, par->lcr + LCR_CFC);
> +	tmio_iowrite16(1, par->lcr + LCR_BBIE);
> +	tmio_iowrite16(0, par->lcr + LCR_CFWS);
> +
> +	return 0;
> +}
> +
> +/*
> + * Sets the LCD controller's output resolution and pixel clock
> + */
> +static void tmiofb_hw_mode(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct fb_videomode		*mode	= info->mode;
> +	struct tmiofb_par		*par	= info->par;
> +	unsigned int			i;
> +
> +	tmio_iowrite16(0, par->lcr + LCR_GM);
> +	data->lcd_set_power(dev, 0);
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +	data->lcd_mode(dev, mode);
> +	data->lcd_set_power(dev, 1);
> +
> +	tmio_iowrite16(i = mode->xres * 2, par->lcr + LCR_VHPN);
> +	tmio_iowrite16(0, par->lcr + LCR_GDSAH);
> +	tmio_iowrite16(0, par->lcr + LCR_GDSAL);
> +	tmio_iowrite16(i >> 16, par->lcr + LCR_VHPCH);
> +	tmio_iowrite16(i, par->lcr + LCR_VHPCL);
> +	tmio_iowrite16(i = 0, par->lcr + LCR_HSS);
> +	tmio_iowrite16(i += mode->hsync_len, par->lcr + LCR_HSE);
> +	tmio_iowrite16(i += mode->left_margin, par->lcr + LCR_HDS);
> +	tmio_iowrite16(i += mode->xres + mode->right_margin, par->lcr + LCR_HT);
> +	tmio_iowrite16(mode->xres, par->lcr + LCR_HNP);
> +	tmio_iowrite16(i = 0, par->lcr + LCR_VSS);
> +	tmio_iowrite16(i += mode->vsync_len, par->lcr + LCR_VSE);
> +	tmio_iowrite16(i += mode->upper_margin, par->lcr + LCR_VDS);
> +	tmio_iowrite16(i += mode->yres, par->lcr + LCR_ILN);
> +	tmio_iowrite16(i += mode->lower_margin, par->lcr + LCR_VT);
> +	tmio_iowrite16(3, par->lcr + LCR_MISC);	/* RGB565 mode */
> +	tmio_iowrite16(1, par->lcr + LCR_GM);	/* VRAM enable */
> +	tmio_iowrite16(0x4007, par->lcr + LCR_LCDCC);
> +	tmio_iowrite16(3, par->lcr + LCR_SP);	 /* sync polarity */
> +
> +	tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> +	mdelay(5);
> +	tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC);	/* STOP_CKP */
> +	mdelay(5);
> +	tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC);	/* STOP_CKP | SOFT_RESET */
> +	tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);

I trust that someone who is reasonably familiar with the hardware will
know why all these magical mdelay()s are here.  But even if they are,
some comments explaining them would enhance maintainability.

> +}
> +
> +/*--------------------------------------------------------------------------*/
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +static int __must_check
> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
> +{
> +	struct tmiofb_par		*par	= info->par;
> +	if (in_atomic() || par->use_polling) {

No, use of in_atomic() is almost always wrong.  It returns "false"
inside a spinlocked section when CONFIG_PREEMPT=n.

I cannot advise you further because (tada) there is no comment
explaining this code to me.  But it will need to be rethought, please.

> +		int i = 0;
> +		while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
> +			udelay(1);
> +			i++;
> +			if (i > 10000) {
> +				printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> +				return -ETIMEDOUT;
> +			}
> +			tmiofb_irq(-1, info);
> +		}
> +	} else {
> +		if (!wait_event_interruptible_timeout(par->wait_acc,
> +				tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
> +			printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>
> ...
>
> +static void
> +tmiofb_fillrect(struct fb_info *fbi, const struct fb_fillrect *rect)
> +{
> +	const u32 cmd [] = {
> +		TMIOFB_ACC_DSADR((rect->dy * fbi->mode->xres + rect->dx) * 2),
> +		TMIOFB_ACC_DHPIX(rect->width	- 1),
> +		TMIOFB_ACC_DVPIX(rect->height	- 1),

you really like those tabs ;)

> +		TMIOFB_ACC_FILL(rect->color),
> +		TMIOFB_ACC_FLGO,
> +	};
> +
> +	if (fbi->state != FBINFO_STATE_RUNNING ||
> +	    fbi->flags & FBINFO_HWACCEL_DISABLED) {
> +		cfb_fillrect(fbi, rect);
> +		return;
> +	}
> +
> +	tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
> +}
> +
>
> ...
>
> +static void tmiofb_clearscreen(struct fb_info *info)
> +{
> +	const struct fb_fillrect rect = {
> +		.dx	= 0,
> +		.dy	= 0,
> +		.width	= info->mode->xres,
> +		.height	= info->mode->yres,
> +		.color	= 0,
> +	};

fyi, the members which are to be initialised to zero don't need to be
listed with this construct.  Although one might choose to fill them in
for readbility/documentation reasons (in which case: where's
fb_fillrect.rop?)

> +	info->fbops->fb_fillrect(info, &rect);
> +}
> +
> +static int tmiofb_vblank(struct fb_info *fbi, struct fb_vblank *vblank)
> +{
> +	struct tmiofb_par	*par	= fbi->par;
> +	struct fb_videomode	*mode	= fbi->mode;
> +	unsigned int		vcount	= tmio_ioread16(par->lcr + LCR_CDLN);
> +	unsigned int		vds	= mode->vsync_len + mode->upper_margin;
> +
> +	vblank->vcount	= vcount;
> +	vblank->flags	= FB_VBLANK_HAVE_VBLANK	| FB_VBLANK_HAVE_VCOUNT
> +						| FB_VBLANK_HAVE_VSYNC;
> +
> +	if (vcount < mode->vsync_len)
> +		vblank->flags |= FB_VBLANK_VSYNCING;
> +
> +	if (vcount < vds || vcount > vds + mode->yres)
> +		vblank->flags |= FB_VBLANK_VBLANKING;
> +
> +	return 0;
> +}
> +
> +
> +static int tmiofb_ioctl(struct fb_info *fbi,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case FBIOGET_VBLANK: {
> +		struct fb_vblank	vblank	= {0};
> +		void __user		*argp	= (void __user *) arg;
> +
> +		tmiofb_vblank(fbi, &vblank);
> +		if (copy_to_user(argp, &vblank, sizeof vblank))
> +				return -EFAULT;

An extra indent there.

> +		return 0;
> +	}
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	case FBIO_TMIO_ACC_SYNC:
> +		tmiofb_sync(fbi);
> +		return 0;
> +
> +	case FBIO_TMIO_ACC_WRITE: {
> +		u32 __user	*argp	= (void __user *) arg;
> +		u32		len;
> +		u32		acc [16];
> +
> +		if (copy_from_user(&len, argp, sizeof(u32)))
> +			return -EFAULT;

get_user() would be simpler and quicker.

> +		if (len > ARRAY_SIZE(acc))
> +			return -EINVAL;
> +		if (copy_from_user(acc, argp + 1, sizeof(u32) * len))
> +			return -EFAULT;
> +
> +		return tmiofb_acc_write(fbi, acc, len);
> +	}

So there's no way for userspace to read back the current setting of
`acc' (whatever that is)?

> +#endif
> +	}
> +
> +	return -EINVAL;

Unimplemented ioctl numbers usually return -ENOTTY, although your
-EINVAL might get converted to -ENOTTY at some higher level (which
would be a bad thing).  Please check.

> +}
> +
> +/*--------------------------------------------------------------------------*/
> +
> +/* Select the smallest mode that allows the desired resolution to be
> + * displayed.  If desired, the x and y parameters can be rounded up to
> + * match the selected mode.
> + */
> +static struct fb_videomode*

A single space before the "*", please.  checkpatch missed this.

> +tmiofb_find_mode(struct fb_info *info, struct fb_var_screeninfo *var)
> +{
> +	struct mfd_cell			*cell	= to_platform_device(info->device)->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct fb_videomode		*best	= NULL;
> +	int				i;
> +
> +	for (i = 0; i < data->num_modes; i++) {
> +		struct fb_videomode *mode = data->modes + i;
> +
> +		if (mode->xres >= var->xres && mode->yres >= var->yres
> +				&& (!best || (mode->xres < best->xres
> +					   && mode->yres < best->yres)))
> +			best = mode;
> +	}
> +
> +	return best;
> +}
> +
> +static int tmiofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> +
> +	struct fb_videomode	*mode;
> +
> +	mode = tmiofb_find_mode(info, var);
> +	if (!mode || var->bits_per_pixel > 16)
> +		return -EINVAL;
> +
> +	fb_videomode_to_var(var, mode);
> +
> +	var->xres_virtual	= mode->xres;
> +	var->yres_virtual	= info->screen_size / (mode->xres * 2);
> +	var->xoffset		= 0;
> +	var->yoffset		= 0;
> +	var->bits_per_pixel	= 16;
> +	var->grayscale		= 0;
> +	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;

One statement per line is preferred, please.

> +	var->nonstd		= 0;
> +	var->height		= 82;	/* mm */
> +	var->width		= 60;	/* mm */
> +	var->rotate		= 0;
> +	return 0;
> +}
> +
>
> ...
>
> +static int tmiofb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	struct tmiofb_par	*par	= info->par;
> +
> +	if (regno < ARRAY_SIZE(par->pseudo_palette)) {
> +		par->pseudo_palette [regno] =
> +			((red	& 0xf800))		|
> +			((green	& 0xfc00) >>  5)	|
> +			((blue	& 0xf800) >> 11);
> +		return 0;
> +	}
> +
> +	return 1;
> +}

Does .fb_setcolreg really return positive non-zero on error?  That
would be a bit sad - it should expect a -ve errno and should propagate
that back.  Oh well.  Please check it?

> +static int tmiofb_blank(int blank, struct fb_info *info)
> +{
> +	/*
> +	 * everything is done in lcd/bl drivers.
> +	 * this is purely to make sysfs happy and work.
> +	 */
> +	return 0;
> +}
> +
>
> ...
>
> +/*
> + * reasons for an interrupt:
> + *	uis	bbisc	lcdis
> + *	0100	0001		accelerator command completed
> + * 	2000		0001	vsync start
> + * 	2000		0002	display start
> + * 	2000		0004	line number match(0x1ff mask???)
> + */
> +static irqreturn_t tmiofb_irq(int irq, void *__info)
> +{
> +	struct fb_info			*info	= __info;
> +	struct tmiofb_par		*par	= info->par;
> +	unsigned int			bbisc	= tmio_ioread16(par->lcr + LCR_BBISC);
> +
> +
> +	if (unlikely(par->use_polling && irq != -1)) {
> +		printk(KERN_INFO "tmiofb: switching to waitq\n");
> +		par->use_polling = false;
> +	}

It'd be nice if there was a comment somewhere explaining to the reader
what's actually happening here.  Some overall design/perspective thing.

> +	tmio_iowrite16(bbisc, par->lcr + LCR_BBISC);
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	if (bbisc & 1)
> +		wake_up(&par->wait_acc);
> +#endif
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit tmiofb_probe(struct platform_device *dev)
> +{
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	struct tmio_fb_data		*data	= cell->driver_data;
> +	struct resource			*ccr	= platform_get_resource(dev, IORESOURCE_MEM, 1);
> +	struct resource			*lcr	= platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	struct resource			*vram	= platform_get_resource(dev, IORESOURCE_MEM, 2);
> +	int				irq	= platform_get_irq(dev, 0);
> +	struct fb_info			*info;
> +	struct tmiofb_par		*par;
> +	int				retval;
> +
> +	if (data == NULL) {
> +		dev_err(&dev->dev, "NULL platform data!\n");

Can this happen?

> +		return -EINVAL;
> +	}
> +
> +	info = framebuffer_alloc(sizeof(struct tmiofb_par), &dev->dev);
> +
> +	if (!info) {
> +		retval = -ENOMEM;
> +		goto err_framebuffer_alloc;
> +	}
> +
> +	par = info->par;
> +	platform_set_drvdata(dev, info);
> +
> +#ifdef CONFIG_FB_TMIO_ACCELL
> +	init_waitqueue_head(&par->wait_acc);
> +
> +	par->use_polling	= true;
> +
> +	info->flags		= FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA
> +						 | FBINFO_HWACCEL_FILLRECT;
> +#else
> +	info->flags		= FBINFO_DEFAULT;
> +#endif
> +
> +	info->fbops		= &tmiofb_ops;
> +
> +	strcpy(info->fix.id, "tmio-fb");
> +	info->fix.smem_start	= vram->start;
> +	info->fix.smem_len	= vram->end - vram->start + 1;
> +	info->fix.type		= FB_TYPE_PACKED_PIXELS;
> +	info->fix.visual	= FB_VISUAL_TRUECOLOR;
> +	info->fix.mmio_start	= lcr->start;
> +	info->fix.mmio_len	= lcr->end - lcr->start + 1;
> +	info->fix.accel		= FB_ACCEL_NONE;
> +	info->screen_size	= info->fix.smem_len - (4 * TMIOFB_FIFO_SIZE);
> +	info->pseudo_palette	= par->pseudo_palette;
> +
> +	par->ccr = ioremap(ccr->start, ccr->end - ccr->start + 1);
> +	if (!par->ccr) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_ccr;
> +	}
> +
> +	par->lcr = ioremap(info->fix.mmio_start, info->fix.mmio_len);
> +	if (!par->lcr) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_lcr;
> +	}
> +
> +	par->vram = ioremap(info->fix.smem_start, info->fix.smem_len);
> +	if (!par->vram) {
> +		retval = -ENOMEM;
> +		goto err_ioremap_vram;
> +	}
> +	info->screen_base = par->vram;
> +
> +	retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
> +					dev->dev.bus_id, info);
> +
> +	if (retval)
> +		goto err_request_irq;
> +
> +	retval = fb_find_mode(&info->var, info, mode_option,
> +			data->modes, data->num_modes,
> +			data->modes, 16);
> +	if (!retval) {
> +		retval = -EINVAL;
> +		goto err_find_mode;
> +	}
> +
> +	if (cell->enable) {
> +		retval = cell->enable(dev);
> +		if (retval)
> +			goto err_enable;
> +	}
> +
> +	retval = tmiofb_hw_init(dev);
> +	if (retval)
> +		goto err_hw_init;
> +
> +/*	retval = tmiofb_set_par(info);
> +	if (retval)
> +		goto err_set_par;*/
> +
> +	fb_videomode_to_modelist(data->modes, data->num_modes,
> +				 &info->modelist);
> +
> +	retval = register_framebuffer(info);
> +	if (retval < 0)
> +		goto err_register_framebuffer;
> +
> +	printk(KERN_INFO "fb%d: %s frame buffer device\n",
> +				info->node, info->fix.id);
> +
> +	return 0;
> +
> +err_register_framebuffer:
> +/*err_set_par:*/
> +	tmiofb_hw_stop(dev);
> +err_hw_init:
> +	if (cell->disable)
> +		cell->disable(dev);
> +err_enable:
> +err_find_mode:
> +	free_irq(irq, info);
> +err_request_irq:
> +	iounmap(par->vram);
> +err_ioremap_vram:
> +	iounmap(par->lcr);
> +err_ioremap_lcr:
> +	iounmap(par->ccr);
> +err_ioremap_ccr:
> +	platform_set_drvdata(dev, NULL);
> +	framebuffer_release(info);
> +err_framebuffer_alloc:
> +	return retval;
> +}
> +
>
> ...
>
> +#ifdef CONFIG_PM
> +static int tmiofb_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	struct fb_info			*info	= platform_get_drvdata(dev);
> +	struct tmiofb_par		*par	= info->par;
> +	struct mfd_cell			*cell	= dev->dev.platform_data;
> +	int				retval	= 0;
> +
> +	acquire_console_sem();
> +
> +	fb_set_suspend(info, 1);
> +
> +	if (info->fbops->fb_sync)
> +		info->fbops->fb_sync(info);
> +
> +
> +	printk(KERN_INFO "tmiofb: switching to polling\n");

why...

> +	par->use_polling = true;
> +	tmiofb_hw_stop(dev);
> +
> +	if (cell->suspend)
> +		retval = cell->suspend(dev);
> +
> +	release_console_sem();
> +
> +	return retval;
> +}
> +
>
> ...
>
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -1,6 +1,8 @@
>  #ifndef MFD_TMIO_H
>  #define MFD_TMIO_H
>  
> +#include <linux/fb.h>
> +
>  #define tmio_ioread8(addr) readb(addr)
>  #define tmio_ioread16(addr) readw(addr)
>  #define tmio_ioread16_rep(r, b, l) readsw(r, b, l)

I realise it isn't your code, but these:

#define tmio_ioread32(addr) \
	(((u32) readw((addr))) | (((u32) readw((addr) + 2)) << 16))

#define tmio_iowrite32(val, addr) \
	do { \
	writew((val),       (addr)); \
	writew((val) >> 16, (addr) + 2); \
	} while (0)

aren't good.  They reference their arguments multiple times and hence
will misbehave if passed an expression with side-effects.

As is so often the case, these never needed to be implemented as macros
and hence should not have been.  C is better.

>
> ...
>

The code looks reasonable overall but the in_atomic() thing is a
show-stopper.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Ian Molton]

Andrew Morton wrote:

> I realise it isn't your code, but these:
> 
> #define tmio_ioread32(addr) \

...are my fault. Sorry about that.

> As is so often the case, these never needed to be implemented as macros
> and hence should not have been.  C is better.

Good point, well made. I'm happy for them to be changed to inline 
functions. They grew out of the MMC code, where they were used to clean 
up the IO transfer loops. I'll send a patch to Samuel for his MFD tree 
to effect this change.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb

[Samuel Ortiz]

Hi Dmitry,

On Tue, Sep 30, 2008 at 12:38:30PM +0400, Dmitry Baryshkov wrote:
> Add support for tmiofb cell found in tc6393xb chip.
> 
> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> Cc: Ian Molton <spyro@f2s.com>
> Cc: Samuel Ortiz <sameo@openedhand.com>
I also pushed this one to mfd-next.
It was conflicting with the OHCI one you sent me earlier, so I fixed the
merge issues and applied it to my tree.
If the fb people want both patches to go through their way, they should let
me know.

Cheers,
Samuel.


> ---
>  drivers/mfd/tc6393xb.c       |  113 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tc6393xb.h |    6 ++
>  2 files changed, 119 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
> index e4c1c78..fc11e65 100644
> --- a/drivers/mfd/tc6393xb.c
> +++ b/drivers/mfd/tc6393xb.c
> @@ -113,6 +113,7 @@ struct tc6393xb {
>  enum {
>  	TC6393XB_CELL_NAND,
>  	TC6393XB_CELL_MMC,
> +	TC6393XB_CELL_FB,
>  };
>  
>  /*--------------------------------------------------------------------------*/
> @@ -170,6 +171,104 @@ static struct resource __devinitdata tc6393xb_mmc_resources[] = {
>  	},
>  };
>  
> +static struct resource __devinitdata tc6393xb_fb_resources[] = {
> +	{
> +		.start	= 0x5000,
> +		.end	= 0x51ff,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	{
> +		.start	= 0x0500,
> +		.end	= 0x05ff,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	{
> +		.start	= 0x100000,
> +		.end	= 0x1fffff,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	{
> +		.start	= IRQ_TC6393_FB,
> +		.end	= IRQ_TC6393_FB,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static int tc6393xb_fb_enable(struct platform_device *dev)
> +{
> +	struct tc6393xb *tc6393xb = dev_get_drvdata(dev->dev.parent);
> +	unsigned long flags;
> +	u16 ccr;
> +
> +	spin_lock_irqsave(&tc6393xb->lock, flags);
> +
> +	ccr = tmio_ioread16(tc6393xb->scr + SCR_CCR);
> +	ccr &= ~SCR_CCR_MCLK_MASK;
> +	ccr |= SCR_CCR_MCLK_48;
> +	tmio_iowrite16(ccr, tc6393xb->scr + SCR_CCR);
> +
> +	spin_unlock_irqrestore(&tc6393xb->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int tc6393xb_fb_disable(struct platform_device *dev)
> +{
> +	struct tc6393xb *tc6393xb = dev_get_drvdata(dev->dev.parent);
> +	unsigned long flags;
> +	u16 ccr;
> +
> +	spin_lock_irqsave(&tc6393xb->lock, flags);
> +
> +	ccr = tmio_ioread16(tc6393xb->scr + SCR_CCR);
> +	ccr &= ~SCR_CCR_MCLK_MASK;
> +	ccr |= SCR_CCR_MCLK_OFF;
> +	tmio_iowrite16(ccr, tc6393xb->scr + SCR_CCR);
> +
> +	spin_unlock_irqrestore(&tc6393xb->lock, flags);
> +
> +	return 0;
> +}
> +
> +int tc6393xb_lcd_set_power(struct platform_device *fb, bool on)
> +{
> +	struct platform_device *dev = to_platform_device(fb->dev.parent);
> +	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
> +	u8 fer;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tc6393xb->lock, flags);
> +
> +	fer = ioread8(tc6393xb->scr + SCR_FER);
> +	if (on)
> +		fer |= SCR_FER_SLCDEN;
> +	else
> +		fer &= ~SCR_FER_SLCDEN;
> +	iowrite8(fer, tc6393xb->scr + SCR_FER);
> +
> +	spin_unlock_irqrestore(&tc6393xb->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tc6393xb_lcd_set_power);
> +
> +int tc6393xb_lcd_mode(struct platform_device *fb,
> +					const struct fb_videomode *mode) {
> +	struct platform_device *dev = to_platform_device(fb->dev.parent);
> +	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tc6393xb->lock, flags);
> +
> +	iowrite16(mode->pixclock, tc6393xb->scr + SCR_PLL1CR + 0);
> +	iowrite16(mode->pixclock >> 16, tc6393xb->scr + SCR_PLL1CR + 2);
> +
> +	spin_unlock_irqrestore(&tc6393xb->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tc6393xb_lcd_mode);
> +
>  static struct mfd_cell __devinitdata tc6393xb_cells[] = {
>  	[TC6393XB_CELL_NAND] = {
>  		.name = "tmio-nand",
> @@ -182,6 +281,15 @@ static struct mfd_cell __devinitdata tc6393xb_cells[] = {
>  		.num_resources = ARRAY_SIZE(tc6393xb_mmc_resources),
>  		.resources = tc6393xb_mmc_resources,
>  	},
> +	[TC6393XB_CELL_FB] = {
> +		.name = "tmio-fb",
> +		.num_resources = ARRAY_SIZE(tc6393xb_fb_resources),
> +		.resources = tc6393xb_fb_resources,
> +		.enable = tc6393xb_fb_enable,
> +		.suspend = tc6393xb_fb_disable,
> +		.resume = tc6393xb_fb_enable,
> +		.disable = tc6393xb_fb_disable,
> +	},
>  };
>  
>  /*--------------------------------------------------------------------------*/
> @@ -498,6 +606,11 @@ static int __devinit tc6393xb_probe(struct platform_device *dev)
>  	tc6393xb_cells[TC6393XB_CELL_MMC].data_size =
>  		sizeof(tc6393xb_cells[TC6393XB_CELL_MMC]);
>  
> +	tc6393xb_cells[TC6393XB_CELL_FB].driver_data = tcpd->fb_data;
> +	tc6393xb_cells[TC6393XB_CELL_FB].platform_data =
> +		&tc6393xb_cells[TC6393XB_CELL_FB];
> +	tc6393xb_cells[TC6393XB_CELL_FB].data_size =
> +		sizeof(tc6393xb_cells[TC6393XB_CELL_FB]);
>  
>  	ret = mfd_add_devices(&dev->dev, dev->id,
>  			tc6393xb_cells, ARRAY_SIZE(tc6393xb_cells),
> diff --git a/include/linux/mfd/tc6393xb.h b/include/linux/mfd/tc6393xb.h
> index fec7b3f..28a80a2 100644
> --- a/include/linux/mfd/tc6393xb.h
> +++ b/include/linux/mfd/tc6393xb.h
> @@ -33,13 +33,19 @@ struct tc6393xb_platform_data {
>  	int	gpio_base;
>  
>  	struct tmio_nand_data	*nand_data;
> +	struct tmio_fb_data	*fb_data;
>  };
>  
> +extern int tc6393xb_lcd_mode(struct platform_device *fb,
> +					const struct fb_videomode *mode);
> +extern int tc6393xb_lcd_set_power(struct platform_device *fb, bool on);
> +
>  /*
>   * Relative to irq_base
>   */
>  #define	IRQ_TC6393_NAND		0
>  #define	IRQ_TC6393_MMC		1
> +#define	IRQ_TC6393_FB		4
>  
>  #define	TC6393XB_NR_IRQS	8
>  
> -- 
> 1.5.6.5
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Dmitry]

Hi,

Thank you for the review,

2008/10/1 Krzysztof Helt <krzysztof.h1@poczta.fm>:
> On Tue, 30 Sep 2008 12:38:29 +0400
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
>> Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
>> chips.
>>
>> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
>> Cc: Ian Molton <spyro@f2s.com>
>> Cc: Samuel Ortiz <sameo@openedhand.com>
>> ---
>
> A more detailed review now:
>
>
>>  drivers/video/Kconfig    |   22 +
>>  drivers/video/Makefile   |    1 +
>>  drivers/video/tmiofb.c   | 1036 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/tmio.h |   15 +
>>  4 files changed, 1074 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/video/tmiofb.c
>>
>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>> index 70d135e..cf71db4 100644
>> --- a/drivers/video/Kconfig
>> +++ b/drivers/video/Kconfig
>> @@ -1876,6 +1876,28 @@ config FB_SH_MOBILE_LCDC
>>       ---help---
>>         Frame buffer driver for the on-chip SH-Mobile LCD controller.
>>
>> +config FB_TMIO
>> +     tristate "Toshiba Mobice IO FrameBuffer support"
>> +     depends on FB && MFD_CORE
>> +     select FB_CFB_FILLRECT
>> +     select FB_CFB_COPYAREA
>> +     select FB_CFB_IMAGEBLIT
>> +     ---help---
>> +       Frame buffer driver for the Toshiba Mobile IO integrated as found
>> +       on the Sharp SL-6000 series
>> +
>> +       This driver is also available as a module ( = code which can be
>> +       inserted and removed from the running kernel whenever you want). The
>> +       module will be called tmiofb. If you want to compile it as a module,
>> +       say M here and read <file:Documentation/kbuild/modules.txt>.
>> +
>> +       If unsure, say N.
>> +
>> +config FB_TMIO_ACCELL
>> +     bool "tmiofb acceleration"
>> +     depends on FB_TMIO
>> +     default y
>> +
>>  config FB_S3C2410
>>       tristate "S3C2410 LCD framebuffer support"
>>       depends on FB && ARCH_S3C2410
>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
>> index a6b5529..32ca1f9 100644
>> --- a/drivers/video/Makefile
>> +++ b/drivers/video/Makefile
>> @@ -98,6 +98,7 @@ obj-$(CONFIG_FB_CIRRUS)               += cirrusfb.o
>>  obj-$(CONFIG_FB_ASILIANT)      += asiliantfb.o
>>  obj-$(CONFIG_FB_PXA)           += pxafb.o
>>  obj-$(CONFIG_FB_W100)                  += w100fb.o
>> +obj-$(CONFIG_FB_TMIO)                  += tmiofb.o
>>  obj-$(CONFIG_FB_AU1100)                += au1100fb.o
>>  obj-$(CONFIG_FB_AU1200)                += au1200fb.o
>>  obj-$(CONFIG_FB_PMAG_AA)       += pmag-aa-fb.o
>> diff --git a/drivers/video/tmiofb.c b/drivers/video/tmiofb.c
>> new file mode 100644
>> index 0000000..6c18f89
>> --- /dev/null
>> +++ b/drivers/video/tmiofb.c
>> @@ -0,0 +1,1036 @@
>> +/*
>> + * Frame Buffer Device for Toshiba Mobile IO(TMIO) controller
>> + *
>> + * Copyright(C) 2005-2006 Chris Humbert
>> + * Copyright(C) 2005 Dirk Opfer
>> + * Copytight(C) 2007,2008 Dmitry Baryshkov
>> + *
>> + * Based on:
>> + *   drivers/video/w100fb.c
>> + *   code written by Sharp/Lineo for 2.4 kernels
>> + *
>> + * 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, or
>> + * (at your option) any later version.
>> + *
>
> I don't know if the clause "or (at your option) any later version" is widely
> accepted for kernel sources. See Linus post here:
>
> http://lkml.org/lkml/2006/1/25/273
>
> A safer clause is:
>
>  * This file is subject to the terms and conditions of the GNU General Public
>  * License. See the file COPYING in the main directory of this archive for
>  * more details.

I'd instead change it to:
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2
 * as published by the Free Software Foundation;

Is it suitable?

>
> If you can cut one tab before values you can fit the section below within 80-column limit.

done

>> +struct tmiofb_par {
>> +     u32                             pseudo_palette[16];
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     wait_queue_head_t               wait_acc;
>> +     bool                            use_polling;
>> +#endif
>> +
>> +     void __iomem                    *ccr;
>> +     void __iomem                    *lcr;
>> +     void __iomem                    *vram;
>
> The vram field is not needed as it is always equal to fb_info->screen_base.

I'd keep it for simplicity and simmery, as par then contains all
ioremapped regions.

>
>> +};
>> +
>> +/*--------------------------------------------------------------------------*/
>> +
>> +static irqreturn_t tmiofb_irq(int irq, void *__info);
>
> You can move the trmiofb_irq earlier and just drop the forward
> declaration.

done

>> +
>> +     data->lcd_set_power(dev, 1);
>> +     mdelay(2);
>
> msleep() is preferred over mdelay() here and below.

changed.

>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +static int __must_check
>> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
>> +{
>> +     struct tmiofb_par               *par    = info->par;
>
> Such a wide formatting does not look good here and in few functions below.

Oops... I thought I've already removed it.

>> +static void tmiofb_clearscreen(struct fb_info *info)
>> +{
>> +     const struct fb_fillrect rect = {
>> +             .dx     = 0,
>> +             .dy     = 0,
>> +             .width  = info->mode->xres,
>> +             .height = info->mode->yres,
>> +             .color  = 0,
>> +     };
>> +
>> +     info->fbops->fb_fillrect(info, &rect);
>> +}
>> +
>
> This function is probably redundant as it is used only inside
> the set_par() function. My experience shows that the screen is
> blanked every time set_par() changes parameters so clearing
> it again does nothing. Please test if this function is needed.

I'll check it next week (don't have the hw at hand now). Next patch generation
will still contain it.

>
>> +static int tmiofb_vblank(struct fb_info *fbi, struct fb_vblank *vblank)
>> +{
>> +     struct tmiofb_par       *par    = fbi->par;
>> +     struct fb_videomode     *mode   = fbi->mode;
>> +     unsigned int            vcount  = tmio_ioread16(par->lcr + LCR_CDLN);
>> +     unsigned int            vds     = mode->vsync_len + mode->upper_margin;
>> +
>> +     vblank->vcount  = vcount;
>> +     vblank->flags   = FB_VBLANK_HAVE_VBLANK | FB_VBLANK_HAVE_VCOUNT
>> +                                             | FB_VBLANK_HAVE_VSYNC;
>> +
>> +     if (vcount < mode->vsync_len)
>> +             vblank->flags |= FB_VBLANK_VSYNCING;
>> +
>> +     if (vcount < vds || vcount > vds + mode->yres)
>> +             vblank->flags |= FB_VBLANK_VBLANKING;
>> +
>> +     return 0;
>> +}
>> +
>> +
>> +static int tmiofb_ioctl(struct fb_info *fbi,
>> +             unsigned int cmd, unsigned long arg)
>> +{
>> +     switch (cmd) {
>> +     case FBIOGET_VBLANK: {
>> +             struct fb_vblank        vblank  = {0};
>> +             void __user             *argp   = (void __user *) arg;
>> +
>> +             tmiofb_vblank(fbi, &vblank);
>> +             if (copy_to_user(argp, &vblank, sizeof vblank))
>> +                             return -EFAULT;
>> +             return 0;
>> +     }
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     case FBIO_TMIO_ACC_SYNC:
>> +             tmiofb_sync(fbi);
>> +             return 0;
>> +
>> +     case FBIO_TMIO_ACC_WRITE: {
>> +             u32 __user      *argp   = (void __user *) arg;
>> +             u32             len;
>> +             u32             acc [16];
>> +
>> +             if (copy_from_user(&len, argp, sizeof(u32)))
>> +                     return -EFAULT;
>> +             if (len > ARRAY_SIZE(acc))
>> +                     return -EINVAL;
>> +             if (copy_from_user(acc, argp + 1, sizeof(u32) * len))
>> +                     return -EFAULT;
>> +
>> +             return tmiofb_acc_write(fbi, acc, len);
>> +     }
>> +#endif
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +/*--------------------------------------------------------------------------*/
>> +
>> +/* Select the smallest mode that allows the desired resolution to be
>> + * displayed.  If desired, the x and y parameters can be rounded up to
>> + * match the selected mode.
>> + */
>> +static struct fb_videomode*
>> +tmiofb_find_mode(struct fb_info *info, struct fb_var_screeninfo *var)
>> +{
>> +     struct mfd_cell                 *cell   = to_platform_device(info->device)->dev.platform_data;
>> +     struct tmio_fb_data             *data   = cell->driver_data;
>> +     struct fb_videomode             *best   = NULL;
>> +     int                             i;
>> +
>> +     for (i = 0; i < data->num_modes; i++) {
>> +             struct fb_videomode *mode = data->modes + i;
>> +
>> +             if (mode->xres >= var->xres && mode->yres >= var->yres
>> +                             && (!best || (mode->xres < best->xres
>> +                                        && mode->yres < best->yres)))
>> +                     best = mode;
>> +     }
>> +
>> +     return best;
>> +}
>> +
>> +static int tmiofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
>> +{
>> +
>> +     struct fb_videomode     *mode;
>> +
>> +     mode = tmiofb_find_mode(info, var);
>> +     if (!mode || var->bits_per_pixel > 16)
>> +             return -EINVAL;
>> +
>> +     fb_videomode_to_var(var, mode);
>> +
>> +     var->xres_virtual       = mode->xres;
>> +     var->yres_virtual       = info->screen_size / (mode->xres * 2);
>
> This could be a mistake as the yres_virtual is half size of the possible
> memory. There is no check if the yres > yres_virtual nor that
> (screen_size >= 2 * xres).

screen_size = xres * yres_virtual * bytes_per_pixel, so /2 is correct.


>> +     var->xoffset            = 0;
>> +     var->yoffset            = 0;
>> +     var->bits_per_pixel     = 16;
>> +     var->grayscale          = 0;
>> +     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;
>> +     var->nonstd             = 0;
>> +     var->height             = 82;   /* mm */
>> +     var->width              = 60;   /* mm */
>
> Shouldn't the height and width be in the platform_data
> as the resolution?

good idea!

>
>> +     var->rotate             = 0;
>> +     return 0;
>> +}
>> +
>> +static int tmiofb_set_par(struct fb_info *info)
>> +{
>> +     struct fb_var_screeninfo        *var    = &info->var;
>> +     struct fb_videomode             *mode;
>> +
>> +     mode = tmiofb_find_mode(info, var);
>> +     if (!mode)
>> +             return -EINVAL;
>> +
>> +/*   if (info->mode == mode)
>> +             return 0;*/
>> +
>> +     info->mode              = mode;
>> +     info->fix.line_length   = info->mode->xres * 2;
>
> I would go for (var->bpp / 8) instead of 2 here (or a comment).

fine with me.

>
>> +
>> +     tmiofb_hw_mode(to_platform_device(info->device));
>> +     tmiofb_clearscreen(info);
>> +     return 0;
>> +}
>> +
>> +static int tmiofb_setcolreg(unsigned regno, unsigned red, unsigned green,
>> +                        unsigned blue, unsigned transp,
>> +                        struct fb_info *info)
>> +{
>> +     struct tmiofb_par       *par    = info->par;
>> +
>> +     if (regno < ARRAY_SIZE(par->pseudo_palette)) {
>> +             par->pseudo_palette [regno] =
>> +                     ((red   & 0xf800))              |
>> +                     ((green & 0xfc00) >>  5)        |
>> +                     ((blue  & 0xf800) >> 11);
>> +             return 0;
>> +     }
>> +
>> +     return 1;
>> +}
>> +
>> +static int tmiofb_blank(int blank, struct fb_info *info)
>> +{
>> +     /*
>> +      * everything is done in lcd/bl drivers.
>> +      * this is purely to make sysfs happy and work.
>> +      */
>> +     return 0;
>> +}
>> +
>> +static struct fb_ops tmiofb_ops = {
>> +     .owner          = THIS_MODULE,
>> +
>> +     .fb_ioctl       = tmiofb_ioctl,
>> +     .fb_check_var   = tmiofb_check_var,
>> +     .fb_set_par     = tmiofb_set_par,
>> +     .fb_setcolreg   = tmiofb_setcolreg,
>> +     .fb_blank       = tmiofb_blank,
>> +     .fb_imageblit   = cfb_imageblit,
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     .fb_sync        = tmiofb_sync,
>> +     .fb_fillrect    = tmiofb_fillrect,
>> +     .fb_copyarea    = tmiofb_copyarea,
>> +#else
>> +     .fb_fillrect    = cfb_fillrect,
>> +     .fb_copyarea    = cfb_copyarea,
>> +#endif
>> +};
>> +
>> +/*--------------------------------------------------------------------------*/
>> +
>> +/*
>> + * reasons for an interrupt:
>> + *   uis     bbisc   lcdis
>> + *   0100    0001            accelerator command completed
>> + *   2000            0001    vsync start
>> + *   2000            0002    display start
>> + *   2000            0004    line number match(0x1ff mask???)
>> + */
>> +static irqreturn_t tmiofb_irq(int irq, void *__info)
>> +{
>> +     struct fb_info                  *info   = __info;
>> +     struct tmiofb_par               *par    = info->par;
>> +     unsigned int                    bbisc   = tmio_ioread16(par->lcr + LCR_BBISC);
>> +
>> +
>> +     if (unlikely(par->use_polling && irq != -1)) {
>> +             printk(KERN_INFO "tmiofb: switching to waitq\n");
>> +             par->use_polling = false;
>> +     }
>> +
>> +     tmio_iowrite16(bbisc, par->lcr + LCR_BBISC);
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     if (bbisc & 1)
>> +             wake_up(&par->wait_acc);
>> +#endif
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit tmiofb_probe(struct platform_device *dev)
>> +{
>> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> +     struct tmio_fb_data             *data   = cell->driver_data;
>> +     struct resource                 *ccr    = platform_get_resource(dev, IORESOURCE_MEM, 1);
>> +     struct resource                 *lcr    = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> +     struct resource                 *vram   = platform_get_resource(dev, IORESOURCE_MEM, 2);
>> +     int                             irq     = platform_get_irq(dev, 0);
>> +     struct fb_info                  *info;
>> +     struct tmiofb_par               *par;
>> +     int                             retval;
>> +
>> +     if (data == NULL) {
>> +             dev_err(&dev->dev, "NULL platform data!\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     info = framebuffer_alloc(sizeof(struct tmiofb_par), &dev->dev);
>> +
>> +     if (!info) {
>> +             retval = -ENOMEM;
>> +             goto err_framebuffer_alloc;
>
> You can do just return -ENOMEM here (as in the condition above).

done

>
>> +     }
>> +
>> +     par = info->par;
>> +     platform_set_drvdata(dev, info);
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     init_waitqueue_head(&par->wait_acc);
>> +
>> +     par->use_polling        = true;
>> +
>> +     info->flags             = FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA
>> +                                              | FBINFO_HWACCEL_FILLRECT;
>> +#else
>> +     info->flags             = FBINFO_DEFAULT;
>> +#endif
>> +
>> +     info->fbops             = &tmiofb_ops;
>> +
>> +     strcpy(info->fix.id, "tmio-fb");
>> +     info->fix.smem_start    = vram->start;
>> +     info->fix.smem_len      = vram->end - vram->start + 1;
>
> resource_size() function is handy here.

done

>
>> +     info->fix.type          = FB_TYPE_PACKED_PIXELS;
>> +     info->fix.visual        = FB_VISUAL_TRUECOLOR;
>> +     info->fix.mmio_start    = lcr->start;
>> +     info->fix.mmio_len      = lcr->end - lcr->start + 1;
>
> resource_size() function is handy here and below in this function.

done

>
>> +     info->fix.accel         = FB_ACCEL_NONE;
>> +     info->screen_size       = info->fix.smem_len - (4 * TMIOFB_FIFO_SIZE);
>> +     info->pseudo_palette    = par->pseudo_palette;
>> +
>> +     par->ccr = ioremap(ccr->start, ccr->end - ccr->start + 1);
>> +     if (!par->ccr) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_ccr;
>> +     }
>> +
>> +     par->lcr = ioremap(info->fix.mmio_start, info->fix.mmio_len);
>> +     if (!par->lcr) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_lcr;
>> +     }
>> +
>> +     par->vram = ioremap(info->fix.smem_start, info->fix.smem_len);
>> +     if (!par->vram) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_vram;
>> +     }
>> +     info->screen_base = par->vram;
>> +
>> +     retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
>> +                                     dev->dev.bus_id, info);
>> +
>> +     if (retval)
>> +             goto err_request_irq;
>> +
>> +     retval = fb_find_mode(&info->var, info, mode_option,
>> +                     data->modes, data->num_modes,
>> +                     data->modes, 16);
>> +     if (!retval) {
>> +             retval = -EINVAL;
>> +             goto err_find_mode;
>> +     }
>> +
>> +     if (cell->enable) {
>> +             retval = cell->enable(dev);
>> +             if (retval)
>> +                     goto err_enable;
>> +     }
>> +
>> +     retval = tmiofb_hw_init(dev);
>> +     if (retval)
>> +             goto err_hw_init;
>> +
>> +/*   retval = tmiofb_set_par(info);
>> +     if (retval)
>> +             goto err_set_par;*/
>> +
>
> Please kill or enable the code above.

killed the code as it was a leftover from earlier driver.

>
>> +     fb_videomode_to_modelist(data->modes, data->num_modes,
>> +                              &info->modelist);
>> +
>> +     retval = register_framebuffer(info);
>> +     if (retval < 0)
>> +             goto err_register_framebuffer;
>> +
>> +     printk(KERN_INFO "fb%d: %s frame buffer device\n",
>> +                             info->node, info->fix.id);
>> +
>> +     return 0;
>> +
>> +err_register_framebuffer:
>> +/*err_set_par:*/
>> +     tmiofb_hw_stop(dev);
>> +err_hw_init:
>> +     if (cell->disable)
>> +             cell->disable(dev);
>> +err_enable:
>> +err_find_mode:
>> +     free_irq(irq, info);
>> +err_request_irq:
>> +     iounmap(par->vram);
>> +err_ioremap_vram:
>> +     iounmap(par->lcr);
>> +err_ioremap_lcr:
>> +     iounmap(par->ccr);
>> +err_ioremap_ccr:
>> +     platform_set_drvdata(dev, NULL);
>> +     framebuffer_release(info);
>> +err_framebuffer_alloc:
>> +     return retval;
>> +}
>> +
>> +static int __devexit tmiofb_remove(struct platform_device *dev)
>> +{
>> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> +     struct fb_info                  *info   = platform_get_drvdata(dev);
>> +     int                             irq     = platform_get_irq(dev, 0);
>> +     struct tmiofb_par               *par;
>> +
>> +     if (info) {
>> +             par = info->par;
>> +             unregister_framebuffer(info);
>> +
>> +             tmiofb_hw_stop(dev);
>> +
>> +             if (cell->disable)
>> +                     cell->disable(dev);
>> +
>> +             free_irq(irq, info);
>> +
>> +             iounmap(par->vram);
>> +             iounmap(par->lcr);
>> +             iounmap(par->ccr);
>> +
>> +             framebuffer_release(info);
>> +             platform_set_drvdata(dev, NULL);
>
> Exchange these two lines above.

done.

>
> The rest of the driver is fine.
>
> Please run checkpatch.pl script and try to keep the code in 80-column format (many of longer lines can be easily converted to something shorter).

I've left only one line > 80 column for comments format consistency.

Final patch following in the next mail.

-- 
With best wishes
Dmitry

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Dmitry]

Hi,

2008/10/3 Andrew Morton <akpm@linux-foundation.org>:
> On Tue, 30 Sep 2008 12:38:29 +0400 Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
>> Add driver for TMIO framebuffer cells as found e.g. in Toshiba TC6393XB
>> chips.
>>
>
> Please feed the diff through scripts/checkpatch.pl and consider the
> resulting report.  afacit all the warnings could/should be repaired,
> except for the 80-col ones.  And even some of the latter could be
> addressed just by being a bit more careful with the tab key.

This and few other issues are already handled in the updated version
of the patch just sent.

>
>>
>> ...
>>
>> +config FB_TMIO
>> +     tristate "Toshiba Mobice IO FrameBuffer support"
>> +     depends on FB && MFD_CORE
>> +     select FB_CFB_FILLRECT
>> +     select FB_CFB_COPYAREA
>> +     select FB_CFB_IMAGEBLIT
>> +     ---help---
>> +       Frame buffer driver for the Toshiba Mobile IO integrated as found
>> +       on the Sharp SL-6000 series
>> +
>> +       This driver is also available as a module ( = code which can be
>> +       inserted and removed from the running kernel whenever you want). The
>> +       module will be called tmiofb. If you want to compile it as a module,
>> +       say M here and read <file:Documentation/kbuild/modules.txt>.
>> +
>> +       If unsure, say N.
>> +
>> +config FB_TMIO_ACCELL
>> +     bool "tmiofb acceleration"
>> +     depends on FB_TMIO
>> +     default y
>
> Why does FB_TMIO_ACCELL exist?  Can we remove it and make that code
> unconditional?

The accel code is a bit experimental. I'd prefer to leave the ability
to disable it
for now.

>> +     tmio_iowrite16(0, par->ccr + CCR_UGCC);
>> +     tmio_iowrite16(0, par->lcr + LCR_GM);
>> +     data->lcd_set_power(dev, 0);
>> +     tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Initializes the LCD host controller.
>> + */
>> +static int tmiofb_hw_init(struct platform_device *dev)
>> +{
>> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> +     struct tmio_fb_data             *data   = cell->driver_data;
>> +     struct fb_info                  *info   = platform_get_drvdata(dev);
>> +     struct tmiofb_par               *par    = info->par;
>> +     const struct resource           *nlcr   = &cell->resources[0];
>> +     const struct resource           *vram   = &cell->resources[2];
>> +     unsigned long                   base;
>> +
>> +     if (nlcr == NULL || vram == NULL)
>> +             return -EINVAL;
>> +
>> +     base = nlcr->start;
>> +
>> +     if (info->mode == NULL) {
>> +             printk(KERN_ERR "tmio-fb: null info->mode\n");
>
> Can this happen?

It does happen during initial probing AFAIR. I'll recheck later.

>
>> +             info->mode = data->modes;
>> +     }
>> +
>> +
>> +     tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
>> +     mdelay(5);
>> +     tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC); /* STOP_CKP */
>> +     mdelay(5);
>> +     tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC); /* STOP_CKP | SOFT_RESET */
>> +     tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);
>
> I trust that someone who is reasonably familiar with the hardware will
> know why all these magical mdelay()s are here.  But even if they are,
> some comments explaining them would enhance maintainability.

that's just for clocks/reset to happen.

>> +}
>> +
>> +/*--------------------------------------------------------------------------*/
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +static int __must_check
>> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
>> +{
>> +     struct tmiofb_par               *par    = info->par;
>> +     if (in_atomic() || par->use_polling) {
>
> No, use of in_atomic() is almost always wrong.  It returns "false"
> inside a spinlocked section when CONFIG_PREEMPT=n.
>
> I cannot advise you further because (tada) there is no comment
> explaining this code to me.  But it will need to be rethought, please.

The problem is that code may call fb functions with interrupts disabled.
One of examples is suspend/resume path, when console seems to be
resumed before interrupts are enabled (this is governed
by par->use_polling). I'm not sure that there are no other uses of fb
functions in atomic context.

>> +             int i = 0;
>> +             while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
>> +                     udelay(1);
>> +                     i++;
>> +                     if (i > 10000) {
>> +                             printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
>> +                             return -ETIMEDOUT;
>> +                     }
>> +                     tmiofb_irq(-1, info);
>> +             }
>> +     } else {
>> +             if (!wait_event_interruptible_timeout(par->wait_acc,
>> +                             tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
>> +                     printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
>> +                     return -ETIMEDOUT;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>
>> ...
>>
>> +static void
>> +tmiofb_fillrect(struct fb_info *fbi, const struct fb_fillrect *rect)
>> +{
>> +     const u32 cmd [] = {
>> +             TMIOFB_ACC_DSADR((rect->dy * fbi->mode->xres + rect->dx) * 2),
>> +             TMIOFB_ACC_DHPIX(rect->width    - 1),
>> +             TMIOFB_ACC_DVPIX(rect->height   - 1),
>
> you really like those tabs ;)
>
>> +             TMIOFB_ACC_FILL(rect->color),
>> +             TMIOFB_ACC_FLGO,
>> +     };
>> +
>> +     if (fbi->state != FBINFO_STATE_RUNNING ||
>> +         fbi->flags & FBINFO_HWACCEL_DISABLED) {
>> +             cfb_fillrect(fbi, rect);
>> +             return;
>> +     }
>> +
>> +     tmiofb_acc_write(fbi, cmd, ARRAY_SIZE(cmd));
>> +}
>> +
>>
>> ...
>>
>> +static void tmiofb_clearscreen(struct fb_info *info)
>> +{
>> +     const struct fb_fillrect rect = {
>> +             .dx     = 0,
>> +             .dy     = 0,
>> +             .width  = info->mode->xres,
>> +             .height = info->mode->yres,
>> +             .color  = 0,
>> +     };
>
> fyi, the members which are to be initialised to zero don't need to be
> listed with this construct.  Although one might choose to fill them in
> for readbility/documentation reasons (in which case: where's
> fb_fillrect.rop?)

I'll think about this.  If you insist, I'll drop those =0 from next generation
of the patch.

>
>> +     info->fbops->fb_fillrect(info, &rect);
>> +}
>> +
>> +static int tmiofb_vblank(struct fb_info *fbi, struct fb_vblank *vblank)
>> +{
>> +     struct tmiofb_par       *par    = fbi->par;
>> +     struct fb_videomode     *mode   = fbi->mode;
>> +     unsigned int            vcount  = tmio_ioread16(par->lcr + LCR_CDLN);
>> +     unsigned int            vds     = mode->vsync_len + mode->upper_margin;
>> +
>> +     vblank->vcount  = vcount;
>> +     vblank->flags   = FB_VBLANK_HAVE_VBLANK | FB_VBLANK_HAVE_VCOUNT
>> +                                             | FB_VBLANK_HAVE_VSYNC;
>> +
>> +     if (vcount < mode->vsync_len)
>> +             vblank->flags |= FB_VBLANK_VSYNCING;
>> +
>> +     if (vcount < vds || vcount > vds + mode->yres)
>> +             vblank->flags |= FB_VBLANK_VBLANKING;
>> +
>> +     return 0;
>> +}
>> +
>> +
>> +static int tmiofb_ioctl(struct fb_info *fbi,
>> +             unsigned int cmd, unsigned long arg)
>> +{
>> +     switch (cmd) {
>> +     case FBIOGET_VBLANK: {
>> +             struct fb_vblank        vblank  = {0};
>> +             void __user             *argp   = (void __user *) arg;
>> +
>> +             tmiofb_vblank(fbi, &vblank);
>> +             if (copy_to_user(argp, &vblank, sizeof vblank))
>> +                             return -EFAULT;
>
> An extra indent there.

removed

>
>> +             return 0;
>> +     }
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     case FBIO_TMIO_ACC_SYNC:
>> +             tmiofb_sync(fbi);
>> +             return 0;
>> +
>> +     case FBIO_TMIO_ACC_WRITE: {
>> +             u32 __user      *argp   = (void __user *) arg;
>> +             u32             len;
>> +             u32             acc [16];
>> +
>> +             if (copy_from_user(&len, argp, sizeof(u32)))
>> +                     return -EFAULT;
>
> get_user() would be simpler and quicker.

in the next generation of the patch

>
>> +             if (len > ARRAY_SIZE(acc))
>> +                     return -EINVAL;
>> +             if (copy_from_user(acc, argp + 1, sizeof(u32) * len))
>> +                     return -EFAULT;
>> +
>> +             return tmiofb_acc_write(fbi, acc, len);
>> +     }
>
> So there's no way for userspace to read back the current setting of
> `acc' (whatever that is)?

acc is a serie of accelerator commands dircetly written to command
FIFO of the card
This hook is used e.g. by proprietary original accelerated libqte.
There is nothing to read back.

>> +#endif
>> +     }
>> +
>> +     return -EINVAL;
>
> Unimplemented ioctl numbers usually return -ENOTTY, although your
> -EINVAL might get converted to -ENOTTY at some higher level (which
> would be a bad thing).  Please check.

That is strange, since fb_ioctl() (see drivers/video/fbmem.c) does
return -EINVAL
in case of unsupported ioctl value. From the man page:

EINVAL Request or argp is not valid.
ENOTTY The specified request does not apply to the kind of object that
the descriptor d references.

So, it a bit unclear from that page what each value means. Changed
anyway per your request.

>> +}
>> +
>> +/*--------------------------------------------------------------------------*/
>> +
>> +/* Select the smallest mode that allows the desired resolution to be
>> + * displayed.  If desired, the x and y parameters can be rounded up to
>> + * match the selected mode.
>> + */
>> +static struct fb_videomode*
>
> A single space before the "*", please.  checkpatch missed this.

changed

>
>> +tmiofb_find_mode(struct fb_info *info, struct fb_var_screeninfo *var)
>> +{
>> +     struct mfd_cell                 *cell   = to_platform_device(info->device)->dev.platform_data;
>> +     struct tmio_fb_data             *data   = cell->driver_data;
>> +     struct fb_videomode             *best   = NULL;
>> +     int                             i;
>> +
>> +     for (i = 0; i < data->num_modes; i++) {
>> +             struct fb_videomode *mode = data->modes + i;
>> +
>> +             if (mode->xres >= var->xres && mode->yres >= var->yres
>> +                             && (!best || (mode->xres < best->xres
>> +                                        && mode->yres < best->yres)))
>> +                     best = mode;
>> +     }
>> +
>> +     return best;
>> +}
>> +
>> +static int tmiofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
>> +{
>> +
>> +     struct fb_videomode     *mode;
>> +
>> +     mode = tmiofb_find_mode(info, var);
>> +     if (!mode || var->bits_per_pixel > 16)
>> +             return -EINVAL;
>> +
>> +     fb_videomode_to_var(var, mode);
>> +
>> +     var->xres_virtual       = mode->xres;
>> +     var->yres_virtual       = info->screen_size / (mode->xres * 2);
>> +     var->xoffset            = 0;
>> +     var->yoffset            = 0;
>> +     var->bits_per_pixel     = 16;
>> +     var->grayscale          = 0;
>> +     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;
>
> One statement per line is preferred, please.

changed

>
>> +     var->nonstd             = 0;
>> +     var->height             = 82;   /* mm */
>> +     var->width              = 60;   /* mm */
>> +     var->rotate             = 0;
>> +     return 0;
>> +}
>> +
>>
>> ...
>>
>> +static int tmiofb_setcolreg(unsigned regno, unsigned red, unsigned green,
>> +                        unsigned blue, unsigned transp,
>> +                        struct fb_info *info)
>> +{
>> +     struct tmiofb_par       *par    = info->par;
>> +
>> +     if (regno < ARRAY_SIZE(par->pseudo_palette)) {
>> +             par->pseudo_palette [regno] =
>> +                     ((red   & 0xf800))              |
>> +                     ((green & 0xfc00) >>  5)        |
>> +                     ((blue  & 0xf800) >> 11);
>> +             return 0;
>> +     }
>> +
>> +     return 1;
>> +}
>
> Does .fb_setcolreg really return positive non-zero on error?  That
> would be a bit sad - it should expect a -ve errno and should propagate
> that back.  Oh well.  Please check it?

You were right. Corrected.

>
>> +static int tmiofb_blank(int blank, struct fb_info *info)
>> +{
>> +     /*
>> +      * everything is done in lcd/bl drivers.
>> +      * this is purely to make sysfs happy and work.
>> +      */
>> +     return 0;
>> +}
>> +
>>
>> ...
>>
>> +/*
>> + * reasons for an interrupt:
>> + *   uis     bbisc   lcdis
>> + *   0100    0001            accelerator command completed
>> + *   2000            0001    vsync start
>> + *   2000            0002    display start
>> + *   2000            0004    line number match(0x1ff mask???)
>> + */
>> +static irqreturn_t tmiofb_irq(int irq, void *__info)
>> +{
>> +     struct fb_info                  *info   = __info;
>> +     struct tmiofb_par               *par    = info->par;
>> +     unsigned int                    bbisc   = tmio_ioread16(par->lcr + LCR_BBISC);
>> +
>> +
>> +     if (unlikely(par->use_polling && irq != -1)) {
>> +             printk(KERN_INFO "tmiofb: switching to waitq\n");
>> +             par->use_polling = false;
>> +     }
>
> It'd be nice if there was a comment somewhere explaining to the reader
> what's actually happening here.  Some overall design/perspective thing.

see the patch (version 3).

>
>> +     tmio_iowrite16(bbisc, par->lcr + LCR_BBISC);
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     if (bbisc & 1)
>> +             wake_up(&par->wait_acc);
>> +#endif
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit tmiofb_probe(struct platform_device *dev)
>> +{
>> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> +     struct tmio_fb_data             *data   = cell->driver_data;
>> +     struct resource                 *ccr    = platform_get_resource(dev, IORESOURCE_MEM, 1);
>> +     struct resource                 *lcr    = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> +     struct resource                 *vram   = platform_get_resource(dev, IORESOURCE_MEM, 2);
>> +     int                             irq     = platform_get_irq(dev, 0);
>> +     struct fb_info                  *info;
>> +     struct tmiofb_par               *par;
>> +     int                             retval;
>> +
>> +     if (data == NULL) {
>> +             dev_err(&dev->dev, "NULL platform data!\n");
>
> Can this happen?

Of course, if you pass incorrect data to your TMIO controller chip.
This is better than kernel Oops.

>
>> +             return -EINVAL;
>> +     }
>> +
>> +     info = framebuffer_alloc(sizeof(struct tmiofb_par), &dev->dev);
>> +
>> +     if (!info) {
>> +             retval = -ENOMEM;
>> +             goto err_framebuffer_alloc;
>> +     }
>> +
>> +     par = info->par;
>> +     platform_set_drvdata(dev, info);
>> +
>> +#ifdef CONFIG_FB_TMIO_ACCELL
>> +     init_waitqueue_head(&par->wait_acc);
>> +
>> +     par->use_polling        = true;
>> +
>> +     info->flags             = FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA
>> +                                              | FBINFO_HWACCEL_FILLRECT;
>> +#else
>> +     info->flags             = FBINFO_DEFAULT;
>> +#endif
>> +
>> +     info->fbops             = &tmiofb_ops;
>> +
>> +     strcpy(info->fix.id, "tmio-fb");
>> +     info->fix.smem_start    = vram->start;
>> +     info->fix.smem_len      = vram->end - vram->start + 1;
>> +     info->fix.type          = FB_TYPE_PACKED_PIXELS;
>> +     info->fix.visual        = FB_VISUAL_TRUECOLOR;
>> +     info->fix.mmio_start    = lcr->start;
>> +     info->fix.mmio_len      = lcr->end - lcr->start + 1;
>> +     info->fix.accel         = FB_ACCEL_NONE;
>> +     info->screen_size       = info->fix.smem_len - (4 * TMIOFB_FIFO_SIZE);
>> +     info->pseudo_palette    = par->pseudo_palette;
>> +
>> +     par->ccr = ioremap(ccr->start, ccr->end - ccr->start + 1);
>> +     if (!par->ccr) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_ccr;
>> +     }
>> +
>> +     par->lcr = ioremap(info->fix.mmio_start, info->fix.mmio_len);
>> +     if (!par->lcr) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_lcr;
>> +     }
>> +
>> +     par->vram = ioremap(info->fix.smem_start, info->fix.smem_len);
>> +     if (!par->vram) {
>> +             retval = -ENOMEM;
>> +             goto err_ioremap_vram;
>> +     }
>> +     info->screen_base = par->vram;
>> +
>> +     retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
>> +                                     dev->dev.bus_id, info);
>> +
>> +     if (retval)
>> +             goto err_request_irq;
>> +
>> +     retval = fb_find_mode(&info->var, info, mode_option,
>> +                     data->modes, data->num_modes,
>> +                     data->modes, 16);
>> +     if (!retval) {
>> +             retval = -EINVAL;
>> +             goto err_find_mode;
>> +     }
>> +
>> +     if (cell->enable) {
>> +             retval = cell->enable(dev);
>> +             if (retval)
>> +                     goto err_enable;
>> +     }
>> +
>> +     retval = tmiofb_hw_init(dev);
>> +     if (retval)
>> +             goto err_hw_init;
>> +
>> +/*   retval = tmiofb_set_par(info);
>> +     if (retval)
>> +             goto err_set_par;*/
>> +
>> +     fb_videomode_to_modelist(data->modes, data->num_modes,
>> +                              &info->modelist);
>> +
>> +     retval = register_framebuffer(info);
>> +     if (retval < 0)
>> +             goto err_register_framebuffer;
>> +
>> +     printk(KERN_INFO "fb%d: %s frame buffer device\n",
>> +                             info->node, info->fix.id);
>> +
>> +     return 0;
>> +
>> +err_register_framebuffer:
>> +/*err_set_par:*/
>> +     tmiofb_hw_stop(dev);
>> +err_hw_init:
>> +     if (cell->disable)
>> +             cell->disable(dev);
>> +err_enable:
>> +err_find_mode:
>> +     free_irq(irq, info);
>> +err_request_irq:
>> +     iounmap(par->vram);
>> +err_ioremap_vram:
>> +     iounmap(par->lcr);
>> +err_ioremap_lcr:
>> +     iounmap(par->ccr);
>> +err_ioremap_ccr:
>> +     platform_set_drvdata(dev, NULL);
>> +     framebuffer_release(info);
>> +err_framebuffer_alloc:
>> +     return retval;
>> +}
>> +
>>
>> ...
>>
>> +#ifdef CONFIG_PM
>> +static int tmiofb_suspend(struct platform_device *dev, pm_message_t state)
>> +{
>> +     struct fb_info                  *info   = platform_get_drvdata(dev);
>> +     struct tmiofb_par               *par    = info->par;
>> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> +     int                             retval  = 0;
>> +
>> +     acquire_console_sem();
>> +
>> +     fb_set_suspend(info, 1);
>> +
>> +     if (info->fbops->fb_sync)
>> +             info->fbops->fb_sync(info);
>> +
>> +
>> +     printk(KERN_INFO "tmiofb: switching to polling\n");
>
> why...
>
>> +     par->use_polling = true;
>> +     tmiofb_hw_stop(dev);
>> +
>> +     if (cell->suspend)
>> +             retval = cell->suspend(dev);
>> +
>> +     release_console_sem();
>> +
>> +     return retval;
>> +}
>> +
>>
>> ...
>>
>> --- a/include/linux/mfd/tmio.h
>> +++ b/include/linux/mfd/tmio.h
>> @@ -1,6 +1,8 @@
>>  #ifndef MFD_TMIO_H
>>  #define MFD_TMIO_H
>>
>> +#include <linux/fb.h>
>> +
>>  #define tmio_ioread8(addr) readb(addr)
>>  #define tmio_ioread16(addr) readw(addr)
>>  #define tmio_ioread16_rep(r, b, l) readsw(r, b, l)
>
> I realise it isn't your code, but these:
>
> #define tmio_ioread32(addr) \
>        (((u32) readw((addr))) | (((u32) readw((addr) + 2)) << 16))
>
> #define tmio_iowrite32(val, addr) \
>        do { \
>        writew((val),       (addr)); \
>        writew((val) >> 16, (addr) + 2); \
>        } while (0)
>
> aren't good.  They reference their arguments multiple times and hence
> will misbehave if passed an expression with side-effects.
>
> As is so often the case, these never needed to be implemented as macros
> and hence should not have been.  C is better.
>
>>
>> ...
>>
>
> The code looks reasonable overall but the in_atomic() thing is a
> show-stopper.

Thanks for the review. Most of the issues you have raised will be adressed
in v3 of this patch being posted now. in_atomic() remains unfixed as I'm waiting
for the advide from your (and fbdev) side.

-- 
With best wishes
Dmitry

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Andrew Morton]

On Sun, 5 Oct 2008 01:41:15 +0400 Dmitry <dbaryshkov@gmail.com> wrote:

> >
> >> +             info->mode = data->modes;
> >> +     }
> >> +
> >> +
> >> +     tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
> >> +     mdelay(5);
> >> +     tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC); /* STOP_CKP */
> >> +     mdelay(5);
> >> +     tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC); /* STOP_CKP | SOFT_RESET */
> >> +     tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);
> >
> > I trust that someone who is reasonably familiar with the hardware will
> > know why all these magical mdelay()s are here.  But even if they are,
> > some comments explaining them would enhance maintainability.
> 
> that's just for clocks/reset to happen.

I wasn't asking what it was doing - I was asking whether it should be
left uncommented.

It's the sort of thing which simply cannot be understood by reading the
C code alone.

> >> +#ifdef CONFIG_FB_TMIO_ACCELL
> >> +static int __must_check
> >> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
> >> +{
> >> +     struct tmiofb_par               *par    = info->par;
> >> +     if (in_atomic() || par->use_polling) {
> >
> > No, use of in_atomic() is almost always wrong.  It returns "false"
> > inside a spinlocked section when CONFIG_PREEMPT=n.
> >
> > I cannot advise you further because (tada) there is no comment
> > explaining this code to me.  But it will need to be rethought, please.
> 
> The problem is that code may call fb functions with interrupts disabled.
> One of examples is suspend/resume path, when console seems to be
> resumed before interrupts are enabled (this is governed
> by par->use_polling). I'm not sure that there are no other uses of fb
> functions in atomic context.

The use of irqs_disabled() is OK (with a comment explaining what it's for!).

> >> +             int i = 0;
> >> +             while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
> >> +                     udelay(1);
> >> +                     i++;
> >> +                     if (i > 10000) {
> >> +                             printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> >> +                             return -ETIMEDOUT;
> >> +                     }
> >> +                     tmiofb_irq(-1, info);
> >> +             }
> >> +     } else {
> >> +             if (!wait_event_interruptible_timeout(par->wait_acc,
> >> +                             tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
> >> +                     printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
> >> +                     return -ETIMEDOUT;
> >> +             }
> >> +     }
> >> +
> >> +     return 0;
> >> +}
>
> ...
>
> >> +static int __devinit tmiofb_probe(struct platform_device *dev)
> >> +{
> >> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
> >> +     struct tmio_fb_data             *data   = cell->driver_data;
> >> +     struct resource                 *ccr    = platform_get_resource(dev, IORESOURCE_MEM, 1);
> >> +     struct resource                 *lcr    = platform_get_resource(dev, IORESOURCE_MEM, 0);
> >> +     struct resource                 *vram   = platform_get_resource(dev, IORESOURCE_MEM, 2);
> >> +     int                             irq     = platform_get_irq(dev, 0);
> >> +     struct fb_info                  *info;
> >> +     struct tmiofb_par               *par;
> >> +     int                             retval;
> >> +
> >> +     if (data == NULL) {
> >> +             dev_err(&dev->dev, "NULL platform data!\n");
> >
> > Can this happen?
> 
> Of course, if you pass incorrect data to your TMIO controller chip.

But would that be a programming error in the caller?

> This is better than kernel Oops.

Not really.  A kernel oops gets us nice debugging information and
prompts people to send us bug reports.  That way, bugs get fixed.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver

[Dmitry]

2008/10/5 Andrew Morton <akpm@linux-foundation.org>:
> On Sun, 5 Oct 2008 01:41:15 +0400 Dmitry <dbaryshkov@gmail.com> wrote:
>
>> >
>> >> +             info->mode = data->modes;
>> >> +     }
>> >> +
>> >> +
>> >> +     tmio_iowrite16(0x0010, par->lcr + LCR_LCDCCRC);
>> >> +     mdelay(5);
>> >> +     tmio_iowrite16(0x0014, par->lcr + LCR_LCDCCRC); /* STOP_CKP */
>> >> +     mdelay(5);
>> >> +     tmio_iowrite16(0x0015, par->lcr + LCR_LCDCCRC); /* STOP_CKP | SOFT_RESET */
>> >> +     tmio_iowrite16(0xfffa, par->lcr + LCR_VCS);
>> >
>> > I trust that someone who is reasonably familiar with the hardware will
>> > know why all these magical mdelay()s are here.  But even if they are,
>> > some comments explaining them would enhance maintainability.
>>
>> that's just for clocks/reset to happen.
>
> I wasn't asking what it was doing - I was asking whether it should be
> left uncommented.
>
> It's the sort of thing which simply cannot be understood by reading the
> C code alone.

I'll add the comment then.

>
>> >> +#ifdef CONFIG_FB_TMIO_ACCELL
>> >> +static int __must_check
>> >> +tmiofb_acc_wait(struct fb_info *info, unsigned int ccs)
>> >> +{
>> >> +     struct tmiofb_par               *par    = info->par;
>> >> +     if (in_atomic() || par->use_polling) {
>> >
>> > No, use of in_atomic() is almost always wrong.  It returns "false"
>> > inside a spinlocked section when CONFIG_PREEMPT=n.
>> >
>> > I cannot advise you further because (tada) there is no comment
>> > explaining this code to me.  But it will need to be rethought, please.
>>
>> The problem is that code may call fb functions with interrupts disabled.
>> One of examples is suspend/resume path, when console seems to be
>> resumed before interrupts are enabled (this is governed
>> by par->use_polling). I'm not sure that there are no other uses of fb
>> functions in atomic context.
>
> The use of irqs_disabled() is OK (with a comment explaining what it's for!).

I'll have to check then on the HW (will take two or three days).

>> >> +             int i = 0;
>> >> +             while (tmio_ioread16(par->lcr + LCR_CCS) > ccs) {
>> >> +                     udelay(1);
>> >> +                     i++;
>> >> +                     if (i > 10000) {
>> >> +                             printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
>> >> +                             return -ETIMEDOUT;
>> >> +                     }
>> >> +                     tmiofb_irq(-1, info);
>> >> +             }
>> >> +     } else {
>> >> +             if (!wait_event_interruptible_timeout(par->wait_acc,
>> >> +                             tmio_ioread16(par->lcr + LCR_CCS) <= ccs, 1000)) {
>> >> +                     printk(KERN_ERR "tmiofb: timeout waiting for %d\n", ccs);
>> >> +                     return -ETIMEDOUT;
>> >> +             }
>> >> +     }
>> >> +
>> >> +     return 0;
>> >> +}
>>
>> ...
>>
>> >> +static int __devinit tmiofb_probe(struct platform_device *dev)
>> >> +{
>> >> +     struct mfd_cell                 *cell   = dev->dev.platform_data;
>> >> +     struct tmio_fb_data             *data   = cell->driver_data;
>> >> +     struct resource                 *ccr    = platform_get_resource(dev, IORESOURCE_MEM, 1);
>> >> +     struct resource                 *lcr    = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> >> +     struct resource                 *vram   = platform_get_resource(dev, IORESOURCE_MEM, 2);
>> >> +     int                             irq     = platform_get_irq(dev, 0);
>> >> +     struct fb_info                  *info;
>> >> +     struct tmiofb_par               *par;
>> >> +     int                             retval;
>> >> +
>> >> +     if (data == NULL) {
>> >> +             dev_err(&dev->dev, "NULL platform data!\n");
>> >
>> > Can this happen?
>>
>> Of course, if you pass incorrect data to your TMIO controller chip.
>
> But would that be a programming error in the caller?

Not really. In this case it also signifies that the fb is not
connected. AFAIR Ian proposed solution to specify
which mfd cells should be registered (see drivers/mfd/mfd-core.c), but
it's currently not implemented.
Passing null platform data here allows us to w/a the inability to
register only several cells.

>> This is better than kernel Oops.
>
> Not really.  A kernel oops gets us nice debugging information and
> prompts people to send us bug reports.  That way, bugs get fixed.
>
>



-- 
With best wishes
Dmitry

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/