Skip to contentSkip to conversation

drivers: add lcd display support

PATCH Superseded Miguel Ojeda Sandonis 4 participants 12 messages Sep 30, 2006 – Oct 1, 2006

Subsystems (5)

v6 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

PATCH v6 1/1 drivers: add lcd display support

miguelojeda-2.6.18-add-lcd-display-support.patch

Changelog (newer first)
-----------------------

V6 - Fixed mistakes found by Andrew Morton
- added locking
- removed "add const to class_create" part, as it is already accepted in 2.6.18-git10
- removed PRINT_PREFIX macro, use KERN_... instead
- reverted bitops to standard |= and &=
- added const to fop's static struct
- added error code checking for class_device_create()
- removed "if(foo > maxfoo) foo=maxfoo;", use min(foo,maxfoo) instead
- coding style:
- used #defined constants directly, not static constants = DEFINED_CONSTANT
- removed blank lines
- changed pointers declarations to "T *name"
- changed for's coding style
- added single spaces around arithmetic operators
- changed counter's nomencalture to i,j,k
- shorter function definitions
- removed newlines at some function calls
- udelay(2)

V5 - Fixed mistakes found by Joe Perches
- coding style
- const, spelling mistake

V4 - Fixed mistakes found by Pavel Machek
- no zero initialization
- GPL version
- use bitop generic functions

V3 - Apply for 2.6.18
- added const to class_create (2.6.18 hasn't add-const-to-class_create.patch)

V2 - Fixed mistakes found by Greg KH
- coding style

V1 - Fixed mistakes found by Alexey Dobriyan
- coding style
- init() and exit() error management

V0 - Original patch

Brief
-----

- Adds const to class_create (2.6.18 hasn't add-const-to-class_create.patch)
- Adds a "lcddisplay" class for registering LCD devices.
- Adds support for the ks0108 LCD Controller as a device driver.
(uses lcddisplay class and parport interface)
- Adds support for the cfag12864b LCD Display as a device driver.
(uses ks0108 device driver)
- Adds a new ioctl() magic number/range (0xFF) in the list for the cfag12864b device.
- Adds the usual Documentation, includes, Makefiles, Kconfigs, MAINTAINERS, CREDITS...

Patched files Index
-------------------

patching file drivers/Kconfig
patching file drivers/lcddisplay/cfag12864b.c patching file drivers/lcddisplay/cfag12864b_image.h patching file drivers/lcddisplay/Kconfig patching file drivers/lcddisplay/ks0108.c patching file drivers/lcddisplay/lcddisplay.c patching file drivers/lcddisplay/Makefile patching file drivers/Makefile
patching file include/linux/cfag12864b.h patching file include/linux/ks0108.h patching file include/linux/lcddisplay.h patching file Documentation/lcddisplay/cfag12864b patching file Documentation/lcddisplay/lcddisplay patching file Documentation/ioctl-number.txt patching file CREDITS
patching file MAINTAINERS

Please review.

miguelojeda-2.6.18-add-lcd-display-support.patch

---

drivers/Kconfig +2 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/Kconfig linux-2.6.18/drivers/Kconfig
@@ -74,4 +74,6 @@ source "drivers/rtc/Kconfig"
 
 source "drivers/dma/Kconfig"
 
+source "drivers/lcddisplay/Kconfig"
+
 endmenu
drivers/lcddisplay/cfag12864b.c +592 -0

592 lines not on this page · raw message

drivers/lcddisplay/cfag12864b_image.h +95 -0

95 lines not on this page · raw message

drivers/lcddisplay/Kconfig +110 -0

110 lines not on this page · raw message

drivers/lcddisplay/ks0108.c +160 -0

160 lines not on this page · raw message

drivers/lcddisplay/lcddisplay.c +79 -0

79 lines not on this page · raw message

drivers/lcddisplay/Makefile +7 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/Makefile linux-2.6.18/drivers/lcddisplay/Makefile
@@ -0,0 +1,7 @@
+#
+# Makefile for the kernel LCD Display device drivers.
+#
+
+obj-$(CONFIG_LCDDISPLAY)	+= lcddisplay.o
+obj-$(CONFIG_KS0108)		+= ks0108.o
+obj-$(CONFIG_CFAG12864B)	+= cfag12864b.o
drivers/Makefile +1 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/Makefile linux-2.6.18/drivers/Makefile
@@ -76,3 +76,4 @@ obj-$(CONFIG_CRYPTO)		+= crypto/
 obj-$(CONFIG_SUPERH)		+= sh/
 obj-$(CONFIG_GENERIC_TIME)	+= clocksource/
 obj-$(CONFIG_DMA_ENGINE)	+= dma/
+obj-$(CONFIG_LCDDISPLAY)	+= lcddisplay/
include/linux/cfag12864b.h +58 -0

58 lines not on this page · raw message

include/linux/ks0108.h +36 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/include/linux/ks0108.h linux-2.6.18/include/linux/ks0108.h
@@ -0,0 +1,36 @@
+/*
+ *    Filename: ks0108.h
+ *     Version: 0.1.0
+ * Description: ks0108 LCD Display Controller Driver Header
+ *     License: GPLv2 or later
+ *
+ *      Author: Copyright (C) Miguel Ojeda Sandonis <[email protected]>
+ *        Date: 2006-09-28
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#ifndef _KS0108_H_
+#define _KS0108_H_
+
+extern void ks0108_writedata(unsigned char byte);
+extern void ks0108_writecontrol(unsigned char byte);
+extern void ks0108_displaystate(unsigned char state);
+extern void ks0108_startline(unsigned char startline);
+extern void ks0108_address(unsigned char address);
+extern void ks0108_page(unsigned char page);
+
+#endif /* _KS0108_H_ */
include/linux/lcddisplay.h +34 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/include/linux/lcddisplay.h linux-2.6.18/include/linux/lcddisplay.h
@@ -0,0 +1,34 @@
+/*
+ *    Filename: lcddisplay.h
+ *     Version: 0.1.0
+ * Description: Display Class Header
+ *     License: GPLv2 or later
+ *
+ *      Author: Copyright (C) Miguel Ojeda Sandonis <[email protected]>
+ *        Date: 2006-09-28
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#ifndef _LCDDISPLAY_H_
+#define _LCDDISPLAY_H_
+
+#include <linux/device.h>
+
+extern struct class *lcddisplay_class;
+
+#endif /* _LCDDISPLAY_H_ */
+
Documentation/lcddisplay/cfag12864b +371 -0

371 lines not on this page · raw message

Documentation/lcddisplay/lcddisplay +37 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/Documentation/lcddisplay/lcddisplay linux-2.6.18/Documentation/lcddisplay/lcddisplay
@@ -0,0 +1,37 @@
+	=================================
+	LCD Display Drivers Documentation
+	=================================
+
+License:		GPL
+Author & Maintainer:	Miguel Ojeda Sandonis <[email protected]>
+Date:			2006-09-30
+
+--------
+0. INDEX
+--------
+
+	1. NEW DISPLAY DRIVERS
+	2. GENERAL TIPS
+
+
+
+----------------------
+1. NEW DISPLAY DRIVERS
+----------------------
+
+Feel free to send me new display drivers. I will try to do my best.
+
+If you don't get any answer, send your patch directly to the linux-kernel ml.
+
+
+
+---------------
+2. GENERAL TIPS
+---------------
+
+- Divide your driver into the controller driver, like ks0108,
+  and the specific LCD display series driver, like cfag12864b.
+
+- Claim for your IO ports in the controller driver.
+
+EOF
Documentation/ioctl-number.txt +2 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/Documentation/ioctl-number.txt linux-2.6.18/Documentation/ioctl-number.txt
@@ -191,3 +191,5 @@ Code	Seq#	Include File		Comments
 					<mailto:[email protected]>
 0xF3	00-3F	video/sisfb.h		sisfb (in development)
 					<mailto:[email protected]>
+0xFF	00-1F	linux/cfag12864b.h	cfag12864b LCD Display Driver
+					<mailto:[email protected]>
CREDITS +8 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/CREDITS linux-2.6.18/CREDITS
@@ -2535,6 +2535,14 @@ S: Subiaco, 6008
 S: Perth, Western Australia
 S: Australia
 
+N: Miguel Ojeda Sandonis
+E: [email protected]
+D: Author: LCD Display Drivers (ks0108, cfag12864b)
+D: Maintainer: LCD Display Drivers Tree (drivers/lcddisplay/*)
+S: C/ Mieses 20, 9-B
+S: Valladolid 47009
+S: Spain
+
 N: Greg Page
 E: [email protected]
 D: IPX development and support
MAINTAINERS +5 -0
diff -uprN -X dontdiff linux-2.6.18-vanilla/MAINTAINERS linux-2.6.18/MAINTAINERS
@@ -1707,6 +1707,11 @@ M:	[email protected]
 L:	[email protected]
 S:	Maintained
 
+LCD DISPLAY DRIVERS
+P:	Miguel Ojeda Sandonis
+M:	[email protected]
+S:	Maintained
+
 LED SUBSYSTEM
 P:	Richard Purdie
 M:	[email protected]
#1 lore raw
Quoted 5 lines · Miguel Ojeda Sandonis

Patched files Index
-------------------
patching file drivers/Kconfig
patching file drivers/lcddisplay/cfag12864b.c patching file drivers/lcddisplay/cfag12864b_image.h

Quoted 8 lines

patching file drivers/lcddisplay/Kconfig
patching file drivers/lcddisplay/ks0108.c
patching file drivers/lcddisplay/lcddisplay.c
patching file drivers/lcddisplay/Makefile
patching file include/linux/lcddisplay.h
patching file Documentation/lcddisplay/cfag12864b
patching file Documentation/lcddisplay/lcddisplay
patching file Documentation/ioctl-number.txt

AFAIK, the 'D' in LCD is already an abbreviation for "display", so could we name it lcdisplay or something like that?

MfG, JBG

Signature · 3 lines
      Jan-Benedict Glaw      [email protected]              +49-172-7608481
Signature of:  The real problem with C++ for kernel modules is: the language just sucks.
the second  :                                            -- Linus Torvalds
#2 lore raw
Quoted 4 lines · Jan-Benedict Glaw

AFAIK, the 'D' in LCD is already an abbreviation for "display", so could we name it lcdisplay or something like that?

My first guess was "lcd" ("d" as display), but there were already devices with such name. So I chose "display", as they are additional small displays (LCD or not). However, people thought it was a very common name. Then I chose "LCD Display". Such name is more expressive (it is "a Display, LCD type") and doesn't makes trouble with new lcd screens, or the "lcd" drivers, or...

I don't care really about the name with or without "double d". Can someone else give his/her opinion about this?

Thank you,
Miguel Ojeda

Quoted 18 lines

MfG, JBG

--

      Jan-Benedict Glaw      [email protected]              +49-172-7608481
Signature of:  The real problem with C++ for kernel modules is: the language just sucks.
the second  :                                            -- Linus Torvalds

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFFHmWAHb1edYOZ4bsRAkdpAJ4g9+GPFYU4Y5W7JdGZXFDCmBDIggCfe6dk 3FbG+mcZQ+10MNkq5wC9J7w=
=KzkJ
-----END PGP SIGNATURE-----

#3 lore raw
In reply to Miguel Ojeda Sandonis
Quoted 2 lines · On Sat, 30 Sep 2006 13:22:53 +0000 Miguel Ojeda Sandonis wrote:

Patched files Index
-------------------

This list should be done with 'diffstat -p1 -w70 patch_file_name' so that we can see the files and the patch sizes.

Quoted 22 lines

patching file drivers/Kconfig
patching file drivers/lcddisplay/cfag12864b.c patching file drivers/lcddisplay/cfag12864b_image.h patching file drivers/lcddisplay/Kconfig patching file drivers/lcddisplay/ks0108.c patching file drivers/lcddisplay/lcddisplay.c patching file drivers/lcddisplay/Makefile patching file drivers/Makefile
patching file include/linux/cfag12864b.h patching file include/linux/ks0108.h patching file include/linux/lcddisplay.h patching file Documentation/lcddisplay/cfag12864b patching file Documentation/lcddisplay/lcddisplay patching file Documentation/ioctl-number.txt patching file CREDITS
patching file MAINTAINERS

---
diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/cfag12864b.c linux-2.6.18/drivers/lcddisplay/cfag12864b.c

drivers/lcddisplay/cfag12864b.c line 9

Quoted 9 lines
drivers/lcddisplay/cfag12864b.c +9 -0
@@ -0,0 +1,592 @@
+
+#define CFAG12864B_NAME "cfag12864b"
+
+/*
+ * Device
+ */
+
+static const unsigned int cfag12864b_firstminor;
+static const unsigned int cfag12864b_ndevices = 1;

This driver only supports one device, right? Is that documented somewhere? I probably missed it.

Quoted 21 lines
drivers/lcddisplay/cfag12864b.c +21 -0
+static int cfag12864b_major;
+static int cfag12864b_minor;
+static dev_t cfag12864b_device;
+struct cdev cfag12864b_chardevice;
+DECLARE_MUTEX(cfag12864b_mutex);
+
+/*
+ * cfag12864b Commands
+ */
+
+#define bit(n)   (((unsigned char)1)<<(n))
+
+static unsigned char cfag12864b_state;
+
+static void cfag12864b_set(void)
+{
+	ks0108_writecontrol(cfag12864b_state);
+}
+
+static void cfag12864b_setbit(unsigned char state, unsigned char n)
+{

drivers/lcddisplay/cfag12864b.c line 76

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+	if (state)
+		cfag12864b_state |= bit(n);
+	else
+		cfag12864b_state &= ~bit(n);
+	cfag12864b_set();
+}
+
+static void cfag12864b_e(unsigned char state)
+{
+	cfag12864b_setbit(state, 0);

bit defintions? (here and below)

Quoted 33 lines
drivers/lcddisplay/cfag12864b.c +33 -0
+}
+
+static void cfag12864b_cs1(unsigned char state)
+{
+	cfag12864b_setbit(state, 2);
+}
+
+static void cfag12864b_cs2(unsigned char state)
+{
+	cfag12864b_setbit(state, 1);
+}
+
+static void cfag12864b_di(unsigned char state)
+{
+	cfag12864b_setbit(state, 3);
+}
+
+static void cfag12864b_setcontrollers(unsigned char first, unsigned char second)
+{
+	if (first)
+		cfag12864b_cs1(0);
+	else
+		cfag12864b_cs1(1);
+
+	if (second)
+		cfag12864b_cs2(0);
+	else
+		cfag12864b_cs2(1);
+}
+
+static void cfag12864b_controller(unsigned char which)
+{
+	if (which == 0)

drivers/lcddisplay/cfag12864b.c line 164

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+		cfag12864b_setcontrollers(1, 0);
+	else if (which == 1)
+		cfag12864b_setcontrollers(0, 1);
+}
+
+/*
+ * Auxiliary
+ */
+
+static void normalizeoffset(unsigned int * offset)

Kernel style is:
static void normalizeoffset(unsigned int *offset)

drivers/lcddisplay/cfag12864b.c line 173

Quoted 9 lines
drivers/lcddisplay/cfag12864b.c +9 -0
+{
+	if (*offset >= CFAG12864B_PAGES*CFAG12864B_ADDRESSES)
+		*offset -= CFAG12864B_PAGES*CFAG12864B_ADDRESSES;
+}
+
+static unsigned char calcaddress(unsigned int offset)
+{
+	normalizeoffset(&offset);
+	return offset%CFAG12864B_ADDRESSES;

spaces around '%'

drivers/lcddisplay/cfag12864b.c line 186

Quoted 13 lines
drivers/lcddisplay/cfag12864b.c +13 -0
+}
+
+static unsigned char calccontroller(unsigned int offset)
+{
+	if (offset < CFAG12864B_PAGES*CFAG12864B_ADDRESSES)
+		return 0;
+	return 1;
+}
+
+static unsigned char calcpage(unsigned int offset)
+{
+	normalizeoffset(&offset);
+	return offset/CFAG12864B_ADDRESSES;

spaces around '/'

drivers/lcddisplay/cfag12864b.c line 264

Quoted 5 lines
drivers/lcddisplay/cfag12864b.c +5 -0
+}
+
+void cfag12864b_format_nolock(unsigned char *src)
+{
+	unsigned short i,j,k,n;

spaces after commas

drivers/lcddisplay/cfag12864b.c line 267

Quoted 3 lines
drivers/lcddisplay/cfag12864b.c +3 -0
+	unsigned char *dest;
+
+	dest = kmalloc(sizeof(unsigned char) * CFAG12864B_SIZE, GFP_KERNEL);

Are there places where sizeof(unsigned char) is not 1?

drivers/lcddisplay/cfag12864b.c line 270

Quoted 3 lines
drivers/lcddisplay/cfag12864b.c +3 -0
+	if (dest == NULL) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "format: ERROR: "
+			"can't alloc memory %i bytes\n",

use "%zd" to print sizeof() values, otherwise gcc says:

drivers/lcddisplay/cfag12864b.c:271: warning: format '%i' expects type 'int', but argument 2 has type 'long unsigned int'

drivers/lcddisplay/cfag12864b.c line 277

Quoted 7 lines
drivers/lcddisplay/cfag12864b.c +7 -0
+			sizeof(unsigned char) * CFAG12864B_SIZE);
+		return;
+	}
+
+	for (i = 0; i < CFAG12864B_CONTROLLERS; i++)
+	for (j = 0; j < CFAG12864B_PAGES; j++)
+	for (k = 0; k < CFAG12864B_ADDRESSES; k++) {

questionable indentation

drivers/lcddisplay/cfag12864b.c line 279

Quoted 2 lines
drivers/lcddisplay/cfag12864b.c +2 -0
+		dest[(i * CFAG12864B_PAGES + j) * CFAG12864B_ADDRESSES + k] = 0;
+		for (n=0; n < 8; n++)

spaces around '='

Quoted 6 lines
drivers/lcddisplay/cfag12864b.c +6 -0
+			if (src[i * CFAG12864B_ADDRESSES + k + (j * 8 + n) * CFAG12864B_WIDTH])
+				dest[(i * CFAG12864B_PAGES + j) * CFAG12864B_ADDRESSES + k] |= bit(n);
+	}
+
+	cfag12864b_write_nolock(0, dest, CFAG12864B_SIZE);
+

drivers/lcddisplay/cfag12864b.c line 295

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+	kfree(dest);
+}
+
+/*
+ * cfag12864b Exported Commands (do lock)
+ */
+
+void cfag12864b_on(void)
+{
+	if(down_interruptible(&cfag12864b_mutex))

space after "if"

drivers/lcddisplay/cfag12864b.c line 305

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+		return;
+
+	cfag12864b_on_nolock();
+
+	up(&cfag12864b_mutex);
+}
+
+void cfag12864b_off(void)
+{
+	if(down_interruptible(&cfag12864b_mutex))

space after "if"

drivers/lcddisplay/cfag12864b.c line 315

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+		return;
+
+	cfag12864b_off_nolock();
+
+	up(&cfag12864b_mutex);
+}
+
+void cfag12864b_clear(void)
+{
+	if(down_interruptible(&cfag12864b_mutex))

ditto

drivers/lcddisplay/cfag12864b.c line 326

Quoted 11 lines
drivers/lcddisplay/cfag12864b.c +11 -0
+		return;
+
+	cfag12864b_clear_nolock();
+
+	up(&cfag12864b_mutex);
+}
+
+void cfag12864b_write(unsigned short offset, const unsigned char *buffer,
+	unsigned short count)
+{
+	if(down_interruptible(&cfag12864b_mutex))

ditto

drivers/lcddisplay/cfag12864b.c line 336

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+		return;
+
+	cfag12864b_write_nolock(offset,buffer,count);
+
+	up(&cfag12864b_mutex);
+}
+
+void cfag12864b_format(unsigned char *src)
+{
+	if(down_interruptible(&cfag12864b_mutex))

ditto

Quoted 8 lines
drivers/lcddisplay/cfag12864b.c +8 -0
+		return;
+
+	cfag12864b_format_nolock(src);
+
+	up(&cfag12864b_mutex);
+}
+
+EXPORT_SYMBOL_GPL(cfag12864b_on);

drivers/lcddisplay/cfag12864b.c line 354

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+EXPORT_SYMBOL_GPL(cfag12864b_off);
+EXPORT_SYMBOL_GPL(cfag12864b_clear);
+EXPORT_SYMBOL_GPL(cfag12864b_write);
+EXPORT_SYMBOL_GPL(cfag12864b_format);
+
+/*
+ * cfag12864b ioctls (don't lock because ioctl fop do)
+ */
+
+static int cfag12864b_fopioctlformat(void __user * arg)

use "*arg" without space

drivers/lcddisplay/cfag12864b.c line 361

Quoted 7 lines
drivers/lcddisplay/cfag12864b.c +7 -0
+{
+	int result;
+	int ret = -ENOTTY;
+	unsigned char *tmpbuffer;
+
+	tmpbuffer = kmalloc(
+		sizeof(unsigned char)*CFAG12864B_MATRIXSIZE,GFP_KERNEL);

spacebar helps readability. add spaces around * and after comma.

drivers/lcddisplay/cfag12864b.c line 364

Quoted 3 lines
drivers/lcddisplay/cfag12864b.c +3 -0
+	if (tmpbuffer == NULL) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "FOP ioctl: ERROR: "
+			"can't alloc memory %i bytes\n",

use "%zd" to print sizeof() values, otherwise gcc complains.

drivers/lcddisplay/cfag12864b.c line 365

Quoted 1 line
drivers/lcddisplay/cfag12864b.c +1 -0
+			sizeof(unsigned char)*CFAG12864B_MATRIXSIZE);

space around '*'

drivers/lcddisplay/cfag12864b.c line 373

Quoted 8 lines
drivers/lcddisplay/cfag12864b.c +8 -0
+		goto none;
+	}
+
+	result = copy_from_user(tmpbuffer, arg,
+		sizeof(unsigned char) * CFAG12864B_MATRIXSIZE);
+	if (result != 0) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "FOP ioctl: ERROR: "
+			"can't copy memory from user\n");

I don't think that we want a printk on every copy_from_user() error. (here and elsewhere)

Quoted 13 lines
drivers/lcddisplay/cfag12864b.c +13 -0
+		goto bufferalloced;
+	}
+	
+	cfag12864b_format_nolock(tmpbuffer);
+
+	ret = 0;
+
+bufferalloced:
+	kfree(tmpbuffer);
+
+none:
+	return ret;
+}

drivers/lcddisplay/cfag12864b.c line 396

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+
+/*
+ * cfag12864b_fops (do lock)
+ */
+
+static loff_t cfag12864b_fopseek(struct file *filp, loff_t offset, int whence)
+{
+	loff_t ret = -EINVAL;
+
+	if(down_interruptible(&cfag12864b_mutex))

space after "if"

Quoted 25 lines
drivers/lcddisplay/cfag12864b.c +25 -0
+		return -ERESTARTSYS;
+
+	switch(whence) {
+	case SEEK_SET:
+		ret = offset;
+		break;
+	case SEEK_CUR:
+		ret = filp->f_pos + offset;
+		break;
+	case SEEK_END:
+		ret = CFAG12864B_SIZE + offset;
+		break;
+	}
+
+	if (ret < 0) {
+		ret = -EINVAL;
+		goto none;
+	}
+
+	filp->f_pos = ret;
+
+none:
+	up(&cfag12864b_mutex);
+	return ret;
+}

drivers/lcddisplay/cfag12864b.c line 431

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+
+
+static ssize_t cfag12864b_fopwrite(struct file *filp,
+	const char __user *buffer, size_t count, loff_t *offset)
+{
+	int ret = -EINVAL;
+	int result;
+	unsigned char *tmpbuffer;
+
+	if(down_interruptible(&cfag12864b_mutex))

space

drivers/lcddisplay/cfag12864b.c line 441

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+		return -ERESTARTSYS;
+
+	if (*offset > CFAG12864B_SIZE) {
+		ret = 0;
+		goto none;
+	}
+	if (*offset + count > CFAG12864B_SIZE)
+		count = CFAG12864B_SIZE-*offset;
+
+	tmpbuffer = kmalloc(count, GFP_KERNEL);

I would be very tempted to allocate a buffer at (open or init?) time, of size CFAG12864B_SIZE. It could be used here for fopwrite and in format_nolock without having to call kmalloc() repeatedly for those operations. However, fopioctlformat would still need to allocate its larger buffer.

drivers/lcddisplay/cfag12864b.c line 444

Quoted 3 lines
drivers/lcddisplay/cfag12864b.c +3 -0
+	if (tmpbuffer == NULL) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "FOP write: ERROR: "
+			"can't alloc memory %i bytes\n",count);

use "%zd" to print sizeof values, otherwise gcc complains. use space after comma.

Quoted 21 lines
drivers/lcddisplay/cfag12864b.c +21 -0
+		ret = -ENOMEM;
+		goto none;
+	}
+
+	result = copy_from_user(tmpbuffer, buffer, count);
+	if (result != 0) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "FOP write: ERROR: "
+			"can't copy memory from user\n");
+		ret = -EFAULT;
+		goto bufferalloced;
+	}
+
+	cfag12864b_write_nolock(*offset, tmpbuffer, count);
+
+	*offset += count;
+	ret = count;
+
+bufferalloced:
+	kfree(tmpbuffer);
+
+none:

drivers/lcddisplay/cfag12864b.c line 475

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+	up(&cfag12864b_mutex);
+	return ret;
+}
+
+static int cfag12864b_fopioctl(struct inode * inode, struct file * filp,
+	unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOTTY;
+
+	if(down_interruptible(&cfag12864b_mutex))

space after "if"

Quoted 69 lines
drivers/lcddisplay/cfag12864b.c +69 -0
+		return -ERESTARTSYS;
+
+	if (_IOC_TYPE(cmd) != CFAG12864B_IOC_MAGIC)
+		goto none;
+	if (_IOC_NR(cmd) > CFAG12864B_IOC_MAXNR)
+		goto none;
+
+	switch(cmd) {
+	case CFAG12864B_IOCON:
+		cfag12864b_on_nolock();
+		ret = 0;
+		break;
+	case CFAG12864B_IOCOFF:
+		cfag12864b_off_nolock();
+		ret = 0;
+		break;
+	case CFAG12864B_IOCCLEAR:
+		cfag12864b_clear_nolock();
+		ret = 0;
+		break;
+	case CFAG12864B_IOCFORMAT:
+		ret = cfag12864b_fopioctlformat((void __user *)arg);
+	}
+
+none:
+	up(&cfag12864b_mutex);
+	return ret;
+}
+
+static const struct file_operations cfag12864b_fops =
+{
+	.owner = THIS_MODULE,
+	.llseek = cfag12864b_fopseek,
+	.write = cfag12864b_fopwrite,
+	.ioctl = cfag12864b_fopioctl,
+};
+
+/*
+ * Module Init & Exit
+ */
+
+static int __init cfag12864b_init(void)
+{
+
+#include "cfag12864b_image.h"
+
+	int result;
+	int ret = -EINVAL;
+
+	result = alloc_chrdev_region(&cfag12864b_device, cfag12864b_firstminor,
+		cfag12864b_ndevices, CFAG12864B_NAME);
+	if (result < 0) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "ERROR: "
+			"can't alloc the char device region\n");
+		ret = result;
+		goto none;
+	}
+
+	cfag12864b_major = MAJOR(cfag12864b_device);
+	cfag12864b_minor = cfag12864b_firstminor;
+	cfag12864b_device = MKDEV(cfag12864b_major, cfag12864b_minor);
+
+	cfag12864b_clear_nolock();
+	cfag12864b_on_nolock();
+	cfag12864b_write_nolock(0, cfag12864b_image, CFAG12864B_SIZE);
+
+	cdev_init(&cfag12864b_chardevice,&cfag12864b_fops);
+	cfag12864b_chardevice.owner = THIS_MODULE;
+	cfag12864b_chardevice.ops = &cfag12864b_fops;

drivers/lcddisplay/cfag12864b.c line 554

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+	result = cdev_add(&cfag12864b_chardevice, cfag12864b_device,
+		cfag12864b_ndevices);
+	if (result < 0) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "ERROR: "
+			"unable to add a new char device\n");
+		ret = result;
+		goto regionalloced;
+	}
+
+	if(class_device_create(lcddisplay_class, NULL, cfag12864b_device, NULL,

space after "if"

drivers/lcddisplay/cfag12864b.c line 562

Quoted 8 lines
drivers/lcddisplay/cfag12864b.c +8 -0
+		"cfag12864b%d", cfag12864b_minor) == NULL) {
+		printk(KERN_ERR CFAG12864B_NAME ": " "ERROR: "
+			"unable to create a device class\n");
+		ret = -EINVAL;
+		goto cdevadded;
+	}
+
+	printk(KERN_INFO CFAG12864B_NAME ": " "Inited\n");

KERN_DEBUG ?

Quoted 12 lines
drivers/lcddisplay/cfag12864b.c +12 -0
+
+	return 0;
+
+cdevadded:
+	cdev_del(&cfag12864b_chardevice);
+
+regionalloced:
+	unregister_chrdev_region(cfag12864b_device, cfag12864b_ndevices);
+
+none:
+	return ret;
+}

drivers/lcddisplay/cfag12864b.c line 584

Quoted 10 lines
drivers/lcddisplay/cfag12864b.c +10 -0
+
+static void __exit cfag12864b_exit(void)
+{
+	cfag12864b_off_nolock();
+
+	class_device_destroy(lcddisplay_class, cfag12864b_device);
+	cdev_del(&cfag12864b_chardevice);
+	unregister_chrdev_region(cfag12864b_device, cfag12864b_ndevices);
+
+	printk(KERN_INFO CFAG12864B_NAME ": " "Exited\n");

KERN_DEBUG ?

drivers/lcddisplay/cfag12864b.c line 592

Quoted 8 lines
drivers/lcddisplay/cfag12864b.c +8 -0
+}
+
+module_init(cfag12864b_init);
+module_exit(cfag12864b_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Miguel Ojeda Sandonis <[email protected]>");
+MODULE_DESCRIPTION("cfag12864b");

That's not a useful MODULE_DESCRIPTION.

drivers/lcddisplay/cfag12864b_image.h line 4

Quoted 8 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/cfag12864b_image.h linux-2.6.18/drivers/lcddisplay/cfag12864b_image.h

drivers/lcddisplay/cfag12864b_image.h +4 -0
@@ -0,0 +1,95 @@
+#ifndef _CFAG12864B_IMAGE_H_
+#define _CFAG12864B_IMAGE_H_
+
+const unsigned char cfag12864b_image[] = {

What is in this image array? and what format is it in? It's not just advertising/logo material, is it? Is it really needed in cfag12864b_init()?

[bits deleted]

drivers/lcddisplay/Kconfig line 8

Quoted 12 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/Kconfig linux-2.6.18/drivers/lcddisplay/Kconfig

drivers/lcddisplay/Kconfig +8 -0
@@ -0,0 +1,110 @@
+#
+# For a description of the syntax of this configuration file,
+# see Documentation/kbuild/kconfig-language.txt.
+#
+# LCD Display drivers configuration.
+#
+# Maintainer: Miguel Ojeda Sandonis <[email protected]>
+#

Don't need any of those comments above.

drivers/lcddisplay/Kconfig line 19

Quoted 10 lines
drivers/lcddisplay/Kconfig +10 -0
+menu "LCD Display support"
+
+config LCDDISPLAY
+	tristate "LCD Display support"
+	default n
+	---help---
+	  If you have a LCD display, say Y.
+
+	  To compile this as a module, choose M here:
+	  module will be called lcddisplay.

"the module will be..."

drivers/lcddisplay/Kconfig line 21

Quoted 1 line
drivers/lcddisplay/Kconfig +1 -0
+	  Most LCD drivers use a I/O port (like the parallel port)

use an I/O port

drivers/lcddisplay/Kconfig line 22

Quoted 1 line
drivers/lcddisplay/Kconfig +1 -0
+	  so you will need to say Y or M at them if you want to see

say Y or M for them

drivers/lcddisplay/Kconfig line 27

Quoted 5 lines
drivers/lcddisplay/Kconfig +5 -0
+	  more options in this menu.
+
+	  If unsure, say N.
+
+	  Maintainer: Miguel Ojeda Sandonis <[email protected]>

Just in the MAINTAINERS file.

drivers/lcddisplay/Kconfig line 36

Quoted 8 lines
drivers/lcddisplay/Kconfig +8 -0
+comment "Parallel port dependent:"
+
+config KS0108
+	tristate "KS0108 LCD Controller"
+	depends on LCDDISPLAY && PARPORT
+	default n
+	---help---
+	  If you have a LCD display controlled by one or more KS0108

have an LCD display

drivers/lcddisplay/Kconfig line 42

Quoted 6 lines
drivers/lcddisplay/Kconfig +6 -0
+	  controllers, say Y. You will need also another more specific
+	  driver for your LCD.
+
+	  Depends on Parallel Port support. If you say Y at
+	  parport, you will be able to compile this as a module (M)
+	  and built-in as well (Y). If you said M at parport,
	s/and/or/

drivers/lcddisplay/Kconfig line 43

Quoted 1 line
drivers/lcddisplay/Kconfig +1 -0
+	  you will be able only to compile this as a module (M).

However, that is true everywhere, so we usually don't say it.

drivers/lcddisplay/Kconfig line 42

Quoted 2 lines
drivers/lcddisplay/Kconfig +2 -0
+	  To compile this as a module, choose M here:
+	  module will be called ks0108.

"the module will be..."

drivers/lcddisplay/Kconfig line 27

Quoted 3 lines
drivers/lcddisplay/Kconfig +3 -0
+	  If unsure, say N.
+
+	  Maintainer: Miguel Ojeda Sandonis <[email protected]>

Just in MAINTAINERS file.

drivers/lcddisplay/Kconfig line 53

Quoted 2 lines
drivers/lcddisplay/Kconfig +2 -0
+config KS0108_PORT
+	hex "Parallel port where the LCD is connected to"

...where the LCD is connected"

drivers/lcddisplay/Kconfig line 57

Quoted 4 lines
drivers/lcddisplay/Kconfig +4 -0
+	depends on KS0108
+	default 0x378
+	---help---
+	  The address of the parallel port where the LCD is connected to.

... where the LCD is connected.

drivers/lcddisplay/Kconfig line 66

Quoted 8 lines
drivers/lcddisplay/Kconfig +8 -0
+	  The first  standard parallel port address is 0x378.
+	  The second standard parallel port address is 0x278.
+	  The third  standard parallel port address is 0x3BC.
+
+	  You can specify a different address if you need.
+
+	  If you don't know what I'm talking about, load the parport module,
+	  and execute "dmesg". You can see there how many parallel ports

comma after "parallel ports"

drivers/lcddisplay/Kconfig line 67

Quoted 1 line
drivers/lcddisplay/Kconfig +1 -0
+	  where detected and which address everyone has.

add comma, change wording, like: "where they are connected," "and which address each one has."

Quoted 22 lines
drivers/lcddisplay/Kconfig +22 -0
+	  Usually you only need to use 0x378.
+
+	  If you compile this as a module, you can still override this
+	  using the module parameters.
+
+config KS0108_DELAY
+	int "Delay between each control writing (microseconds)"
+	depends on KS0108
+	default "2"
+	---help---
+	  Amount of time the ks0108 should wait between each control write
+	  to the parallel port.
+
+	  If your driver seems to miss random writings, increment this.
+
+	  If you don't know what I'm talking about, ignore it.
+
+	  If you compile this as a module, you can still override this
+	  using the module parameters.
+
+config CFAG12864B
+	tristate "CFAG12864B LCD Display"

drivers/lcddisplay/Kconfig line 100

Quoted 10 lines
drivers/lcddisplay/Kconfig +10 -0
+	depends on KS0108
+	default n
+	---help---
+	  If you have a Crystalfontz 128x64 2-color LCD display,
+	  cfag12864b Series, say Y. You also need the ks0108 LCD
+	  Controller driver.
+
+	  For help about how to wire your LCD to the parallel port,
+	  check this image: http://www.skippari.net/lcd/sekalaista
+	                    /crystalfontz_cfag12864B-TMI-V.png

check this image: <newline and put URL all on one line>

drivers/lcddisplay/Kconfig line 94

Quoted 2 lines
drivers/lcddisplay/Kconfig +2 -0
+	  To compile this as a module, choose M here:
+	  module will be called cfag12864b.

"the module will be..."

drivers/lcddisplay/Kconfig line 27

Quoted 3 lines
drivers/lcddisplay/Kconfig +3 -0
+	  If unsure, say N.
+
+	  Maintainer: Miguel Ojeda Sandonis <[email protected]>

Only in MAINTAINERS file.

drivers/lcddisplay/ks0108.c line 5

Quoted 11 lines
drivers/lcddisplay/Kconfig +2 -0
+endmenu
+

diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/ks0108.c linux-2.6.18/drivers/lcddisplay/ks0108.c

drivers/lcddisplay/ks0108.c +5 -0
@@ -0,0 +1,160 @@
+/*
+ * ks0108 Exported cmds (don't lock)
+ *
+ *   you _should_ lock in the top driver, so
+ *   this functions _should not_ get race conditions in any way.

these functions

drivers/lcddisplay/ks0108.c line 74

Quoted 9 lines
drivers/lcddisplay/ks0108.c +9 -0
+ *   Locking for each byte here would be so slow and useless.
+ */
+
+#define bit(n) (((unsigned char)1)<<(n))
+
+void ks0108_writecontrol(unsigned char byte)
+{
+	udelay(ks0108_delay);
+	parport_write_control(ks0108_parport, byte ^ (bit(0) | bit(1) | bit(3)));

what does the xor do? what are the bits?

drivers/lcddisplay/ks0108.c line 79

Quoted 5 lines
drivers/lcddisplay/ks0108.c +5 -0
+}
+
+void ks0108_displaystate(unsigned char state)
+{
+	ks0108_writedata((state ? bit(0) : 0) | bit(1) | bit(2) | bit(3) | bit(4) | bit(5));

Are the state bits documented somewhere? Can you use meaningful mnemonic names for the bits?

drivers/lcddisplay/ks0108.c line 84

Quoted 5 lines
drivers/lcddisplay/ks0108.c +5 -0
+}
+
+void ks0108_startline(unsigned char startline)
+{
+	ks0108_writedata(min(startline,(unsigned char)63) | bit(6) | bit(7));

bit definitions?

drivers/lcddisplay/ks0108.c line 89

Quoted 5 lines
drivers/lcddisplay/ks0108.c +5 -0
+}
+
+void ks0108_address(unsigned char address)
+{
+	ks0108_writedata(min(address,(unsigned char)63) | bit(6));

bit definition?

drivers/lcddisplay/ks0108.c line 94

Quoted 5 lines
drivers/lcddisplay/ks0108.c +5 -0
+}
+
+void ks0108_page(unsigned char page)
+{
+	ks0108_writedata(min(page,(unsigned char)7) | bit(3) | bit(4) | bit(5) | bit(7));

bit definitions?

drivers/lcddisplay/ks0108.c line 115

Quoted 10 lines
drivers/lcddisplay/ks0108.c +10 -0
+}
+
+static int __init ks0108_init(void)
+{
+	int result;
+	int ret = -EINVAL;
+
+	ks0108_parport = parport_find_base(ks0108_port);
+	if (ks0108_parport == NULL) {
+		printk(KERN_ERR KS0108_NAME ": " "ERROR: "

merge strings together, like:

		printk(KERN_ERR KS0108_NAME ": ERROR: "

(throughout the source files)

drivers/lcddisplay/ks0108.c line 116

Quoted 1 line
drivers/lcddisplay/ks0108.c +1 -0
+			"parport didn't find %i port\n",ks0108_port);

space after ","

Quoted 5 lines
drivers/lcddisplay/ks0108.c +5 -0
+		goto none;
+	}
+
+	ks0108_pardevice = parport_register_device(ks0108_parport, KS0108_NAME,
+		NULL, NULL, NULL, PARPORT_DEV_EXCL, NULL);

drivers/lcddisplay/ks0108.c line 131

Quoted 10 lines
drivers/lcddisplay/ks0108.c +10 -0
+	if (ks0108_pardevice == NULL) {
+		printk(KERN_ERR KS0108_NAME ": " "ERROR: "
+			"parport didn't register new device\n");
+		goto none;
+	}
+
+	result = parport_claim(ks0108_pardevice);
+	if (result != 0) {
+		printk(KERN_ERR KS0108_NAME ": " "ERROR: "
+			"can't claim %i parport, maybe in use\n",ks0108_port);

space after comma

Quoted 8 lines
drivers/lcddisplay/ks0108.c +8 -0
+		ret = result;
+		goto registered;
+	}
+
+	printk(KERN_INFO KS0108_NAME ": " "Inited - ks0108_port=0x%X ks0108_delay=%i\n", ks0108_port, ks0108_delay);
+	return 0;
+
+registered:

drivers/lcddisplay/ks0108.c line 159

Quoted 10 lines
drivers/lcddisplay/ks0108.c +10 -0
+	parport_unregister_device(ks0108_pardevice);
+
+none:
+	return ret;
+}
+
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Miguel Ojeda Sandonis <[email protected]>");
+MODULE_DESCRIPTION("ks0108");

Use a real MODULE_DESCRIPTION, please.

drivers/lcddisplay/lcddisplay.c line 8

Quoted 12 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/drivers/lcddisplay/lcddisplay.c linux-2.6.18/drivers/lcddisplay/lcddisplay.c

drivers/lcddisplay/lcddisplay.c +8 -0
@@ -0,0 +1,79 @@
+
+static int __init lcddisplay_init(void)
+{
+	int ret = -EINVAL;
+
+	lcddisplay_class = class_create(THIS_MODULE, LCDDISPLAY_NAME);
+	if (IS_ERR(lcddisplay_class)) {
+		printk(KERN_ERR LCDDISPLAY_NAME ": " "ERROR: "

The multiple quotation marks is a bit odd & difficult to read. How about just doing:

printk(KERN_ERR LCDDISPLAY_NAME ": ERROR: "

(in MANY places)?

drivers/lcddisplay/lcddisplay.c line 58

Quoted 5 lines
drivers/lcddisplay/lcddisplay.c +5 -0
+			"can't create %s class\n", LCDDISPLAY_NAME);
+		goto none;
+	}
+
+	printk(KERN_INFO LCDDISPLAY_NAME ": " "Inited\n");
	printk(KERN_INFO LCDDISPLAY_NAME ": Inited\n");
or better yet,
	printk(KERN_DEBUG LCDDISPLAY_NAME ": Inited\n");

drivers/lcddisplay/lcddisplay.c line 70

Quoted 11 lines
drivers/lcddisplay/lcddisplay.c +11 -0
+	return 0;
+
+none:
+	return ret;
+}
+
+static void __exit lcddisplay_exit(void)
+{
+	class_destroy(lcddisplay_class);
+
+	printk(KERN_INFO LCDDISPLAY_NAME ": " "Exited\n");

KERN_DEBUG & merge the strings?

drivers/lcddisplay/lcddisplay.c line 78

Quoted 8 lines
drivers/lcddisplay/lcddisplay.c +8 -0
+}
+
+module_init(lcddisplay_init);
+module_exit(lcddisplay_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Miguel Ojeda Sandonis <[email protected]>");
+MODULE_DESCRIPTION("lcddisplay");

Not a useful MODULE_DESCRIPTION.

include/linux/cfag12864b.h line 9

Quoted 13 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/include/linux/cfag12864b.h linux-2.6.18/include/linux/cfag12864b.h

include/linux/cfag12864b.h +9 -0
@@ -0,0 +1,58 @@
+
+#ifndef _CFAG12864B_H_
+#define _CFAG12864B_H_
+
+#include <linux/ioctl.h>
+
+#define CFAG12864B_WIDTH	128
+#define CFAG12864B_HEIGHT	64
+#define CFAG12864B_MATRIXSIZE	CFAG12864B_WIDTH*CFAG12864B_HEIGHT

Use parens around defined expressions (above).

include/linux/cfag12864b.h line 40

Quoted 6 lines
include/linux/cfag12864b.h +6 -0
+#define CFAG12864B_CONTROLLERS	2
+#define CFAG12864B_PAGES	8
+#define CFAG12864B_ADDRESSES	64
+#define CFAG12864B_SIZE		CFAG12864B_CONTROLLERS * \
+				CFAG12864B_PAGES * \
+				CFAG12864B_ADDRESSES

use parens.

Quoted 4 lines
include/linux/cfag12864b.h +4 -0
+#define CFAG12864B_IOC_MAGIC	0xFF
+#define CFAG12864B_IOC_MAXNR	0x03
+
+#define CFAG12864B_IOCOFF	_IO(CFAG12864B_IOC_MAGIC,0)

include/linux/cfag12864b.h line 55

Quoted 10 lines
include/linux/cfag12864b.h +10 -0
+#define CFAG12864B_IOCON	_IO(CFAG12864B_IOC_MAGIC,1)
+#define CFAG12864B_IOCCLEAR	_IO(CFAG12864B_IOC_MAGIC,2)
+#define CFAG12864B_IOCFORMAT	_IOW(CFAG12864B_IOC_MAGIC,3,void *)
+
+extern void cfag12864b_on(void);
+extern void cfag12864b_off(void);
+extern void cfag12864b_clear(void);
+extern void cfag12864b_write(unsigned short offset,
+	const unsigned char *buffer, unsigned short count);
+extern void cfag12864b_format(unsigned char *src);

All function prototypes that have parameters (above):
kernel style is to use type + param_name, not just type.

Documentation/lcddisplay/cfag12864b line 6

Quoted 12 lines
include/linux/cfag12864b.h +2 -0
+#endif /* _CFAG12864B_H_ */
+

diff -uprN -X dontdiff linux-2.6.18-vanilla/Documentation/lcddisplay/cfag12864b linux-2.6.18/Documentation/lcddisplay/cfag12864b

Documentation/lcddisplay/cfag12864b +6 -0
@@ -0,0 +1,371 @@
+
+---------
+2. WIRING
+---------
+
+The cfag12864b LCD Display Series don't have a official wiring.

... don't have official wiring.

Quoted 11 lines
Documentation/lcddisplay/cfag12864b +10 -0
+The common wiring is done to the parallel port:
+
+http://www.skippari.net/lcd/sekalaista/crystalfontz_cfag12864B-TMI-V.png
+
+You can get help at Crystalfontz and LCDInfo forums.
+
+
 
+3.1. ioctl & 128*64 boolean matrix
+-------------------------------------------------------
+

Documentation/lcddisplay/cfag12864b line 91

Quoted 10 lines
Documentation/lcddisplay/cfag12864b +10 -0
+This method is easier, but you have to update the entire display
+each time you want to change it.
+
+Note:
+
+	CFAG12864B_FORMATSIZE ==
+	CFAG12864B_WIDTH * CFAG12864B_HEIGHT ==
+	128 * 64
+
+Declare the matrix and other one:

other one what? another matrix buffer?

Documentation/lcddisplay/cfag12864b line 97

Quoted 5 lines
Documentation/lcddisplay/cfag12864b +5 -0
+	unsigned char MyDrawing[CFAG12864B_WIDTH][CFAG12864B_HEIGHT];
+
+	unsigned char Buffer[CFAG12864B_FORMATSIZE];
+
+Copy the 2d matrix to the buffer , like:

to the buffer,

Quoted 7 lines
Documentation/lcddisplay/cfag12864b +7 -0
+
+	for(i = 0; i < CFAG12864B_WIDTH; i++)
+		for(j = 0; j < CFAG12864B_HEIGHT; j++)
+			Buffer[i + j * CFAG12864B_WIDTH] = MyDrawing[i][j];
+
+Call the ioctl:
+

Documentation/lcddisplay/cfag12864b line 114

Quoted 10 lines
Documentation/lcddisplay/cfag12864b +10 -0
+	ioctl(fdisplay, CFAG12864B_IOCFORMAT, Buffer);
+
+Voila! Your drawing should appear on the screen.
+
+
+
+3.2. Direct writing
+-------------------
+
+This methods allows you to change each byte of the device,

This method

Documentation/lcddisplay/cfag12864b line 205

Quoted 8 lines
Documentation/lcddisplay/cfag12864b +7 -0
+so you can achieve a higher update rate updating only the pixels
+you are going to change.
 
+4.2 Example BMP writer
+----------------------
+
+You can take ideas from this code and start programming. I think it is useful
+for understanding how the driver can be used. It just work, don't expect

just works;

Documentation/lcddisplay/cfag12864b line 208

Quoted 3 lines
Documentation/lcddisplay/cfag12864b +3 -0
+good BMP-related code. I chose such bitmap format because it is simple.
+
+The program reads a .bmp 128x64 2-colors file, convert it to a

"converts it to a"

Quoted 1 line

+boolean [128*64] buffer and then use ioctl to display it on the screen.

"then uses"

Documentation/ioctl-number.txt Code Seq# Include File Comments · line 195

Quoted 9 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/Documentation/ioctl-number.txt linux-2.6.18/Documentation/ioctl-number.txt

Documentation/ioctl-number.txt +2 -0
@@ -191,3 +191,5 @@ Code	Seq#	Include File		Comments
 					<mailto:[email protected]>
 0xF3	00-3F	video/sisfb.h		sisfb (in development)
 					<mailto:[email protected]>
+0xFF	00-1F	linux/cfag12864b.h	cfag12864b LCD Display Driver
+					<mailto:[email protected]>

Also need additions to Documenation/ABI/ ? Please read/check Documentation/ABI/README. and please read/check Documentation/SubmitChecklist.

MAINTAINERS M: [email protected] · line 1714

Quoted 12 lines

diff -uprN -X dontdiff linux-2.6.18-vanilla/MAINTAINERS linux-2.6.18/MAINTAINERS

MAINTAINERS +5 -0
@@ -1707,6 +1707,11 @@ M:	[email protected]
 L:	[email protected]
 S:	Maintained
 
+LCD DISPLAY DRIVERS
+P:	Miguel Ojeda Sandonis
+M:	[email protected]
+S:	Maintained
+

Needs a mailing list (L:) entry.

---
~Randy

#4 lore raw

...

Quoted 1 line
Quoted 1 line

patching file drivers/lcddisplay/cfag12864b.c

...

What does the D in LCD stand for? I suggest this is named drivers/lcdisplay/ instead.

Signature · 3 lines
Stefan Richter
-=====-=-==- =--= ====-
http://arcgraph.de/sr/
#5 lore raw
Quoted 6 lines · On Sat, 30 Sep 2006 23:36:44 +0200 Stefan Richter wrote:

...

Quoted 1 line
Quoted 1 line

patching file drivers/lcddisplay/cfag12864b.c

...

What does the D in LCD stand for? I suggest this is named drivers/lcdisplay/ instead.

Yes, someone else mentioned that too.
It does mean Display.
not a big deal to me in this age of acronyms.

---
~Randy

#6 lore raw

Randy, I'm testing right now the V7 of the driver after fixing all your mistakes found. Thank you for your deep work: the sharing of the buffer was a very good point. Finally I allocated both 2 (the bigger and the smaller one at the initialization).

In a few minutes I will send the V7 patch.

About "lcdisplay":

Quoted 4 lines · Randy Dunlap
Quoted 2 lines · On Sat, 30 Sep 2006 23:36:44 +0200 Stefan Richter wrote:

...

Quoted 1 line
Quoted 1 line

patching file drivers/lcddisplay/cfag12864b.c

Quoted 9 lines
Quoted 4 lines

...

What does the D in LCD stand for? I suggest this is named drivers/lcdisplay/ instead.

Yes, someone else mentioned that too.
It does mean Display.
not a big deal to me in this age of acronyms.

Well, I can rename it, but there is a problem:

How should I write the name in other places?

I mean, for example, in Kconfig:

tristate "KS0108 LCD Controller"

if the ks0108 is a LCD controller, then

tristate "CFAG12864B LCD Display"

should be the right way to write it?

tristate "CFAG12864B LCDisplay"

seems so ugly to me. Is it? Remember, this applies for all the other places in the documentation / source code / ...

IMHO it seems better as LCD Display. Yeah, drivers/lcdisplay/ would be nice, but I don't think the same about other places:

LCD Controller.
LCDisplay??

#7 lore raw
Quoted 28 lines · On Sat, 30 Sep 2006 22:45:56 +0000 Miguel Ojeda wrote:

Randy, I'm testing right now the V7 of the driver after fixing all your mistakes found. Thank you for your deep work: the sharing of the buffer was a very good point. Finally I allocated both 2 (the bigger and the smaller one at the initialization).

In a few minutes I will send the V7 patch.

About "lcdisplay":

Quoted 4 lines · Randy Dunlap
Quoted 2 lines · On Sat, 30 Sep 2006 23:36:44 +0200 Stefan Richter wrote:

...

Quoted 1 line
Quoted 1 line

patching file drivers/lcddisplay/cfag12864b.c

Quoted 9 lines
Quoted 4 lines

...

What does the D in LCD stand for? I suggest this is named drivers/lcdisplay/ instead.

Yes, someone else mentioned that too.
It does mean Display.
not a big deal to me in this age of acronyms.

Well, I can rename it, but there is a problem:

How should I write the name in other places?

Quoted 7 lines

I mean, for example, in Kconfig:

tristate "KS0108 LCD Controller"

if the ks0108 is a LCD controller, then

tristate "CFAG12864B LCD Display"

That seems very common to me.

Quoted 11 lines

should be the right way to write it?

tristate "CFAG12864B LCDisplay"

seems so ugly to me. Is it? Remember, this applies for all the other places in the documentation / source code / ...

IMHO it seems better as LCD Display. Yeah, drivers/lcdisplay/ would be nice, but I don't think the same about other places:

LCD Controller.

OK

Quoted 1 line

LCDisplay??

I would continue to use LCD display (small d).

---
~Randy

#8 lore raw
Quoted 1 line · Randy Dunlap

On Sat, 30 Sep 2006 22:45:56 +0000 Miguel Ojeda wrote:

...

Quoted 3 lines
Quoted 1 line

tristate "CFAG12864B LCD Display"

That seems very common to me.

It is as common as it is wrong. http://en.wikipedia.org/wiki/RAS_Syndrome

...

Quoted 1 line
Quoted 1 line

LCDisplay??

A contraction like LCDisplay, like my suggested lcdisplay for the path name, is IMO not suitable for use in normal written language. Use it at most for path names where we contract words into one or would write lc-display or lc_display.

Quoted 1 line

I would continue to use LCD display (small d).

Still wrong language.

"LCD" and "LC display" are correct.

Signature · 3 lines
Stefan Richter
-=====-=-==- =-=- ----=
http://arcgraph.de/sr/
#9 lore raw
Quoted 17 lines · On Sun, 01 Oct 2006 10:20:41 +0200 Stefan Richter wrote:
Quoted 1 line · Randy Dunlap

On Sat, 30 Sep 2006 22:45:56 +0000 Miguel Ojeda wrote:

...

Quoted 3 lines
Quoted 1 line

tristate "CFAG12864B LCD Display"

That seems very common to me.

It is as common as it is wrong. http://en.wikipedia.org/wiki/RAS_Syndrome

...

Quoted 1 line
Quoted 1 line

LCDisplay??

A contraction like LCDisplay, like my suggested lcdisplay for the path name, is IMO not suitable for use in normal written language. Use it at most for path names where we contract words into one or would write lc-display or lc_display.

Quoted 5 lines
Quoted 1 line

I would continue to use LCD display (small d).

Still wrong language.

"LCD" and "LC display" are correct.

whatever.
I've never heard anyone speak of or write about an LC display.

---
~Randy

#10 lore raw
Quoted 1 line · Randy Dunlap

On Sun, 01 Oct 2006 10:20:41 +0200 Stefan Richter wrote:

...

Quoted 1 line
Quoted 1 line

It is as common as it is wrong. http://en.wikipedia.org/wiki/RAS_Syndrome

...

Quoted 4 lines
Quoted 1 line

"LCD" and "LC display" are correct.

whatever.
I've never heard anyone speak of or write about an LC display.

Nor does anybody say or write AT machine. A web search finds 15 times more occurrences of "LC-Display" on German pages than of "LC display" on English pages. So it seams my suggestion of "LC display" is a bad one for usage outside of Germany. BTW, "LCD-Display" is doubly as often found on German web pages than "LC-Display" on German pages, and I'm not surprised.

At some point, widely used words and expressions become accepted parts of a language. "LCD display" however is evidently still considered bad style --- in English as well as in German. But maybe this stylistic mistake is more prone to criticism among Germans than among North Americans. (Perhaps not) coincidentally, the other person who criticized it in this thread is obviously German too.

Signature · 3 lines
Stefan Richter
-=====-=-==- =-=- ----=
http://arcgraph.de/sr/
#11 lore raw
Quoted 8 lines · Stefan Richter

Nor does anybody say or write AT machine. A web search finds 15 times more occurrences of "LC-Display" on German pages than of "LC display" on English pages. So it seams my suggestion of "LC display" is a bad one for usage outside of Germany. BTW, "LCD-Display" is doubly as often found on German web pages than "LC-Display" on German pages, and I'm not surprised.

Searching Google for "pantalla LCD" you can find 1.590.000 pages.

And it is wrong ;-)

(Please continue the discussion the same "thread" (V7) as it is the last version of the patch I sent.)