On Tue, 2014-05-27 at 14:44 +0800, hnnne...@gmail.com wrote:
> From: kevin <hnnne...@gmail.com>
Hi Kevin.
You subject line should be something like:
[PATCH] Add support for GeneralTouch serial screen
There should be a commit message too.
> ---
> gtserio.c | 188
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
In what directory should this file be placed?
Will you submit a Makefile/Kconfig mechanism to make it?
You should run your patch through scripts/checkpatch.pl
It will flag a number of things you should consider fixing.
> create mode 100755 gtserio.c
755 is not correct. 644 please.
> diff --git a/gtserio.c b/gtserio.c
[]
> + * gtserio.c - Create an input/output character device for GeneralTouch
> serial screen
[]
> +MODULE_AUTHOR("GeneralTouch <supp...@generaltouch.com>");
Kevin, do you work for generaltouch
or did you get this file from somewhere?
> +static int Device_Open = 0;
> +struct input_dev *devq;
> +char Message[BUF_LEN];
> +dev_t gts_t;
> +unsigned char val[10];
> +int gtsdata[4];
> +unsigned long min_x = 0;
> +unsigned long max_x = 32767;
> +unsigned long min_y = 0;
> +unsigned long max_y = 32767;
All of these should probably be static.
> +char *Message_Ptr;
CamelCase is not generally used in linux-kernel.
> +static int device_open(struct inode *inode, struct file *file)
> +{
> + printk("GTS device_open(%p)\n", file);
printk should use a KERN_<LEVEL>
> +
> +#ifdef DEBUG
> + printk("device_open(%p)\n", file);
> +#endif
Unnecessary duplication, this DEBUG should be removed.
> +static int device_release(struct inode *inode, struct file *file)
> +{
> +#ifdef DEBUG
> + printk("device_release(%p,%p)\n", inode, file);
> +#endif
pr_debug is used instead of
#ifdef DEBUG
printk(foo...)
#endif
> + return SUCCESS;
return 0;
instead of a somewhat unnecessary #define
> + printk("val[3] = %x,val[4] = %x\nval[5] = %x,val[6] = %x\n", val[3],
> + val[4], val[5], val[6]);
I suspect this should be pr_debug too.
> + gtsdata[0] = val[2];
> + gtsdata[1] = (val[4] << 8) | (val[3]);
> + gtsdata[2] = (val[6] << 8) | (val[5]);
> + gtsdata[3] = val[9];
> + printk("gtsdata[1] = %x\ngtsdata[2] = %x\n", gtsdata[1], gtsdata[2]);
and this
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
