On Fri, 22 Sep 2006 22:03:46 +0200
Miguel Ojeda Sandonis <[email protected]> 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 <[email protected]>
> + * 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
