Matthew Wilcox wrote:
On Thu, Jul 26, 2007 at 03:38:56PM -0400, Jeff Garzik wrote:
+ ioport = edev->base_addr + 0xc30;
+
+ for (i = 0; i < 2; i++, ioport += 0x20) {
+ if (!AscFindSignature(ioport))
+ continue;
+ inw(ioport + 4);
4) would be nice to have a comment noting what this inw() does.
Yeah, it would. I just copied it from the previous code:
while (iop_base <= ASC_EISA_MAX_IOP_ADDR) {
eisa_product_id = AscGetEisaProductID(iop_base);
if ((eisa_product_id == ASC_EISA_ID_740) ||
(eisa_product_id == ASC_EISA_ID_750)) {
if (AscFindSignature(iop_base)) {
inpw(iop_base + 4);
return (iop_base);
}
}
Anyone got a manual and can tell me why we do this? ;-)
Would it make sense to put in a comment that says "No idea why we do this"?
IMO, yes. It should be noticeable at the very least.
5) I would suggest putting a "remove inpw/outpw pointless wrappers"
cleanup patch before patches #2 .. #N.
Next thing on my todo list is to convert the driver over to ioread8 and
friends. Currently, only the wide boards get accessed through PCIMEM,
but the narrow boards also have PCIMEM BARs. It would also allow the wide boards to be used on machines where PCIMEM is broken, like the VisWS.
PCIMEM versus PCIMEM... huh?
+static __devexit int advansys_eisa_remove(struct device *dev)
+{
+ int i, ioport;
+ struct eisa_scsi_data *data = dev_get_drvdata(dev);
+
+ for (i = 0; i < 2; i++) {
+ struct Scsi_Host *shost = data->host[i];
+ if (!shost)
+ continue;
+ ioport = shost->io_port;
+ advansys_remove(data->host[i]);
+ }
+
+ return 0;
6) set drvdata to NULL
Any reason why?
See other email. Safe object lifetime practice, plus it ensures a nice
oops if there is a bug.
Setting drvdata to NULL in ->remove() is standard for Linux drivers.
grep around.
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
