linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Osamu Tomita <tomita@cinet.co.jp>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alan Cox <alan@lxorguk.ukuu.org.uk>,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (16/21) SCSI
Date: Sun, 23 Feb 2003 10:52:28 +0000	[thread overview]
Message-ID: <20030223105228.A16083@infradead.org> (raw)
In-Reply-To: <20030223095528.GQ1324@yuzuki.cinet.co.jp>; from tomita@cinet.co.jp on Sun, Feb 23, 2003 at 06:55:28PM +0900

> +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?


  reply	other threads:[~2003-02-23 10:42 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2003-02-23  9:21 [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (0/21) summary Osamu Tomita
2003-02-23  9:34 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (1/21) ALSA Osamu Tomita
2003-02-23  9:36 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (2/21) APM Osamu Tomita
2003-02-23  9:38 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (3/21) console Osamu Tomita
2003-02-23  9:40 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (4/21) Misc core Osamu Tomita
2003-02-23  9:41 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (5/21) DMA Osamu Tomita
2003-02-23  9:43 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (6/21) FS & partiton Osamu Tomita
2003-02-23 10:29   ` Christoph Hellwig
2003-02-23 10:45     ` Osamu Tomita
2003-02-24 14:05       ` Osamu Tomita
2003-02-24 14:16         ` Christoph Hellwig
2003-02-23 10:47     ` John Bradford
2003-02-23  9:44 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (7/21) IDE Osamu Tomita
2003-02-23  9:46 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (8/21) kanji Osamu Tomita
2003-02-23  9:47 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (9/21) kconfig Osamu Tomita
2003-02-23  9:48 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (10/21) NIC Osamu Tomita
2003-02-23  9:50 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (11/21) parport Osamu Tomita
2003-02-23  9:51 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (12/21) PCI Osamu Tomita
2003-02-23  9:52 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (13/21) PCMCIA Osamu Tomita
2003-02-23  9:53 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (14/21) PNP Osamu Tomita
2003-02-23  9:54 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (15/21) RTC Osamu Tomita
2003-02-23  9:55 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (16/21) SCSI Osamu Tomita
2003-02-23 10:52   ` Christoph Hellwig [this message]
2003-02-23  9:56 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (17/21) serial Osamu Tomita
2003-02-23  9:57 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (18/21) setup Osamu Tomita
2003-02-23  9:58 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (19/21) SMP Osamu Tomita
2003-02-23  9:59 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (20/21) timer Osamu Tomita
2003-02-23 10:00 ` [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (21/21) traps Osamu Tomita
2003-02-24  7:52 [PATCH] PC-9800 subarch. support for 2.5.62-AC1 (16/21) SCSI Osamu Tomita
2003-02-24 13:59 ` Osamu Tomita
2003-03-06 10:44   ` Geert Uytterhoeven
2003-03-09  2:31     ` Osamu Tomita

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20030223105228.A16083@infradead.org \
    --to=hch@infradead.org \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tomita@cinet.co.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).