On Mon, Feb 17, 2003 at 11:21:23PM +0900, Osamu Tomita wrote:
> diff -Nru linux/drivers/scsi/pc980155.c linux98/drivers/scsi/pc980155.c
> --- linux/drivers/scsi/pc980155.c 1970-01-01 09:00:00.000000000 +0900
> +++ linux98/drivers/scsi/pc980155.c 2003-02-17 21:05:25.000000000 +0900
> @@ -0,0 +1,264 @@
> +#include <linux/kernel.h>
> +#include <linux/types.h>
no copyright statement?
> +#include <linux/mm.h>
> +#include <linux/blk.h>
> +#include <linux/sched.h>
> +#include <linux/version.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/interrupt.h>
> +
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/irq.h>
> +#include <asm/dma.h>
> +#include <linux/module.h>
> +
> +#include "scsi.h"
> +#include "hosts.h"
> +#include "wd33c93.h"
> +#include "pc980155.h"
> +#include "pc980155regs.h"
> +
> +#define DEBUG
> +
> +#include<linux/stat.h>
I doj't think you need this.
> +
> +static inline void __print_debug_info(unsigned int);
> +static inline void __print_debug_info(unsigned int a){}
> +#define print_debug_info() __print_debug_info(base_io);
Please remove unused declarations.
> +
> +#define NR_BASE_IOS 4
> +static int nr_base_ios = NR_BASE_IOS;
> +static unsigned int base_ios[NR_BASE_IOS] = {0xcc0, 0xcd0, 0xce0, 0xcf0};
> +static unsigned int SASR;
> +static unsigned int SCMD;
> +static wd33c93_regs regs = {&SASR, &SCMD};
> +
> +static struct Scsi_Host *pc980155_host = NULL;
> +static void pc980155_intr_handle(int irq, void *dev_id, struct pt_regs *regp);
> +
> +inline void pc980155_dma_enable(unsigned int base_io){
> + outb(0x01, REG_CWRITE);
> + WAIT();
> +}
> +inline void pc980155_dma_disable(unsigned int base_io){
> + outb(0x02, REG_CWRITE);
> + WAIT();
> +}
This whole file wants to be reformatted to follow Documentation/CodingStyle.
> + continue;
> + }
> + if (request_dma(dma, "PC-9801-55")) {
> + printk(KERN_ERR "PC-9801-55: "
> + "unable to allocate DMA channel %d\n", dma);
> + free_irq(irq, NULL);
> + continue;
> + }
You seems to sometimes miss some cleanup on failures. cleanups gotos might
help :)
> +
> + pc980155_host = scsi_register(tpnt, sizeof(struct WD33C93_hostdata));
This can return NULL and you need to handle that.
> +int pc980155_proc_info(char *buf, char **start, off_t off, int len,
> + int hostno, int in)
> +{
> + /* NOT SUPPORTED YET! */
> +
> + if (in) {
> + return -EPERM;
> + }
> + *start = buf;
> + return sprintf(buf, "Sorry, not supported yet.\n");
> +}
So if it's not supported don't implement this entry point :)
> +
> +int pc980155_setup(char *str)
> +{
> +next:
> + if (!strncmp(str, "io:", 3)){
> + base_ios[0] = simple_strtoul(str+3,NULL,0);
> + nr_base_ios = 1;
> + while (*str > ' ' && *str != ',')
> + str++;
> + if (*str == ','){
> + str++;
> + goto next;
> + }
> + }
> + return 0;
This might be easier to read when rewritten into a while loop..
> +}
> +
> +int scsi_pc980155_release(struct Scsi_Host *pc980155_host)
> +{
> +#ifdef MODULE
> + pc980155_int_disable(regs);
> + release_region(pc980155_host->io_port, pc980155_host->n_io_port);
> + free_irq(pc980155_host->irq, NULL);
> + free_dma(pc980155_host->dma_channel);
> + wd33c93_release();
> +#endif
no need for the ifdef module here.
> + return 1;
> +}
> +
> +__setup("pc980155=", pc980155_setup);
> +
> +Scsi_Host_Template driver_template = SCSI_PC980155;
Please don't use the SCSI_PC980155 macro - declare the struct members here
directly.
> +int pc98_bios_param(struct block_device *bdev, int *ip)
This should _not_ be in sd.c but the lowlevel driver.
> +static spinlock_t wd_lock = SPIN_LOCK_UNLOCKED;
Where is this used?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Feb 23 2003 - 22:00:21 EST