Re: [PATCH 2.6.18 real-V5] drivers: add lcd display support

On Fri, 22 Sep 2006 22:03:46 +0200
Miguel Ojeda Sandonis <maxextreme@xxxxxxxxx> wrote:

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

...

diff -uprN -X dontdiff linux-2.6.18-vanilla/include/linux/device.h linux-2.6.18/include/linux/device.h
--- linux-2.6.18-vanilla/include/linux/device.h 2006-09-20 14:52:00.000000000 +0200
+++ linux-2.6.18/include/linux/device.h 2006-09-20 14:55:56.000000000 +0200
@@ -271,7 +271,7 @@ struct class_interface {
extern int class_interface_register(struct class_interface *);
extern void class_interface_unregister(struct class_interface *);

-extern struct class *class_create(struct module *owner, char *name);
+extern struct class *class_create(struct module *owner, const char *name);

Please prepare a separate patch for this, send to Greg.

--- linux-2.6.18-vanilla/drivers/lcddisplay/cfag12864b.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.18/drivers/lcddisplay/cfag12864b.c 2006-09-22 21:46:52.000000000 +0200
@@ -0,0 +1,529 @@
+/*
+ * Filename: cfag12864b.c
+ * Version: 0.1.0
+ * Description: cfag12864b LCD Display Driver
+ * License: GPLv2
+ * Depends: lcddisplay ks0108
+ *
+ * Author: Miguel Ojeda Sandonis <maxextreme@xxxxxxxxx>
+ * Date: 2006-09-21
+ */
+
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/lcddisplay.h>
+#include <linux/ks0108.h>
+#include <linux/cfag12864b.h>
+#include <asm/uaccess.h>
+
+#define NAME "cfag12864b"
+#define PRINTK_PREFIX KERN_INFO NAME ": "

In numerous places the driver uses PRINTK_PREFIX for reporting errors. It
should use KERN_ERR. I suggest that PRINTK_PREFIX be removed and that you
open-code all the printk facility levels, make sure that the appropriate
level is being used at each site.

+
+
+/*
+ * Device
+ */
+
+static const unsigned int cfag12864b_firstminor;
+static const unsigned int cfag12864b_ndevices = 1;
+static const char * cfag12864b_name = NAME;

The more usual kernel style is to not put a space after the `*':

static const char *cfag12864b_name = NAME;

(this affects the entire patch).


Suggest that cfag12864b_name be removed - just use NAME at the single place
where this variable is used.

+static int cfag12864b_major;
+static int cfag12864b_minor;
+static dev_t cfag12864b_device;
+struct cdev cfag12864b_chardevice;
+
+
+
+

One blank line is sufficient (there are many instances of this)

+/*
+ * cfag12864b Commands
+ */
+
+static const unsigned int cfag12864b_bits = 8;
+static const unsigned int cfag12864b_width = CFAG12864B_WIDTH;
+static const unsigned int cfag12864b_height = CFAG12864B_HEIGHT;
+static const unsigned int cfag12864b_matrixsize = CFAG12864B_MATRIXSIZE;
+static const unsigned int cfag12864b_controllers = CFAG12864B_CONTROLLERS;
+static const unsigned int cfag12864b_pages = CFAG12864B_PAGES;
+static const unsigned int cfag12864b_addresses = CFAG12864B_ADDRESSES;
+static const unsigned int cfag12864b_size = CFAG12864B_SIZE;
+static unsigned char cfag12864b_state;
+
+static void cfag12864b_set(void)
+{
+ ks0108_writecontrol(cfag12864b_state);
+}
+
+static void cfag12864b_setbit(unsigned char state, unsigned char bit)
+{
+ if (state)
+ set_bit(bit, (void*)&cfag12864b_state);
+ else
+ clear_bit(bit, (void*)&cfag12864b_state);
+ cfag12864b_set();
+}

bitops are defined on an unsigned long only. This trick is as ugly as sin
and is buggy on big-endian CPUs. Suggest that cfag12864b_state be
converted to unsigned long.

+{
+ cfag12864b_startline(0);
+}
+
+
+

blank lines

+/*
+ * Auxiliary
+ */
+
+static void normalizeoffset(unsigned int * offset)

coding style.

+{
+ if (*offset >= cfag12864b_pages*cfag12864b_addresses)
+ *offset -= cfag12864b_pages*cfag12864b_addresses;
+}

Ths usual kernel style is to put a single space around arithmetic operators
and around comparison operators. This affects the entire patch, please.


+void cfag12864b_clear(void)
+{
+ unsigned char page,address;
+
+ cfag12864b_setcontrollers(1, 1);
+ for (page=0; page<cfag12864b_pages; ++page) {

For example,

for (page = 0; page < cfag12864b_pages; page++) {

would be more kernelish.

The use of the identifier `page' here is unfortunate. We usually expect
such a thing to have type `struct page *'. I understand that "Each
controller is divided into 8 pages", but it'd be nice if some different
nomenclature could be used here. If nothing else comes to mind, we can
live with it as-is.

+void cfag12864b_write(
+ unsigned short offset,
+ const unsigned char * buffer,
+ unsigned short count)
+{

It is more usual to do

void cfag12864b_write(unsigned short offset, const unsigned char *buffer,
unsigned short count)
{

+ for (controller=0; controller < cfag12864b_controllers; ++controller)
+ for (page=0; page < cfag12864b_pages; ++page)
+ for (address=0; address < cfag12864b_addresses; ++address) {
+ dest[(controller*cfag12864b_pages+page)*cfag12864b_addresses+address]=0;
+ for (bit=0; bit < cfag12864b_bits; ++bit)
+ if (src[controller*cfag12864b_addresses+address+(page*cfag12864b_bits+bit)*cfag12864b_width])
+ set_bit(bit, (void*)(dest+(controller*cfag12864b_pages+page)*cfag12864b_addresses+address));
+ }

That's rather ugly-looking.

It's also probably-incorrect on big-endian CPUs. Perhaps you should not
use bitops at all for this driver, use open-coded | and &/~ instead?


The driver doesn't have any locking. Is it racy on SMP and/or
CONFIG_PREEMPT?

+static int cfag12864b_fopioctlformat(void __user * arg)
+{
+ int result;
+ int ret = -ENOTTY;
+
+ unsigned char * tmpbuffer;
+
+ tmpbuffer = kmalloc(
+ sizeof(unsigned char)*cfag12864b_matrixsize,GFP_KERNEL);
+ if (tmpbuffer == NULL) {
+ printk(PRINTK_PREFIX "FOP ioctl: ERROR: "
+ "can't alloc memory %i bytes\n",
+ sizeof(unsigned char)*cfag12864b_matrixsize);
+ goto none;
+ }
+
+ result = copy_from_user(
+ tmpbuffer,
+ arg,
+ sizeof(unsigned char)*cfag12864b_matrixsize);

result = copy_from_user(tmpbuffer, arg, cfag12864b_matrixsize);


+ if (result != 0) {
+ printk(PRINTK_PREFIX "FOP ioctl: ERROR: "
+ "can't copy memory from user\n");
+ goto bufferalloced;
+ }
+
+ cfag12864b_format(tmpbuffer);
+
+ ret = 0;
+
+bufferalloced:
+ kfree(tmpbuffer);
+
+none:
+ return ret;
+}
+

...

+
+static struct file_operations cfag12864b_fops =

`static const struct'

+
+ class_device_create(
+ lcddisplay_class,NULL,
+ cfag12864b_device,
+ NULL,"cfag12864b%d", cfag12864b_minor);

class_device_create() can fail.

+void ks0108_writecontrol(unsigned char byte)
+{
+ const unsigned int ecycledelay = 2;
+ udelay(ecycledelay);
+ parport_write_control(ks0108_parport, byte^(bit(3)|bit(1)|bit(0)));
+}

udelay(2) would be clearer.

+void ks0108_displaystate(unsigned char state)
+{
+ unsigned char cmd = bit(1) | bit(2) | bit(3) | bit(4) | bit(5);
+ if (state)
+ set_bit(0, (void*)&cmd);

argh. Even if this driver will only ever run on big-endian hardware,
please don't do this.

+void ks0108_startline(unsigned char startline)
+{
+ const unsigned char maxstartline = 63;
+ unsigned char cmd = bit(6) | bit(7);
+ if (startline > maxstartline)
+ startline = maxstartline;

There are a lot of open-coded min() and max() operations in this driver.
Suggest it be changed to use min() and max().


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/