linux-kernel - Re: [PATCH] This is my first commit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 May 2014 00:03:55 -0700
From:	Joe Perches <joe@...ches.com>
To:	hnnnet48@...il.com
Cc:	gregkh@...e.de, devel@...uxdriverproject.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] This is my first commit

On Tue, 2014-05-27 at 14:44 +0800, hnnnet48@...il.com wrote:
> From: kevin <hnnnet48@...il.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 <support@...eraltouch.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


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