* [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
@ 2008-09-30 8:38 Dmitry Baryshkov
2008-09-30 8:38 ` [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb Dmitry Baryshkov
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2008-09-30 8:38 UTC (permalink / raw)
To: linux-fbdev-devel; +Cc: Dmitry Baryshkov, Ian Molton, Samuel Ortiz
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=/
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
@ 2008-09-30 8:38 ` Dmitry Baryshkov
2008-10-03 22:56 ` Samuel Ortiz
2008-09-30 9:17 ` [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Geert Uytterhoeven
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2008-09-30 8:38 UTC (permalink / raw)
To: linux-fbdev-devel; +Cc: Dmitry Baryshkov, Ian Molton, Samuel Ortiz
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=/
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb
2008-09-30 8:38 ` [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb Dmitry Baryshkov
@ 2008-10-03 22:56 ` Samuel Ortiz
0 siblings, 0 replies; 13+ messages in thread
From: Samuel Ortiz @ 2008-10-03 22:56 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: Ian Molton, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
2008-09-30 8:38 ` [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb Dmitry Baryshkov
@ 2008-09-30 9:17 ` Geert Uytterhoeven
2008-09-30 14:37 ` Ian Molton
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2008-09-30 9:17 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
2008-09-30 8:38 ` [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb Dmitry Baryshkov
2008-09-30 9:17 ` [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Geert Uytterhoeven
@ 2008-09-30 14:37 ` Ian Molton
2008-09-30 22:04 ` Krzysztof Helt
2008-10-03 9:23 ` Andrew Morton
4 siblings, 0 replies; 13+ messages in thread
From: Ian Molton @ 2008-09-30 14:37 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
` (2 preceding siblings ...)
2008-09-30 14:37 ` Ian Molton
@ 2008-09-30 22:04 ` Krzysztof Helt
2008-10-04 13:38 ` Dmitry
2008-10-03 9:23 ` Andrew Morton
4 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Helt @ 2008-09-30 22:04 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-09-30 22:04 ` Krzysztof Helt
@ 2008-10-04 13:38 ` Dmitry
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry @ 2008-10-04 13:38 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
` (3 preceding siblings ...)
2008-09-30 22:04 ` Krzysztof Helt
@ 2008-10-03 9:23 ` Andrew Morton
2008-10-03 11:35 ` Ian Molton
2008-10-04 21:41 ` Dmitry
4 siblings, 2 replies; 13+ messages in thread
From: Andrew Morton @ 2008-10-03 9:23 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-10-03 9:23 ` Andrew Morton
@ 2008-10-03 11:35 ` Ian Molton
2008-10-04 21:41 ` Dmitry
1 sibling, 0 replies; 13+ messages in thread
From: Ian Molton @ 2008-10-03 11:35 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dmitry Baryshkov, Samuel Ortiz, linux-fbdev-devel
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-10-03 9:23 ` Andrew Morton
2008-10-03 11:35 ` Ian Molton
@ 2008-10-04 21:41 ` Dmitry
2008-10-04 21:58 ` Andrew Morton
1 sibling, 1 reply; 13+ messages in thread
From: Dmitry @ 2008-10-04 21:41 UTC (permalink / raw)
To: Andrew Morton
Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel, Andrew W Woodward
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-10-04 21:41 ` Dmitry
@ 2008-10-04 21:58 ` Andrew Morton
2008-10-05 8:09 ` Dmitry
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2008-10-04 21:58 UTC (permalink / raw)
To: Dmitry; +Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel, Andrew W Woodward
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
2008-10-04 21:58 ` Andrew Morton
@ 2008-10-05 8:09 ` Dmitry
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry @ 2008-10-05 8:09 UTC (permalink / raw)
To: Andrew Morton
Cc: Ian Molton, Samuel Ortiz, linux-fbdev-devel, Andrew W Woodward
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver
@ 2008-09-30 9:11 krzysztof.h1
0 siblings, 0 replies; 13+ messages in thread
From: krzysztof.h1 @ 2008-09-30 9:11 UTC (permalink / raw)
To: linux-fbdev-devel@lists.sourceforge.net
Cc: Dmitry Baryshkov, Ian Molton, Samuel Ortiz
> 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>
> ---
This is not detailed review but only two issues:
> 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 @@
(...)
> +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;
> +
if info->screen_base == par->vram you can drop vram field from the par structure.
> + 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 devicen",
> + 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);
Please set NULL to drvdata before releasing the fb_info structure.
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=/
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-10-05 8:13 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-30 8:38 [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Dmitry Baryshkov
2008-09-30 8:38 ` [RESEND][PATCH 2/2] mfd: support tmiofb cell on tc6393xb Dmitry Baryshkov
2008-10-03 22:56 ` Samuel Ortiz
2008-09-30 9:17 ` [RESEND][PATCH 1/2] fbdev: add new TMIO framebuffer driver Geert Uytterhoeven
2008-09-30 14:37 ` Ian Molton
2008-09-30 22:04 ` Krzysztof Helt
2008-10-04 13:38 ` Dmitry
2008-10-03 9:23 ` Andrew Morton
2008-10-03 11:35 ` Ian Molton
2008-10-04 21:41 ` Dmitry
2008-10-04 21:58 ` Andrew Morton
2008-10-05 8:09 ` Dmitry
-- strict thread matches above, loose matches on Subject: below --
2008-09-30 9:11 krzysztof.h1
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).