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

[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 1/3] i2c-piix4: eliminate piix4_smba global variable

[Andrew Armenia]

Some chipsets have multiple sets of piix4-compatible SMBus registers.
Eliminating the global variable will allow these chipsets to be fully
supported.

Return value from piix4_setup and piix4_sb800_setup now returns the smba
value detected. This is stored in a struct i2c_piix4_adapdata. Thus
the global variable is eliminated.

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

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c14d48d..61a85c9 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,15 @@ 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)
 {
 	unsigned char temp;
+	unsigned short piix4_smba;
 
 	if ((PIIX4_dev->vendor == PCI_VENDOR_ID_SERVERWORKS) &&
 	    (PIIX4_dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB5))
@@ -206,7 +210,6 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 			dev_err(&PIIX4_dev->dev,
 				"Host SMBus controller not enabled!\n");
 			release_region(piix4_smba, SMBIOSIZE);
-			piix4_smba = 0;
 			return -ENODEV;
 		}
 	}
@@ -224,12 +227,13 @@ static int __devinit piix4_setup(struct pci_dev *PIIX4_dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, temp);
 
-	return 0;
+	return piix4_smba;
 }
 
 static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 				const struct pci_device_id *id)
 {
+	unsigned short piix4_smba;
 	unsigned short smba_idx = 0xcd6;
 	u8 smba_en_lo, smba_en_hi, i2ccfg, i2ccfg_offset = 0x10, smb_en = 0x2c;
 
@@ -273,7 +277,6 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 		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;
 		return -EBUSY;
 	}
 	i2ccfg = inb_p(piix4_smba + i2ccfg_offset);
@@ -288,10 +291,10 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
 
-	return 0;
+	return piix4_smba;
 }
 
-static int piix4_transaction(void)
+static int piix4_transaction(unsigned short piix4_smba)
 {
 	int temp;
 	int result = 0;
@@ -372,6 +375,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 = i2c_get_adapdata(adap);
+	piix4_smba = adapdata->smba;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -426,7 +434,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 +480,8 @@ static struct i2c_adapter piix4_adapter = {
 	.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) },
@@ -510,33 +520,45 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 	else
 		retval = piix4_setup(dev, id);
 
-	if (retval)
+	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_smba);
+		"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_smba, SMBIOSIZE);
-		piix4_smba = 0;
+		release_region(piix4_adapter_data.smba, SMBIOSIZE);
+		piix4_adapter_data.smba = 0;
 	}
 
 	return retval;
 }
 
-static void __devexit piix4_remove(struct pci_dev *dev)
+static void __devinit 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 = 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 piix4_smba global variable

[Jean Delvare]

On Wed, 13 Jun 2012 12:59:07 -0400, Andrew Armenia wrote:
> Some chipsets have multiple sets of piix4-compatible SMBus registers.
> Eliminating the global variable will allow these chipsets to be fully
> supported.
> 
> Return value from piix4_setup and piix4_sb800_setup now returns the smba
> value detected. This is stored in a struct i2c_piix4_adapdata. Thus
> the global variable is eliminated.
> 
> Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   54 ++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index c14d48d..61a85c9 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> (...)
> -static void __devexit piix4_remove(struct pci_dev *dev)
> +static void __devinit piix4_adap_remove(struct i2c_adapter *adap)

You mean __devexit here.

Other than this, looks good, applied, thanks.

-- 
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 i2c_piix4_adapdata
structures are now dynamically allocated.

Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
---
 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@asquaredlabs.com>
> ---
>  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

[PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller

[Andrew Armenia]

Some AMD chipsets, such as the SP5100, have an auxiliary SMBus
controller with a second set of registers. This patch adds
support for this auxiliary controller.

Tested on ASUS KCMA-D8 motherboard.

Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
---
 Documentation/i2c/busses/i2c-piix4 |   13 +++++--
 drivers/i2c/busses/Kconfig         |    6 +++-
 drivers/i2c/busses/i2c-piix4.c     |   69 ++++++++++++++++++++++++++++++++++--
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/Documentation/i2c/busses/i2c-piix4 b/Documentation/i2c/busses/i2c-piix4
index 475bb4a..474779e 100644
--- a/Documentation/i2c/busses/i2c-piix4
+++ b/Documentation/i2c/busses/i2c-piix4
@@ -8,12 +8,17 @@ Supported adapters:
     Datasheet: Only available via NDA from ServerWorks
   * ATI IXP200, IXP300, IXP400, SB600, SB700 and SB800 southbridges
     Datasheet: Not publicly available
+    SB700 register reference available at:
+    http://support.amd.com/us/Embedded_TechDocs/43009_sb7xx_rrg_pub_1.00.pdf
+  * AMD SP5100 (SB700 derivative found on some server mainboards)
+    Datasheet: Publicly available at the AMD website
+    http://support.amd.com/us/Embedded_TechDocs/44413.pdf
   * AMD Hudson-2
     Datasheet: Not publicly available
   * Standard Microsystems (SMSC) SLC90E66 (Victory66) southbridge
     Datasheet: Publicly available at the SMSC website http://www.smsc.com
 
-Authors: 
+Authors:
 	Frodo Looijaard <frodol@dds.nl>
 	Philip Edelbrock <phil@netroedge.com>
 
@@ -32,7 +37,7 @@ Description
 
 The PIIX4 (properly known as the 82371AB) is an Intel chip with a lot of
 functionality. Among other things, it implements the PCI bus. One of its
-minor functions is implementing a System Management Bus. This is a true 
+minor functions is implementing a System Management Bus. This is a true
 SMBus - you can not access it on I2C levels. The good news is that it
 natively understands SMBus commands and you do not have to worry about
 timing problems. The bad news is that non-SMBus devices connected to it can
@@ -68,6 +73,10 @@ this driver on those mainboards.
 The ServerWorks Southbridges, the Intel 440MX, and the Victory66 are
 identical to the PIIX4 in I2C/SMBus support.
 
+The AMD SB700 and SP5100 chipsets implement two PIIX4-compatible SMBus
+controllers. If your BIOS initializes the secondary controller, it will
+be detected by this driver as an "Auxiliary SMBus Host Controller".
+
 If you own Force CPCI735 motherboard or other OSB4 based systems you may need
 to change the SMBus Interrupt Select register so the SMBus controller uses
 the SMI mode.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 7244c8b..2e7530a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -133,7 +133,7 @@ config I2C_PIIX4
 	    ATI IXP300
 	    ATI IXP400
 	    ATI SB600
-	    ATI SB700
+	    ATI SB700/SP5100
 	    ATI SB800
 	    AMD Hudson-2
 	    Serverworks OSB4
@@ -143,6 +143,10 @@ config I2C_PIIX4
 	    Serverworks HT-1100
 	    SMSC Victory66
 
+	  Some AMD chipsets contain two PIIX4-compatible SMBus
+	  controllers. This driver will attempt to use both controllers
+	  on the SB700/SP5100, if they have been initialized by the BIOS.
+
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-piix4.
 
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8181963..85d3db9 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -21,11 +21,12 @@
    Supports:
 	Intel PIIX4, 440MX
 	Serverworks OSB4, CSB5, CSB6, HT-1000, HT-1100
-	ATI IXP200, IXP300, IXP400, SB600, SB700, SB800
+	ATI IXP200, IXP300, IXP400, SB600, SB700/SP5100, SB800
 	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 more
+   SMBus interfaces.
 */
 
 #include <linux/module.h>
@@ -60,6 +61,7 @@
 #define SMBIOSIZE	8
 
 /* PCI Address Constants */
+#define SMBAUXBA	0x058
 #define SMBBA		0x090
 #define SMBHSTCFG	0x0D2
 #define SMBSLVC		0x0D3
@@ -293,6 +295,43 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
+static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
+				const struct pci_device_id *id,
+				unsigned short base_reg_addr)
+{
+	/* Set up auxiliary SMBus controllers found on some
+	 * AMD chipsets e.g. SP5100 (SB700 derivative) */
+
+	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_dbg(&PIIX4_dev->dev,
+			"Auxiliary SMBus base address uninitialized");
+
+		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, "Auxiliary 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
+	);
+
+	return piix4_smba;
+}
+
 static int piix4_transaction(struct i2c_adapter *piix4_adapter)
 {
 	int temp;
@@ -504,6 +543,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;
 
 static int __devinit piix4_add_adapter(struct pci_dev *dev,
 					unsigned short smba,
@@ -565,10 +605,28 @@ static int __devinit piix4_probe(struct pci_dev *dev,
 	else
 		retval = piix4_setup(dev, id);
 
+	/* if no main SMBus found, give up */
 	if (retval < 0)
 		return retval;
 
-	return piix4_add_adapter(dev, retval, &piix4_main_adapter);
+	/* try to register main SMBus adapter, give up if we can't */
+	retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
+	if (retval < 0)
+		return retval;
+
+	/* check for auxiliary SMBus on some AMD chipsets */
+	if (dev->vendor == PCI_VENDOR_ID_ATI &&
+	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
+	    dev->revision < 0x40) {
+		retval = piix4_setup_aux(dev, id, SMBAUXBA);
+		if (retval > 0) {
+			/* try to add the aux adapter if it exists,
+			 * piix4_add_adapter will clean up if this fails */
+			piix4_add_adapter(dev, retval, &piix4_aux_adapter);
+		}
+	}
+
+	return 0;
 }
 
 static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
@@ -590,6 +648,11 @@ static void __devexit piix4_remove(struct pci_dev *dev)
 		piix4_adap_remove(piix4_main_adapter);
 		piix4_main_adapter = NULL;
 	}
+
+	if (piix4_aux_adapter) {
+		piix4_adap_remove(piix4_aux_adapter);
+		piix4_aux_adapter = NULL;
+	}
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.7.10

Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller

[Jean Delvare]

Hi Andrew,

For next time... When you post or repost a patch series, please always
start a new discussion thread (don't use Reply), otherwise discussion
threading becomes very hard to follow, both in e-mail clients and
online list archives.

On Wed, 13 Jun 2012 12:59:09 -0400, Andrew Armenia wrote:
> Some AMD chipsets, such as the SP5100, have an auxiliary SMBus
> controller with a second set of registers. This patch adds
> support for this auxiliary controller.
> 
> Tested on ASUS KCMA-D8 motherboard.
> 
> Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
> ---
>  Documentation/i2c/busses/i2c-piix4 |   13 +++++--
>  drivers/i2c/busses/Kconfig         |    6 +++-
>  drivers/i2c/busses/i2c-piix4.c     |   69 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 82 insertions(+), 6 deletions(-)

A few issues remaining in this patch:

> diff --git a/Documentation/i2c/busses/i2c-piix4 b/Documentation/i2c/busses/i2c-piix4
> index 475bb4a..474779e 100644
> --- a/Documentation/i2c/busses/i2c-piix4
> +++ b/Documentation/i2c/busses/i2c-piix4
> @@ -8,12 +8,17 @@ Supported adapters:
>      Datasheet: Only available via NDA from ServerWorks
>    * ATI IXP200, IXP300, IXP400, SB600, SB700 and SB800 southbridges
>      Datasheet: Not publicly available
> +    SB700 register reference available at:
> +    http://support.amd.com/us/Embedded_TechDocs/43009_sb7xx_rrg_pub_1.00.pdf
> +  * AMD SP5100 (SB700 derivative found on some server mainboards)
> +    Datasheet: Publicly available at the AMD website
> +    http://support.amd.com/us/Embedded_TechDocs/44413.pdf
>    * AMD Hudson-2
>      Datasheet: Not publicly available
>    * Standard Microsystems (SMSC) SLC90E66 (Victory66) southbridge
>      Datasheet: Publicly available at the SMSC website http://www.smsc.com
>  
> -Authors: 
> +Authors:

Never include white-space changes in a non-cleanup patch, they make
review and backporting harder.

>  	Frodo Looijaard <frodol@dds.nl>
>  	Philip Edelbrock <phil@netroedge.com>
>  
> @@ -32,7 +37,7 @@ Description
>  
>  The PIIX4 (properly known as the 82371AB) is an Intel chip with a lot of
>  functionality. Among other things, it implements the PCI bus. One of its
> -minor functions is implementing a System Management Bus. This is a true 
> +minor functions is implementing a System Management Bus. This is a true
>  SMBus - you can not access it on I2C levels. The good news is that it
>  natively understands SMBus commands and you do not have to worry about
>  timing problems. The bad news is that non-SMBus devices connected to it can
> @@ -68,6 +73,10 @@ this driver on those mainboards.
>  The ServerWorks Southbridges, the Intel 440MX, and the Victory66 are
>  identical to the PIIX4 in I2C/SMBus support.
>  
> +The AMD SB700 and SP5100 chipsets implement two PIIX4-compatible SMBus
> +controllers. If your BIOS initializes the secondary controller, it will
> +be detected by this driver as an "Auxiliary SMBus Host Controller".
> +
>  If you own Force CPCI735 motherboard or other OSB4 based systems you may need
>  to change the SMBus Interrupt Select register so the SMBus controller uses
>  the SMI mode.
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 7244c8b..2e7530a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -133,7 +133,7 @@ config I2C_PIIX4
>  	    ATI IXP300
>  	    ATI IXP400
>  	    ATI SB600
> -	    ATI SB700
> +	    ATI SB700/SP5100
>  	    ATI SB800
>  	    AMD Hudson-2
>  	    Serverworks OSB4
> @@ -143,6 +143,10 @@ config I2C_PIIX4
>  	    Serverworks HT-1100
>  	    SMSC Victory66
>  
> +	  Some AMD chipsets contain two PIIX4-compatible SMBus
> +	  controllers. This driver will attempt to use both controllers
> +	  on the SB700/SP5100, if they have been initialized by the BIOS.
> +
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-piix4.
>  
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 8181963..85d3db9 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -21,11 +21,12 @@
>     Supports:
>  	Intel PIIX4, 440MX
>  	Serverworks OSB4, CSB5, CSB6, HT-1000, HT-1100
> -	ATI IXP200, IXP300, IXP400, SB600, SB700, SB800
> +	ATI IXP200, IXP300, IXP400, SB600, SB700/SP5100, SB800
>  	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 more
> +   SMBus interfaces.
>  */
>  
>  #include <linux/module.h>
> @@ -60,6 +61,7 @@
>  #define SMBIOSIZE	8
>  
>  /* PCI Address Constants */
> +#define SMBAUXBA	0x058

AFAICS this is SB700-specific, so this should either be clarified in
the name, or maybe we don't even want a name. After all, register
offset is hard-coded in piix4_setup_sb800.

>  #define SMBBA		0x090
>  #define SMBHSTCFG	0x0D2
>  #define SMBSLVC		0x0D3
> @@ -293,6 +295,43 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  	return piix4_smba;
>  }
>  
> +static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
> +				const struct pci_device_id *id,
> +				unsigned short base_reg_addr)
> +{
> +	/* Set up auxiliary SMBus controllers found on some
> +	 * AMD chipsets e.g. SP5100 (SB700 derivative) */
> +
> +	unsigned short piix4_smba;
> +
> +	/* Read address of auxiliary SMBus controller */
> +	pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
> +	piix4_smba &= 0xffe0;

You must check bit 0 first. You want the I/O base to be set AND
decoding thereof enabled, otherwise the controller can't be used.

Also, mask 0xffe0 is OK for the SB700 but not for the SB600 which only
has bits 3:1 marked as reserved. Given that reserved bits are supposed
to be 0 anyway, I think it is OK to mask with 0xfff0 always.

> +
> +	if (piix4_smba == 0) {
> +		dev_dbg(&PIIX4_dev->dev,
> +			"Auxiliary SMBus base address uninitialized");

Missing trailing new line.

> +

Needless blank line.

> +		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, "Auxiliary 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
> +	);

Undesirable line break.

> +
> +	return piix4_smba;
> +}
> +
>  static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>  {
>  	int temp;
> @@ -504,6 +543,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;
>  
>  static int __devinit piix4_add_adapter(struct pci_dev *dev,
>  					unsigned short smba,
> @@ -565,10 +605,28 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>  	else
>  		retval = piix4_setup(dev, id);
>  
> +	/* if no main SMBus found, give up */
>  	if (retval < 0)
>  		return retval;
>  
> -	return piix4_add_adapter(dev, retval, &piix4_main_adapter);
> +	/* try to register main SMBus adapter, give up if we can't */
> +	retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
> +	if (retval < 0)
> +		return retval;
> +
> +	/* check for auxiliary SMBus on some AMD chipsets */
> +	if (dev->vendor == PCI_VENDOR_ID_ATI &&
> +	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
> +	    dev->revision < 0x40) {
> +		retval = piix4_setup_aux(dev, id, SMBAUXBA);
> +		if (retval > 0) {
> +			/* try to add the aux adapter if it exists,
> +			 * piix4_add_adapter will clean up if this fails */
> +			piix4_add_adapter(dev, retval, &piix4_aux_adapter);
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
> @@ -590,6 +648,11 @@ static void __devexit piix4_remove(struct pci_dev *dev)
>  		piix4_adap_remove(piix4_main_adapter);
>  		piix4_main_adapter = NULL;
>  	}
> +
> +	if (piix4_aux_adapter) {
> +		piix4_adap_remove(piix4_aux_adapter);
> +		piix4_aux_adapter = NULL;
> +	}
>  }
>  
>  static struct pci_driver piix4_driver = {

I've fixed them all, and applied your patch. The whole series with my
changes incorporated is now visible at:
http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/

I would appreciate if you could test them and confirm I didn't break
anything. Your patches will then be merged in kernel 3.6.

Thanks,
-- 
Jean Delvare

Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller

[Andrew Armenia]

On Fri, Jun 15, 2012 at 4:31 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Andrew,
>
> For next time... When you post or repost a patch series, please always
> start a new discussion thread (don't use Reply), otherwise discussion
> threading becomes very hard to follow, both in e-mail clients and
> online list archives.
>
> On Wed, 13 Jun 2012 12:59:09 -0400, Andrew Armenia wrote:
>> Some AMD chipsets, such as the SP5100, have an auxiliary SMBus
>> controller with a second set of registers. This patch adds
>> support for this auxiliary controller.
>>
>> Tested on ASUS KCMA-D8 motherboard.
>>
>> Signed-off-by: Andrew Armenia <andrew@asquaredlabs.com>
>> ---
>>  Documentation/i2c/busses/i2c-piix4 |   13 +++++--
>>  drivers/i2c/busses/Kconfig         |    6 +++-
>>  drivers/i2c/busses/i2c-piix4.c     |   69 ++++++++++++++++++++++++++++++++++--
>>  3 files changed, 82 insertions(+), 6 deletions(-)
>
> A few issues remaining in this patch:
>
>> diff --git a/Documentation/i2c/busses/i2c-piix4 b/Documentation/i2c/busses/i2c-piix4
>> index 475bb4a..474779e 100644
>> --- a/Documentation/i2c/busses/i2c-piix4
>> +++ b/Documentation/i2c/busses/i2c-piix4
>> @@ -8,12 +8,17 @@ Supported adapters:
>>      Datasheet: Only available via NDA from ServerWorks
>>    * ATI IXP200, IXP300, IXP400, SB600, SB700 and SB800 southbridges
>>      Datasheet: Not publicly available
>> +    SB700 register reference available at:
>> +    http://support.amd.com/us/Embedded_TechDocs/43009_sb7xx_rrg_pub_1.00.pdf
>> +  * AMD SP5100 (SB700 derivative found on some server mainboards)
>> +    Datasheet: Publicly available at the AMD website
>> +    http://support.amd.com/us/Embedded_TechDocs/44413.pdf
>>    * AMD Hudson-2
>>      Datasheet: Not publicly available
>>    * Standard Microsystems (SMSC) SLC90E66 (Victory66) southbridge
>>      Datasheet: Publicly available at the SMSC website http://www.smsc.com
>>
>> -Authors:
>> +Authors:
>
> Never include white-space changes in a non-cleanup patch, they make
> review and backporting harder.
>
>>       Frodo Looijaard <frodol@dds.nl>
>>       Philip Edelbrock <phil@netroedge.com>
>>
>> @@ -32,7 +37,7 @@ Description
>>
>>  The PIIX4 (properly known as the 82371AB) is an Intel chip with a lot of
>>  functionality. Among other things, it implements the PCI bus. One of its
>> -minor functions is implementing a System Management Bus. This is a true
>> +minor functions is implementing a System Management Bus. This is a true
>>  SMBus - you can not access it on I2C levels. The good news is that it
>>  natively understands SMBus commands and you do not have to worry about
>>  timing problems. The bad news is that non-SMBus devices connected to it can
>> @@ -68,6 +73,10 @@ this driver on those mainboards.
>>  The ServerWorks Southbridges, the Intel 440MX, and the Victory66 are
>>  identical to the PIIX4 in I2C/SMBus support.
>>
>> +The AMD SB700 and SP5100 chipsets implement two PIIX4-compatible SMBus
>> +controllers. If your BIOS initializes the secondary controller, it will
>> +be detected by this driver as an "Auxiliary SMBus Host Controller".
>> +
>>  If you own Force CPCI735 motherboard or other OSB4 based systems you may need
>>  to change the SMBus Interrupt Select register so the SMBus controller uses
>>  the SMI mode.
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 7244c8b..2e7530a 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -133,7 +133,7 @@ config I2C_PIIX4
>>           ATI IXP300
>>           ATI IXP400
>>           ATI SB600
>> -         ATI SB700
>> +         ATI SB700/SP5100
>>           ATI SB800
>>           AMD Hudson-2
>>           Serverworks OSB4
>> @@ -143,6 +143,10 @@ config I2C_PIIX4
>>           Serverworks HT-1100
>>           SMSC Victory66
>>
>> +       Some AMD chipsets contain two PIIX4-compatible SMBus
>> +       controllers. This driver will attempt to use both controllers
>> +       on the SB700/SP5100, if they have been initialized by the BIOS.
>> +
>>         This driver can also be built as a module.  If so, the module
>>         will be called i2c-piix4.
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 8181963..85d3db9 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -21,11 +21,12 @@
>>     Supports:
>>       Intel PIIX4, 440MX
>>       Serverworks OSB4, CSB5, CSB6, HT-1000, HT-1100
>> -     ATI IXP200, IXP300, IXP400, SB600, SB700, SB800
>> +     ATI IXP200, IXP300, IXP400, SB600, SB700/SP5100, SB800
>>       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 more
>> +   SMBus interfaces.
>>  */
>>
>>  #include <linux/module.h>
>> @@ -60,6 +61,7 @@
>>  #define SMBIOSIZE    8
>>
>>  /* PCI Address Constants */
>> +#define SMBAUXBA     0x058
>
> AFAICS this is SB700-specific, so this should either be clarified in
> the name, or maybe we don't even want a name. After all, register
> offset is hard-coded in piix4_setup_sb800.
>
>>  #define SMBBA                0x090
>>  #define SMBHSTCFG    0x0D2
>>  #define SMBSLVC              0x0D3
>> @@ -293,6 +295,43 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>>       return piix4_smba;
>>  }
>>
>> +static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
>> +                             const struct pci_device_id *id,
>> +                             unsigned short base_reg_addr)
>> +{
>> +     /* Set up auxiliary SMBus controllers found on some
>> +      * AMD chipsets e.g. SP5100 (SB700 derivative) */
>> +
>> +     unsigned short piix4_smba;
>> +
>> +     /* Read address of auxiliary SMBus controller */
>> +     pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
>> +     piix4_smba &= 0xffe0;
>
> You must check bit 0 first. You want the I/O base to be set AND
> decoding thereof enabled, otherwise the controller can't be used.
>
> Also, mask 0xffe0 is OK for the SB700 but not for the SB600 which only
> has bits 3:1 marked as reserved. Given that reserved bits are supposed
> to be 0 anyway, I think it is OK to mask with 0xfff0 always.
>
>> +
>> +     if (piix4_smba == 0) {
>> +             dev_dbg(&PIIX4_dev->dev,
>> +                     "Auxiliary SMBus base address uninitialized");
>
> Missing trailing new line.
>
>> +
>
> Needless blank line.
>
>> +             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, "Auxiliary 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
>> +     );
>
> Undesirable line break.
>
>> +
>> +     return piix4_smba;
>> +}
>> +
>>  static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>>  {
>>       int temp;
>> @@ -504,6 +543,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;
>>
>>  static int __devinit piix4_add_adapter(struct pci_dev *dev,
>>                                       unsigned short smba,
>> @@ -565,10 +605,28 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>>       else
>>               retval = piix4_setup(dev, id);
>>
>> +     /* if no main SMBus found, give up */
>>       if (retval < 0)
>>               return retval;
>>
>> -     return piix4_add_adapter(dev, retval, &piix4_main_adapter);
>> +     /* try to register main SMBus adapter, give up if we can't */
>> +     retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
>> +     if (retval < 0)
>> +             return retval;
>> +
>> +     /* check for auxiliary SMBus on some AMD chipsets */
>> +     if (dev->vendor == PCI_VENDOR_ID_ATI &&
>> +         dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>> +         dev->revision < 0x40) {
>> +             retval = piix4_setup_aux(dev, id, SMBAUXBA);
>> +             if (retval > 0) {
>> +                     /* try to add the aux adapter if it exists,
>> +                      * piix4_add_adapter will clean up if this fails */
>> +                     piix4_add_adapter(dev, retval, &piix4_aux_adapter);
>> +             }
>> +     }
>> +
>> +     return 0;
>>  }
>>
>>  static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
>> @@ -590,6 +648,11 @@ static void __devexit piix4_remove(struct pci_dev *dev)
>>               piix4_adap_remove(piix4_main_adapter);
>>               piix4_main_adapter = NULL;
>>       }
>> +
>> +     if (piix4_aux_adapter) {
>> +             piix4_adap_remove(piix4_aux_adapter);
>> +             piix4_aux_adapter = NULL;
>> +     }
>>  }
>>
>>  static struct pci_driver piix4_driver = {
>
> I've fixed them all, and applied your patch. The whole series with my
> changes incorporated is now visible at:
> http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/
>
> I would appreciate if you could test them and confirm I didn't break
> anything. Your patches will then be merged in kernel 3.6.
>
> Thanks,
> --
> Jean Delvare

I've just tested your updated patches and nothing seems broken. Thanks
for your patience with a first time submitter.

-Andrew

Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller

[Jean Delvare]

On Fri, 15 Jun 2012 09:43:40 -0400, Andrew Armenia wrote:
> I've just tested your updated patches and nothing seems broken. Thanks
> for your patience with a first time submitter.

You're welcome. It was pretty good for a first submission!

-- 
Jean Delvare

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 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@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) {
+	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