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

[Andrew Morton]

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/