All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: Paul Mackerras <paulus@samba.org>,
	linuxppc-dev@ozlabs.org, Jens Axboe <axboe@kernel.dk>,
	"James E.J. Bottomley" <James.Bottomley@SteelEye.com>,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [patch 5/6] ps3: ROM Storage Driver
Date: Tue, 19 Jun 2007 07:51:58 +0200	[thread overview]
Message-ID: <20070619055158.GC18542@lst.de> (raw)
In-Reply-To: <20070615120848.454492000@pademelon.sonytel.be>


Looks pretty nice.

Some more comments:

 - no need for the BLK_DEV_SR dependency, it should depend on SCSI
   instead (which might be implied by beeing in the scsi menu, don't
   remember that detail)
 - ps3rom_priv should probably be an inline
 - the cmd field in struct ps3rom_private should probably be renamed
   to curr_cmd or something along the line to make sure this is the
   current command we deal with and the driver can only deal with one
   command at a time.
 - The comment in ps3rom_queuecommand should explain why we prefer
   lv1_storage_{read,write} over just sending down the scsi cdb,
   not just that we prefer it.
 - I don't think you need to decode the 6 byte commands at all,
   ATAPI only has the 10 byte variants and you can tell the scsi
   layer not to ever send the 6 byte ones by setting the use_10_for_rw
   flag in ->slave_configure
 - same comment about the free_irq/request_irq pair as in the disk
   driver.
 - please use the new shost_priv helper in scsi-misc for accessing
   the hostdata file in struct Scsi_Host so that the ugly casts
   can go away.

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	"James E.J. Bottomley" <James.Bottomley@SteelEye.com>,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@ozlabs.org, Paul Mackerras <paulus@samba.org>
Subject: Re: [patch 5/6] ps3: ROM Storage Driver
Date: Tue, 19 Jun 2007 07:51:58 +0200	[thread overview]
Message-ID: <20070619055158.GC18542@lst.de> (raw)
In-Reply-To: <20070615120848.454492000@pademelon.sonytel.be>


Looks pretty nice.

Some more comments:

 - no need for the BLK_DEV_SR dependency, it should depend on SCSI
   instead (which might be implied by beeing in the scsi menu, don't
   remember that detail)
 - ps3rom_priv should probably be an inline
 - the cmd field in struct ps3rom_private should probably be renamed
   to curr_cmd or something along the line to make sure this is the
   current command we deal with and the driver can only deal with one
   command at a time.
 - The comment in ps3rom_queuecommand should explain why we prefer
   lv1_storage_{read,write} over just sending down the scsi cdb,
   not just that we prefer it.
 - I don't think you need to decode the 6 byte commands at all,
   ATAPI only has the 10 byte variants and you can tell the scsi
   layer not to ever send the 6 byte ones by setting the use_10_for_rw
   flag in ->slave_configure
 - same comment about the free_irq/request_irq pair as in the disk
   driver.
 - please use the new shost_priv helper in scsi-misc for accessing
   the hostdata file in struct Scsi_Host so that the ugly casts
   can go away.

  parent reply	other threads:[~2007-06-19  5:52 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-15 11:39 [patch 0/6] PS3 Storage Drivers for 2.6.23, take 2 Geert Uytterhoeven
2007-06-15 11:39 ` Geert Uytterhoeven
2007-06-15 11:39 ` Geert Uytterhoeven
2007-06-15 11:39 ` [patch 1/6] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 13:27   ` Benjamin Herrenschmidt
2007-06-15 13:27     ` Benjamin Herrenschmidt
2007-06-15 13:32     ` Geert Uytterhoeven
2007-06-15 13:32       ` Geert Uytterhoeven
2007-06-18 16:25       ` [patch 1/6] ps3: Preallocate bootmem memory for the PS3 FLASH Milton Miller
2007-06-19 11:36         ` Geert Uytterhoeven
2007-06-15 11:39 ` [patch 2/6] ps3: Storage Driver Core Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39 ` [patch 3/6] PS3: Storage device registration routines Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39 ` [patch 4/6] ps3: Disk Storage Driver Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 14:35   ` David Woodhouse
2007-06-15 14:35     ` David Woodhouse
2007-06-15 14:41     ` Arnd Bergmann
2007-06-15 14:41       ` Arnd Bergmann
2007-06-15 14:41       ` Arnd Bergmann
2007-06-15 14:43     ` Geert Uytterhoeven
2007-06-15 16:15       ` Alan Cox
2007-06-15 16:15         ` Alan Cox
2007-06-15 18:05         ` Geert Uytterhoeven
2007-06-15 21:19         ` David Miller
2007-06-15 21:19           ` David Miller
2007-06-15 22:40           ` James Bottomley
2007-06-15 22:40             ` James Bottomley
2007-06-15 23:08             ` David Miller
2007-06-15 23:08               ` David Miller
2007-06-15 23:28               ` James Bottomley
2007-06-15 23:28                 ` James Bottomley
2007-06-15 23:37                 ` David Miller
2007-06-15 23:37                   ` David Miller
2007-06-19  5:56               ` Christoph Hellwig
2007-06-19  6:07                 ` David Miller
2007-06-19  8:15                   ` Christoph Hellwig
2007-06-19  8:15                     ` Christoph Hellwig
2007-06-15 23:11           ` Alan Cox
2007-06-15 23:11             ` Alan Cox
2007-06-16  6:21           ` Geert Uytterhoeven
2007-06-16  6:21             ` Geert Uytterhoeven
2007-06-19  5:53           ` Christoph Hellwig
2007-06-19  5:53             ` Christoph Hellwig
2007-06-19  6:03             ` David Miller
2007-06-19  6:03               ` David Miller
2007-06-19  7:06               ` Geert Uytterhoeven
2007-06-19  7:06                 ` Geert Uytterhoeven
2007-06-15 21:17       ` David Miller
2007-06-15 21:17         ` David Miller
2007-06-15 21:28         ` Jeff Garzik
2007-06-15 21:28           ` Jeff Garzik
2007-06-15 21:37           ` David Miller
2007-06-15 21:37             ` David Miller
2007-06-15 21:50         ` Jeff Garzik
2007-06-15 21:50           ` Jeff Garzik
2007-06-15 21:55           ` David Miller
2007-06-15 21:55             ` David Miller
2007-06-19  5:43   ` Christoph Hellwig
2007-06-19  5:43     ` Christoph Hellwig
2007-06-19 12:51     ` Geert Uytterhoeven
2007-06-19 12:51       ` Geert Uytterhoeven
2007-06-19 17:19       ` Christoph Hellwig
2007-06-19 17:19         ` Christoph Hellwig
2007-06-15 11:39 ` [patch 5/6] ps3: ROM " Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-18 16:30   ` Milton Miller
2007-06-19  5:44     ` Christoph Hellwig
2007-06-19 12:29     ` Geert Uytterhoeven
2007-06-20 12:16       ` Milton Miller
2007-06-19  5:51   ` Christoph Hellwig [this message]
2007-06-19  5:51     ` Christoph Hellwig
2007-06-15 11:39 ` [patch 6/6] ps3: FLASH " Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-15 11:39   ` Geert Uytterhoeven
2007-06-18 16:30   ` Milton Miller
2007-06-19 12:02     ` Geert Uytterhoeven

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=20070619055158.GC18542@lst.de \
    --to=hch@lst.de \
    --cc=Geert.Uytterhoeven@sonycom.com \
    --cc=James.Bottomley@SteelEye.com \
    --cc=axboe@kernel.dk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=paulus@samba.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.