Re: [PATCH] This is my first commit

[Joe Perches]

On Tue, 2014-05-27 at 14:44 +0800, hnnnet48@xxxxxxxxx wrote:
> From: kevin <hnnnet48@xxxxxxxxx>

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 <support@xxxxxxxxxxxxxxxx>");

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


--
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/