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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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