[PATCH] sound/oss/pss: set_io_base() always returns success, mark it void

[PATCH] sound/oss/pss: set_io_base() always returns success, mark it void

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 sound/oss/pss.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/sound/oss/pss.c b/sound/oss/pss.c
index ece428b..e970f75 100644
--- a/sound/oss/pss.c
+++ b/sound/oss/pss.c
@@ -232,14 +232,12 @@ static int set_irq(pss_confdata * devc, int dev, int irq)
 	return 1;
 }
 
-static int set_io_base(pss_confdata * devc, int dev, int base)
+static void set_io_base(pss_confdata * devc, int dev, int base)
 {
 	unsigned short  tmp = inw(REG(dev)) & 0x003f;
 	unsigned short  bits = (base & 0x0ffc) << 4;
 
 	outw(bits | tmp, REG(dev));
-
-	return 1;
 }
 
 static int set_dma(pss_confdata * devc, int dev, int dma)
@@ -677,16 +675,11 @@ static void configure_nonsound_components(void)
 	{
 		printk(KERN_INFO "PSS: CDROM port not enabled.\n");
 	}
-	else if(check_region(pss_cdrom_port, 2))
-	{
+	else if(check_region(pss_cdrom_port, 2)) {
 		printk(KERN_ERR "PSS: CDROM I/O port conflict.\n");
 	}
-	else if(!set_io_base(devc, CONF_CDROM, pss_cdrom_port))
-	{
-		printk(KERN_ERR "PSS: CDROM I/O port could not be set.\n");
-	}
-	else					/* CDROM port successfully configured */
-	{
+	else {
+		set_io_base(devc, CONF_CDROM, pss_cdrom_port);
 		printk(KERN_INFO "PSS: CDROM I/O port set to 0x%x.\n", pss_cdrom_port);
 	}
 }
@@ -758,10 +751,7 @@ static int __init probe_pss_mpu(struct address_info *hw_config)
 		printk(KERN_ERR "PSS: MPU I/O port conflict\n");
 		return 0;
 	}
-	if (!set_io_base(devc, CONF_MIDI, hw_config->io_base)) {
-		printk(KERN_ERR "PSS: MIDI base could not be set.\n");
-		goto fail;
-	}
+	set_io_base(devc, CONF_MIDI, hw_config->io_base);
 	if (!set_irq(devc, CONF_MIDI, hw_config->irq)) {
 		printk(KERN_ERR "PSS: MIDI IRQ allocation error.\n");
 		goto fail;
@@ -1057,10 +1047,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
 		release_region(hw_config->io_base, 4);
 		return 0;
 	}
-	if (!set_io_base(devc, CONF_WSS, hw_config->io_base)) {
-		printk("PSS: WSS base not settable.\n");
-		goto fail;
-	}
+	set_io_base(devc, CONF_WSS, hw_config->io_base);
 	if (!set_irq(devc, CONF_WSS, hw_config->irq)) {
 		printk("PSS: WSS IRQ allocation error.\n");
 		goto fail;
-- 
1.5.2.4

[PATCH 1/3] [IDE] Add helper __ide_setup_pci_device()

[Jeff Garzik]

Like ide_setup_pci_device(), except with additional array argument
'u8 *idx' that permits low-level driver to become aware of its assigned
hwifs.

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ide/setup-pci.c |   17 ++++++++++++-----
 include/linux/ide.h     |    1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
index 02d14bf..4960b9f 100644
--- a/drivers/ide/setup-pci.c
+++ b/drivers/ide/setup-pci.c
@@ -666,12 +666,10 @@ out:
 	return ret;
 }
 
-int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
+int __ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d,
+			   u8 *idx)
 {
-	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
-	int ret;
-
-	ret = do_ide_setup_pci_device(dev, d, &idx[0], 1);
+	int ret = do_ide_setup_pci_device(dev, d, idx, 1);
 
 	if (ret >= 0)
 		ide_device_add(idx);
@@ -679,6 +677,15 @@ int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
 	return ret;
 }
 
+EXPORT_SYMBOL_GPL(__ide_setup_pci_device);
+
+int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
+{
+	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
+
+	return __ide_setup_pci_device(dev, d, &idx[0]);
+}
+
 EXPORT_SYMBOL_GPL(ide_setup_pci_device);
 
 int ide_setup_pci_devices(struct pci_dev *dev1, struct pci_dev *dev2,
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 4ed4777..3404fb9 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1244,6 +1244,7 @@ struct ide_port_info {
 	u8			udma_mask;
 };
 
+int __ide_setup_pci_device(struct pci_dev *, const struct ide_port_info *, u8 *);
 int ide_setup_pci_device(struct pci_dev *, const struct ide_port_info *);
 int ide_setup_pci_devices(struct pci_dev *, struct pci_dev *, const struct ide_port_info *);
 
-- 
1.5.2.4

[PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop

[Jeff Garzik]

Store our hwif indices at probe time, in order to eliminate a needless
and ugly loop across all hwifs, searching for our pci device.

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ide/pci/sc1200.c |   76 +++++++++++++++++++++++++++++++---------------
 1 files changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c
index d2c8b55..17e58d6 100644
--- a/drivers/ide/pci/sc1200.c
+++ b/drivers/ide/pci/sc1200.c
@@ -41,6 +41,8 @@
 #define PCI_CLK_66	0x02
 #define PCI_CLK_33A	0x03
 
+#define SC1200_IFS	4
+
 static unsigned short sc1200_get_pci_clock (void)
 {
 	unsigned char chip_id, silicon_revision;
@@ -261,31 +263,32 @@ static void sc1200_set_pio_mode(ide_drive_t *drive, const u8 pio)
 }
 
 #ifdef CONFIG_PM
-static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev)
-{
-	int	h;
-
-	for (h = 0; h < MAX_HWIFS; h++) {
-		ide_hwif_t *hwif = &ide_hwifs[h];
-		if (prev) {
-			if (hwif == prev)
-				prev = NULL;	// found previous, now look for next match
-		} else {
-			if (hwif && hwif->pci_dev == dev)
-				return hwif;	// found next match
-		}
-	}
-	return NULL;	// not found
-}
-
 typedef struct sc1200_saved_state_s {
 	__u32		regs[4];
 } sc1200_saved_state_t;
 
+static unsigned int pack_hwif_idx(u8 *idx)
+{
+	return	(((unsigned int) idx[0]) << 0) |
+		(((unsigned int) idx[1]) << 8) |
+		(((unsigned int) idx[2]) << 16) |
+		(((unsigned int) idx[3]) << 24);
+}
+
+static ide_hwif_t *sc1200_hwif(struct pci_dev *pdev, unsigned int iface)
+{
+	unsigned int packed_hwifs, idx;
+
+	packed_hwifs = (unsigned long) pci_get_drvdata(pdev);
+	idx = (packed_hwifs >> (iface * 8)) & 0xff;
+
+	return (idx == 0xff) ? NULL : &ide_hwifs[idx];
+}
 
 static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 {
-	ide_hwif_t		*hwif = NULL;
+	ide_hwif_t		*hwif;
+	int			i;
 
 	printk("SC1200: suspend(%u)\n", state.event);
 
@@ -295,9 +298,14 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 		//
 		// Loop over all interfaces that are part of this PCI device:
 		//
-		while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
+		for (i = 0; i < SC1200_IFS; i++) {
 			sc1200_saved_state_t	*ss;
 			unsigned int		basereg, r;
+
+			hwif = sc1200_hwif(dev, i);
+			if (!hwif)
+				continue;
+
 			//
 			// allocate a permanent save area, if not already allocated
 			//
@@ -310,7 +318,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 			}
 			ss = (sc1200_saved_state_t *)hwif->config_data;
 			//
-			// Save timing registers:  this may be unnecessary if 
+			// Save timing registers:  this may be unnecessary if
 			// BIOS also does it
 			//
 			basereg = hwif->channel ? 0x50 : 0x40;
@@ -320,7 +328,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 		}
 	}
 
-	/* You don't need to iterate over disks -- sysfs should have done that for you already */ 
+	/* You don't need to iterate over disks -- sysfs should have done that for you already */
 
 	pci_disable_device(dev);
 	pci_set_power_state(dev, pci_choose_state(dev, state));
@@ -330,7 +338,8 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 
 static int sc1200_resume (struct pci_dev *dev)
 {
-	ide_hwif_t	*hwif = NULL;
+	ide_hwif_t		*hwif;
+	int			i;
 
 	pci_set_power_state(dev, PCI_D0);	// bring chip back from sleep state
 	dev->current_state = PM_EVENT_ON;
@@ -338,9 +347,15 @@ static int sc1200_resume (struct pci_dev *dev)
 	//
 	// loop over all interfaces that are part of this pci device:
 	//
-	while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
+	for (i = 0; i < SC1200_IFS; i++) {
 		unsigned int		basereg, r;
-		sc1200_saved_state_t	*ss = (sc1200_saved_state_t *)hwif->config_data;
+		sc1200_saved_state_t	*ss;
+
+		hwif = sc1200_hwif(dev, i);
+		if (!hwif)
+			continue;
+
+		ss = (sc1200_saved_state_t *)hwif->config_data;
 
 		//
 		// Restore timing registers:  this may be unnecessary if BIOS also does it
@@ -386,7 +401,18 @@ static const struct ide_port_info sc1200_chipset __devinitdata = {
 
 static int __devinit sc1200_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	return ide_setup_pci_device(dev, &sc1200_chipset);
+	unsigned int packed_hwifs;
+	int rc;
+	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
+
+	rc = __ide_setup_pci_device(dev, &sc1200_chipset, &idx[0]);
+	if (rc)
+		return rc;
+	
+	packed_hwifs = pack_hwif_idx(&idx[0]);
+
+	pci_set_drvdata(dev, (void *)(unsigned long) packed_hwifs);
+	return 0;
 }
 
 static const struct pci_device_id sc1200_pci_tbl[] = {
-- 
1.5.2.4

[PATCH 3/3] drivers/ide/pci/sc1200.c: fix suspend/resume buglets and warnings

[Jeff Garzik]

* We shouldn't bother with dev->current_state, the PCI API functions we
  call manage this for us (and do a far better job at it too).

* Remove pci_set_power_state(dev, PCI_D0) call in resume, as
  pci_enable_device() does the same thing.

* Check pci_enable_device() return value.  If it failed, fail
  the entire resume and avoid programming timings into the [potentially
  dead/asleep] chip.

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ide/pci/sc1200.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c
index 17e58d6..10c79a5 100644
--- a/drivers/ide/pci/sc1200.c
+++ b/drivers/ide/pci/sc1200.c
@@ -332,7 +332,6 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 
 	pci_disable_device(dev);
 	pci_set_power_state(dev, pci_choose_state(dev, state));
-	dev->current_state = state.event;
 	return 0;
 }
 
@@ -341,9 +340,10 @@ static int sc1200_resume (struct pci_dev *dev)
 	ide_hwif_t		*hwif;
 	int			i;
 
-	pci_set_power_state(dev, PCI_D0);	// bring chip back from sleep state
-	dev->current_state = PM_EVENT_ON;
-	pci_enable_device(dev);
+	i = pci_enable_device(dev);
+	if (i)
+		return i;
+
 	//
 	// loop over all interfaces that are part of this pci device:
 	//
-- 
1.5.2.4

[PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
 drivers/scsi/ips.h |   20 +++----
 2 files changed, 91 insertions(+), 107 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 5c5a9b2..595a91a 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -707,7 +707,7 @@ ips_release(struct Scsi_Host *sh)
 		release_region(ha->io_addr, ha->io_len);
 
 	/* free IRQ */
-	free_irq(ha->irq, ha);
+	free_irq(ha->pcidev->irq, ha);
 
 	scsi_host_put(sh);
 
@@ -1637,7 +1637,7 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, ips_scb_t *scb, int intr)
 				return (IPS_FAILURE);
 			}
 
-			if (ha->device_id == IPS_DEVICEID_COPPERHEAD &&
+			if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD &&
 			    pt->CoppCP.cmd.flashfw.op_code ==
 			    IPS_CMD_RW_BIOSFW) {
 				ret = ips_flash_copperhead(ha, pt, scb);
@@ -2021,7 +2021,7 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_scb_t * scb)
 	pt->ExtendedStatus = scb->extended_status;
 	pt->AdapterType = ha->ad_type;
 
-	if (ha->device_id == IPS_DEVICEID_COPPERHEAD &&
+	if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD &&
 	    (scb->cmd.flashfw.op_code == IPS_CMD_DOWNLOAD ||
 	     scb->cmd.flashfw.op_code == IPS_CMD_RW_BIOSFW))
 		ips_free_flash_copperhead(ha);
@@ -2075,7 +2075,7 @@ ips_host_info(ips_ha_t * ha, char *ptr, off_t offset, int len)
 			  ha->mem_ptr);
 	}
 
-	copy_info(&info, "\tIRQ number                        : %d\n", ha->irq);
+	copy_info(&info, "\tIRQ number                        : %d\n", ha->pcidev->irq);
 
     /* For the Next 3 lines Check for Binary 0 at the end and don't include it if it's there. */
     /* That keeps everything happy for "text" operations on the proc file.                    */
@@ -2232,31 +2232,31 @@ ips_identify_controller(ips_ha_t * ha)
 {
 	METHOD_TRACE("ips_identify_controller", 1);
 
-	switch (ha->device_id) {
+	switch (ha->pcidev->device) {
 	case IPS_DEVICEID_COPPERHEAD:
-		if (ha->revision_id <= IPS_REVID_SERVERAID) {
+		if (ha->pcidev->revision <= IPS_REVID_SERVERAID) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID;
-		} else if (ha->revision_id == IPS_REVID_SERVERAID2) {
+		} else if (ha->pcidev->revision == IPS_REVID_SERVERAID2) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID2;
-		} else if (ha->revision_id == IPS_REVID_NAVAJO) {
+		} else if (ha->pcidev->revision == IPS_REVID_NAVAJO) {
 			ha->ad_type = IPS_ADTYPE_NAVAJO;
-		} else if ((ha->revision_id == IPS_REVID_SERVERAID2)
+		} else if ((ha->pcidev->revision == IPS_REVID_SERVERAID2)
 			   && (ha->slot_num == 0)) {
 			ha->ad_type = IPS_ADTYPE_KIOWA;
-		} else if ((ha->revision_id >= IPS_REVID_CLARINETP1) &&
-			   (ha->revision_id <= IPS_REVID_CLARINETP3)) {
+		} else if ((ha->pcidev->revision >= IPS_REVID_CLARINETP1) &&
+			   (ha->pcidev->revision <= IPS_REVID_CLARINETP3)) {
 			if (ha->enq->ucMaxPhysicalDevices == 15)
 				ha->ad_type = IPS_ADTYPE_SERVERAID3L;
 			else
 				ha->ad_type = IPS_ADTYPE_SERVERAID3;
-		} else if ((ha->revision_id >= IPS_REVID_TROMBONE32) &&
-			   (ha->revision_id <= IPS_REVID_TROMBONE64)) {
+		} else if ((ha->pcidev->revision >= IPS_REVID_TROMBONE32) &&
+			   (ha->pcidev->revision <= IPS_REVID_TROMBONE64)) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID4H;
 		}
 		break;
 
 	case IPS_DEVICEID_MORPHEUS:
-		switch (ha->subdevice_id) {
+		switch (ha->pcidev->subsystem_device) {
 		case IPS_SUBDEVICEID_4L:
 			ha->ad_type = IPS_ADTYPE_SERVERAID4L;
 			break;
@@ -2285,7 +2285,7 @@ ips_identify_controller(ips_ha_t * ha)
 		break;
 
 	case IPS_DEVICEID_MARCO:
-		switch (ha->subdevice_id) {
+		switch (ha->pcidev->subsystem_device) {
 		case IPS_SUBDEVICEID_6M:
 			ha->ad_type = IPS_ADTYPE_SERVERAID6M;
 			break;
@@ -2332,20 +2332,20 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 	strncpy(ha->bios_version, "       ?", 8);
 
-	if (ha->device_id == IPS_DEVICEID_COPPERHEAD) {
+	if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) {
 		if (IPS_USE_MEMIO(ha)) {
 			/* Memory Mapped I/O */
 
 			/* test 1st byte */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0x55)
 				return;
 
 			writel(1, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0xAA)
@@ -2353,20 +2353,20 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* Get Major version */
 			writel(0x1FF, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			major = readb(ha->mem_ptr + IPS_REG_FLDP);
 
 			/* Get Minor version */
 			writel(0x1FE, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 			minor = readb(ha->mem_ptr + IPS_REG_FLDP);
 
 			/* Get SubMinor version */
 			writel(0x1FD, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 			subminor = readb(ha->mem_ptr + IPS_REG_FLDP);
 
@@ -2375,14 +2375,14 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* test 1st byte */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (inb(ha->io_addr + IPS_REG_FLDP) != 0x55)
 				return;
 
 			outl(cpu_to_le32(1), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (inb(ha->io_addr + IPS_REG_FLDP) != 0xAA)
@@ -2390,21 +2390,21 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* Get Major version */
 			outl(cpu_to_le32(0x1FF), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			major = inb(ha->io_addr + IPS_REG_FLDP);
 
 			/* Get Minor version */
 			outl(cpu_to_le32(0x1FE), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			minor = inb(ha->io_addr + IPS_REG_FLDP);
 
 			/* Get SubMinor version */
 			outl(cpu_to_le32(0x1FD), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			subminor = inb(ha->io_addr + IPS_REG_FLDP);
@@ -4903,7 +4903,7 @@ ips_init_copperhead(ips_ha_t * ha)
 	/* Enable busmastering */
 	outb(IPS_BIT_EBM, ha->io_addr + IPS_REG_SCPR);
 
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		/* fix for anaconda64 */
 		outl(0, ha->io_addr + IPS_REG_NDAE);
 
@@ -4997,7 +4997,7 @@ ips_init_copperhead_memio(ips_ha_t * ha)
 	/* Enable busmastering */
 	writeb(IPS_BIT_EBM, ha->mem_ptr + IPS_REG_SCPR);
 
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		/* fix for anaconda64 */
 		writel(0, ha->mem_ptr + IPS_REG_NDAE);
 
@@ -5142,7 +5142,7 @@ ips_reset_copperhead(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_copperhead", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_copperhead: io addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->io_addr, ha->irq);
+		  ips_name, ha->host_num, ha->io_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -5187,7 +5187,7 @@ ips_reset_copperhead_memio(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_copperhead_memio", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_copperhead_memio: mem addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->mem_addr, ha->irq);
+		  ips_name, ha->host_num, ha->mem_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -5233,7 +5233,7 @@ ips_reset_morpheus(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_morpheus", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_morpheus: mem addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->mem_addr, ha->irq);
+		  ips_name, ha->host_num, ha->mem_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -6196,32 +6196,32 @@ ips_erase_bios(ips_ha_t * ha)
 
 	/* Clear the status register */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	outb(0x50, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Setup */
 	outb(0x20, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Confirm */
 	outb(0xD0, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Status */
 	outb(0x70, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	timeout = 80000;	/* 80 seconds */
 
 	while (timeout > 0) {
-		if (ha->revision_id == IPS_REVID_TROMBONE64) {
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 			outl(0, ha->io_addr + IPS_REG_FLAP);
 			udelay(25);	/* 25 us */
 		}
@@ -6241,13 +6241,13 @@ ips_erase_bios(ips_ha_t * ha)
 
 		/* try to suspend the erase */
 		outb(0xB0, ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait for 10 seconds */
 		timeout = 10000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				outl(0, ha->io_addr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6277,12 +6277,12 @@ ips_erase_bios(ips_ha_t * ha)
 	/* Otherwise, we were successful */
 	/* clear status */
 	outb(0x50, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* enable reads */
 	outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6308,32 +6308,32 @@ ips_erase_bios_memio(ips_ha_t * ha)
 
 	/* Clear the status register */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	writeb(0x50, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Setup */
 	writeb(0x20, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Confirm */
 	writeb(0xD0, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Status */
 	writeb(0x70, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	timeout = 80000;	/* 80 seconds */
 
 	while (timeout > 0) {
-		if (ha->revision_id == IPS_REVID_TROMBONE64) {
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
 			udelay(25);	/* 25 us */
 		}
@@ -6353,13 +6353,13 @@ ips_erase_bios_memio(ips_ha_t * ha)
 
 		/* try to suspend the erase */
 		writeb(0xB0, ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait for 10 seconds */
 		timeout = 10000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				writel(0, ha->mem_ptr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6389,12 +6389,12 @@ ips_erase_bios_memio(ips_ha_t * ha)
 	/* Otherwise, we were successful */
 	/* clear status */
 	writeb(0x50, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* enable reads */
 	writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6423,21 +6423,21 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 0; i < buffersize; i++) {
 		/* write a byte */
 		outl(cpu_to_le32(i + offset), ha->io_addr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		outb(0x40, ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		outb(buffer[i], ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait up to one second */
 		timeout = 1000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				outl(0, ha->io_addr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6454,11 +6454,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (timeout == 0) {
 			/* timeout error */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6468,11 +6468,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (status & 0x18) {
 			/* programming error */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6481,11 +6481,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* Enable reading */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6514,21 +6514,21 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 0; i < buffersize; i++) {
 		/* write a byte */
 		writel(i + offset, ha->mem_ptr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		writeb(0x40, ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		writeb(buffer[i], ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait up to one second */
 		timeout = 1000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				writel(0, ha->mem_ptr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6545,11 +6545,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (timeout == 0) {
 			/* timeout error */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6559,11 +6559,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (status & 0x18) {
 			/* programming error */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6572,11 +6572,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* Enable reading */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6601,14 +6601,14 @@ ips_verify_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* test 1st byte */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	if (inb(ha->io_addr + IPS_REG_FLDP) != 0x55)
 		return (1);
 
 	outl(cpu_to_le32(1), ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 	if (inb(ha->io_addr + IPS_REG_FLDP) != 0xAA)
 		return (1);
@@ -6617,7 +6617,7 @@ ips_verify_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 2; i < buffersize; i++) {
 
 		outl(cpu_to_le32(i + offset), ha->io_addr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		checksum = (uint8_t) checksum + inb(ha->io_addr + IPS_REG_FLDP);
@@ -6650,14 +6650,14 @@ ips_verify_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* test 1st byte */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0x55)
 		return (1);
 
 	writel(1, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 	if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0xAA)
 		return (1);
@@ -6666,7 +6666,7 @@ ips_verify_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 2; i < buffersize; i++) {
 
 		writel(i + offset, ha->mem_ptr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		checksum =
@@ -6837,9 +6837,9 @@ ips_register_scsi(int index)
 	}
 	ha = IPS_HA(sh);
 	memcpy(ha, oldha, sizeof (ips_ha_t));
-	free_irq(oldha->irq, oldha);
+	free_irq(oldha->pcidev->irq, oldha);
 	/* Install the interrupt handler with the new ha */
-	if (request_irq(ha->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
+	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
 		scsi_host_put(sh);
@@ -6851,10 +6851,7 @@ ips_register_scsi(int index)
 	ips_ha[index] = ha;
 
 	/* Store away needed values for later use */
-	sh->io_port = ha->io_addr;
-	sh->n_io_port = ha->io_addr ? 255 : 0;
 	sh->unique_id = (ha->io_addr) ? ha->io_addr : ha->mem_addr;
-	sh->irq = ha->irq;
 	sh->sg_tablesize = sh->hostt->sg_tablesize;
 	sh->can_queue = sh->hostt->can_queue;
 	sh->cmd_per_lun = sh->hostt->cmd_per_lun;
@@ -6992,8 +6989,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	uint32_t mem_len;
 	uint8_t bus;
 	uint8_t func;
-	uint8_t irq;
-	uint16_t subdevice_id;
 	int j;
 	int index;
 	dma_addr_t dma_address;
@@ -7014,7 +7009,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		return -1;
 
 	/* stuff that we get in dev */
-	irq = pci_dev->irq;
 	bus = pci_dev->bus->number;
 	func = pci_dev->devfn;
 
@@ -7068,8 +7062,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		}
 	}
 
-	subdevice_id = pci_dev->subsystem_device;
-
 	/* found a controller */
 	ha = kzalloc(sizeof (ips_ha_t), GFP_KERNEL);
 	if (ha == NULL) {
@@ -7084,7 +7076,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	ha->active = 1;
 
 	/* Store info in HA structure */
-	ha->irq = irq;
 	ha->io_addr = io_addr;
 	ha->io_len = io_len;
 	ha->mem_addr = mem_addr;
@@ -7092,10 +7083,7 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	ha->mem_ptr = mem_ptr;
 	ha->ioremap_ptr = ioremap_ptr;
 	ha->host_num = (uint32_t) index;
-	ha->revision_id = pci_dev->revision;
 	ha->slot_num = PCI_SLOT(pci_dev->devfn);
-	ha->device_id = pci_dev->device;
-	ha->subdevice_id = subdevice_id;
 	ha->pcidev = pci_dev;
 
 	/*
@@ -7240,7 +7228,7 @@ ips_init_phase2(int index)
 	}
 
 	/* Install the interrupt handler */
-	if (request_irq(ha->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
+	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
 		return ips_abort_init(ha, index);
@@ -7253,14 +7241,14 @@ ips_init_phase2(int index)
 	if (!ips_allocatescbs(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to allocate a CCB\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 
 	if (!ips_hainit(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to initialize controller\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 	/* Free the temporary SCB */
@@ -7270,7 +7258,7 @@ ips_init_phase2(int index)
 	if (!ips_allocatescbs(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to allocate CCBs\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index 3bcbd9f..fb4a036 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -60,14 +60,14 @@
     */
    #define IPS_HA(x)                   ((ips_ha_t *) x->hostdata)
    #define IPS_COMMAND_ID(ha, scb)     (int) (scb - ha->scbs)
-   #define IPS_IS_TROMBONE(ha)         (((ha->device_id == IPS_DEVICEID_COPPERHEAD) && \
-                                         (ha->revision_id >= IPS_REVID_TROMBONE32) && \
-                                         (ha->revision_id <= IPS_REVID_TROMBONE64)) ? 1 : 0)
-   #define IPS_IS_CLARINET(ha)         (((ha->device_id == IPS_DEVICEID_COPPERHEAD) && \
-                                         (ha->revision_id >= IPS_REVID_CLARINETP1) && \
-                                         (ha->revision_id <= IPS_REVID_CLARINETP3)) ? 1 : 0)
-   #define IPS_IS_MORPHEUS(ha)         (ha->device_id == IPS_DEVICEID_MORPHEUS)
-   #define IPS_IS_MARCO(ha)            (ha->device_id == IPS_DEVICEID_MARCO)
+   #define IPS_IS_TROMBONE(ha)         (((ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) && \
+                                         (ha->pcidev->revision >= IPS_REVID_TROMBONE32) && \
+                                         (ha->pcidev->revision <= IPS_REVID_TROMBONE64)) ? 1 : 0)
+   #define IPS_IS_CLARINET(ha)         (((ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) && \
+                                         (ha->pcidev->revision >= IPS_REVID_CLARINETP1) && \
+                                         (ha->pcidev->revision <= IPS_REVID_CLARINETP3)) ? 1 : 0)
+   #define IPS_IS_MORPHEUS(ha)         (ha->pcidev->device == IPS_DEVICEID_MORPHEUS)
+   #define IPS_IS_MARCO(ha)            (ha->pcidev->device == IPS_DEVICEID_MARCO)
    #define IPS_USE_I2O_DELIVER(ha)     ((IPS_IS_MORPHEUS(ha) || \
                                          (IPS_IS_TROMBONE(ha) && \
                                           (ips_force_i2o))) ? 1 : 0)
@@ -1034,7 +1034,6 @@ typedef struct ips_ha {
    uint8_t            ha_id[IPS_MAX_CHANNELS+1];
    uint32_t           dcdb_active[IPS_MAX_CHANNELS];
    uint32_t           io_addr;            /* Base I/O address           */
-   uint8_t            irq;                /* IRQ for adapter            */
    uint8_t            ntargets;           /* Number of targets          */
    uint8_t            nbus;               /* Number of buses            */
    uint8_t            nlun;               /* Number of Luns             */
@@ -1066,10 +1065,7 @@ typedef struct ips_ha {
    int                ioctl_reset;        /* IOCTL Requested Reset Flag */
    uint16_t           reset_count;        /* number of resets           */
    time_t             last_ffdc;          /* last time we sent ffdc info*/
-   uint8_t            revision_id;        /* Revision level             */
-   uint16_t           device_id;          /* PCI device ID              */
    uint8_t            slot_num;           /* PCI Slot Number            */
-   uint16_t           subdevice_id;       /* Subsystem device ID        */
    int                ioctl_len;          /* size of ioctl buffer       */
    dma_addr_t         ioctl_busaddr;      /* dma address of ioctl buffer*/
    uint8_t            bios_version[8];    /* BIOS Revision              */
-- 
1.5.2.4

[PATCH 2/4] [SCSI] ips: trim trailing whitespace

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   44 ++++++++++++++++++++++----------------------
 drivers/scsi/ips.h |   12 ++++++------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 595a91a..c9f3e1f 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -389,17 +389,17 @@ static struct  pci_device_id  ips_pci_table[] = {
 MODULE_DEVICE_TABLE( pci, ips_pci_table );
 
 static char ips_hot_plug_name[] = "ips";
-   
+
 static int __devinit  ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent);
 static void __devexit ips_remove_device(struct pci_dev *pci_dev);
-   
+
 static struct pci_driver ips_pci_driver = {
 	.name		= ips_hot_plug_name,
 	.id_table	= ips_pci_table,
 	.probe		= ips_insert_device,
 	.remove		= __devexit_p(ips_remove_device),
 };
-           
+
 
 /*
  * Necessary forward function protoypes
@@ -587,7 +587,7 @@ static void
 ips_setup_funclist(ips_ha_t * ha)
 {
 
-	/*                                
+	/*
 	 * Setup Functions
 	 */
 	if (IPS_IS_MORPHEUS(ha) || IPS_IS_MARCO(ha)) {
@@ -2081,7 +2081,7 @@ ips_host_info(ips_ha_t * ha, char *ptr, off_t offset, int len)
     /* That keeps everything happy for "text" operations on the proc file.                    */
 
 	if (le32_to_cpu(ha->nvram->signature) == IPS_NVRAM_P5_SIG) {
-        if (ha->nvram->bios_low[3] == 0) { 
+        if (ha->nvram->bios_low[3] == 0) {
             copy_info(&info,
 			          "\tBIOS Version                      : %c%c%c%c%c%c%c\n",
 			          ha->nvram->bios_high[0], ha->nvram->bios_high[1],
@@ -2782,8 +2782,8 @@ ips_next(ips_ha_t * ha, int intr)
 
         /* Allow a WRITE BUFFER Command to Have no Data */
         /* This is Used by Tape Flash Utilites          */
-        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
-            scb->dcdb.cmd_attribute = 0;                  
+        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0))
+            scb->dcdb.cmd_attribute = 0;
 
 		if (!(scb->dcdb.cmd_attribute & 0x3))
 			scb->dcdb.transfer_length = 0;
@@ -3404,7 +3404,7 @@ ips_map_status(ips_ha_t * ha, ips_scb_t * scb, ips_stat_t * sp)
 
 				/* Restrict access to physical DASD */
 				if (scb->scsi_cmd->cmnd[0] == INQUIRY) {
-				    ips_scmd_buf_read(scb->scsi_cmd, 
+				    ips_scmd_buf_read(scb->scsi_cmd,
                                       &inquiryData, sizeof (inquiryData));
  				    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK) {
 				        errcode = DID_TIME_OUT;
@@ -4090,10 +4090,10 @@ ips_chkstatus(ips_ha_t * ha, IPS_STATUS * pstatus)
 			scb->scsi_cmd->result = errcode << 16;
 		} else {	/* bus == 0 */
 			/* restrict access to physical drives */
-			if (scb->scsi_cmd->cmnd[0] == INQUIRY) { 
-			    ips_scmd_buf_read(scb->scsi_cmd, 
+			if (scb->scsi_cmd->cmnd[0] == INQUIRY) {
+			    ips_scmd_buf_read(scb->scsi_cmd,
                                   &inquiryData, sizeof (inquiryData));
-			    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK) 
+			    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK)
 			        scb->scsi_cmd->result = DID_TIME_OUT << 16;
 			}
 		}		/* else */
@@ -4661,8 +4661,8 @@ ips_isinit_morpheus(ips_ha_t * ha)
 	uint32_t bits;
 
 	METHOD_TRACE("ips_is_init_morpheus", 1);
-   
-	if (ips_isintr_morpheus(ha)) 
+
+	if (ips_isintr_morpheus(ha))
 	    ips_flush_and_reset(ha);
 
 	post = readl(ha->mem_ptr + IPS_REG_I960_MSG0);
@@ -4686,7 +4686,7 @@ ips_isinit_morpheus(ips_ha_t * ha)
 /*   state ( was trying to INIT and an interrupt was already pending ) ...  */
 /*                                                                          */
 /****************************************************************************/
-static void 
+static void
 ips_flush_and_reset(ips_ha_t *ha)
 {
 	ips_scb_t *scb;
@@ -4718,9 +4718,9 @@ ips_flush_and_reset(ips_ha_t *ha)
 	    if (ret == IPS_SUCCESS) {
 	        time = 60 * IPS_ONE_SEC;	              /* Max Wait time is 60 seconds */
 	        done = 0;
-	            
+
 	        while ((time > 0) && (!done)) {
-	           done = ips_poll_for_flush_complete(ha); 	   
+	           done = ips_poll_for_flush_complete(ha);
 	           /* This may look evil, but it's only done during extremely rare start-up conditions ! */
 	           udelay(1000);
 	           time--;
@@ -4749,17 +4749,17 @@ static int
 ips_poll_for_flush_complete(ips_ha_t * ha)
 {
 	IPS_STATUS cstatus;
-    
+
 	while (TRUE) {
 	    cstatus.value = (*ha->func.statupd) (ha);
 
 	    if (cstatus.value == 0xffffffff)      /* If No Interrupt to process */
 			break;
-            
+
 	    /* Success is when we see the Flush Command ID */
-	    if (cstatus.fields.command_id == IPS_MAX_CMDS ) 
+	    if (cstatus.fields.command_id == IPS_MAX_CMDS )
 	        return 1;
-	 }	
+	 }
 
 	return 0;
 }
@@ -5920,7 +5920,7 @@ ips_read_config(ips_ha_t * ha, int intr)
 
 		return (0);
 	}
-	
+
 	memcpy(ha->conf, ha->ioctl_data, sizeof(*ha->conf));
 	return (1);
 }
@@ -5959,7 +5959,7 @@ ips_readwrite_page5(ips_ha_t * ha, int write, int intr)
 	scb->cmd.nvram.buffer_addr = ha->ioctl_busaddr;
 	if (write)
 		memcpy(ha->ioctl_data, ha->nvram, sizeof(*ha->nvram));
-	
+
 	/* issue the command */
 	if (((ret =
 	      ips_send_wait(ha, scb, ips_cmd_timeout, intr)) == IPS_FAILURE)
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index fb4a036..e0657b6 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -92,7 +92,7 @@
    #ifndef min
       #define min(x,y) ((x) < (y) ? x : y)
    #endif
-   
+
    #ifndef __iomem       /* For clean compiles in earlier kernels without __iomem annotations */
       #define __iomem
    #endif
@@ -171,7 +171,7 @@
    #define IPS_CMD_DOWNLOAD             0x20
    #define IPS_CMD_RW_BIOSFW            0x22
    #define IPS_CMD_GET_VERSION_INFO     0xC6
-   #define IPS_CMD_RESET_CHANNEL        0x1A  
+   #define IPS_CMD_RESET_CHANNEL        0x1A
 
    /*
     * Adapter Equates
@@ -458,7 +458,7 @@ typedef struct {
    uint32_t reserved3;
    uint32_t buffer_addr;
    uint32_t reserved4;
-} IPS_IOCTL_CMD, *PIPS_IOCTL_CMD; 
+} IPS_IOCTL_CMD, *PIPS_IOCTL_CMD;
 
 typedef struct {
    uint8_t  op_code;
@@ -552,7 +552,7 @@ typedef struct {
    uint32_t cccr;
 } IPS_NVRAM_CMD, *PIPS_NVRAM_CMD;
 
-typedef struct 
+typedef struct
 {
     uint8_t  op_code;
     uint8_t  command_id;
@@ -650,7 +650,7 @@ typedef struct {
    uint8_t   device_address;
    uint8_t   cmd_attribute;
    uint8_t   cdb_length;
-   uint8_t   reserved_for_LUN; 	 
+   uint8_t   reserved_for_LUN;
    uint32_t  transfer_length;
    uint32_t  buffer_pointer;
    uint16_t  sg_count;
@@ -790,7 +790,7 @@ typedef struct {
                                              /* SubSystem Parameter[4]      */
 #define  IPS_GET_VERSION_SUPPORT 0x00018000  /* Mask for Versioning Support */
 
-typedef struct 
+typedef struct
 {
    uint32_t  revision;
    uint8_t   bootBlkVersion[32];
-- 
1.5.2.4

[PATCH 3/4] [SCSI] ips: PCI API cleanups

[Jeff Garzik]

* pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
  allowing us to eliminate the ips_ha[] search loop and call
  ips_release() directly.

* call pci_{request,release}_regions() and eliminate individual
  request/release_[mem_]region() calls

* call pci_disable_device(), paired with pci_enable_device()

* s/0/NULL/ in a few places

* check ioremap() return value

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   72 ++++++++++++++++++++++-----------------------------
 1 files changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index c9f3e1f..fb90b6c 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -702,10 +702,6 @@ ips_release(struct Scsi_Host *sh)
 	/* free extra memory */
 	ips_free(ha);
 
-	/* Free I/O Region */
-	if (ha->io_addr)
-		release_region(ha->io_addr, ha->io_len);
-
 	/* free IRQ */
 	free_irq(ha->pcidev->irq, ha);
 
@@ -4393,8 +4389,6 @@ ips_free(ips_ha_t * ha)
 			ha->mem_ptr = NULL;
 		}
 
-		if (ha->mem_addr)
-			release_mem_region(ha->mem_addr, ha->mem_len);
 		ha->mem_addr = 0;
 
 	}
@@ -6879,20 +6873,14 @@ ips_register_scsi(int index)
 static void __devexit
 ips_remove_device(struct pci_dev *pci_dev)
 {
-	int i;
-	struct Scsi_Host *sh;
-	ips_ha_t *ha;
+	struct Scsi_Host *sh = pci_get_drvdata(pci_dev);
 
-	for (i = 0; i < IPS_MAX_ADAPTERS; i++) {
-		ha = ips_ha[i];
-		if (ha) {
-			if ((pci_dev->bus->number == ha->pcidev->bus->number) &&
-			    (pci_dev->devfn == ha->pcidev->devfn)) {
-				sh = ips_sh[i];
-				ips_release(sh);
-			}
-		}
-	}
+	pci_set_drvdata(pci_dev, NULL);
+
+	ips_release(sh);
+
+	pci_release_regions(pci_dev);
+	pci_disable_device(pci_dev);
 }
 
 /****************************************************************************/
@@ -6946,12 +6934,17 @@ module_exit(ips_module_exit);
 static int __devinit
 ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent)
 {
-	int uninitialized_var(index);
+	int index = -1;
 	int rc;
 
 	METHOD_TRACE("ips_insert_device", 1);
-	if (pci_enable_device(pci_dev))
-		return -1;
+	rc = pci_enable_device(pci_dev);
+	if (rc)
+		return rc;
+
+	rc = pci_request_regions(pci_dev, "ips");
+	if (rc)
+		goto err_out;
 
 	rc = ips_init_phase1(pci_dev, &index);
 	if (rc == SUCCESS)
@@ -6967,6 +6960,19 @@ ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent)
 		ips_num_controllers++;
 
 	ips_next_controller = ips_num_controllers;
+
+	if (rc < 0) {
+		rc = -ENODEV;
+		goto err_out_regions;
+	}
+
+	pci_set_drvdata(pci_dev, ips_sh[index]);
+	return 0;
+
+err_out_regions:
+	pci_release_regions(pci_dev);
+err_out:
+	pci_disable_device(pci_dev);
 	return rc;
 }
 
@@ -6999,7 +7005,7 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	METHOD_TRACE("ips_init_phase1", 1);
 	index = IPS_MAX_ADAPTERS;
 	for (j = 0; j < IPS_MAX_ADAPTERS; j++) {
-		if (ips_ha[j] == 0) {
+		if (ips_ha[j] == NULL) {
 			index = j;
 			break;
 		}
@@ -7036,32 +7042,17 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		uint32_t base;
 		uint32_t offs;
 
-		if (!request_mem_region(mem_addr, mem_len, "ips")) {
-			IPS_PRINTK(KERN_WARNING, pci_dev,
-				   "Couldn't allocate IO Memory space %x len %d.\n",
-				   mem_addr, mem_len);
-			return -1;
-		}
-
 		base = mem_addr & PAGE_MASK;
 		offs = mem_addr - base;
 		ioremap_ptr = ioremap(base, PAGE_SIZE);
+		if (!ioremap_ptr)
+			return -1;
 		mem_ptr = ioremap_ptr + offs;
 	} else {
 		ioremap_ptr = NULL;
 		mem_ptr = NULL;
 	}
 
-	/* setup I/O mapped area (if applicable) */
-	if (io_addr) {
-		if (!request_region(io_addr, io_len, "ips")) {
-			IPS_PRINTK(KERN_WARNING, pci_dev,
-				   "Couldn't allocate IO space %x len %d.\n",
-				   io_addr, io_len);
-			return -1;
-		}
-	}
-
 	/* found a controller */
 	ha = kzalloc(sizeof (ips_ha_t), GFP_KERNEL);
 	if (ha == NULL) {
@@ -7070,7 +7061,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		return -1;
 	}
 
-
 	ips_sh[index] = NULL;
 	ips_ha[index] = ha;
 	ha->active = 1;
-- 
1.5.2.4

[PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index fb90b6c..b8e2f5a 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -6836,13 +6836,10 @@ ips_register_scsi(int index)
 	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
-		scsi_host_put(sh);
-		return -1;
+		goto err_out_sh;
 	}
 
 	kfree(oldha);
-	ips_sh[index] = sh;
-	ips_ha[index] = ha;
 
 	/* Store away needed values for later use */
 	sh->unique_id = (ha->io_addr) ? ha->io_addr : ha->mem_addr;
@@ -6858,10 +6855,21 @@ ips_register_scsi(int index)
 	sh->max_channel = ha->nbus - 1;
 	sh->can_queue = ha->max_cmds - 1;
 
-	scsi_add_host(sh, NULL);
+	if (scsi_add_host(sh, &ha->pcidev->dev))
+		goto err_out;
+
+	ips_sh[index] = sh;
+	ips_ha[index] = ha;
+
 	scsi_scan_host(sh);
 
 	return 0;
+
+err_out:
+	free_irq(ha->pcidev->irq, ha);
+err_out_sh:
+	scsi_host_put(sh);
+	return -1;
 }
 
 /*---------------------------------------------------------------------------*/
-- 
1.5.2.4

[PATCH] [libata] fix 'if(' and similar areas that lack whitespace

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ata/pata_acpi.c         |    4 +-
 drivers/ata/pata_optidma.c      |    2 +-
 drivers/ata/pata_pdc2027x.c     |    2 +-
 drivers/ata/pata_pdc202xx_old.c |    4 +-
 drivers/ata/pata_via.c          |    2 +-
 drivers/ata/pata_winbond.c      |    2 +-
 drivers/ata/sata_nv.c           |   46 +++++++++++++++++++-------------------
 drivers/ata/sata_sx4.c          |    4 +-
 8 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/ata/pata_acpi.c b/drivers/ata/pata_acpi.c
index 0f6f7bc..e4542ab 100644
--- a/drivers/ata/pata_acpi.c
+++ b/drivers/ata/pata_acpi.c
@@ -181,7 +181,7 @@ static void pacpi_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	int unit = adev->devno;
 	struct pata_acpi *acpi = ap->private_data;
 
-	if(!(acpi->gtm.flags & 0x10))
+	if (!(acpi->gtm.flags & 0x10))
 		unit = 0;
 
 	/* Now stuff the nS values into the structure */
@@ -202,7 +202,7 @@ static void pacpi_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	int unit = adev->devno;
 	struct pata_acpi *acpi = ap->private_data;
 
-	if(!(acpi->gtm.flags & 0x10))
+	if (!(acpi->gtm.flags & 0x10))
 		unit = 0;
 
 	/* Now stuff the nS values into the structure */
diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
index 6b07b5b..f9b485a 100644
--- a/drivers/ata/pata_optidma.c
+++ b/drivers/ata/pata_optidma.c
@@ -449,7 +449,7 @@ static int optiplus_with_udma(struct pci_dev *pdev)
 
 	/* Find function 1 */
 	dev1 = pci_get_device(0x1045, 0xC701, NULL);
-	if(dev1 == NULL)
+	if (dev1 == NULL)
 		return 0;
 
 	/* Rev must be >= 0x10 */
diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
index 3d3f155..2622577 100644
--- a/drivers/ata/pata_pdc2027x.c
+++ b/drivers/ata/pata_pdc2027x.c
@@ -348,7 +348,7 @@ static unsigned long pdc2027x_mode_filter(struct ata_device *adev, unsigned long
 	ata_id_c_string(pair->id, model_num, ATA_ID_PROD,
 			  ATA_ID_PROD_LEN + 1);
 	/* If the master is a maxtor in UDMA6 then the slave should not use UDMA 6 */
-	if(strstr(model_num, "Maxtor") == 0 && pair->dma_mode == XFER_UDMA_6)
+	if (strstr(model_num, "Maxtor") == 0 && pair->dma_mode == XFER_UDMA_6)
 		mask &= ~ (1 << (6 + ATA_SHIFT_UDMA));
 
 	return ata_pci_default_filter(adev, mask);
diff --git a/drivers/ata/pata_pdc202xx_old.c b/drivers/ata/pata_pdc202xx_old.c
index 65d9516..bc7c2d5 100644
--- a/drivers/ata/pata_pdc202xx_old.c
+++ b/drivers/ata/pata_pdc202xx_old.c
@@ -351,9 +351,9 @@ static int pdc202xx_init_one(struct pci_dev *dev, const struct pci_device_id *id
 		struct pci_dev *bridge = dev->bus->self;
 		/* Don't grab anything behind a Promise I2O RAID */
 		if (bridge && bridge->vendor == PCI_VENDOR_ID_INTEL) {
-			if( bridge->device == PCI_DEVICE_ID_INTEL_I960)
+			if (bridge->device == PCI_DEVICE_ID_INTEL_I960)
 				return -ENODEV;
-			if( bridge->device == PCI_DEVICE_ID_INTEL_I960RM)
+			if (bridge->device == PCI_DEVICE_ID_INTEL_I960RM)
 				return -ENODEV;
 		}
 	}
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index ea7a9a6..a4175fb 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -176,7 +176,7 @@ static int via_cable_detect(struct ata_port *ap) {
 	if ((config->flags & VIA_UDMA) < VIA_UDMA_66)
 		return ATA_CBL_PATA40;
 	/* UDMA 66 chips have only drive side logic */
-	else if((config->flags & VIA_UDMA) < VIA_UDMA_100)
+	else if ((config->flags & VIA_UDMA) < VIA_UDMA_100)
 		return ATA_CBL_PATA_UNK;
 	/* UDMA 100 or later */
 	pci_read_config_dword(pdev, 0x50, &ata66);
diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
index 549cbbe..311cdb3 100644
--- a/drivers/ata/pata_winbond.c
+++ b/drivers/ata/pata_winbond.c
@@ -279,7 +279,7 @@ static __init int winbond_init(void)
 
 			if (request_region(port, 2, "pata_winbond")) {
 				ret = winbond_init_one(port);
-				if(ret <= 0)
+				if (ret <= 0)
 					release_region(port, 2);
 				else ct+= ret;
 			}
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 2e0279f..9210bb2 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1012,7 +1012,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 				u32 check_commands;
 				int pos, error = 0;
 
-				if(ata_tag_valid(ap->link.active_tag))
+				if (ata_tag_valid(ap->link.active_tag))
 					check_commands = 1 << ap->link.active_tag;
 				else
 					check_commands = ap->link.sactive;
@@ -1028,7 +1028,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 		}
 	}
 
-	if(notifier_clears[0] || notifier_clears[1]) {
+	if (notifier_clears[0] || notifier_clears[1]) {
 		/* Note: Both notifier clear registers must be written
 		   if either is set, even if one is zero, according to NVIDIA. */
 		struct nv_adma_port_priv *pp = host->ports[0]->private_data;
@@ -1119,7 +1119,7 @@ static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc)
 {
 	struct nv_adma_port_priv *pp = qc->ap->private_data;
 
-	if(pp->flags & NV_ADMA_PORT_REGISTER_MODE)
+	if (pp->flags & NV_ADMA_PORT_REGISTER_MODE)
 		ata_bmdma_post_internal_cmd(qc);
 }
 
@@ -1194,10 +1194,10 @@ static int nv_adma_port_start(struct ata_port *ap)
 
 	tmp = readw(mmio + NV_ADMA_CTL);
 	writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
 	udelay(1);
 	writew(tmp & ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
 
 	return 0;
 }
@@ -1255,10 +1255,10 @@ static int nv_adma_port_resume(struct ata_port *ap)
 
 	tmp = readw(mmio + NV_ADMA_CTL);
 	writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
 	udelay(1);
 	writew(tmp & ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
 
 	return 0;
 }
@@ -1359,12 +1359,12 @@ static int nv_adma_use_reg_mode(struct ata_queued_cmd *qc)
 	/* ADMA engine can only be used for non-ATAPI DMA commands,
 	   or interrupt-driven no-data commands, where a result taskfile
 	   is not required. */
-	if((pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
+	if ((pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
 	   (qc->tf.flags & ATA_TFLAG_POLLING) ||
 	   (qc->flags & ATA_QCFLAG_RESULT_TF))
 		return 1;
 
-	if((qc->flags & ATA_QCFLAG_DMAMAP) ||
+	if ((qc->flags & ATA_QCFLAG_DMAMAP) ||
 	   (qc->tf.protocol == ATA_PROT_NODATA))
 		return 0;
 
@@ -1401,7 +1401,7 @@ static void nv_adma_qc_prep(struct ata_queued_cmd *qc)
 
 	nv_adma_tf_to_cpb(&qc->tf, cpb->tf);
 
-	if(qc->flags & ATA_QCFLAG_DMAMAP) {
+	if (qc->flags & ATA_QCFLAG_DMAMAP) {
 		nv_adma_fill_sg(qc, cpb);
 		ctl_flags |= NV_CPB_CTL_APRD_VALID;
 	} else
@@ -1435,7 +1435,7 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc)
 	   and (number of cpbs to append -1) in top 8 bits */
 	wmb();
 
-	if(curr_ncq != pp->last_issue_ncq) {
+	if (curr_ncq != pp->last_issue_ncq) {
 	   	/* Seems to need some delay before switching between NCQ and non-NCQ
 		   commands, else we get command timeouts and such. */
 		udelay(20);
@@ -1641,12 +1641,12 @@ static void nv_error_handler(struct ata_port *ap)
 static void nv_adma_error_handler(struct ata_port *ap)
 {
 	struct nv_adma_port_priv *pp = ap->private_data;
-	if(!(pp->flags & NV_ADMA_PORT_REGISTER_MODE)) {
+	if (!(pp->flags & NV_ADMA_PORT_REGISTER_MODE)) {
 		void __iomem *mmio = pp->ctl_block;
 		int i;
 		u16 tmp;
 
-		if(ata_tag_valid(ap->link.active_tag) || ap->link.sactive) {
+		if (ata_tag_valid(ap->link.active_tag) || ap->link.sactive) {
 			u32 notifier = readl(mmio + NV_ADMA_NOTIFIER);
 			u32 notifier_error = readl(mmio + NV_ADMA_NOTIFIER_ERROR);
 			u32 gen_ctl = readl(pp->gen_block + NV_ADMA_GEN_CTL);
@@ -1660,9 +1660,9 @@ static void nv_adma_error_handler(struct ata_port *ap)
 				notifier, notifier_error, gen_ctl, status,
 				cpb_count, next_cpb_idx);
 
-			for( i=0;i<NV_ADMA_MAX_CPBS;i++) {
+			for (i = 0; i < NV_ADMA_MAX_CPBS; i++) {
 				struct nv_adma_cpb *cpb = &pp->cpb[i];
-				if( (ata_tag_valid(ap->link.active_tag) && i == ap->link.active_tag) ||
+				if ((ata_tag_valid(ap->link.active_tag) && i == ap->link.active_tag) ||
 				    ap->link.sactive & (1 << i) )
 					ata_port_printk(ap, KERN_ERR,
 						"CPB %d: ctl_flags 0x%x, resp_flags 0x%x\n",
@@ -1674,7 +1674,7 @@ static void nv_adma_error_handler(struct ata_port *ap)
 		nv_adma_register_mode(ap);
 
 		/* Mark all of the CPBs as invalid to prevent them from being executed */
-		for( i=0;i<NV_ADMA_MAX_CPBS;i++)
+		for (i = 0; i < NV_ADMA_MAX_CPBS; i++)
 			pp->cpb[i].ctl_flags &= ~NV_CPB_CTL_CPB_VALID;
 
 		/* clear CPB fetch count */
@@ -1683,10 +1683,10 @@ static void nv_adma_error_handler(struct ata_port *ap)
 		/* Reset channel */
 		tmp = readw(mmio + NV_ADMA_CTL);
 		writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-		readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+		readw(mmio + NV_ADMA_CTL);	/* flush posted write */
 		udelay(1);
 		writew(tmp & ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
-		readw( mmio + NV_ADMA_CTL );	/* flush posted write */
+		readw(mmio + NV_ADMA_CTL);	/* flush posted write */
 	}
 
 	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset,
@@ -2440,32 +2440,32 @@ static int nv_pci_device_resume(struct pci_dev *pdev)
 	int rc;
 
 	rc = ata_pci_device_do_resume(pdev);
-	if(rc)
+	if (rc)
 		return rc;
 
 	if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
-		if(hpriv->type >= CK804) {
+		if (hpriv->type >= CK804) {
 			u8 regval;
 
 			pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
 			regval |= NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
 			pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
 		}
-		if(hpriv->type == ADMA) {
+		if (hpriv->type == ADMA) {
 			u32 tmp32;
 			struct nv_adma_port_priv *pp;
 			/* enable/disable ADMA on the ports appropriately */
 			pci_read_config_dword(pdev, NV_MCP_SATA_CFG_20, &tmp32);
 
 			pp = host->ports[0]->private_data;
-			if(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)
+			if (pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)
 				tmp32 &= ~(NV_MCP_SATA_CFG_20_PORT0_EN |
 				 	   NV_MCP_SATA_CFG_20_PORT0_PWB_EN);
 			else
 				tmp32 |=  (NV_MCP_SATA_CFG_20_PORT0_EN |
 				 	   NV_MCP_SATA_CFG_20_PORT0_PWB_EN);
 			pp = host->ports[1]->private_data;
-			if(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)
+			if (pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)
 				tmp32 &= ~(NV_MCP_SATA_CFG_20_PORT1_EN |
 				 	   NV_MCP_SATA_CFG_20_PORT1_PWB_EN);
 			else
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index b6026bc..16b3b8a 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -1091,7 +1091,7 @@ static int pdc20621_detect_dimm(struct ata_host *host)
 		return 0;
 
 	if (pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, 9, &data)) {
-		if(data <= 0x75)
+		if (data <= 0x75)
 			return 133;
    	} else
 		return 0;
@@ -1254,7 +1254,7 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host)
 	   If SX4 is on PCI-X bus, after 3 seconds, the timer counter
 	   register should be >= (0xffffffff - 3x10^8).
 	*/
-	if(tcount >= PCI_X_TCOUNT) {
+	if (tcount >= PCI_X_TCOUNT) {
 		ticks = (time_period - tcount);
 		VPRINTK("Num counters 0x%x (%d)\n", ticks, ticks);
 
-- 
1.5.2.4

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Andrew Morton]

On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:

>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------

this driver seems a bit of a basket case :(


What's going on here?

		scb->dcdb.cmd_attribute =
		    ips_command_direction[scb->scsi_cmd->cmnd[0]];

        /* Allow a WRITE BUFFER Command to Have no Data */
        /* This is Used by Tape Flash Utilites          */
        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
            scb->dcdb.cmd_attribute = 0;                  

		if (!(scb->dcdb.cmd_attribute & 0x3))
			scb->dcdb.transfer_length = 0;

		if (scb->data_len >= IPS_MAX_XFER) {

I hope that's just busted indentation and not a missing {} block.

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Jeff Garzik]

Andrew Morton wrote:
> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
> 
>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
> 
> this driver seems a bit of a basket case :(
> 
> 
> What's going on here?
> 
> 		scb->dcdb.cmd_attribute =
> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
> 
>         /* Allow a WRITE BUFFER Command to Have no Data */
>         /* This is Used by Tape Flash Utilites          */
>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>             scb->dcdb.cmd_attribute = 0;                  
> 
> 		if (!(scb->dcdb.cmd_attribute & 0x3))
> 			scb->dcdb.transfer_length = 0;
> 
> 		if (scb->data_len >= IPS_MAX_XFER) {
> 
> I hope that's just busted indentation and not a missing {} block.

The driver is one of the grotty drivers people are afraid to touch, 
therefore it bitrots even faster, in a vicious cycle.  You don't have to 
look hard at all to find checkpatch pukeage, very real bugs, and 
Pythonesque silliness.

Not having hardware I went only for changes that are provable and 
obvious, really...

	Jeff

Re: [PATCH 3/4] [SCSI] ips: PCI API cleanups

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

Jeff Garzik wrote:
> * pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
>   allowing us to eliminate the ips_ha[] search loop and call
>   ips_release() directly.
>
> * call pci_{request,release}_regions() and eliminate individual
>   request/release_[mem_]region() calls
>
> * call pci_disable_device(), paired with pci_enable_device()
>
> * s/0/NULL/ in a few places
>
> * check ioremap() return value

> @@ -7036,32 +7042,17 @@ ips_init_phase1(struct pci_dev *pci_dev, int
> *indexPtr) uint32_t base;
>  		uint32_t offs;
>
> -		if (!request_mem_region(mem_addr, mem_len, "ips")) {
> -			IPS_PRINTK(KERN_WARNING, pci_dev,
> -				   "Couldn't allocate IO Memory space %x len %d.\n",
> -				   mem_addr, mem_len);
> -			return -1;
> -		}
> -
>  		base = mem_addr & PAGE_MASK;
>  		offs = mem_addr - base;
>  		ioremap_ptr = ioremap(base, PAGE_SIZE);

This looks odd. What are we actually doing here?

It seems that we want to map that PCI BAR. Since we're playing with PAGE_MASK 
I assume the BAR always has a length <PAGE_SIZE, else we would get page 
aligned memory anyway. If that's true something like

mem_ptr = pci_iomap(pci_dev, bar, 0)

should do the trick. Later we would do

pci_iounmap(pci_dev, mem_ptr)

This whole ioremap_ptr stuff can go away at all. Am I right? Then I'll cook up 
a patch soon.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

RE: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Salyzyn, Mark]

ACK. Inspected; Mechanical, precise and no introduction of bugs.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 1/4] [SCSI] ips: remove ips_ha members that 
> duplicate struct pci_dev members
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |  178 
> ++++++++++++++++++++++++----------------------------
>  drivers/scsi/ips.h |   20 +++----
>  2 files changed, 91 insertions(+), 107 deletions(-)

RE: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Salyzyn, Mark]

ACK. Inspected; Mechanical, precise and no introduction of bugs.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 1/4] [SCSI] ips: remove ips_ha members that 
> duplicate struct pci_dev members
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |  178 
> ++++++++++++++++++++++++----------------------------
>  drivers/scsi/ips.h |   20 +++----
>  2 files changed, 91 insertions(+), 107 deletions(-)

RE: [PATCH 2/4] [SCSI] ips: trim trailing whitespace

[Salyzyn, Mark]

ACK. Inspected; trivial, clean and no sign of any code changes.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 2/4] [SCSI] ips: trim trailing whitespace
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   44 
> ++++++++++++++++++++++----------------------
>  drivers/scsi/ips.h |   12 ++++++------
>  2 files changed, 28 insertions(+), 28 deletions(-)

RE: [PATCH 2/4] [SCSI] ips: trim trailing whitespace

[Salyzyn, Mark]

ACK. Inspected; trivial, clean and no sign of any code changes.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 2/4] [SCSI] ips: trim trailing whitespace
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   44 
> ++++++++++++++++++++++----------------------
>  drivers/scsi/ips.h |   12 ++++++------
>  2 files changed, 28 insertions(+), 28 deletions(-)

RE: [PATCH 3/4] [SCSI] ips: PCI API cleanups

[Salyzyn, Mark]

ACK. Inspected only. Looks ok.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 3/4] [SCSI] ips: PCI API cleanups
> 
> * pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
>   allowing us to eliminate the ips_ha[] search loop and call
>   ips_release() directly.
> 
> * call pci_{request,release}_regions() and eliminate individual
>   request/release_[mem_]region() calls
> 
> * call pci_disable_device(), paired with pci_enable_device()
> 
> * s/0/NULL/ in a few places
> 
> * check ioremap() return value
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   72 
> ++++++++++++++++++++++-----------------------------
>  1 files changed, 31 insertions(+), 41 deletions(-)

RE: [PATCH 3/4] [SCSI] ips: PCI API cleanups

[Salyzyn, Mark]

ACK. Inspected only. Looks ok.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 3/4] [SCSI] ips: PCI API cleanups
> 
> * pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
>   allowing us to eliminate the ips_ha[] search loop and call
>   ips_release() directly.
> 
> * call pci_{request,release}_regions() and eliminate individual
>   request/release_[mem_]region() calls
> 
> * call pci_disable_device(), paired with pci_enable_device()
> 
> * s/0/NULL/ in a few places
> 
> * check ioremap() return value
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   72 
> ++++++++++++++++++++++-----------------------------
>  1 files changed, 31 insertions(+), 41 deletions(-)

RE: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups

[Salyzyn, Mark]

ACK. Inspected. cleanup with zero risk.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() 
> failure, and other err cleanups
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)

RE: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups

[Salyzyn, Mark]

ACK. Inspected. cleanup with zero risk.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() 
> failure, and other err cleanups
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Boaz Harrosh]

On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik <jeff@garzik.org> wrote:
> Andrew Morton wrote:
>> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
>>
>>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
>> this driver seems a bit of a basket case :(
>>
>>
>> What's going on here?
>>
>> 		scb->dcdb.cmd_attribute =
>> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
>>
>>         /* Allow a WRITE BUFFER Command to Have no Data */
>>         /* This is Used by Tape Flash Utilites          */
>>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>>             scb->dcdb.cmd_attribute = 0;                  
>>
>> 		if (!(scb->dcdb.cmd_attribute & 0x3))
>> 			scb->dcdb.transfer_length = 0;
>>
>> 		if (scb->data_len >= IPS_MAX_XFER) {
>>
>> I hope that's just busted indentation and not a missing {} block.
> 
> The driver is one of the grotty drivers people are afraid to touch, 
> therefore it bitrots even faster, in a vicious cycle.  You don't have to 
> look hard at all to find checkpatch pukeage, very real bugs, and 
> Pythonesque silliness.
> 
> Not having hardware I went only for changes that are provable and 
> obvious, really...
> 
> 	Jeff
> 

>From the experience with gdth I would say that a first patch of any
serious cleanup, should be the whitespace and formating cleanup.
And now that we can all read what it says, start changing.

In the gdth case I know of 3 bugs that happen just because of that mess.
And I promise to send that patch for gdth soooon.

I found that lint, even with the command line options recommended by 
kernel, is to aggressive, and leaves lots of work to be fixed by hand.
(e.g it will touch the comments)

I found that a small util called astyle with the right settings for the 
tab==8/use-tabs will give you a clean tab indent, but will not touch the
longer than 80, broken out lines, which keeps things better formated.

Morton checkpatch is very good in whining about bad files, but did anyone
attempt a script for linux-style Indentation, formating, and whitespace cleanup,
that give you something like 95% success. As it is lint gives you 50-70%
and astyle 60-80%

Boaz

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Matthew Wilcox]

On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote:
> I found that lint, even with the command line options recommended by 

Do you mean Lindent / indent?

> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
> (e.g it will touch the comments)

It's not perfect, but code beautification is an art, not a science ;-)

A lot of ugly code can't be made beautiful by a simple parser like
indent because what it really needs is refactoring.  But you can't
refactor until you've made it at least partially readable, so Lindent is
the first step.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] [libata] fix 'if(' and similar areas that lack whitespace

[Richard Knutsson]

Jeff Garzik wrote:
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/ata/pata_acpi.c         |    4 +-
>  drivers/ata/pata_optidma.c      |    2 +-
>  drivers/ata/pata_pdc2027x.c     |    2 +-
>  drivers/ata/pata_pdc202xx_old.c |    4 +-
>  drivers/ata/pata_via.c          |    2 +-
>  drivers/ata/pata_winbond.c      |    2 +-
>  drivers/ata/sata_nv.c           |   46 +++++++++++++++++++-------------------
>  drivers/ata/sata_sx4.c          |    4 +-
>  8 files changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/ata/pata_acpi.c b/drivers/ata/pata_acpi.c
> index 0f6f7bc..e4542ab 100644
> --- a/drivers/ata/pata_acpi.c
> +++ b/drivers/ata/pata_acpi.c
> @@ -181,7 +181,7 @@ static void pacpi_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  	int unit = adev->devno;
>  	struct pata_acpi *acpi = ap->private_data;
>  
> -	if(!(acpi->gtm.flags & 0x10))
> +	if (!(acpi->gtm.flags & 0x10))
>  		unit = 0;
>  
>  	/* Now stuff the nS values into the structure */
> @@ -202,7 +202,7 @@ static void pacpi_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  	int unit = adev->devno;
>  	struct pata_acpi *acpi = ap->private_data;
>  
> -	if(!(acpi->gtm.flags & 0x10))
> +	if (!(acpi->gtm.flags & 0x10))
>  		unit = 0;
>  
>  	/* Now stuff the nS values into the structure */
> diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
> index 6b07b5b..f9b485a 100644
> --- a/drivers/ata/pata_optidma.c
> +++ b/drivers/ata/pata_optidma.c
> @@ -449,7 +449,7 @@ static int optiplus_with_udma(struct pci_dev *pdev)
>  
>  	/* Find function 1 */
>  	dev1 = pci_get_device(0x1045, 0xC701, NULL);
> -	if(dev1 == NULL)
> +	if (dev1 == NULL)
>   
Maybe "if (!dev1)" instead?
>  		return 0;
>  
>  	/* Rev must be >= 0x10 */
>
>   
<snip>
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 2e0279f..9210bb2 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -1012,7 +1012,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
>  				u32 check_commands;
>  				int pos, error = 0;
>  
> -				if(ata_tag_valid(ap->link.active_tag))
> +				if (ata_tag_valid(ap->link.active_tag))
>  					check_commands = 1 << ap->link.active_tag;
>  				else
>  					check_commands = ap->link.sactive;
> @@ -1028,7 +1028,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
>  		}
>  	}
>  
> -	if(notifier_clears[0] || notifier_clears[1]) {
> +	if (notifier_clears[0] || notifier_clears[1]) {
>  		/* Note: Both notifier clear registers must be written
>  		   if either is set, even if one is zero, according to NVIDIA. */
>  		struct nv_adma_port_priv *pp = host->ports[0]->private_data;
> @@ -1119,7 +1119,7 @@ static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc)
>  {
>  	struct nv_adma_port_priv *pp = qc->ap->private_data;
>  
> -	if(pp->flags & NV_ADMA_PORT_REGISTER_MODE)
> +	if (pp->flags & NV_ADMA_PORT_REGISTER_MODE)
>  		ata_bmdma_post_internal_cmd(qc);
>  }
>  
> @@ -1194,10 +1194,10 @@ static int nv_adma_port_start(struct ata_port *ap)
>  
>  	tmp = readw(mmio + NV_ADMA_CTL);
>  	writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
> -	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
> +	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
>   
Missed the space before closing parentheses...
>  	udelay(1);
>  	writew(tmp & ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
> -	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
> +	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
>   
and here
>  
>  	return 0;
>  }
> @@ -1255,10 +1255,10 @@ static int nv_adma_port_resume(struct ata_port *ap)
>  
>  	tmp = readw(mmio + NV_ADMA_CTL);
>  	writew(tmp | NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
> -	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
> +	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
>   
and here
>  	udelay(1);
>  	writew(tmp & ~NV_ADMA_CTL_CHANNEL_RESET, mmio + NV_ADMA_CTL);
> -	readw( mmio + NV_ADMA_CTL );	/* flush posted write */
> +	readw(mmio + NV_ADMA_CTL );	/* flush posted write */
>   
and here
>  
>  	return 0;
>  }
>   
The rest looks good.

cu
Richard Knutsson

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Boaz Harrosh]

Matthew Wilcox wrote:
> On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote:
>> I found that lint, even with the command line options recommended by 
> 
> Do you mean Lindent / indent?
> 
Yes "indent". I found that there are better switches to indent than
what's in Lindent. But both are crap as for instance it does not
understand the linux style multi-line-comments and mess them up,
or it tabafy's broken-out-lines. Which is not what we usually
want. And lots of other stuff. astyle does a much better job
just by being less aggressive.

>> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
>> (e.g it will touch the comments)
> 
> It's not perfect, but code beautification is an art, not a science ;-)
> 
> A lot of ugly code can't be made beautiful by a simple parser like
> indent because what it really needs is refactoring.  But you can't
> refactor until you've made it at least partially readable, so Lindent is
> the first step.
> 

I think I disagree 89% and agree 11%.
- remove trailing spaces
- Indent, 
- Convert "Indents" to tabs
- split long lines to multi lines, indent up to the last indent
  level, than fill with spaces, right align.
These can all be done by a machine, the Linux way.

Than
- Adjust broken out lines, like if and while statements to be more
  readable 
- Remove/add blank lines for readability or after end of locals.
That can be adjusted by a person.

But I think with a given code it can be made 89% acceptable by
a machine and 11% adjusted by a man. That is before I start
a single char of coding.

Boaz
 

Re: [PATCH 1/3] [IDE] Add helper __ide_setup_pci_device()

[Bartlomiej Zolnierkiewicz]

[ The last few days were extra busy with pushing previously queued
  IDE bits upstream but I'm back to reviewing the new stuff. ]

Thanks for splitting your patch on smaller chunks.

On Thursday 25 October 2007, Jeff Garzik wrote:
> Like ide_setup_pci_device(), except with additional array argument
> 'u8 *idx' that permits low-level driver to become aware of its assigned
> hwifs.

I would prefer to avoid exporting this information to host drivers
if possible (they shouldn't need to know about higher-layer decisions,
plus this change opens the door for various "creative" abuses).

In case of sc1200 host driver fixes (patches #2-3/3) it should be
possible to remove the need for the below patch and at the same time
simplify sc1200 code further (more details in review of patch #2/2).

> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/ide/setup-pci.c |   17 ++++++++++++-----
>  include/linux/ide.h     |    1 +
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
> index 02d14bf..4960b9f 100644
> --- a/drivers/ide/setup-pci.c
> +++ b/drivers/ide/setup-pci.c
> @@ -666,12 +666,10 @@ out:
>  	return ret;
>  }
>  
> -int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
> +int __ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d,
> +			   u8 *idx)
>  {
> -	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
> -	int ret;
> -
> -	ret = do_ide_setup_pci_device(dev, d, &idx[0], 1);
> +	int ret = do_ide_setup_pci_device(dev, d, idx, 1);
>  
>  	if (ret >= 0)
>  		ide_device_add(idx);
> @@ -679,6 +677,15 @@ int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
>  	return ret;
>  }
>  
> +EXPORT_SYMBOL_GPL(__ide_setup_pci_device);
> +
> +int ide_setup_pci_device(struct pci_dev *dev, const struct ide_port_info *d)
> +{
> +	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
> +
> +	return __ide_setup_pci_device(dev, d, &idx[0]);
> +}
> +
>  EXPORT_SYMBOL_GPL(ide_setup_pci_device);
>  
>  int ide_setup_pci_devices(struct pci_dev *dev1, struct pci_dev *dev2,
> diff --git a/include/linux/ide.h b/include/linux/ide.h
> index 4ed4777..3404fb9 100644
> --- a/include/linux/ide.h
> +++ b/include/linux/ide.h
> @@ -1244,6 +1244,7 @@ struct ide_port_info {
>  	u8			udma_mask;
>  };
>  
> +int __ide_setup_pci_device(struct pci_dev *, const struct ide_port_info *, u8 *);
>  int ide_setup_pci_device(struct pci_dev *, const struct ide_port_info *);
>  int ide_setup_pci_devices(struct pci_dev *, struct pci_dev *, const struct ide_port_info *);

Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop

[Bartlomiej Zolnierkiewicz]

On Thursday 25 October 2007, Jeff Garzik wrote:
> Store our hwif indices at probe time, in order to eliminate a needless
> and ugly loop across all hwifs, searching for our pci device.

It seems that we can simplify it even further and remove knowledge about
hwifs altogether from suspend/resume methods.

> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/ide/pci/sc1200.c |   76 +++++++++++++++++++++++++++++++---------------
>  1 files changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c
> index d2c8b55..17e58d6 100644
> --- a/drivers/ide/pci/sc1200.c
> +++ b/drivers/ide/pci/sc1200.c
> @@ -41,6 +41,8 @@
>  #define PCI_CLK_66	0x02
>  #define PCI_CLK_33A	0x03
>  
> +#define SC1200_IFS	4
> +
>  static unsigned short sc1200_get_pci_clock (void)
>  {
>  	unsigned char chip_id, silicon_revision;
> @@ -261,31 +263,32 @@ static void sc1200_set_pio_mode(ide_drive_t *drive, const u8 pio)
>  }
>  
>  #ifdef CONFIG_PM
> -static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev)
> -{
> -	int	h;
> -
> -	for (h = 0; h < MAX_HWIFS; h++) {
> -		ide_hwif_t *hwif = &ide_hwifs[h];
> -		if (prev) {
> -			if (hwif == prev)
> -				prev = NULL;	// found previous, now look for next match
> -		} else {
> -			if (hwif && hwif->pci_dev == dev)
> -				return hwif;	// found next match
> -		}
> -	}
> -	return NULL;	// not found
> -}
> -
>  typedef struct sc1200_saved_state_s {
>  	__u32		regs[4];
>  } sc1200_saved_state_t;
>  
> +static unsigned int pack_hwif_idx(u8 *idx)
> +{
> +	return	(((unsigned int) idx[0]) << 0) |
> +		(((unsigned int) idx[1]) << 8) |
> +		(((unsigned int) idx[2]) << 16) |
> +		(((unsigned int) idx[3]) << 24);
> +}
> +
> +static ide_hwif_t *sc1200_hwif(struct pci_dev *pdev, unsigned int iface)
> +{
> +	unsigned int packed_hwifs, idx;
> +
> +	packed_hwifs = (unsigned long) pci_get_drvdata(pdev);
> +	idx = (packed_hwifs >> (iface * 8)) & 0xff;
> +
> +	return (idx == 0xff) ? NULL : &ide_hwifs[idx];
> +}
>  
>  static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>  {
> -	ide_hwif_t		*hwif = NULL;
> +	ide_hwif_t		*hwif;
> +	int			i;
>  
>  	printk("SC1200: suspend(%u)\n", state.event);
>  
> @@ -295,9 +298,14 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>  		//
>  		// Loop over all interfaces that are part of this PCI device:
>  		//
> -		while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
> +		for (i = 0; i < SC1200_IFS; i++) {
>  			sc1200_saved_state_t	*ss;
>  			unsigned int		basereg, r;
> +
> +			hwif = sc1200_hwif(dev, i);
> +			if (!hwif)
> +				continue;
> +
>  			//
>  			// allocate a permanent save area, if not already allocated
>  			//
> @@ -310,7 +318,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>  			}
>  			ss = (sc1200_saved_state_t *)hwif->config_data;
>  			//
> -			// Save timing registers:  this may be unnecessary if 
> +			// Save timing registers:  this may be unnecessary if
>  			// BIOS also does it
>  			//
>  			basereg = hwif->channel ? 0x50 : 0x40;

Please take a close look at the line above and the next three lines:

for (r = 0; r < 4; ++r) {
	pci_read_config_dword (hwif->pci_dev, basereg + (r<<2), &ss->regs[r]);
}

It is highly obfuscated but the sc1200_suspend() reads 16 bytes from
the offset 0x40 (for the primary port) and puts them in the corresponding
struct sc1200_saved_state_s buffer, then it reads another 16 bytes from the
offset 0x50 (for the secondary port) and puts it in the another buffer.

In summary sc1200_suspend() reads 32 continuous bytes from offset 0x40
and the exactly reverse operation happens in sc1200_resume().

Given that and the fact that struct sc1200_save_state_s buffers are used
_only_ by sc1200_{suspend,resume}() we may safely convert the code to use
one buffer for both ports (the whole PCI device).  We just need to bump
the size of struct sc1200_saved_state_s (from 4 to 8 double-words) and use
pci_{get,set}_drvdata() instead of hwif->config_data.  Then we can remove
looping over interfaces completely from sc1200_{suspend,resume}()! :)

> @@ -320,7 +328,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>  		}
>  	}
>  
> -	/* You don't need to iterate over disks -- sysfs should have done that for you already */ 
> +	/* You don't need to iterate over disks -- sysfs should have done that for you already */
>  
>  	pci_disable_device(dev);
>  	pci_set_power_state(dev, pci_choose_state(dev, state));
> @@ -330,7 +338,8 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>  
>  static int sc1200_resume (struct pci_dev *dev)
>  {
> -	ide_hwif_t	*hwif = NULL;
> +	ide_hwif_t		*hwif;
> +	int			i;
>  
>  	pci_set_power_state(dev, PCI_D0);	// bring chip back from sleep state
>  	dev->current_state = PM_EVENT_ON;
> @@ -338,9 +347,15 @@ static int sc1200_resume (struct pci_dev *dev)
>  	//
>  	// loop over all interfaces that are part of this pci device:
>  	//
> -	while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
> +	for (i = 0; i < SC1200_IFS; i++) {
>  		unsigned int		basereg, r;
> -		sc1200_saved_state_t	*ss = (sc1200_saved_state_t *)hwif->config_data;
> +		sc1200_saved_state_t	*ss;
> +
> +		hwif = sc1200_hwif(dev, i);
> +		if (!hwif)
> +			continue;
> +
> +		ss = (sc1200_saved_state_t *)hwif->config_data;
>  
>  		//
>  		// Restore timing registers:  this may be unnecessary if BIOS also does it
> @@ -386,7 +401,18 @@ static const struct ide_port_info sc1200_chipset __devinitdata = {
>  
>  static int __devinit sc1200_init_one(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> -	return ide_setup_pci_device(dev, &sc1200_chipset);
> +	unsigned int packed_hwifs;
> +	int rc;
> +	u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };
> +
> +	rc = __ide_setup_pci_device(dev, &sc1200_chipset, &idx[0]);
> +	if (rc)
> +		return rc;
> +	
> +	packed_hwifs = pack_hwif_idx(&idx[0]);
> +
> +	pci_set_drvdata(dev, (void *)(unsigned long) packed_hwifs);
> +	return 0;
>  }
>  
>  static const struct pci_device_id sc1200_pci_tbl[] = {

Re: [PATCH 3/3] drivers/ide/pci/sc1200.c: fix suspend/resume buglets and warnings

[Bartlomiej Zolnierkiewicz]

On Thursday 25 October 2007, Jeff Garzik wrote:
> * We shouldn't bother with dev->current_state, the PCI API functions we
>   call manage this for us (and do a far better job at it too).
> 
> * Remove pci_set_power_state(dev, PCI_D0) call in resume, as
>   pci_enable_device() does the same thing.
> 
> * Check pci_enable_device() return value.  If it failed, fail
>   the entire resume and avoid programming timings into the [potentially
>   dead/asleep] chip.
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

applied, thanks

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Jeff Garzik]

Boaz Harrosh wrote:
> On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik <jeff@garzik.org> wrote:
>> Andrew Morton wrote:
>>> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
>>>
>>>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
>>> this driver seems a bit of a basket case :(
>>>
>>>
>>> What's going on here?
>>>
>>> 		scb->dcdb.cmd_attribute =
>>> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
>>>
>>>         /* Allow a WRITE BUFFER Command to Have no Data */
>>>         /* This is Used by Tape Flash Utilites          */
>>>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>>>             scb->dcdb.cmd_attribute = 0;                  
>>>
>>> 		if (!(scb->dcdb.cmd_attribute & 0x3))
>>> 			scb->dcdb.transfer_length = 0;
>>>
>>> 		if (scb->data_len >= IPS_MAX_XFER) {
>>>
>>> I hope that's just busted indentation and not a missing {} block.
>> The driver is one of the grotty drivers people are afraid to touch, 
>> therefore it bitrots even faster, in a vicious cycle.  You don't have to 
>> look hard at all to find checkpatch pukeage, very real bugs, and 
>> Pythonesque silliness.
>>
>> Not having hardware I went only for changes that are provable and 
>> obvious, really...
>>
>> 	Jeff
>>
> 
>>From the experience with gdth I would say that a first patch of any
> serious cleanup, should be the whitespace and formating cleanup.
> And now that we can all read what it says, start changing.
> 
> In the gdth case I know of 3 bugs that happen just because of that mess.
> And I promise to send that patch for gdth soooon.
> 
> I found that lint, even with the command line options recommended by 
> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
> (e.g it will touch the comments)

Yeah, I hear you, but don't mistake my kill-warnings work for embarking 
upon a new clean-this-driver project ;-)

I tend to make incremental improvements all over the tree, "rising tide 
lifts all boats" and that sort of thing.

If I remained and worked on cleaning every grotty driver I come across 
while killing warnings or fixing interrupt handlers, I would have no 
free time at all :)

	Jeff

Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members

[Jeff Garzik]

thanks for reviewing these!

Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Thursday 25 October 2007, Jeff Garzik wrote:
>> Store our hwif indices at probe time, in order to eliminate a needless
>> and ugly loop across all hwifs, searching for our pci device.
> 
> It seems that we can simplify it even further and remove knowledge about
> hwifs altogether from suspend/resume methods.
> 
>> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
>> ---
>>  drivers/ide/pci/sc1200.c |   76 +++++++++++++++++++++++++++++++---------------
>>  1 files changed, 51 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c
>> index d2c8b55..17e58d6 100644
>> --- a/drivers/ide/pci/sc1200.c
>> +++ b/drivers/ide/pci/sc1200.c
>> @@ -41,6 +41,8 @@
>>  #define PCI_CLK_66	0x02
>>  #define PCI_CLK_33A	0x03
>>  
>> +#define SC1200_IFS	4
>> +
>>  static unsigned short sc1200_get_pci_clock (void)
>>  {
>>  	unsigned char chip_id, silicon_revision;
>> @@ -261,31 +263,32 @@ static void sc1200_set_pio_mode(ide_drive_t *drive, const u8 pio)
>>  }
>>  
>>  #ifdef CONFIG_PM
>> -static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev)
>> -{
>> -	int	h;
>> -
>> -	for (h = 0; h < MAX_HWIFS; h++) {
>> -		ide_hwif_t *hwif = &ide_hwifs[h];
>> -		if (prev) {
>> -			if (hwif == prev)
>> -				prev = NULL;	// found previous, now look for next match
>> -		} else {
>> -			if (hwif && hwif->pci_dev == dev)
>> -				return hwif;	// found next match
>> -		}
>> -	}
>> -	return NULL;	// not found
>> -}
>> -
>>  typedef struct sc1200_saved_state_s {
>>  	__u32		regs[4];
>>  } sc1200_saved_state_t;
>>  
>> +static unsigned int pack_hwif_idx(u8 *idx)
>> +{
>> +	return	(((unsigned int) idx[0]) << 0) |
>> +		(((unsigned int) idx[1]) << 8) |
>> +		(((unsigned int) idx[2]) << 16) |
>> +		(((unsigned int) idx[3]) << 24);
>> +}
>> +
>> +static ide_hwif_t *sc1200_hwif(struct pci_dev *pdev, unsigned int iface)
>> +{
>> +	unsigned int packed_hwifs, idx;
>> +
>> +	packed_hwifs = (unsigned long) pci_get_drvdata(pdev);
>> +	idx = (packed_hwifs >> (iface * 8)) & 0xff;
>> +
>> +	return (idx == 0xff) ? NULL : &ide_hwifs[idx];
>> +}
>>  
>>  static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>>  {
>> -	ide_hwif_t		*hwif = NULL;
>> +	ide_hwif_t		*hwif;
>> +	int			i;
>>  
>>  	printk("SC1200: suspend(%u)\n", state.event);
>>  
>> @@ -295,9 +298,14 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>>  		//
>>  		// Loop over all interfaces that are part of this PCI device:
>>  		//
>> -		while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
>> +		for (i = 0; i < SC1200_IFS; i++) {
>>  			sc1200_saved_state_t	*ss;
>>  			unsigned int		basereg, r;
>> +
>> +			hwif = sc1200_hwif(dev, i);
>> +			if (!hwif)
>> +				continue;
>> +
>>  			//
>>  			// allocate a permanent save area, if not already allocated
>>  			//
>> @@ -310,7 +318,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
>>  			}
>>  			ss = (sc1200_saved_state_t *)hwif->config_data;
>>  			//
>> -			// Save timing registers:  this may be unnecessary if 
>> +			// Save timing registers:  this may be unnecessary if
>>  			// BIOS also does it
>>  			//
>>  			basereg = hwif->channel ? 0x50 : 0x40;
> 
> Please take a close look at the line above and the next three lines:
> 
> for (r = 0; r < 4; ++r) {
> 	pci_read_config_dword (hwif->pci_dev, basereg + (r<<2), &ss->regs[r]);
> }
> 
> It is highly obfuscated but the sc1200_suspend() reads 16 bytes from
> the offset 0x40 (for the primary port) and puts them in the corresponding
> struct sc1200_saved_state_s buffer, then it reads another 16 bytes from the
> offset 0x50 (for the secondary port) and puts it in the another buffer.
> 
> In summary sc1200_suspend() reads 32 continuous bytes from offset 0x40
> and the exactly reverse operation happens in sc1200_resume().
> 
> Given that and the fact that struct sc1200_save_state_s buffers are used
> _only_ by sc1200_{suspend,resume}() we may safely convert the code to use
> one buffer for both ports (the whole PCI device).  We just need to bump
> the size of struct sc1200_saved_state_s (from 4 to 8 double-words) and use
> pci_{get,set}_drvdata() instead of hwif->config_data.  Then we can remove
> looping over interfaces completely from sc1200_{suspend,resume}()! :)

May I assume you'll handle that task?  :)  It sounds like you have a far 
better idea than mine, and my main goal -- fixing bugs and warning -- is 
accomplished anyway with the merging of patch #3.

	Jeff

Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop

[Bartlomiej Zolnierkiewicz]

On Friday 26 October 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 25 October 2007, Jeff Garzik wrote:
> >> Store our hwif indices at probe time, in order to eliminate a needless
> >> and ugly loop across all hwifs, searching for our pci device.
> > 
> > It seems that we can simplify it even further and remove knowledge about
> > hwifs altogether from suspend/resume methods.
> > 
> >> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> >> ---
> >>  drivers/ide/pci/sc1200.c |   76 +++++++++++++++++++++++++++++++---------------
> >>  1 files changed, 51 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c
> >> index d2c8b55..17e58d6 100644
> >> --- a/drivers/ide/pci/sc1200.c
> >> +++ b/drivers/ide/pci/sc1200.c
> >> @@ -41,6 +41,8 @@
> >>  #define PCI_CLK_66	0x02
> >>  #define PCI_CLK_33A	0x03
> >>  
> >> +#define SC1200_IFS	4
> >> +
> >>  static unsigned short sc1200_get_pci_clock (void)
> >>  {
> >>  	unsigned char chip_id, silicon_revision;
> >> @@ -261,31 +263,32 @@ static void sc1200_set_pio_mode(ide_drive_t *drive, const u8 pio)
> >>  }
> >>  
> >>  #ifdef CONFIG_PM
> >> -static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev)
> >> -{
> >> -	int	h;
> >> -
> >> -	for (h = 0; h < MAX_HWIFS; h++) {
> >> -		ide_hwif_t *hwif = &ide_hwifs[h];
> >> -		if (prev) {
> >> -			if (hwif == prev)
> >> -				prev = NULL;	// found previous, now look for next match
> >> -		} else {
> >> -			if (hwif && hwif->pci_dev == dev)
> >> -				return hwif;	// found next match
> >> -		}
> >> -	}
> >> -	return NULL;	// not found
> >> -}
> >> -
> >>  typedef struct sc1200_saved_state_s {
> >>  	__u32		regs[4];
> >>  } sc1200_saved_state_t;
> >>  
> >> +static unsigned int pack_hwif_idx(u8 *idx)
> >> +{
> >> +	return	(((unsigned int) idx[0]) << 0) |
> >> +		(((unsigned int) idx[1]) << 8) |
> >> +		(((unsigned int) idx[2]) << 16) |
> >> +		(((unsigned int) idx[3]) << 24);
> >> +}
> >> +
> >> +static ide_hwif_t *sc1200_hwif(struct pci_dev *pdev, unsigned int iface)
> >> +{
> >> +	unsigned int packed_hwifs, idx;
> >> +
> >> +	packed_hwifs = (unsigned long) pci_get_drvdata(pdev);
> >> +	idx = (packed_hwifs >> (iface * 8)) & 0xff;
> >> +
> >> +	return (idx == 0xff) ? NULL : &ide_hwifs[idx];
> >> +}
> >>  
> >>  static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
> >>  {
> >> -	ide_hwif_t		*hwif = NULL;
> >> +	ide_hwif_t		*hwif;
> >> +	int			i;
> >>  
> >>  	printk("SC1200: suspend(%u)\n", state.event);
> >>  
> >> @@ -295,9 +298,14 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
> >>  		//
> >>  		// Loop over all interfaces that are part of this PCI device:
> >>  		//
> >> -		while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
> >> +		for (i = 0; i < SC1200_IFS; i++) {
> >>  			sc1200_saved_state_t	*ss;
> >>  			unsigned int		basereg, r;
> >> +
> >> +			hwif = sc1200_hwif(dev, i);
> >> +			if (!hwif)
> >> +				continue;
> >> +
> >>  			//
> >>  			// allocate a permanent save area, if not already allocated
> >>  			//
> >> @@ -310,7 +318,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
> >>  			}
> >>  			ss = (sc1200_saved_state_t *)hwif->config_data;
> >>  			//
> >> -			// Save timing registers:  this may be unnecessary if 
> >> +			// Save timing registers:  this may be unnecessary if
> >>  			// BIOS also does it
> >>  			//
> >>  			basereg = hwif->channel ? 0x50 : 0x40;
> > 
> > Please take a close look at the line above and the next three lines:
> > 
> > for (r = 0; r < 4; ++r) {
> > 	pci_read_config_dword (hwif->pci_dev, basereg + (r<<2), &ss->regs[r]);
> > }
> > 
> > It is highly obfuscated but the sc1200_suspend() reads 16 bytes from
> > the offset 0x40 (for the primary port) and puts them in the corresponding
> > struct sc1200_saved_state_s buffer, then it reads another 16 bytes from the
> > offset 0x50 (for the secondary port) and puts it in the another buffer.
> > 
> > In summary sc1200_suspend() reads 32 continuous bytes from offset 0x40
> > and the exactly reverse operation happens in sc1200_resume().
> > 
> > Given that and the fact that struct sc1200_save_state_s buffers are used
> > _only_ by sc1200_{suspend,resume}() we may safely convert the code to use
> > one buffer for both ports (the whole PCI device).  We just need to bump
> > the size of struct sc1200_saved_state_s (from 4 to 8 double-words) and use
> > pci_{get,set}_drvdata() instead of hwif->config_data.  Then we can remove
> > looping over interfaces completely from sc1200_{suspend,resume}()! :)
> 
> May I assume you'll handle that task?  :)  It sounds like you have a far 
> better idea than mine, and my main goal -- fixing bugs and warning -- is 
> accomplished anyway with the merging of patch #3.

Uh, OK...

[PATCH] sc1200: remove pointless hwif lookup loop

Save PCI regs values for both IDE ports in one buffer, in order to eliminate
a needless and ugly loop across all hwifs, searching for our PCI device.

Partially based on the previous patch by Jeff Garzik.

Cc: Jeff Garzik <jeff@garzik.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/pci/sc1200.c |  106 ++++++++++++++++-------------------------------
 1 file changed, 37 insertions(+), 69 deletions(-)

Index: b/drivers/ide/pci/sc1200.c
===================================================================
--- a/drivers/ide/pci/sc1200.c
+++ b/drivers/ide/pci/sc1200.c
@@ -261,66 +261,39 @@ static void sc1200_set_pio_mode(ide_driv
 }
 
 #ifdef CONFIG_PM
-static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev)
-{
-	int	h;
-
-	for (h = 0; h < MAX_HWIFS; h++) {
-		ide_hwif_t *hwif = &ide_hwifs[h];
-		if (prev) {
-			if (hwif == prev)
-				prev = NULL;	// found previous, now look for next match
-		} else {
-			if (hwif && hwif->pci_dev == dev)
-				return hwif;	// found next match
-		}
-	}
-	return NULL;	// not found
-}
-
-typedef struct sc1200_saved_state_s {
-	__u32		regs[4];
-} sc1200_saved_state_t;
-
+struct sc1200_saved_state {
+	u32 regs[8];
+};
 
 static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)
 {
-	ide_hwif_t		*hwif = NULL;
-
 	printk("SC1200: suspend(%u)\n", state.event);
 
+	/*
+	 * we only save state when going from full power to less
+	 */
 	if (state.event == PM_EVENT_ON) {
-		// we only save state when going from full power to less
+		struct sc1200_saved_state *ss;
+		unsigned int r;
 
-		//
-		// Loop over all interfaces that are part of this PCI device:
-		//
-		while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
-			sc1200_saved_state_t	*ss;
-			unsigned int		basereg, r;
-			//
-			// allocate a permanent save area, if not already allocated
-			//
-			ss = (sc1200_saved_state_t *)hwif->config_data;
-			if (ss == NULL) {
-				ss = kmalloc(sizeof(sc1200_saved_state_t), GFP_KERNEL);
-				if (ss == NULL)
-					return -ENOMEM;
-				hwif->config_data = (unsigned long)ss;
-			}
-			ss = (sc1200_saved_state_t *)hwif->config_data;
-			//
-			// Save timing registers:  this may be unnecessary if 
-			// BIOS also does it
-			//
-			basereg = hwif->channel ? 0x50 : 0x40;
-			for (r = 0; r < 4; ++r) {
-				pci_read_config_dword (hwif->pci_dev, basereg + (r<<2), &ss->regs[r]);
-			}
+		/*
+		 * allocate a permanent save area, if not already allocated
+		 */
+		ss = (struct sc1200_saved_state *)pci_get_drvdata(dev);
+		if (ss == NULL) {
+			ss = kmalloc(sizeof(*ss), GFP_KERNEL);
+			if (ss == NULL)
+				return -ENOMEM;
+			pci_set_drvdata(dev, ss);
 		}
-	}
 
-	/* You don't need to iterate over disks -- sysfs should have done that for you already */ 
+		/*
+		 * save timing registers
+		 * (this may be unnecessary if BIOS also does it)
+		 */
+		for (r = 0; r < 8; r++)
+			pci_read_config_dword(dev, 0x40 + r * 4, &ss->regs[r]);
+	}
 
 	pci_disable_device(dev);
 	pci_set_power_state(dev, pci_choose_state(dev, state));
@@ -329,30 +302,25 @@ static int sc1200_suspend (struct pci_de
 
 static int sc1200_resume (struct pci_dev *dev)
 {
-	ide_hwif_t	*hwif = NULL;
-	int		i;
+	struct sc1200_saved_state *ss;
+	unsigned int r;
+	int i;
 
 	i = pci_enable_device(dev);
 	if (i)
 		return i;
 
-	//
-	// loop over all interfaces that are part of this pci device:
-	//
-	while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) {
-		unsigned int		basereg, r;
-		sc1200_saved_state_t	*ss = (sc1200_saved_state_t *)hwif->config_data;
-
-		//
-		// Restore timing registers:  this may be unnecessary if BIOS also does it
-		//
-		basereg = hwif->channel ? 0x50 : 0x40;
-		if (ss != NULL) {
-			for (r = 0; r < 4; ++r) {
-				pci_write_config_dword(hwif->pci_dev, basereg + (r<<2), ss->regs[r]);
-			}
-		}
+	ss = (struct sc1200_saved_state *)pci_get_drvdata(dev);
+
+	/*
+	 * restore timing registers
+	 * (this may be unnecessary if BIOS also does it)
+	 */
+	if (ss) {
+		for (r = 0; r < 8; r++)
+			pci_write_config_dword(dev, 0x40 + r * 4, ss->regs[r]);
 	}
+
 	return 0;
 }
 #endif