Re: [PATCH] sym53c500_cs PCMCIA SCSI driver (new)

[Bill Davidsen]

Bob Tracy wrote:
> Christoph Hellwig wrote:
>
> > I've given it a short spin and here's a bunch of comments:
>
>
> Thank you for taking the time and trouble to review it.
>
>
> > - the split into three source files is supserflous, one file should do it
>
>
> Given that the driver currently supports only PCMCIA implementations,
> I agree.  My thinking was if someone comes up with a host adapter that
> isn't PCMCIA, the SYM53C500.c file is to the sym53c500_cs driver what
> the qlogicfas.c file is to the qlogic_cs driver, that is, core functions
> that could support multiple types of host adapters.  The logic to
> handle the different types of adapters isn't there, and I don't know
> that it ever will be (else, it's probable that someone would have
> written the Linux driver long before now).  However, after baring my
> ignorance to the world and saying I was unaware of non-PCMCIA
> implementations, I found a FreeBSD driver for the NCR 53c500.  Never
> say "never," I guess...  Your opinion counts for much, but you're the
> only person I've heard from.  Is there a consensus I should forget
> about the non-PCMCIA cases?
>
>
> > - please don't use host.h or scsi.h from drivers/scsi/.  The defintions
> >   not present in include/scsi/ are deprecated and shall not be used (the
> >   most prominent example in your driver are the Scsi_<Foo> typedefs that
> >   have been replaced by struct scsi_foo
>
>
> I caught that in the coding style guidelines (and in the mentioned
> include files), and will fix for the next submission.
>
>
> > - the driver doesn't even try to deal with multiple HBAs
>
>
> Guilty as charged.  Functionally, there's nothing in the driver I
> submitted that wasn't in the original.  Suggestions welcome...  Which
> of the existing PCMCIA SCSI drivers do a proper job of handling
> multiple host adapters in your opinion?  I'll try to adapt that code to
> fit this driver.  If I have to "roll my own" from scratch, I'm probably
> in over my head.
>
>
> > - your detection logic could be streamlined a little, e.g. the request/release
> >   resource mess
>
>
> I'll see what I can do.
>
> Although I touched on it above, by way of apology/explanation, the goal
> for the initial port was to replicate the functionality I already had in
> older kernel versions.  It appears I faithfully replicated the
> deficiencies of the old driver as well :-).  Again, thank you for the
> feedback.
>
> Anyone else have input before I act on the recommendations I've been
> given?  Unless I hear otherwise, I'll start work on the code
> consolidation and removal of dependencies on deprecated include files.
> The detection logic and handling multiple HBAs will take a bit more
> effort...
WRT the split of code... if there is some reason why there will never be 
another type of card then the split is unnessessary. But otherwise, 
you've done the work, and it matches the way other drivers were split, 
so why scrap it?

As you guessed I don't feel strongly one way or the other, just thought 
you could use a little support for having made the effort to design for 
the future.

--
   -bill davidsen (davidsen@xxxxxxx)
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/