[PATCH] i2c-piix4: support multiple PIIX4 SMBus hosts

[PATCH] i2c-piix4: support multiple PIIX4 SMBus hosts

[Andrew Armenia]

Some AMD chipsets have a second PIIX4-compatible host adapter accessible
through a second set of registers (e.g. SP5100). Moved the global base
address variable to an extension of struct i2c_adapter; added logic
to detect chipset known to have this feature. Tested on ASUS KCMA-D8 board.

Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
---
 drivers/i2c/busses/i2c-piix4.c |  242 ++++++++++++++++++++++++++++------------
 1 file changed, 170 insertions(+), 72 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c14d48d..a029a47 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -43,24 +43,25 @@
 
 
 /* PIIX4 SMBus address offsets */
-#define SMBHSTSTS	(0 + piix4_smba)
-#define SMBHSLVSTS	(1 + piix4_smba)
-#define SMBHSTCNT	(2 + piix4_smba)
-#define SMBHSTCMD	(3 + piix4_smba)
-#define SMBHSTADD	(4 + piix4_smba)
-#define SMBHSTDAT0	(5 + piix4_smba)
-#define SMBHSTDAT1	(6 + piix4_smba)
-#define SMBBLKDAT	(7 + piix4_smba)
-#define SMBSLVCNT	(8 + piix4_smba)
-#define SMBSHDWCMD	(9 + piix4_smba)
-#define SMBSLVEVT	(0xA + piix4_smba)
-#define SMBSLVDAT	(0xC + piix4_smba)
+#define SMBHSTSTS	(0 + piix4_adap->smba)
+#define SMBHSLVSTS	(1 + piix4_adap->smba)
+#define SMBHSTCNT	(2 + piix4_adap->smba)
+#define SMBHSTCMD	(3 + piix4_adap->smba)
+#define SMBHSTADD	(4 + piix4_adap->smba)
+#define SMBHSTDAT0	(5 + piix4_adap->smba)
+#define SMBHSTDAT1	(6 + piix4_adap->smba)
+#define SMBBLKDAT	(7 + piix4_adap->smba)
+#define SMBSLVCNT	(8 + piix4_adap->smba)
+#define SMBSHDWCMD	(9 + piix4_adap->smba)
+#define SMBSLVEVT	(0xA + piix4_adap->smba)
+#define SMBSLVDAT	(0xC + piix4_adap->smba)
 
 /* count for request_region */
 #define SMBIOSIZE	8
 
 /* PCI Address Constants */
 #define SMBBA		0x090
+#define SMBAUXBA        0x058
 #define SMBHSTCFG	0x0D2
 #define SMBSLVC		0x0D3
 #define SMBSHDW1	0x0D4
@@ -94,10 +95,16 @@ MODULE_PARM_DESC(force_addr,
 		 "Forcibly enable the PIIX4 at the given address. "
 		 "EXTREMELY DANGEROUS!");
 
-static unsigned short piix4_smba;
 static int srvrworks_csb5_delay;
 static struct pci_driver piix4_driver;
-static struct i2c_adapter piix4_adapter;
+
+struct piix4_adapter {
+	struct i2c_adapter i2c_adapter;
+	unsigned short smba;
+};
+
+static struct piix4_adapter piix4_main_adapter;
+static struct piix4_adapter piix4_aux_adapter;
 
 static struct dmi_system_id __devinitdata piix4_dmi_blacklist[] = {
 	{
@@ -128,7 +135,9 @@ static struct dmi_system_id __devinitdata piix4_dmi_ibm[] = {
 };
 
 static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
-				const struct pci_device_id *id)
+				const struct pci_device_id *id,
+				struct piix4_adapter *adp)
+
 {
 	unsigned char temp;
 
@@ -155,12 +164,12 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 
 	/* Determine the address of the SMBus areas */
 	if (force_addr) {
-		piix4_smba = force_addr & 0xfff0;
+		adp->smba = force_addr & 0xfff0;
 		force = 0;
 	} else {
-		pci_read_config_word(PIIX4_dev, SMBBA, &piix4_smba);
-		piix4_smba &= 0xfff0;
-		if(piix4_smba == 0) {
+		pci_read_config_word(PIIX4_dev, SMBBA, &adp->smba);
+		adp->smba &= 0xfff0;
+		if (adp->smba == 0) {
 			dev_err(&PIIX4_dev->dev, "SMBus base address "
 				"uninitialized - upgrade BIOS or use "
 				"force_addr=0xaddr\n");
@@ -168,12 +177,12 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 		}
 	}
 
-	if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
+	if (acpi_check_region(adp->smba, SMBIOSIZE, piix4_driver.name))
 		return -ENODEV;
 
-	if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
+	if (!request_region(adp->smba, SMBIOSIZE, piix4_driver.name)) {
 		dev_err(&PIIX4_dev->dev, "SMBus region 0x%x already in use!\n",
-			piix4_smba);
+			adp->smba);
 		return -EBUSY;
 	}
 
@@ -183,10 +192,10 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 	   sure, we disable the PIIX4 first. */
 	if (force_addr) {
 		pci_write_config_byte(PIIX4_dev, SMBHSTCFG, temp & 0xfe);
-		pci_write_config_word(PIIX4_dev, SMBBA, piix4_smba);
+		pci_write_config_word(PIIX4_dev, SMBBA, adp->smba);
 		pci_write_config_byte(PIIX4_dev, SMBHSTCFG, temp | 0x01);
 		dev_info(&PIIX4_dev->dev, "WARNING: SMBus interface set to "
-			"new address %04x!\n", piix4_smba);
+			"new address %04x!\n", adp->smba);
 	} else if ((temp & 1) == 0) {
 		if (force) {
 			/* This should never need to be done, but has been
@@ -205,8 +214,8 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 		} else {
 			dev_err(&PIIX4_dev->dev,
 				"Host SMBus controller not enabled!\n");
-			release_region(piix4_smba, SMBIOSIZE);
-			piix4_smba = 0;
+			release_region(adp->smba, SMBIOSIZE);
+			adp->smba = 0;
 			return -ENODEV;
 		}
 	}
@@ -222,13 +231,46 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 	pci_read_config_byte(PIIX4_dev, SMBREV, &temp);
 	dev_info(&PIIX4_dev->dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
-		 piix4_smba, temp);
+		 adp->smba, temp);
+
+	return 0;
+}
+
+static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
+				const struct pci_device_id *id,
+				struct piix4_adapter *adp,
+				unsigned short base_reg_addr)
+{
+	/* Read address of auxiliary SMBus controller */
+	pci_read_config_word(PIIX4_dev, base_reg_addr, &adp->smba);
+	adp->smba &= 0xffe0;
+
+	if (adp->smba == 0) {
+		dev_err(&PIIX4_dev->dev, "Aux SMBus base address "
+			"uninitialized - upgrade BIOS\n");
+		return -ENODEV;
+	}
+
+	if (acpi_check_region(adp->smba, SMBIOSIZE, piix4_driver.name))
+		return -ENODEV;
+
+	if (!request_region(adp->smba, SMBIOSIZE, piix4_driver.name)) {
+		dev_err(&PIIX4_dev->dev, "Aux SMBus region 0x%x already"
+			" in use!\n", adp->smba);
+		return -EBUSY;
+	}
+
+	dev_info(&PIIX4_dev->dev,
+		"Auxiliary SMBus Host Controller at 0x%x\n",
+		adp->smba
+	);
 
 	return 0;
 }
 
 static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
-				const struct pci_device_id *id)
+				const struct pci_device_id *id,
+				struct piix4_adapter *adp)
 {
 	unsigned short smba_idx = 0xcd6;
 	u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
@@ -258,26 +300,26 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 		return -ENODEV;
 	}
 
-	piix4_smba = ((smba_en_hi << 8) | smba_en_lo) & 0xffe0;
-	if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
+	adp->smba = ((smba_en_hi << 8) | smba_en_lo) & 0xffe0;
+	if (acpi_check_region(adp->smba, SMBIOSIZE, piix4_driver.name))
 		return -ENODEV;
 
-	if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
+	if (!request_region(adp->smba, SMBIOSIZE, piix4_driver.name)) {
 		dev_err(&PIIX4_dev->dev, "SMBus region 0x%x already in use!\n",
-			piix4_smba);
+			adp->smba);
 		return -EBUSY;
 	}
 
 	/* Request the SMBus I2C bus config region */
-	if (!request_region(piix4_smba + i2ccfg_offset, 1, "i2ccfg")) {
+	if (!request_region(adp->smba + i2ccfg_offset, 1, "i2ccfg")) {
 		dev_err(&PIIX4_dev->dev, "SMBus I2C bus config region "
-			"0x%x already in use!\n", piix4_smba + i2ccfg_offset);
-		release_region(piix4_smba, SMBIOSIZE);
-		piix4_smba = 0;
+			"0x%x already in use!\n", adp->smba + i2ccfg_offset);
+		release_region(adp->smba, SMBIOSIZE);
+		adp->smba = 0;
 		return -EBUSY;
 	}
-	i2ccfg = inb_p(piix4_smba + i2ccfg_offset);
-	release_region(piix4_smba + i2ccfg_offset, 1);
+	i2ccfg = inb_p(adp->smba + i2ccfg_offset);
+	release_region(adp->smba + i2ccfg_offset, 1);
 
 	if (i2ccfg & 1)
 		dev_dbg(&PIIX4_dev->dev, "Using IRQ for SMBus.\n");
@@ -286,32 +328,35 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 
 	dev_info(&PIIX4_dev->dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
-		 piix4_smba, i2ccfg >> 4);
+		 adp->smba, i2ccfg >> 4);
 
 	return 0;
 }
 
-static int piix4_transaction(void)
+static int piix4_transaction(struct piix4_adapter *piix4_adap)
 {
 	int temp;
 	int result = 0;
 	int timeout = 0;
 
-	dev_dbg(&piix4_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
+	dev_dbg(&piix4_adap->i2c_adapter.dev,
+		"Transaction (pre): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
 		inb_p(SMBHSTDAT1));
 
 	/* Make sure the SMBus host is ready to start transmitting */
 	if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-		dev_dbg(&piix4_adapter.dev, "SMBus busy (%02x). "
+		dev_dbg(&piix4_adap->i2c_adapter.dev, "SMBus busy (%02x). "
 			"Resetting...\n", temp);
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-			dev_err(&piix4_adapter.dev, "Failed! (%02x)\n", temp);
+			dev_err(&piix4_adap->i2c_adapter.dev,
+				"Failed! (%02x)\n", temp);
 			return -EBUSY;
 		} else {
-			dev_dbg(&piix4_adapter.dev, "Successful!\n");
+			dev_dbg(&piix4_adap->i2c_adapter.dev,
+				"Successful!\n");
 		}
 	}
 
@@ -330,35 +375,39 @@ static int piix4_transaction(void)
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout == MAX_TIMEOUT) {
-		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
+		dev_err(&piix4_adap->i2c_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
 		result = -EIO;
-		dev_err(&piix4_adapter.dev, "Error: Failed bus transaction\n");
+		dev_err(&piix4_adap->i2c_adapter.dev,
+			"Error: Failed bus transaction\n");
 	}
 
 	if (temp & 0x08) {
 		result = -EIO;
-		dev_dbg(&piix4_adapter.dev, "Bus collision! SMBus may be "
+		dev_dbg(&piix4_adap->i2c_adapter.dev,
+			"Bus collision! SMBus may be "
 			"locked until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & 0x04) {
 		result = -ENXIO;
-		dev_dbg(&piix4_adapter.dev, "Error: no response!\n");
+		dev_dbg(&piix4_adap->i2c_adapter.dev,
+			"Error: no response!\n");
 	}
 
 	if (inb_p(SMBHSTSTS) != 0x00)
 		outb_p(inb(SMBHSTSTS), SMBHSTSTS);
 
 	if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-		dev_err(&piix4_adapter.dev, "Failed reset at end of "
-			"transaction (%02x)\n", temp);
+		dev_err(&piix4_adap->i2c_adapter.dev,
+			"Failed reset at end of transaction (%02x)\n", temp);
 	}
-	dev_dbg(&piix4_adapter.dev, "Transaction (post): CNT=%02x, CMD=%02x, "
+	dev_dbg(&piix4_adap->i2c_adapter.dev,
+		"Transaction (post): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
 		inb_p(SMBHSTDAT1));
@@ -373,6 +422,9 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 	int i, len;
 	int status;
 
+	struct piix4_adapter *piix4_adap = container_of(adap,
+		struct piix4_adapter, i2c_adapter);
+
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		outb_p((addr << 1) | read_write,
@@ -426,7 +478,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	status = piix4_transaction();
+	status = piix4_transaction(piix4_adap);
 	if (status)
 		return status;
 
@@ -466,12 +518,6 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= piix4_func,
 };
 
-static struct i2c_adapter piix4_adapter = {
-	.owner		= THIS_MODULE,
-	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-	.algo		= &smbus_algorithm,
-};
-
 static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -496,45 +542,97 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
+static void piix4_adapter_init(struct piix4_adapter *adap)
+{
+	memset(adap, 0, sizeof(struct piix4_adapter));
+
+	adap->i2c_adapter.owner = THIS_MODULE;
+	adap->i2c_adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	adap->i2c_adapter.algo = &smbus_algorithm;
+}
+
 static int __devinit piix4_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
 	int retval;
 
+	piix4_adapter_init(&piix4_main_adapter);
+	piix4_adapter_init(&piix4_aux_adapter);
+
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
 	     dev->revision >= 0x40) ||
 	    dev->vendor == PCI_VENDOR_ID_AMD)
 		/* base address location etc changed in SB800 */
-		retval = piix4_setup_sb800(dev, id);
+		retval = piix4_setup_sb800(dev, id, &piix4_main_adapter);
 	else
-		retval = piix4_setup(dev, id);
+		retval = piix4_setup(dev, id, &piix4_main_adapter);
 
 	if (retval)
 		return retval;
 
 	/* set up the sysfs linkage to our parent device */
-	piix4_adapter.dev.parent = &dev->dev;
+	piix4_main_adapter.i2c_adapter.dev.parent = &dev->dev;
 
-	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
-		"SMBus PIIX4 adapter at %04x", piix4_smba);
+	snprintf(piix4_main_adapter.i2c_adapter.name,
+		sizeof(piix4_main_adapter.i2c_adapter.name),
+		"SMBus PIIX4 adapter at %04x", piix4_main_adapter.smba);
 
-	if ((retval = i2c_add_adapter(&piix4_adapter))) {
+	retval = i2c_add_adapter(&piix4_main_adapter.i2c_adapter);
+	if (retval) {
 		dev_err(&dev->dev, "Couldn't register adapter!\n");
-		release_region(piix4_smba, SMBIOSIZE);
-		piix4_smba = 0;
+		release_region(piix4_main_adapter.smba, SMBIOSIZE);
+		piix4_main_adapter.smba = 0;
+		return retval;
+	}
+
+	if (dev->vendor == PCI_VENDOR_ID_ATI &&
+	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
+	    dev->revision == 0x3d) {
+		/* Setup aux SMBus port on certain AMD chipsets e.g. SP5100 */
+		retval = piix4_setup_aux(dev, id,
+			&piix4_aux_adapter, SMBAUXBA);
+
+		if (retval) {
+			dev_err(&dev->dev, "Aux SMBus setup failed!\n");
+			release_region(piix4_main_adapter.smba, SMBIOSIZE);
+			piix4_aux_adapter.smba = 0;
+			goto aux_fail;
+		}
+
+		piix4_aux_adapter.i2c_adapter.dev.parent = &dev->dev;
+		snprintf(piix4_aux_adapter.i2c_adapter.name,
+			sizeof(piix4_aux_adapter.i2c_adapter.name),
+			"SMBus PIIX4 adapter (aux) at %04x",
+			piix4_aux_adapter.smba);
+
+		retval = i2c_add_adapter(&piix4_aux_adapter.i2c_adapter);
+		if (retval) {
+			dev_err(&dev->dev,
+				"Couldn't register aux adapter!\n");
+			release_region(piix4_aux_adapter.smba, SMBIOSIZE);
+			piix4_aux_adapter.smba = 0;
+			goto aux_fail;
+		}
 	}
 
-	return retval;
+aux_fail:
+	return 0;
+}
+
+static void piix4_adapter_remove(struct piix4_adapter *adp)
+{
+	if (adp->smba) {
+		i2c_del_adapter(&adp->i2c_adapter);
+		release_region(adp->smba, SMBIOSIZE);
+		adp->smba = 0;
+	}
 }
 
 static void __devexit piix4_remove(struct pci_dev *dev)
 {
-	if (piix4_smba) {
-		i2c_del_adapter(&piix4_adapter);
-		release_region(piix4_smba, SMBIOSIZE);
-		piix4_smba = 0;
-	}
+	piix4_adapter_remove(&piix4_main_adapter);
+	piix4_adapter_remove(&piix4_aux_adapter);
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.7.10

Re: [PATCH] i2c-piix4: support multiple PIIX4 SMBus hosts

[Jean Delvare]

Hi Andrew,

On Fri,  1 Jun 2012 14:16:12 -0400, Andrew Armenia wrote:
> Some AMD chipsets have a second PIIX4-compatible host adapter accessible
> through a second set of registers (e.g. SP5100). Moved the global base
> address variable to an extension of struct i2c_adapter; added logic
> to detect chipset known to have this feature. Tested on ASUS KCMA-D8 board.

This would be much easier to review if you would split this change into
two patches, one moving the per-adapter settings out of the global
scope, and one adding support for the second base address.

Furthermore, the use of container_of seems inappropriate here, as there
is a proper interface for per-adapter attributes: i2c_set_adapdata() and
i2c_get_adapdata().

-- 
Jean Delvare

[PATCH 0/3] Multiple piix4-compatible SMBus support

[Andrew Armenia]

These patches make it easier to support multiple SMBus controllers
that may be present within one piix4-compatible PCI device, and
enable support for the secondary SMBus controller found in the AMD
SP5100 chipset.

Andrew Armenia (3):
  i2c-piix4: eliminate global I/O base address
  i2c-piix4: separate registration and probing code
  i2c-piix4: support aux SMBus on AMD chipsets

 drivers/i2c/busses/i2c-piix4.c |  179 +++++++++++++++++++++++++++++++++-------
 1 file changed, 151 insertions(+), 28 deletions(-)

-- 
1.7.10

[PATCH 1/3] i2c-piix4: eliminate global I/O base address

[Andrew Armenia]

The i2c-piix4 driver used a global variable to store the base address
of the piix4 i2c registers. Some chipsets contain multiple sets of
i2c registers; thus it is desirable to eliminate global state.

i2c_get_adapdata()/i2c_set_adapdata() are used to keep track
of the IO base address; this is passed into piix4_transaction() as
an argument, eliminating the global variable.

Signed-off-by: Andrew Armenia <andrew-Lwj1yN59in/Ib2jZbfQ/kQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c |   61 ++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c14d48d..7a5889c 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -94,7 +94,6 @@ MODULE_PARM_DESC(force_addr,
 		 "Forcibly enable the PIIX4 at the given address. "
 		 "EXTREMELY DANGEROUS!");
 
-static unsigned short piix4_smba;
 static int srvrworks_csb5_delay;
 static struct pci_driver piix4_driver;
 static struct i2c_adapter piix4_adapter;
@@ -127,10 +126,16 @@ static struct dmi_system_id __devinitdata piix4_dmi_ibm[] = {
 	{ },
 };
 
+struct i2c_piix4_adapdata {
+	unsigned short smba;
+};
+
 static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
-				const struct pci_device_id *id)
+				const struct pci_device_id *id,
+				unsigned short *smba)
 {
 	unsigned char temp;
+	unsigned short piix4_smba;
 
 	if ((PIIX4_dev->vendor == PCI_VENDOR_ID_SERVERWORKS) &&
 	    (PIIX4_dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB5))
@@ -224,12 +229,15 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, temp);
 
+	*smba = piix4_smba;
 	return 0;
 }
 
 static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
-				const struct pci_device_id *id)
+				const struct pci_device_id *id,
+				unsigned short *smba)
 {
+	unsigned short piix4_smba;
 	unsigned short smba_idx = 0xcd6;
 	u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
 
@@ -288,10 +296,11 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
 
+	*smba = piix4_smba;
 	return 0;
 }
 
-static int piix4_transaction(void)
+static int piix4_transaction(unsigned short piix4_smba)
 {
 	int temp;
 	int result = 0;
@@ -372,6 +381,11 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 {
 	int i, len;
 	int status;
+	unsigned short piix4_smba;
+	struct i2c_piix4_adapdata *adapdata;
+
+	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);
+	piix4_smba = adapdata->smba;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -426,7 +440,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	status = piix4_transaction();
+	status = piix4_transaction(piix4_smba);
 	if (status)
 		return status;
 
@@ -472,6 +486,10 @@ static struct i2c_adapter piix4_adapter = {
 	.algo		= &smbus_algorithm,
 };
 
+static struct i2c_piix4_adapdata piix4_adapter_data = {
+	.smba		= 0,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -500,15 +518,16 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
 	int retval;
+	unsigned short smba;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
 	     dev->revision >= 0x40) ||
 	    dev->vendor == PCI_VENDOR_ID_AMD)
 		/* base address location etc changed in SB800 */
-		retval = piix4_setup_sb800(dev, id);
+		retval = piix4_setup_sb800(dev, id, &smba);
 	else
-		retval = piix4_setup(dev, id);
+		retval = piix4_setup(dev, id, &smba);
 
 	if (retval)
 		return retval;
@@ -517,26 +536,38 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 	piix4_adapter.dev.parent = &dev->dev;
 
 	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
-		"SMBus PIIX4 adapter at %04x", piix4_smba);
+		"SMBus PIIX4 adapter at %04x", smba);
+
+	piix4_adapter_data.smba = smba;
+
+	i2c_set_adapdata(&piix4_adapter, &piix4_adapter_data);
 
 	if ((retval = i2c_add_adapter(&piix4_adapter))) {
 		dev_err(&dev->dev, "Couldn't register adapter!\n");
-		release_region(piix4_smba, SMBIOSIZE);
-		piix4_smba = 0;
+		release_region(smba, SMBIOSIZE);
+		piix4_adapter_data.smba = 0;
 	}
 
 	return retval;
 }
 
-static void __devexit piix4_remove(struct pci_dev *dev)
+static void piix4_adap_remove(struct i2c_adapter *adap)
 {
-	if (piix4_smba) {
-		i2c_del_adapter(&piix4_adapter);
-		release_region(piix4_smba, SMBIOSIZE);
-		piix4_smba = 0;
+	struct i2c_piix4_adapdata *adapdata;
+
+	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);
+	if (adapdata->smba) {
+		i2c_del_adapter(adap);
+		release_region(adapdata->smba, SMBIOSIZE);
+		adapdata->smba = 0;
 	}
 }
 
+static void __devexit piix4_remove(struct pci_dev *dev)
+{
+	piix4_adap_remove(&piix4_adapter);
+}
+
 static struct pci_driver piix4_driver = {
 	.name		= "piix4_smbus",
 	.id_table	= piix4_ids,
-- 
1.7.10

Re: [PATCH 1/3] i2c-piix4: eliminate global I/O base address

[Jean Delvare]

Hi Andrew,

On Mon,  4 Jun 2012 21:49:22 -0400, Andrew Armenia wrote:
> The i2c-piix4 driver used a global variable to store the base address
> of the piix4 i2c registers. Some chipsets contain multiple sets of
> i2c registers; thus it is desirable to eliminate global state.
> 
> i2c_get_adapdata()/i2c_set_adapdata() are used to keep track
> of the IO base address; this is passed into piix4_transaction() as
> an argument, eliminating the global variable.
> 
> Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   61 ++++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index c14d48d..7a5889c 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -94,7 +94,6 @@ MODULE_PARM_DESC(force_addr,
>  		 "Forcibly enable the PIIX4 at the given address. "
>  		 "EXTREMELY DANGEROUS!");
>  
> -static unsigned short piix4_smba;
>  static int srvrworks_csb5_delay;
>  static struct pci_driver piix4_driver;
>  static struct i2c_adapter piix4_adapter;
> @@ -127,10 +126,16 @@ static struct dmi_system_id __devinitdata piix4_dmi_ibm[] = {
>  	{ },
>  };
>  
> +struct i2c_piix4_adapdata {
> +	unsigned short smba;
> +};
> +
>  static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
> -				const struct pci_device_id *id)
> +				const struct pci_device_id *id,
> +				unsigned short *smba)

You could instead return the smba value, as the return value is
currently only used for errors. This would avoid the extra parameter
and possibly false-positive compiler warnings.

>  {
>  	unsigned char temp;
> +	unsigned short piix4_smba;
>  
>  	if ((PIIX4_dev->vendor == PCI_VENDOR_ID_SERVERWORKS) &&
>  	    (PIIX4_dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB5))
> @@ -224,12 +229,15 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
>  		 "SMBus Host Controller at 0x%x, revision %d\n",
>  		 piix4_smba, temp);
>  
> +	*smba = piix4_smba;
>  	return 0;
>  }
>  
>  static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> -				const struct pci_device_id *id)
> +				const struct pci_device_id *id,
> +				unsigned short *smba)
>  {
> +	unsigned short piix4_smba;
>  	unsigned short smba_idx = 0xcd6;
>  	u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
>  
> @@ -288,10 +296,11 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  		 "SMBus Host Controller at 0x%x, revision %d\n",
>  		 piix4_smba, i2ccfg >> 4);
>  
> +	*smba = piix4_smba;
>  	return 0;
>  }
>  
> -static int piix4_transaction(void)
> +static int piix4_transaction(unsigned short piix4_smba)
>  {
>  	int temp;
>  	int result = 0;
> @@ -372,6 +381,11 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>  {
>  	int i, len;
>  	int status;
> +	unsigned short piix4_smba;
> +	struct i2c_piix4_adapdata *adapdata;
> +
> +	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);

Useless cast.

> +	piix4_smba = adapdata->smba;
>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> @@ -426,7 +440,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>  
>  	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
>  
> -	status = piix4_transaction();
> +	status = piix4_transaction(piix4_smba);
>  	if (status)
>  		return status;
>  
> @@ -472,6 +486,10 @@ static struct i2c_adapter piix4_adapter = {
>  	.algo		= &smbus_algorithm,
>  };
>  
> +static struct i2c_piix4_adapdata piix4_adapter_data = {
> +	.smba		= 0,
> +};

Useless initialization, 0 is the default.

> +
>  static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
> @@ -500,15 +518,16 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
>  	int retval;
> +	unsigned short smba;
>  
>  	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
>  	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>  	     dev->revision >= 0x40) ||
>  	    dev->vendor == PCI_VENDOR_ID_AMD)
>  		/* base address location etc changed in SB800 */
> -		retval = piix4_setup_sb800(dev, id);
> +		retval = piix4_setup_sb800(dev, id, &smba);
>  	else
> -		retval = piix4_setup(dev, id);
> +		retval = piix4_setup(dev, id, &smba);
>  
>  	if (retval)
>  		return retval;
> @@ -517,26 +536,38 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>  	piix4_adapter.dev.parent = &dev->dev;
>  
>  	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
> -		"SMBus PIIX4 adapter at %04x", piix4_smba);
> +		"SMBus PIIX4 adapter at %04x", smba);
> +
> +	piix4_adapter_data.smba = smba;
> +
> +	i2c_set_adapdata(&piix4_adapter, &piix4_adapter_data);
>  
>  	if ((retval = i2c_add_adapter(&piix4_adapter))) {
>  		dev_err(&dev->dev, "Couldn't register adapter!\n");
> -		release_region(piix4_smba, SMBIOSIZE);
> -		piix4_smba = 0;
> +		release_region(smba, SMBIOSIZE);
> +		piix4_adapter_data.smba = 0;
>  	}
>  
>  	return retval;
>  }
>  
> -static void __devexit piix4_remove(struct pci_dev *dev)
> +static void piix4_adap_remove(struct i2c_adapter *adap)

Called from a __devinit function so it can be made __devinit too.

>  {
> -	if (piix4_smba) {
> -		i2c_del_adapter(&piix4_adapter);
> -		release_region(piix4_smba, SMBIOSIZE);
> -		piix4_smba = 0;
> +	struct i2c_piix4_adapdata *adapdata;
> +
> +	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);

Useless cast.

> +	if (adapdata->smba) {
> +		i2c_del_adapter(adap);
> +		release_region(adapdata->smba, SMBIOSIZE);
> +		adapdata->smba = 0;
>  	}
>  }
>  
> +static void __devexit piix4_remove(struct pci_dev *dev)
> +{
> +	piix4_adap_remove(&piix4_adapter);
> +}
> +
>  static struct pci_driver piix4_driver = {
>  	.name		= "piix4_smbus",
>  	.id_table	= piix4_ids,


-- 
Jean Delvare

[PATCH 2/3] i2c-piix4: separate registration and probing code

[Andrew Armenia]

Some chipsets have multiple sets of SMBus registers each controlling a
separate SMBus. Supporting these chipsets properly will require registering
multiple I2C adapters for one piix4.

The code to initialize and register the i2c_adapter structure has been
separated from piix4_probe and allows registration of a piix4 adapter
given its base address. Note that the i2c_adapter and piix4_adapdata
structures are now dynamically allocated.

Signed-off-by: Andrew Armenia <andrew-Lwj1yN59in/Ib2jZbfQ/kQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c |   79 ++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 7a5889c..8a87b3f 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -480,16 +480,6 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= piix4_func,
 };
 
-static struct i2c_adapter piix4_adapter = {
-	.owner		= THIS_MODULE,
-	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-	.algo		= &smbus_algorithm,
-};
-
-static struct i2c_piix4_adapdata piix4_adapter_data = {
-	.smba		= 0,
-};
-
 static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -514,6 +504,48 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
+static struct i2c_adapter *piix4_main_adapter;
+
+/* register piix4 I2C adapter at the given base address */
+static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
+				struct i2c_adapter **padap) {
+	struct i2c_adapter *piix4_adapter;
+	struct i2c_piix4_adapdata *piix4_adapdata;
+	int retval;
+
+	piix4_adapter = kmalloc(sizeof(*piix4_adapter), GFP_KERNEL);
+	memset(piix4_adapter, 0, sizeof(*piix4_adapter));
+	piix4_adapter->owner = THIS_MODULE;
+	piix4_adapter->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	piix4_adapter->algo = &smbus_algorithm;
+
+	piix4_adapdata = kmalloc(sizeof(*piix4_adapdata), GFP_KERNEL);
+	memset(piix4_adapdata, 0, sizeof(*piix4_adapdata));
+	piix4_adapdata->smba = smba;
+
+	/* set up the sysfs linkage to our parent device */
+	piix4_adapter->dev.parent = &dev->dev;
+
+	snprintf(piix4_adapter->name, sizeof(piix4_adapter->name),
+		"SMBus PIIX4 adapter at %04x", smba);
+
+	piix4_adapdata->smba = smba;
+
+	i2c_set_adapdata(piix4_adapter, piix4_adapdata);
+
+	retval = i2c_add_adapter(piix4_adapter);
+	if (retval) {
+		dev_err(&dev->dev, "Couldn't register adapter!\n");
+		release_region(smba, SMBIOSIZE);
+		kfree(piix4_adapdata);
+		kfree(piix4_adapter);
+	}
+
+	*padap = piix4_adapter;
+
+	return retval;
+}
+
 static int __devinit piix4_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
@@ -532,25 +564,10 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 	if (retval)
 		return retval;
 
-	/* set up the sysfs linkage to our parent device */
-	piix4_adapter.dev.parent = &dev->dev;
-
-	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
-		"SMBus PIIX4 adapter at %04x", smba);
-
-	piix4_adapter_data.smba = smba;
-
-	i2c_set_adapdata(&piix4_adapter, &piix4_adapter_data);
-
-	if ((retval = i2c_add_adapter(&piix4_adapter))) {
-		dev_err(&dev->dev, "Couldn't register adapter!\n");
-		release_region(smba, SMBIOSIZE);
-		piix4_adapter_data.smba = 0;
-	}
-
-	return retval;
+	return piix4_add_adapter(dev, smba, &piix4_main_adapter);
 }
 
+/* Remove the adapter and its associated IO region */
 static void piix4_adap_remove(struct i2c_adapter *adap)
 {
 	struct i2c_piix4_adapdata *adapdata;
@@ -561,11 +578,17 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 		release_region(adapdata->smba, SMBIOSIZE);
 		adapdata->smba = 0;
 	}
+
+	kfree(adapdata);
+	kfree(adap);
 }
 
 static void __devexit piix4_remove(struct pci_dev *dev)
 {
-	piix4_adap_remove(&piix4_adapter);
+	if (piix4_main_adapter) {
+		piix4_adap_remove(piix4_main_adapter);
+		piix4_main_adapter = NULL;
+	}
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.7.10

Re: [PATCH 2/3] i2c-piix4: separate registration and probing code

[Jean Delvare]

On Mon,  4 Jun 2012 21:49:23 -0400, Andrew Armenia wrote:
> Some chipsets have multiple sets of SMBus registers each controlling a
> separate SMBus. Supporting these chipsets properly will require registering
> multiple I2C adapters for one piix4.
> 
> The code to initialize and register the i2c_adapter structure has been
> separated from piix4_probe and allows registration of a piix4 adapter
> given its base address. Note that the i2c_adapter and piix4_adapdata
> structures are now dynamically allocated.
> 
> Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   79 ++++++++++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 7a5889c..8a87b3f 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -480,16 +480,6 @@ static const struct i2c_algorithm smbus_algorithm = {
>  	.functionality	= piix4_func,
>  };
>  
> -static struct i2c_adapter piix4_adapter = {
> -	.owner		= THIS_MODULE,
> -	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -	.algo		= &smbus_algorithm,
> -};
> -
> -static struct i2c_piix4_adapdata piix4_adapter_data = {
> -	.smba		= 0,
> -};
> -
>  static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
> @@ -514,6 +504,48 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
>  
>  MODULE_DEVICE_TABLE (pci, piix4_ids);
>  
> +static struct i2c_adapter *piix4_main_adapter;
> +
> +/* register piix4 I2C adapter at the given base address */
> +static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> +				struct i2c_adapter **padap) {

Called from a __devinit function, could be made __devinit.

> +	struct i2c_adapter *piix4_adapter;
> +	struct i2c_piix4_adapdata *piix4_adapdata;

It would be pleasant to use the same names here and in
piix4_adap_remove(), for consistency.

> +	int retval;
> +
> +	piix4_adapter = kmalloc(sizeof(*piix4_adapter), GFP_KERNEL);
> +	memset(piix4_adapter, 0, sizeof(*piix4_adapter));

Please use kzalloc. Furthermore, k*alloc() can theoretically fail, so
you have to check for this unlikely case and handle it gracefully.

> +	piix4_adapter->owner = THIS_MODULE;
> +	piix4_adapter->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	piix4_adapter->algo = &smbus_algorithm;
> +
> +	piix4_adapdata = kmalloc(sizeof(*piix4_adapdata), GFP_KERNEL);
> +	memset(piix4_adapdata, 0, sizeof(*piix4_adapdata));

Same here.

> +	piix4_adapdata->smba = smba;
> +
> +	/* set up the sysfs linkage to our parent device */
> +	piix4_adapter->dev.parent = &dev->dev;
> +
> +	snprintf(piix4_adapter->name, sizeof(piix4_adapter->name),
> +		"SMBus PIIX4 adapter at %04x", smba);
> +
> +	piix4_adapdata->smba = smba;

You already did that a few lines above.

> +
> +	i2c_set_adapdata(piix4_adapter, piix4_adapdata);
> +
> +	retval = i2c_add_adapter(piix4_adapter);
> +	if (retval) {
> +		dev_err(&dev->dev, "Couldn't register adapter!\n");
> +		release_region(smba, SMBIOSIZE);
> +		kfree(piix4_adapdata);
> +		kfree(piix4_adapter);
> +	}
> +
> +	*padap = piix4_adapter;

It's pointless do to that in the error case.

> +
> +	return retval;
> +}
> +
>  static int __devinit piix4_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> @@ -532,25 +564,10 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>  	if (retval)
>  		return retval;
>  
> -	/* set up the sysfs linkage to our parent device */
> -	piix4_adapter.dev.parent = &dev->dev;
> -
> -	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
> -		"SMBus PIIX4 adapter at %04x", smba);
> -
> -	piix4_adapter_data.smba = smba;
> -
> -	i2c_set_adapdata(&piix4_adapter, &piix4_adapter_data);
> -
> -	if ((retval = i2c_add_adapter(&piix4_adapter))) {
> -		dev_err(&dev->dev, "Couldn't register adapter!\n");
> -		release_region(smba, SMBIOSIZE);
> -		piix4_adapter_data.smba = 0;
> -	}
> -
> -	return retval;
> +	return piix4_add_adapter(dev, smba, &piix4_main_adapter);
>  }
>  
> +/* Remove the adapter and its associated IO region */
>  static void piix4_adap_remove(struct i2c_adapter *adap)
>  {
>  	struct i2c_piix4_adapdata *adapdata;
> @@ -561,11 +578,17 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  		release_region(adapdata->smba, SMBIOSIZE);
>  		adapdata->smba = 0;

This one can go away, no point in cleaning up a structure you're about
to free.

>  	}
> +
> +	kfree(adapdata);
> +	kfree(adap);
>  }
>  
>  static void __devexit piix4_remove(struct pci_dev *dev)
>  {
> -	piix4_adap_remove(&piix4_adapter);
> +	if (piix4_main_adapter) {
> +		piix4_adap_remove(piix4_main_adapter);
> +		piix4_main_adapter = NULL;
> +	}
>  }
>  
>  static struct pci_driver piix4_driver = {


-- 
Jean Delvare

[PATCH 3/3] i2c-piix4: support aux SMBus on AMD chipsets

[Andrew Armenia]

Some AMD chipsets, such as the SP5100, have an auxiliary SMBus with a second
set of registers. This patch adds support for the SP5100 and should work on
similar chipsets. Tested on ASUS KCMA-D8 motherboard.

Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
---
 drivers/i2c/busses/i2c-piix4.c |   83 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 76 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8a87b3f..972b604 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -25,7 +25,8 @@
 	AMD Hudson-2
 	SMSC Victory66
 
-   Note: we assume there can only be one device, with one SMBus interface.
+   Note: we assume there can only be one device, with one or two SMBus
+   interfaces.
 */
 
 #include <linux/module.h>
@@ -66,6 +67,7 @@
 #define SMBSHDW1	0x0D4
 #define SMBSHDW2	0x0D5
 #define SMBREV		0x0D6
+#define SMBAUXBA	0x058
 
 /* Other settings */
 #define MAX_TIMEOUT	500
@@ -130,6 +132,8 @@ struct i2c_piix4_adapdata {
 	unsigned short smba;
 };
 
+static void piix4_adap_remove(struct i2c_adapter *adap);
+
 static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 				const struct pci_device_id *id,
 				unsigned short *smba)
@@ -300,6 +304,45 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	return 0;
 }
 
+static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
+				const struct pci_device_id *id,
+				unsigned short base_reg_addr,
+				unsigned short *smba)
+{
+	/* Set up auxiliary SMBus controllers found on some AMD
+	 * chipsets e.g. SP5100 */
+	unsigned short piix4_smba;
+
+	/* Read address of auxiliary SMBus controller */
+	pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
+	piix4_smba &= 0xffe0;
+
+	if (piix4_smba == 0) {
+		dev_err(&PIIX4_dev->dev, "Aux SMBus base address "
+			"uninitialized - upgrade BIOS\n");
+		return -ENODEV;
+	}
+
+	if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
+		return -ENODEV;
+
+	if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
+		dev_err(&PIIX4_dev->dev, "Aux SMBus region 0x%x already"
+			" in use!\n", piix4_smba);
+		return -EBUSY;
+	}
+
+	dev_info(&PIIX4_dev->dev,
+		"Auxiliary SMBus Host Controller at 0x%x\n",
+		piix4_smba
+	);
+
+	*smba = piix4_smba;
+
+	return 0;
+}
+
+
 static int piix4_transaction(unsigned short piix4_smba)
 {
 	int temp;
@@ -505,6 +548,7 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
 static struct i2c_adapter *piix4_main_adapter;
+static struct i2c_adapter *piix4_aux_adapter;
 
 /* register piix4 I2C adapter at the given base address */
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
@@ -562,9 +606,33 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 		retval = piix4_setup(dev, id, &smba);
 
 	if (retval)
-		return retval;
+		goto error;
+
+	retval = piix4_add_adapter(dev, smba, &piix4_main_adapter);
+	if (retval)
+		goto error;
 
-	return piix4_add_adapter(dev, smba, &piix4_main_adapter);
+	/* check for AMD SP5100 (maybe others) with aux SMBus port */
+	if (dev->vendor == PCI_VENDOR_ID_ATI &&
+	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
+		dev->revision == 0x3d) {
+
+		retval = piix4_setup_aux(dev, id, SMBAUXBA, &smba);
+		if (retval)
+			goto error;
+
+		retval = piix4_add_adapter(dev, smba, &piix4_aux_adapter);
+		if (retval)
+			goto error;
+	}
+
+	return 0;
+
+error:
+	/* clean up any adapters that were already added */
+	piix4_adap_remove(piix4_main_adapter);
+	piix4_adap_remove(piix4_aux_adapter);
+	return retval;
 }
 
 /* Remove the adapter and its associated IO region */
@@ -572,6 +640,9 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 {
 	struct i2c_piix4_adapdata *adapdata;
 
+	if (adap == NULL)
+		return;
+
 	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
@@ -585,10 +656,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 static void __devexit piix4_remove(struct pci_dev *dev)
 {
-	if (piix4_main_adapter) {
-		piix4_adap_remove(piix4_main_adapter);
-		piix4_main_adapter = NULL;
-	}
+	piix4_adap_remove(piix4_main_adapter);
+	piix4_adap_remove(piix4_aux_adapter);
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.7.10

Re: [PATCH 3/3] i2c-piix4: support aux SMBus on AMD chipsets

[Jean Delvare]

On Mon,  4 Jun 2012 21:49:24 -0400, Andrew Armenia wrote:
> Some AMD chipsets, such as the SP5100, have an auxiliary SMBus with a second
> set of registers. This patch adds support for the SP5100 and should work on
> similar chipsets. Tested on ASUS KCMA-D8 motherboard.

The SP5100 isn't even documented as being supported. Can you please add
it to Documentation/i2c/busses/i2c-piix4, drivers/i2c/busses/Kconfig,
and the header comment? Or is it a different code name for an already
supported south bridge? I admit I stopped following AMD chipsets some
times ago.

> 
> Signed-off-by: Andrew Armenia <andrew-Lwj1yN59in/Ib2jZbfQ/kQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   83 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 76 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 8a87b3f..972b604 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -25,7 +25,8 @@
>  	AMD Hudson-2
>  	SMSC Victory66
>  
> -   Note: we assume there can only be one device, with one SMBus interface.
> +   Note: we assume there can only be one device, with one or two SMBus
> +   interfaces.
>  */
>  
>  #include <linux/module.h>
> @@ -66,6 +67,7 @@
>  #define SMBSHDW1	0x0D4
>  #define SMBSHDW2	0x0D5
>  #define SMBREV		0x0D6
> +#define SMBAUXBA	0x058

Keeping the list sorted by register offset would seem reasonable.

>  
>  /* Other settings */
>  #define MAX_TIMEOUT	500
> @@ -130,6 +132,8 @@ struct i2c_piix4_adapdata {
>  	unsigned short smba;
>  };
>  
> +static void piix4_adap_remove(struct i2c_adapter *adap);
> +

Please just reorder the functions (ideally at the time you introduce
them) so that you don't need this forward declaration.

>  static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
>  				const struct pci_device_id *id,
>  				unsigned short *smba)
> @@ -300,6 +304,45 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  	return 0;
>  }
>  
> +static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
> +				const struct pci_device_id *id,
> +				unsigned short base_reg_addr,
> +				unsigned short *smba)
> +{
> +	/* Set up auxiliary SMBus controllers found on some AMD
> +	 * chipsets e.g. SP5100 */
> +	unsigned short piix4_smba;
> +
> +	/* Read address of auxiliary SMBus controller */
> +	pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
> +	piix4_smba &= 0xffe0;
> +
> +	if (piix4_smba == 0) {
> +		dev_err(&PIIX4_dev->dev, "Aux SMBus base address "
> +			"uninitialized - upgrade BIOS\n");

This case could be OK if the board vendor simply did not need a second
SMBus channel. So we shouldn't spit an error message in this case,
maybe a debug message if you want but that's about it.

> +		return -ENODEV;
> +	}
> +
> +	if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
> +		return -ENODEV;
> +
> +	if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
> +		dev_err(&PIIX4_dev->dev, "Aux SMBus region 0x%x already"
> +			" in use!\n", piix4_smba);

White space at end of first half please, for consistency.

> +		return -EBUSY;
> +	}
> +
> +	dev_info(&PIIX4_dev->dev,
> +		"Auxiliary SMBus Host Controller at 0x%x\n",

You should probably write "Auxiliary" in the previous message messages
too, for consistency and readability.

> +		piix4_smba
> +	);
> +
> +	*smba = piix4_smba;
> +
> +	return 0;
> +}
> +
> +
>  static int piix4_transaction(unsigned short piix4_smba)
>  {
>  	int temp;
> @@ -505,6 +548,7 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
>  MODULE_DEVICE_TABLE (pci, piix4_ids);
>  
>  static struct i2c_adapter *piix4_main_adapter;
> +static struct i2c_adapter *piix4_aux_adapter;
>  
>  /* register piix4 I2C adapter at the given base address */
>  static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> @@ -562,9 +606,33 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>  		retval = piix4_setup(dev, id, &smba);
>  
>  	if (retval)
> -		return retval;
> +		goto error;
> +
> +	retval = piix4_add_adapter(dev, smba, &piix4_main_adapter);
> +	if (retval)
> +		goto error;
>  
> -	return piix4_add_adapter(dev, smba, &piix4_main_adapter);
> +	/* check for AMD SP5100 (maybe others) with aux SMBus port */
> +	if (dev->vendor == PCI_VENDOR_ID_ATI &&
> +	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
> +		dev->revision == 0x3d) {

Hmm, that would be an ATI SB600 or SB700, so not a recent chip? This
strict check on revision is likely to exclude some systems where this
code should run. Given that the SB800 started at revision 0x40 as far
as I know, shouldn't we test for < 0x40 here?

> +
> +		retval = piix4_setup_aux(dev, id, SMBAUXBA, &smba);
> +		if (retval)
> +			goto error;
> +
> +		retval = piix4_add_adapter(dev, smba, &piix4_aux_adapter);
> +		if (retval)
> +			goto error;

I don't get the logic here. If we fail to register the auxiliary SMBus,
it is certainly not a reason to give up the working main SMBus.
Especially with my comment above that the Auxiliary SMBus may have been
disabled by the vendor on purpose.

> +	}
> +
> +	return 0;
> +
> +error:
> +	/* clean up any adapters that were already added */
> +	piix4_adap_remove(piix4_main_adapter);
> +	piix4_adap_remove(piix4_aux_adapter);

You're not clearing the pointers, this could cause trouble on hot-plug.

> +	return retval;
>  }
>  
>  /* Remove the adapter and its associated IO region */
> @@ -572,6 +640,9 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  {
>  	struct i2c_piix4_adapdata *adapdata;
>  
> +	if (adap == NULL)
> +		return;
> +

If you want to do that, it would be better done right in patch 2/3, to
avoid changing code back and forth.

>  	adapdata = (struct i2c_piix4_adapdata *)i2c_get_adapdata(adap);
>  	if (adapdata->smba) {
>  		i2c_del_adapter(adap);
> @@ -585,10 +656,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  
>  static void __devexit piix4_remove(struct pci_dev *dev)
>  {
> -	if (piix4_main_adapter) {
> -		piix4_adap_remove(piix4_main_adapter);
> -		piix4_main_adapter = NULL;
> -	}
> +	piix4_adap_remove(piix4_main_adapter);
> +	piix4_adap_remove(piix4_aux_adapter);

Here again you're no longer clearing the pointers afterward, this could
cause trouble (unlikely, but better safe than sorry.)

>  }
>  
>  static struct pci_driver piix4_driver = {


-- 
Jean Delvare

Re: [PATCH] i2c multiplexer driver for Proliant microserver N36L

[Jean Delvare]

Hi Eddi,

On Wed, 13 Jun 2012 07:44:34 +0200, Eddi De Pieri wrote:
> It works for me too
> 
> What about including this patch in the mainline?

Yesterday I reviewed a patch set adding support for the auxiliary SMBus
controller on the SB600/SB700:
http://marc.info/?t=133886124500001&r=1&w=2
http://marc.info/?t=133886124300004&r=1&w=2
http://marc.info/?t=133886124300003&r=1&w=2

It touches the exact same area as touched by your patch, so they will
conflict. Best would be to rebase the SB800 multiport support patch on
top of the other patch set.

Andrew, do you think you can update per my comments and resubmit your
patch set quickly, so that we can then proceed with the SB800 multiport
support before the next merge window?

Thanks,
-- 
Jean Delvare

[PATCH 0/3] i2c-piix4: Multiple piix4-compatible SMBus support (revised)

[Andrew Armenia]

This is a revision to the earlier patch set I submitted, which adds
support for the secondary SMBus controller found on certain AMD
chipsets, notably the SB700 and its derivative the SP5100.

Andrew Armenia (3):
  i2c-piix4: eliminate piix4_smba global variable
  i2c-piix4: separate registration and probing code
  i2c-piix4: support AMD auxiliary SMBus controller

 Documentation/i2c/busses/i2c-piix4 |   13 ++-
 drivers/i2c/busses/Kconfig         |    6 +-
 drivers/i2c/busses/i2c-piix4.c     |  200 ++++++++++++++++++++++++++++--------
 3 files changed, 175 insertions(+), 44 deletions(-)

-- 
1.7.10

[PATCH 2/3] i2c-piix4: separate registration and probing code

[Andrew Armenia]

Some chipsets have multiple sets of SMBus registers each controlling a
separate SMBus. Supporting these chipsets properly will require registering
multiple I2C adapters for one piix4.

The code to initialize and register the i2c_adapter structure has been
separated from piix4_probe and allows registration of a piix4 adapter
given its base address. Note that the i2c_adapter and i2c_piix4_adapdata
structures are now dynamically allocated.

Signed-off-by: Andrew Armenia <andrew-Lwj1yN59in/Ib2jZbfQ/kQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-piix4.c |  113 ++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 40 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 61a85c9..8181963 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -96,7 +96,6 @@ MODULE_PARM_DESC(force_addr,
 
 static int srvrworks_csb5_delay;
 static struct pci_driver piix4_driver;
-static struct i2c_adapter piix4_adapter;
 
 static struct dmi_system_id __devinitdata piix4_dmi_blacklist[] = {
 	{
@@ -294,27 +293,33 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
-static int piix4_transaction(unsigned short piix4_smba)
+static int piix4_transaction(struct i2c_adapter *piix4_adapter)
 {
 	int temp;
 	int result = 0;
 	int timeout = 0;
 
-	dev_dbg(&piix4_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
+	struct i2c_piix4_adapdata *adapdata;
+	unsigned short piix4_smba;
+
+	adapdata = i2c_get_adapdata(piix4_adapter);
+	piix4_smba = adapdata->smba;
+
+	dev_dbg(&piix4_adapter->dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
 		inb_p(SMBHSTDAT1));
 
 	/* Make sure the SMBus host is ready to start transmitting */
 	if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-		dev_dbg(&piix4_adapter.dev, "SMBus busy (%02x). "
+		dev_dbg(&piix4_adapter->dev, "SMBus busy (%02x). "
 			"Resetting...\n", temp);
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-			dev_err(&piix4_adapter.dev, "Failed! (%02x)\n", temp);
+			dev_err(&piix4_adapter->dev, "Failed! (%02x)\n", temp);
 			return -EBUSY;
 		} else {
-			dev_dbg(&piix4_adapter.dev, "Successful!\n");
+			dev_dbg(&piix4_adapter->dev, "Successful!\n");
 		}
 	}
 
@@ -333,35 +338,35 @@ static int piix4_transaction(unsigned short piix4_smba)
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout == MAX_TIMEOUT) {
-		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
+		dev_err(&piix4_adapter->dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
 		result = -EIO;
-		dev_err(&piix4_adapter.dev, "Error: Failed bus transaction\n");
+		dev_err(&piix4_adapter->dev, "Error: Failed bus transaction\n");
 	}
 
 	if (temp & 0x08) {
 		result = -EIO;
-		dev_dbg(&piix4_adapter.dev, "Bus collision! SMBus may be "
+		dev_dbg(&piix4_adapter->dev, "Bus collision! SMBus may be "
 			"locked until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & 0x04) {
 		result = -ENXIO;
-		dev_dbg(&piix4_adapter.dev, "Error: no response!\n");
+		dev_dbg(&piix4_adapter->dev, "Error: no response!\n");
 	}
 
 	if (inb_p(SMBHSTSTS) != 0x00)
 		outb_p(inb(SMBHSTSTS), SMBHSTSTS);
 
 	if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
-		dev_err(&piix4_adapter.dev, "Failed reset at end of "
+		dev_err(&piix4_adapter->dev, "Failed reset at end of "
 			"transaction (%02x)\n", temp);
 	}
-	dev_dbg(&piix4_adapter.dev, "Transaction (post): CNT=%02x, CMD=%02x, "
+	dev_dbg(&piix4_adapter->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
 		inb_p(SMBHSTDAT1));
@@ -434,7 +439,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	status = piix4_transaction(piix4_smba);
+	status = piix4_transaction(adap);
 	if (status)
 		return status;
 
@@ -474,14 +479,6 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= piix4_func,
 };
 
-static struct i2c_adapter piix4_adapter = {
-	.owner		= THIS_MODULE,
-	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-	.algo		= &smbus_algorithm,
-};
-
-static struct i2c_piix4_adapdata piix4_adapter_data;
-
 static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -506,6 +503,54 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
+static struct i2c_adapter *piix4_main_adapter;
+
+static int __devinit piix4_add_adapter(struct pci_dev *dev,
+					unsigned short smba,
+					struct i2c_adapter **padap)
+{
+	struct i2c_adapter *adap;
+	struct i2c_piix4_adapdata *adapdata;
+
+	int retval;
+
+	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
+	if (NULL == adap)
+		return -ENOMEM;
+
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	adap->algo = &smbus_algorithm;
+
+	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
+	if (NULL == adapdata) {
+		kfree(adap);
+		return -ENOMEM;
+	}
+
+	adapdata->smba = smba;
+
+	/* set up the sysfs linkage to our parent device */
+	adap->dev.parent = &dev->dev;
+
+	snprintf(adap->name, sizeof(adap->name),
+		"SMBus PIIX4 adapter at %04x", smba);
+
+	i2c_set_adapdata(adap, adapdata);
+
+	retval = i2c_add_adapter(adap);
+	if (retval) {
+		dev_err(&dev->dev, "Couldn't register adapter!\n");
+		release_region(smba, SMBIOSIZE);
+		kfree(adapdata);
+		kfree(adap);
+		return retval;
+	}
+
+	*padap = adap;
+	return 0;
+}
+
 static int __devinit piix4_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
@@ -523,23 +568,7 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 	if (retval < 0)
 		return retval;
 
-	/* set up the sysfs linkage to our parent device */
-	piix4_adapter.dev.parent = &dev->dev;
-
-	piix4_adapter_data.smba = retval;
-
-	snprintf(piix4_adapter.name, sizeof(piix4_adapter.name),
-		"SMBus PIIX4 adapter at %04x", piix4_adapter_data.smba);
-
-	i2c_set_adapdata(&piix4_adapter, &piix4_adapter_data);
-
-	if ((retval = i2c_add_adapter(&piix4_adapter))) {
-		dev_err(&dev->dev, "Couldn't register adapter!\n");
-		release_region(piix4_adapter_data.smba, SMBIOSIZE);
-		piix4_adapter_data.smba = 0;
-	}
-
-	return retval;
+	return piix4_add_adapter(dev, retval, &piix4_main_adapter);
 }
 
 static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
@@ -550,13 +579,17 @@ static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
 		release_region(adapdata->smba, SMBIOSIZE);
-		adapdata->smba = 0;
+		kfree(adapdata);
+		kfree(adap);
 	}
 }
 
 static void __devexit piix4_remove(struct pci_dev *dev)
 {
-	piix4_adap_remove(&piix4_adapter);
+	if (piix4_main_adapter) {
+		piix4_adap_remove(piix4_main_adapter);
+		piix4_main_adapter = NULL;
+	}
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.7.10

Re: [PATCH 2/3] i2c-piix4: separate registration and probing code

[Jean Delvare]

Hi Andrew,

On Wed, 13 Jun 2012 12:59:08 -0400, Andrew Armenia wrote:
> Some chipsets have multiple sets of SMBus registers each controlling a
> separate SMBus. Supporting these chipsets properly will require registering
> multiple I2C adapters for one piix4.
> 
> The code to initialize and register the i2c_adapter structure has been
> separated from piix4_probe and allows registration of a piix4 adapter
> given its base address. Note that the i2c_adapter and i2c_piix4_adapdata
> structures are now dynamically allocated.
> 
> Signed-off-by: Andrew Armenia <andrew-Lwj1yN59in/Ib2jZbfQ/kQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-piix4.c |  113 ++++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 40 deletions(-)
> (...)

Applied, with the minor changes below:

> -static int piix4_transaction(unsigned short piix4_smba)
> +static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>  {
>  	int temp;
>  	int result = 0;
>  	int timeout = 0;
>  
> -	dev_dbg(&piix4_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
> +	struct i2c_piix4_adapdata *adapdata;
> +	unsigned short piix4_smba;
> +
> +	adapdata = i2c_get_adapdata(piix4_adapter);
> +	piix4_smba = adapdata->smba;

It is customary to declare and assign all at once in this case:

	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
	unsigned short piix4_smba = adapdata->smba;

This makes the code more compact and more readable too. I've fixed it
and everywhere else.

> (...)
> +static int __devinit piix4_add_adapter(struct pci_dev *dev,
> +					unsigned short smba,
> +					struct i2c_adapter **padap)
> +{
> +	struct i2c_adapter *adap;
> +	struct i2c_piix4_adapdata *adapdata;
> +
> +	int retval;
> +
> +	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> +	if (NULL == adap)

There's no point in inverting comparisons this way. Compilers would let
you know if you had it wrong, they do for at least 10 years now.

-- 
Jean Delvare