: frame buffer driver for 2700G LCD controller

This patch adds frame buffer driver for 2700G LCD controller present on CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

+#include <linux/config.h>

drivers/video/mbx/mbxfb.c new file +6 -0

+	.id =		"MBX",
+	.type =		FB_TYPE_PACKED_PIXELS,
+	.visual =	FB_VISUAL_TRUECOLOR,
+	.xpanstep =	0,
+	.ypanstep =	0,
+	.ywrapstep =	0,

drivers/video/mbx/mbxfb.c new file +11 -0

+		for ( n = 1; n < 8; n++ ) {
+			for ( p = 0; p < 8; p++ ) {
+				clk = (ref_clk * m) / (n * (1 << p));
+				err = (clk > pixclock) ? (clk - pixclock) : 
+					(pixclock - clk);
+				if ( err < min_err ) {
+					min_err = err;
+					best_clk = clk;
+					div->m = m;
+					div->n = n;
+					div->p = p;

+static int mbxfb_remove(struct platform_device *dev)

Quoted 9 lines · Mike Rapoport

This patch adds frame buffer driver for 2700G LCD controller present on CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

Review

Quoted 6 lines

(quoted diff) +4 -0

+
+static int
+mbxfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
+		   u_int trans, struct fb_info *info)

Coding style, "static int" and "mbxfb_setcolreg" in one line.

Review

Quoted 4 lines

(quoted diff) +2 -0

+{
+	uint val, ret = 1;

If you can, avoid u_int and family and just use u32 and family.

Quoted 6 lines

(quoted diff) +6 -0

+
+	if ( regno < 255 ) {
+		val = (red & 0xff) << 16;
+		val |= (green & 0xff) << 8;
+		val |= (blue & 0xff) << 0;
+		GPLUT = Gplut_Lutadr(regno) | Gplut_Lutdata(val);

Review

Quoted 4 lines

(quoted diff) +2 -0

+		udelay(1000);
+		ret = 0;

If you want this to work as a console driver, make sure you also write 'val' to info->pseudo_palette (for bpp 16 and above) and only for regno's 0-15.

Review

Quoted 5 lines

(quoted diff) +3 -0

+#include "mbxsysfs.c"
+
+#define res_size(_r) (((_r)->end - (_r)->start) + 1)

No arch-specific function/macro's to determine the resource length?

Review

Quoted 7 lines

(quoted diff) +5 -0

+static struct platform_driver mbxfb_driver = {
+	.probe		= mbxfb_probe,
+	.remove		= mbxfb_remove,
+
+#ifdef CONFIG_PM

Redundant #ifdef, suspend and resume are already defined as NULL if CONFIG_PM is false.

drivers/video/mbx/mbxsysfs.c

Quoted 11 lines

(quoted diff) +3 -0

+	},
+};
+

drivers/video/mbx/mbxsysfs.c new file +0 -0

new file mode 100644
index 0000000..4b9571a
@@ -0,0 +1,129 @@

Most of the attributes violate the "one file, one value" rule. GregKH won't like it. Try using debugfs if you cannot separate them.

Tony

Quoted 8 lines · Antonino A. Daplas

drivers/video/mbx/mbxfb.c line 499

Quoted 4 lines · Mike Rapoport

drivers/video/mbx/mbxfb.c new file +3 -0

+#include "mbxsysfs.c"
+
+#define res_size(_r) (((_r)->end - (_r)->start) + 1)

No arch-specific function/macro's to determine the resource length?

I searched for one but in vain.

Quoted 14 lines · Antonino A. Daplas

Quoted 8 lines · Mike Rapoport

This patch adds frame buffer driver for 2700G LCD controller present on CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

Quoted 10 lines

drivers/video/mbx/mbxfb.c line 139

Quoted 5 lines

drivers/video/mbx/mbxfb.c new file +4 -0

+
+static int
+mbxfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
+           u_int trans, struct fb_info *info)

Coding style, "static int" and "mbxfb_setcolreg" in one line.

I've tried hard to remove all the coding styles mistakes and misprints, but some maybe still in there.

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+{
+    uint val, ret = 1;

If you can, avoid u_int and family and just use u32 and family.

Done

Quoted 10 lines

Quoted 9 lines

drivers/video/mbx/mbxfb.c new file +8 -0

+
+    if ( regno < 255 ) {
+        val = (red & 0xff) << 16;
+        val |= (green & 0xff) << 8;
+        val |= (blue & 0xff) << 0;
+        GPLUT = Gplut_Lutadr(regno) | Gplut_Lutdata(val);
+        udelay(1000);
+        ret = 0;

Quoted 6 lines

If you want this to work as a console driver, make sure you also write 'val' to info->pseudo_palette (for bpp 16 and above) and only for regno's 0-15.

I've tried to use this driver as console, but all I see is a blank screen. So, at least for now it cannot be used as a console :(

Quoted 9 lines

drivers/video/mbx/mbxfb.c line 499

Quoted 5 lines

drivers/video/mbx/mbxfb.c new file +3 -0

+#include "mbxsysfs.c"
+
+#define res_size(_r) (((_r)->end - (_r)->start) + 1)

No arch-specific function/macro's to determine the resource length?

I searched for one but in vain.

Quoted 13 lines

drivers/video/mbx/mbxfb.c

Quoted 7 lines

drivers/video/mbx/mbxfb.c new file +5 -0

+static struct platform_driver mbxfb_driver = {
+    .probe        = mbxfb_probe,
+    .remove        = mbxfb_remove,
+
+#ifdef CONFIG_PM

Redundant #ifdef, suspend and resume are already defined as NULL if CONFIG_PM is false.

Quoted 17 lines

drivers/video/mbx/mbxsysfs.c

Quoted 10 lines

drivers/video/mbx/mbxfb.c new file +3 -0

+    },
+};
+

drivers/video/mbx/mbxsysfs.c new file +0 -0

new file mode 100644
index 0000000..4b9571a
@@ -0,0 +1,129 @@

Most of the attributes violate the "one file, one value" rule. GregKH won't
like it. Try using debugfs if you cannot separate them.

Changed to debugfs implementation.

Quoted 20 lines · Mike Rapoport

Quoted 14 lines · Antonino A. Daplas

Quoted 9 lines · Mike Rapoport

This patch adds frame buffer driver for 2700G LCD controller present on CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

drivers/video/mbx/regs.h line 5

Quoted 2 lines

drivers/video/mbx/regs.h new file +2 -0

+/* extern unsigned long virt_base_2700; */
+#define __REG_2700G(x)	(*(volatile unsigned long*)((x)+virt_base_2700))

BTW, you will need to use generic accessors (readb|w|l, writeb|w|l or __raw_readb|w|l, __raw_writeb|w|l, etc). This will entail a lot of changes. Otherwise, this driver won't get past Andrew Morton and Rusty.

See include/asm/io.h.

Tony

Quoted 29 lines · Antonino A. Daplas

Quoted 18 lines · Mike Rapoport

Quoted 13 lines · Antonino A. Daplas

Quoted 9 lines · Mike Rapoport

This patch adds frame buffer driver for 2700G LCD controller present on
CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

(quoted diff) +2 -0

+/* extern unsigned long virt_base_2700; */
+#define __REG_2700G(x)    (*(volatile unsigned

Quoted 2 lines

long*)((x)+virt_base_2700))

BTW, you will need to use generic accessors (readb|w|l, writeb|w|l or __raw_readb|w|l, __raw_writeb|w|l, etc). This will entail a lot of changes.
Otherwise, this driver won't get past Andrew Morton and Rusty.

Quoted 3 lines

See include/asm/io.h.

What's the better way?

{
   ...
   writel((readl(REG) | bits), REG);
   ...
}

or

{
   ...
   u32 some_reg = readl(REG);
   ...
   some_reg |= bits;
   ...
   writel(some_reg, REG)
   ...
}

Quoted 9 lines · Mike Rapoport

Quoted 5 lines · Antonino A. Daplas

Mike Rapoport wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

This patch adds frame buffer driver for 2700G LCD controller present on CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

It's because you don't have an info->pseudo_palette. And if you accidentally load the framebuffer console, your kernel will just crash because the drawing functions (cfb_{fillrect,imageblit}) unconditionally refers to that array.

So you have to do either one:

1. Create a properly filled up info->pseudo_palette; or

BTW, you will need to use generic accessors (readb|w|l, writeb|w|l or __raw_readb|w|l, __raw_writeb|w|l, etc). This will entail a lot of changes. Otherwise, this driver won't get past Andrew Morton and Rusty.

See include/asm/io.h.

Tony

Quoted 23 lines · Antonino A. Daplas

Quoted 7 lines · Mike Rapoport

Quoted 4 lines · Antonino A. Daplas

Mike Rapoport wrote:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

This patch adds frame buffer driver for 2700G LCD controller present on
CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Signed-off-by Mike Rapoport <[email protected]>

It's because you don't have an info->pseudo_palette. And if you accidentally
load the framebuffer console, your kernel will just crash because the drawing functions (cfb_{fillrect,imageblit}) unconditionally refers to that
array.

Quoted 5 lines

So you have to do either one:

1. Create a properly filled up info->pseudo_palette; or

I think I've succeded to make it. At least I can see tux logo and getty output at startup.

Quoted 5 lines

BTW, you will need to use generic accessors (readb|w|l, writeb|w|l or __raw_readb|w|l, __raw_writeb|w|l, etc). This will entail a lot of changes.
Otherwise, this driver won't get past Andrew Morton and Rusty.

Quoted 3 lines

See include/asm/io.h.

Done.

Quoted 17 lines · Mike Rapoport

Quoted 14 lines · Antonino A. Daplas

Quoted 11 lines · Mike Rapoport

Quoted 8 lines · Antonino A. Daplas

Quoted 5 lines · Mike Rapoport

This patch adds frame buffer driver for 2700G LCD controller present on
CompuLab CM-X270 computer module.
This pacth is versus current Linus git tree.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Mike Rapoport <[email protected]>

Definitely much, much better. I'm attaching a clean-up patch. If you approve, I'll send your patch + cleanup upstream.

Tony

mbxfb: Various fixes

From: <>

- Add more informative help text to Kconfig
- Make DEBUG a Kconfig option as FB_MBX_DEBUG
- Remove #include mbxdebug.c, this is frowned upon
- Remove redundant casts
- Arrange #include's alphabetically
- Trivial whitespace changes

Signed-off-by Antonino Daplas <[email protected]>

---

drivers/video/Kconfig      |   12 +++++++++++-
drivers/video/mbx/Makefile |    3 ++-
drivers/video/mbx/mbxfb.c  |   24 +++++++++++-------------
3 files changed, 24 insertions(+), 15 deletions(-)

drivers/video/Kconfig +11 -1

index 0f4af08..153e23e 100644
@@ -1547,8 +1547,18 @@ config FB_MBX
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	---help---
+	  Framebuffer driver for the Intel 2700G (Marathon) Graphics
+	  Accelerator
 
-	  If unsure, say N.
+config FB_MBX_DEBUG
+       bool "Enable debugging info via debugfs"
+       depends on FB_MBX && DEBUG_FS
+       default n
+       ---help---
+         Enable this if you want debugging information using the debug
+         filesystem (debugfs)
+
+         If unsure, say N.
 

config FB_W100
tristate "W100 frame buffer support"

drivers/video/mbx/Makefile +2 -1

index ad042f5..16c1165 100644
@@ -1,3 +1,4 @@
 # Makefile for the 2700G controller driver.
 
-obj-$(CONFIG_FB_MBX)	+= mbxfb.o
+obj-$(CONFIG_FB_MBX)	   += mbxfb.o
+obj-$(CONFIG_FB_MBX_DEBUG) += mbxfbdebugfs.o

drivers/video/mbx/mbxfb.c +11 -13

index 7834586..a5836b5 100644
@@ -14,22 +14,21 @@
 *
 */
 
-#include <linux/module.h>
-#include <linux/fb.h>
 #include <linux/delay.h>
+#include <linux/fb.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include <video/mbxfb.h>
-
 #include <asm/io.h>
 
-#undef DEBUG
+#include <video/mbxfb.h>
 
-static unsigned long virt_base_2700;
 #include "regs.h"
 #include "reg_bits.h"
 
+static unsigned long virt_base_2700;
+
 #define MIN_XRES	16
 #define MIN_YRES	16
 #define MAX_XRES	2048

@@ -59,7 +58,7 @@ struct mbxfb_info {
 	int (*platform_probe) (struct fb_info * fb);
 	int (*platform_remove) (struct fb_info * fb);
 
-#if defined(CONFIG_DEBUG_FS) && defined(DEBUG)
+#ifdef CONFIG_FB_MBX_DEBUG
 	void *debugfs_data;
 #endif
 

@@ -145,7 +144,8 @@ static int mbxfb_setcolreg(u_int regno, 
 	if (regno < MAX_PALETTES) {
 		u32 *pal = info->pseudo_palette;
 
-		val = (red & 0xf800) | ((green & 0xfc00) >> 5) | ((blue & 0xf800) >> 11);
+		val = (red & 0xf800) | ((green & 0xfc00) >> 5) |
+			((blue & 0xf800) >> 11);
 		pal[regno] = val;
 		ret = 0;
 	}

@@ -477,7 +477,7 @@ static int mbxfb_suspend(struct platform
 
 static int mbxfb_resume(struct platform_device *dev)
 {
-	struct fb_info *fbi = (struct fb_info *)platform_get_drvdata(dev);
+	struct fb_info *fbi = platform_get_drvdata(dev);
 
 	enable_clocks(fbi);
 /* 	setup_graphics(fbi); */

@@ -492,9 +492,7 @@ #define mbxfb_resume	NULL
 #endif
 
 /* debugfs entries */
-#if defined(CONFIG_DEBUG_FS) && defined(DEBUG)
-#include "mbxdebugfs.c"
-#else
+#ifndef CONFIG_FB_MBX_DEBUG
 #define mbxfb_debugfs_init(x)	do {} while(0)
 #define mbxfb_debugfs_remove(x)	do {} while(0)
 #endif

@@ -632,7 +630,7 @@ err1:
 
 static int __devexit mbxfb_remove(struct platform_device *dev)
 {
-	struct fb_info *fbi = (struct fb_info *)platform_get_drvdata(dev);
+	struct fb_info *fbi = platform_get_drvdata(dev);
 
 	writel(SYSRST_RST, SYSRST);
 	udelay(1000);

drivers/video/Kconfig +0 -0

@@ -1518,6 +1518,16 @@ config FB_PXA_PARAMETERS
 

drivers/video/Kconfig +2 -0

+	  If unsure, say N.
+

config FB_W100

	tristate "W100 frame buffer support"
	depends on FB && PXA_SHARPSL

drivers/video/mbx/mbxfb.c new file +0 -0

new file mode 100644
index 0000000..c41a678

drivers/video/mbx/mbxfb.c new file +18 -0

+#ifdef CONFIG_PM
+/*
+ * Power management hooks.  Note that we won't be called from IRQ context,
+ * unlike the blank functions above, so we may sleep.
+ */
+static int mbxfb_suspend(struct platform_device *dev, pm_message_t state)
+{
+	/* make frame buffer memory enter self-refresh mode */
+	writel(LMPWR_MC_PWR_SRM, LMPWR);
+	while (LMPWRSTAT != LMPWRSTAT_MC_PWR_SRM)
+		; /* empty statement */
+
+	/* reset the device, since it's initial state is 'mostly sleeping' */
+	writel(SYSRST_RST, SYSRST);
+	return 0;
+}
+
+static int mbxfb_resume(struct platform_device *dev)

Could some kind soul do a recursive s/timming/timing/g in drivers/video, please? TIA.

IIRC it was already stated that it is better to only set the .suspend and .resume ops depending on CONFIG_PM (see below instead of the #if #else #endif stanza here).

drivers/video/mbx/mbxfb.c line 492

Quoted 4 lines

drivers/video/mbx/mbxfb.c new file +4 -0

+#else
+#define mbxfb_suspend	NULL
+#define mbxfb_resume	NULL
+#endif