Re: [RFC] Host Virtual Serial Interface driver

["Randy.Dunlap" <rddunlap@osdl.org>]

On Fri, 06 Aug 2004 16:23:05 -0500 Hollis Blanchard wrote:

| Hi, I have a new char driver I'd like to get comments on. It is specific
| to IBM's p5 server line; I've included a description from the comments
| here:
...
| I've included the whole file below; it's pretty much self-contained. All
| comments welcome.
| 
| -- 
|  
| #include <linux/init.h>
| #include <linux/module.h>
| #include <linux/console.h>
| #include <linux/major.h>
| #include <linux/kernel.h>
| #include <linux/sysrq.h>
| #include <linux/tty.h>
| #include <linux/tty_flip.h>
| #include <linux/sched.h>
| #include <linux/kbd_kern.h>
| #include <linux/spinlock.h>
| #include <linux/ctype.h>
| #include <linux/interrupt.h>
| #include <linux/delay.h>

To the extent possible, we like to put the linux/* files in alpha
order, and same with the asm/* files.  Separately, as you have them.

| #include <asm/uaccess.h>
| #include <asm/hvconsole.h>
| #include <asm/prom.h>
| #include <asm/hvcall.h>
| #include <asm/vio.h>
| 
| #define __ALIGNED__	__attribute__((__aligned__(sizeof(long))))

You should explain this bit (__ALIGNED__).

| static inline int hdrlen(const uint8_t *packet)
| {
| 	const int lengths[] = { 4, 6, 6, 8, };
| 	struct hvsi_header *header = (struct hvsi_header *)packet;
| 
| 	return lengths[VS_DATA_PACKET_HEADER - header->type];
| }

Any chance of bad data (value) in header->type ?

| 		if (hangup) {
| 			tty_hangup(hangup);
| 		}

extra braces (style); maybe in a few other places also.

--
~Randy