[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