From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Date: Mon, 31 Jan 2005 21:30:59 +0000
Subject: Re: [PATCH 2.6.11] Altix patch for device driver support for the CX port of SGI's TIO chip
Message-Id: <20050131213059.GA20161@infradead.org>
List-Id: <linux-ia64.vger.kernel.org>
References: <Pine.LNX.4.44.0501311506590.30646-100000@fsgi025.americas.sgi.com>
In-Reply-To: <Pine.LNX.4.44.0501311506590.30646-100000@fsgi025.americas.sgi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: linux-ia64@vger.kernel.org

On Mon, Jan 31, 2005 at 03:11:01PM -0600, Bruce Losure wrote:
> 
> This patch provide support for the CX port of the SGI TIO chip.
> The CX port of the SGI TIO chip may be connected to FPGA based
> hardware.  The changes in this patch may be used in support of device
> driver developers who are writing drivers for that FPGA hardware.
> The module provides driver registration/unregistration routines, device
> registration/unregistration routines, interrupt allocation/deallocation
> and an ioctl.

Comment one: please use the driver model to get rid of all your internal
bookkeeping.

Comment two:  without an actual user this won't go into the tree.

> +typedef struct cx_id_s {
> +	unsigned int part_num;
> +	unsigned int mfg_num;
> +	int nasid;
> +} cx_id_t;

please avoid typedefs (also in lots of other places)

> +#define TIOCX_IOCTL_BASE	0xdd	// see Documentation/ioctl-number.txt
> +#define TIOCX_IOCTL_CX_RELOAD   _IO(TIOCX_IOCTL_BASE, 1)
> +#define TIOCX_IOCTL_DEV_LIST    _IO(TIOCX_IOCTL_BASE, 2)

also avoid new ioctl-based interfaces.

> +#include <asm-ia64/sn/types.h>

always use <asm/foo.h>

> +obj-m 				+= tiocx.o

this is broken.  Either it's unconditionally built in or should
be a config option - unconditionally modular is not an option.

>  DEFINE_PER_CPU(struct pda_s, pda_percpu);
> +EXPORT_PER_CPU_SYMBOL(pda_percpu);

don't think it's something that should be exported.  explain what you
need and provide accessors.

> +#include <asm/sn/types.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/proc_fs.h>
> +#include <asm/sn/sn_sal.h>
> +#include <asm/sn/addrs.h>
> +#include <asm/sn/io.h>
> +#include "shubio.h"
> +#include "tio.h"
> +#include "tiocx.h"
> +#include "xtalk/xwidgetdev.h"
> +#include "xtalk/hubdev.h"
> +#include <asm-ia64/delay.h>
> +#include <asm/uaccess.h>

<linux/*.h> before <asm/*.h> before local headers please.