Re: [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (16/21) SCSI

[Christoph Hellwig]

> +int pc98_bios_param(struct block_device *bdev, int *ip)
> +{
> +	/* Note: This function is called from fs/partitions/nec98.c too. */
> +	/* So we creat 'sdp' from 'bdev' here.				 */
> +	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);

this is still not good - you shouldn't expose struct scsi_disk outside
sd.c.  Please change the pc98_bios_param() prototype to that of the
bios_param entry point (direct passing of capacity).

Can you explain what this first_real_host() stuff is for - we need some
way to handle this better.

> +static int io = 0;

no need to initialize to zero - it's in .bss and thus cleared implicitly.

> +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();
> +}

shouldn't these be static?

> +	err2:
> +	free_irq(irq, NULL);
> +	err1:
> +	release_region(base_io, 6);
> +	return 0;

small codingstyle nitpick:  this should be either

err2:
	free_irq(irq, NULL);
err1:
	release_region(base_io, 6);
	return 0;

or:

 err2:
	free_irq(irq, NULL);
 err1:
	release_region(base_io, 6);
	return 0;

> +Scsi_Host_Template driver_template = {

static?

> +#ifndef _SCSI_PC9801_55_H
> +#define _SCSI_PC9801_55_H
> +
> +#include <linux/types.h>
> +#include <linux/kdev_t.h>
> +#include <scsi/scsicam.h>
> +
> +int wd33c93_queuecommand(Scsi_Cmnd *, void (*done)(Scsi_Cmnd *));
> +int wd33c93_abort(Scsi_Cmnd *);
> +int wd33c93_reset(Scsi_Cmnd *, unsigned int);
> +int scsi_pc980155_detect(Scsi_Host_Template *);
> +int scsi_pc980155_release(struct Scsi_Host *);
> +int pc980155_proc_info(char *, char **, off_t, int, int, int);
> +
> +#ifndef CMD_PER_LUN
> +#define CMD_PER_LUN 2
> +#endif
> +
> +#ifndef CAN_QUEUE
> +#define CAN_QUEUE 16
> +#endif
> +
> +#endif /* _SCSI_PC9801_55_H */

Move that all into the actual c source file.  In addition all those
functions can be static.

> +	BIOS_PARAM_OVERRIDE(sdp, bdev, sdkp->capacity, diskinfo);
> +

the way this is done is ugly.  I'm still not sure how this is done
best.  When do you need the pc98 geometry exactly?  i.e. can it happen
with one of the existing linux scsi drivers?

> +#if defined(CONFIG_SCSI_PC980155) || defined(CONFIG_SCSI_PC980155_MODULE)
> +#include "pc980155regs.h"
> +#else /* !CONFIG_SCSI_PC980155 */
>  
>  static inline uchar read_wd33c93(const wd33c93_regs regs, uchar reg_num)
>  {
> @@ -203,6 +206,7 @@
>     *regs.SCMD = cmd;
>     mb();
>  }
> +#endif /* CONFIG_SCSI_PC980155 */

The wd33c93 changes are ugly as hell, but that's not your fault.  I'll
try to rework it to abstract out the different implementations better.
Could you perform some testing for me if I send you updated versions?