Messages in this thread | | | Subject | Re: [PATCH] sym53c500_cs PCMCIA SCSI driver (new) | Date | Fri, 16 Apr 2004 09:17:20 -0500 (CDT) | From | (Bob Tracy) |
| |
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...
-- ----------------------------------------------------------------------- Bob Tracy WTO + WIPO = DMCA? http://www.anti-dmca.org rct@frus.com ----------------------------------------------------------------------- - 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/
| |