i2c: Adding support for Intel iSMT SMBus 2.0 host controller

The iSMT (Intel SMBus Message Transport) supports multi-master I2C/SMBus, as well as IPMI. It's operation is DMA-based and utilizes descriptors to initiate transactions on the bus.

The iSMT hardware can act as both a master and a target, although this driver only supports being a master.

Signed-off-by Bill Brown <[email protected]>

---
 drivers/i2c/busses/Kconfig    |   10 +
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-ismt.c |  845 +++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-ismt.h |  111 ++++++
 4 files changed, 967 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-ismt.c
 create mode 100644 drivers/i2c/busses/i2c-ismt.h

drivers/i2c/busses/Kconfig +10 -0

index 7244c8b..64d5756 100644
@@ -119,6 +119,16 @@ config I2C_ISCH
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-isch.
 
+config I2C_ISMT
+	tristate "Intel iSMT SMBus Controller"
+	depends on PCI
+	help
+	  If you say yes to this option, support will be included for the Intel
+	  iSMT SMBus host controller interface.
+
+	  This driver can also be built as a module.  If so, the module will be
+	  called i2c-ismt.
+
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI

drivers/i2c/busses/Makefile +0 -0

index ce3c2be..694711a 100644

drivers/i2c/busses/Makefile +0 -0

 # Mac SMBus host controller drivers
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o

drivers/i2c/busses/i2c-ismt.c new file +0 -0

new file mode 100644
index 0000000..46df042

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *

drivers/i2c/busses/i2c-ismt.c new file +20 -0

+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in
+ *     the documentation and/or other materials provided with the
+ *     distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ *     contributors may be used to endorse or promote products derived
+ *     from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,

drivers/i2c/busses/i2c-ismt.c new file +71 -0

+	struct device *dev = &priv->pci_dev->dev;
+	struct ismt_desc *desc = &priv->hw[priv->head];
+
+	dev_dbg(dev, "Dump of the descriptor struct:  0x%X\n", priv->head);
+	dev_dbg(dev, "\ttgtaddr_rw=0x%02X\n", desc->tgtaddr_rw);
+	dev_dbg(dev, "\twr_len_cmd=0x%02X\n", desc->wr_len_cmd);
+	dev_dbg(dev, "\trd_len=    0x%02X\n", desc->rd_len);
+	dev_dbg(dev, "\tcontrol=   0x%02X\n", desc->control);
+	dev_dbg(dev, "\tstatus=    0x%02X\n", desc->status);
+	dev_dbg(dev, "\tretry=     0x%02X\n", desc->retry);
+	dev_dbg(dev, "\trxbytes=   0x%02X\n", desc->rxbytes);
+	dev_dbg(dev, "\ttxbytes=   0x%02X\n", desc->txbytes);
+	dev_dbg(dev, "\tdptr_low=  0x%08X\n", desc->dptr_low);
+	dev_dbg(dev, "\tdptr_high= 0x%08X\n", desc->dptr_high);
+}
+
+/**
+ * ismt_gen_reg_dump() - dump the iSMT General Registers
+ * @adap: the I2C host adapter
+ */
+static void ismt_gen_reg_dump(struct i2c_adapter *adap)
+{
+	struct ismt_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = &priv->pci_dev->dev;
+
+	dev_dbg(dev, "Dump of the iSMT General Registers\n");
+	dev_dbg(dev, "  GCTRL.... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_GCTRL,
+		readl(priv->smba + ISMT_GR_GCTRL));
+	dev_dbg(dev, "  SMTICL... : (0x%p)=0x%016lX\n",
+		priv->smba + ISMT_GR_SMTICL,
+		readq(priv->smba + ISMT_GR_SMTICL));
+	dev_dbg(dev, "  ERRINTMSK : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRINTMSK,
+		readl(priv->smba + ISMT_GR_ERRINTMSK));
+	dev_dbg(dev, "  ERRAERMSK : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRAERMSK,
+		readl(priv->smba + ISMT_GR_ERRAERMSK));
+	dev_dbg(dev, "  ERRSTS... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRSTS,
+		readl(priv->smba + ISMT_GR_ERRSTS));
+	dev_dbg(dev, "  ERRINFO.. : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRINFO,
+		readl(priv->smba + ISMT_GR_ERRINFO));
+}
+
+/**
+ * ismt_mstr_reg_dump() - dump the iSMT Master Registers
+ * @adap: the I2C host adapter
+ */
+static void ismt_mstr_reg_dump(struct i2c_adapter *adap)
+{
+	struct ismt_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = &priv->pci_dev->dev;
+
+	dev_dbg(dev, "Dump of the iSMT Master Registers\n");
+	dev_dbg(dev, "  MDBA..... : (0x%p)=0x%016lX\n",
+		priv->smba + ISMT_MSTR_MDBA,
+		readq(priv->smba + ISMT_MSTR_MDBA));
+	dev_dbg(dev, "  MCTRL.... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MCTRL,
+		readl(priv->smba + ISMT_MSTR_MCTRL));
+	dev_dbg(dev, "  MSTS..... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MSTS,
+		readl(priv->smba + ISMT_MSTR_MSTS));
+	dev_dbg(dev, "  MDS...... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MDS,
+		readl(priv->smba + ISMT_MSTR_MDS));
+	dev_dbg(dev, "  RPOLICY.. : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_RPOLICY,
+		readl(priv->smba + ISMT_MSTR_RPOLICY));

drivers/i2c/busses/i2c-ismt.c new file +7 -0

+}
+
+/**
+ * ismt_access() - process an SMBus command
+ * @adap: the i2c host adapter
+ * @addr: address of the i2c/SMBus target
+ * @flags: command options

drivers/i2c/busses/i2c-ismt.c new file +94 -0

+		desc->control |= ISMT_DESC_PEC;
+
+	if (fair)
+		desc->control |= ISMT_DESC_FAIR;
+
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_QUICK\n");
+		break;
+
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/*
+			 * Send Byte
+			 * The command field contains the write data
+			 */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE:  WRITE\n");
+			desc->control |= ISMT_DESC_CWRL;
+			desc->wr_len_cmd = command;
+		} else {
+			/* Receive Byte */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE:  READ\n");
+			dma_size = 1;
+			dma_direction = DMA_FROM_DEVICE;
+			map_dma_flag = 1;
+			desc->rd_len = 1;
+		}
+
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/*
+			 * Write Byte
+			 * Command plus 1 data byte
+			 */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:  WRITE\n");
+			desc->wr_len_cmd = 2;
+
+			/* Stuff the command ahead of the data in the buffer */
+			ismt_insert_cmd(data, command);
+			dma_size = 2;
+			dma_direction = DMA_TO_DEVICE;
+		} else {
+			/* Read Byte */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
+			desc->control |= ISMT_DESC_CWRL;
+			desc->wr_len_cmd = command;
+			desc->rd_len = 1;
+			dma_size = 1;
+			dma_direction = DMA_FROM_DEVICE;
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/* Write Word */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:  WRITE\n");
+			desc->wr_len_cmd = 3;
+
+			/* Stuff the command ahead of the data in the buffer */
+			ismt_insert_cmd(data, command);
+			dma_size = 3;
+			dma_direction = DMA_TO_DEVICE;
+		} else {
+			/* Read Word */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:  READ\n");
+			desc->wr_len_cmd = command;
+			desc->control |= ISMT_DESC_CWRL;
+			desc->rd_len = 2;
+			dma_size = 2;
+			dma_direction = DMA_FROM_DEVICE;
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_PROC_CALL:
+		dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_PROC_CALL\n");
+		desc->wr_len_cmd = 3;
+		desc->rd_len = 2;
+
+		/* Stuff the command ahead of the data in the buffer */
+		ismt_insert_cmd(data, command);
+		dma_size = 3;
+		dma_direction = DMA_BIDIRECTIONAL;
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/* Block Write */

drivers/i2c/busses/i2c-ismt.c new file +44 -0

+			dma_direction = DMA_FROM_DEVICE;
+			desc->rd_len = dma_size;
+			desc->wr_len_cmd = command;
+			desc->control |= (ISMT_DESC_BLK | ISMT_DESC_CWRL);
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	default:
+		dev_err(&priv->pci_dev->dev, "Unsupported transaction %d\n",
+			size);
+		return -EOPNOTSUPP;
+	}
+
+	/* map the data buffer */
+	if (map_dma_flag) {
+		dev_dbg(&priv->pci_dev->dev,
+			" &priv->pci_dev->dev=%p\n", &priv->pci_dev->dev);
+		dev_dbg(&priv->pci_dev->dev, " data=%p\n", data);
+		dev_dbg(&priv->pci_dev->dev, " dma_size=%d\n", dma_size);
+		dev_dbg(&priv->pci_dev->dev,
+			" dma_direction=%d\n", dma_direction);
+
+		dma_addr = dma_map_single(&priv->pci_dev->dev,
+				      data,
+				      dma_size,
+				      dma_direction);
+
+		dev_dbg(&priv->pci_dev->dev, " dma_addr = 0x%016llX\n",
+			dma_addr);
+
+		desc->dptr_low = lower_32_bits(dma_addr);
+		desc->dptr_high = upper_32_bits(dma_addr);
+	}
+
+	INIT_COMPLETION(priv->cmp);
+
+	/* Add the descriptor */
+	ismt_submit_desc(adap);
+
+	/* Now we wait for interrupt completion, 1s */
+	ret = wait_for_completion_interruptible_timeout(&priv->cmp, HZ*1);
+

drivers/i2c/busses/i2c-ismt.c new file +14 -0

+	} else
+		/* do any post processing of the descriptor here */
+		ret = ismt_process_desc(adap);
+
+	/* Update the ring pointer */
+	priv->head++;
+	priv->head %= ISMT_DESC_ENTRIES;
+
+	return ret;
+}
+
+/**
+ * ismt_func() - report which i2c commands are supported by this adapter
+ * @adap: the i2c host adapter

drivers/i2c/busses/i2c-ismt.c new file +14 -0

+ * @smbus_xfer: the adapter algorithm
+ * @functionality: functionality supported by the adapter
+ */
+static const struct i2c_algorithm smbus_algorithm = {
+	.smbus_xfer	= ismt_access,
+	.functionality	= ismt_func,
+};
+
+/**
+ * ismt_handle_isr() - interrupt handler bottom half
+ * @priv: iSMT private data
+ */
+static irqreturn_t ismt_handle_isr(struct ismt_priv *priv)
+{

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+
+	return ismt_handle_isr(priv);
+}
+
+/**

drivers/i2c/busses/i2c-ismt.c new file +19 -0

+{
+	struct ismt_priv *priv = pci_get_drvdata(pdev);
+
+	priv->entries = ISMT_DESC_ENTRIES;
+
+	/* allocate memory for the descriptor */
+	priv->hw = dmam_alloc_coherent(&pdev->dev,
+				       (ISMT_DESC_ENTRIES
+					       * sizeof(struct ismt_desc)),
+				       &priv->io_rng_dma,
+				       GFP_KERNEL);
+	if (!priv->hw)
+		return -ENOMEM;
+
+	memset(priv->hw, 0, (ISMT_DESC_ENTRIES * sizeof(struct ismt_desc)));
+
+	priv->head = 0;
+	priv->tail = 0;
+	init_completion(&priv->cmp);

drivers/i2c/busses/i2c-ismt.c new file +8 -0

+		goto intx;
+
+	err = devm_request_irq(&pdev->dev,
+			       pdev->irq,
+			       ismt_do_msi_interrupt,
+			       0,
+			       "ismt-msi",
+			       priv);

drivers/i2c/busses/i2c-ismt.c new file +28 -0

+	err = devm_request_irq(&pdev->dev,
+			       pdev->irq,
+			       ismt_do_interrupt,
+			       IRQF_SHARED,
+			       "ismt-intx",
+			       priv);
+	if (err) {
+		dev_err(&pdev->dev, "no usable interrupts\n");
+		return -ENODEV;
+	}
+
+done:
+	return 0;
+}
+
+static struct pci_driver ismt_driver;
+
+/**
+ * ismt_probe() - probe for iSMT devices
+ * @pdev: PCI-Express device
+ * @id: PCI-Express device ID
+ */
+static int __devinit
+ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	int err;
+	struct ismt_priv *priv;
+	unsigned long start, len;

drivers/i2c/busses/i2c-ismt.c new file +18 -0

+
+	priv->adapter.algo = &smbus_algorithm;
+	priv->pci_dev = pdev;
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable SMBus PCI device (%d)\n",
+			err);
+		return err;
+	}
+
+	/* enable bus mastering */
+	pci_set_master(pdev);
+
+	/* Determine the address of the SMBus area */
+	start = pci_resource_start(pdev, SMBBAR);
+	if (!start) {
+		dev_err(&pdev->dev,

drivers/i2c/busses/i2c-ismt.c new file +30 -0

+
+	dev_dbg(&priv->pci_dev->dev, " start=0x%lX\n", start);
+	dev_dbg(&priv->pci_dev->dev, " len=0x%lX\n", len);
+
+	err = acpi_check_resource_conflict(&pdev->resource[SMBBAR]);
+	if (err) {
+		dev_err(&pdev->dev, "ACPI resource conflict!\n");
+		return err;
+	}
+
+	err = pci_request_region(pdev, SMBBAR, ismt_driver.name);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Failed to request SMBus region 0x%lx-0x%lx\n",
+			start, start + len);
+		return err;
+	}
+
+	priv->smba = pcim_iomap(pdev, SMBBAR, len);
+	if (!priv->smba) {
+		dev_err(&pdev->dev, "Unable to ioremap SMBus BAR\n");
+		err = -ENODEV;
+		goto fail;
+	}
+
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
+	if (err) {
+		err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+		if (err)
+			goto fail;

drivers/i2c/busses/i2c-ismt.c new file +7 -0

+		goto fail;
+	}
+
+	ismt_hw_init(pdev);
+
+	err = ismt_int_init(pdev, priv);
+	if (err)

drivers/i2c/busses/i2c-ismt.c new file +13 -0

+	err = i2c_add_adapter(&priv->adapter);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to add SMBus iSMT adapter\n");
+		err = -ENODEV;
+		goto fail;
+	}
+	return 0;
+
+fail:
+	pci_release_region(pdev, SMBBAR);
+	return err;
+}
+

drivers/i2c/busses/i2c-ismt.c new file +50 -0

+	pci_release_region(pdev, SMBBAR);
+}
+
+/**
+ * ismt_suspend() - place the device in suspend
+ * @pdev: PCI-Express device
+ * @mesg: PM message
+ */
+#ifdef CONFIG_PM
+static int ismt_suspend(struct pci_dev *pdev, pm_message_t mesg)
+{
+	pci_save_state(pdev);
+	pci_set_power_state(pdev, pci_choose_state(pdev, mesg));
+	return 0;
+}
+
+/**
+ * ismt_resume() - PCI resume code
+ * @pdev: PCI-Express device
+ */
+static int ismt_resume(struct pci_dev *pdev)
+{
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	return pci_enable_device(pdev);
+}
+
+#else
+
+#define ismt_suspend NULL
+#define ismt_resume NULL
+
+#endif
+
+static struct pci_driver ismt_driver = {
+	.name = "ismt_smbus",
+	.id_table = ismt_ids,
+	.probe = ismt_probe,
+	.remove = __devexit_p(ismt_remove),
+	.suspend = ismt_suspend,
+	.resume = ismt_resume,
+};
+
+/**
+ * i2c_ismt_init() - iSMT driver initialization
+ */
+static int __init i2c_ismt_init(void)
+{
+	pr_debug("Loading the iSMT SMBus driver\n");
+	return pci_register_driver(&ismt_driver);

drivers/i2c/busses/i2c-ismt.h new file +13 -0

+#define ISMT_MAX_RETRIES 3   /* number of SMBus retries to attempt */
+
+/* Hardware Descriptor Constants - Control Field */
+#define ISMT_DESC_CWRL  0x01    /* Command/Write Length */
+#define ISMT_DESC_BLK   0X04    /* Perform Block Transaction */
+#define ISMT_DESC_FAIR  0x08    /* Set fairness flag upon successful arbit. */
+#define ISMT_DESC_PEC   0x10    /* Packet Error Code */
+#define ISMT_DESC_I2C   0x20    /* I2C Enable */
+#define ISMT_DESC_INT   0x40    /* Interrupt */
+#define ISMT_DESC_SOE   0x80    /* Stop On Error */
+
+/* Hardware Descriptor Constants - Status Field */
+#define ISMT_DESC_SCS   0x01    /* Success */

drivers/i2c/busses/i2c-ismt.h new file +25 -0

+#define ISMT_GR_GCTRL      0x000 /* General Control */
+#define ISMT_GR_SMTICL     0x008 /* SMT Interrupt Cause Location */
+#define ISMT_GR_ERRINTMSK  0x010 /* Error Interrupt Mask */
+#define ISMT_GR_ERRAERMSK  0x014 /* Error AER Mask */
+#define ISMT_GR_ERRSTS     0x018 /* Error Status */
+#define ISMT_GR_ERRINFO    0x01c /* Error Information */
+
+/* iSMT Master Registers */
+#define ISMT_MSTR_MDBA    0x100  /* Master Descriptor Base Address */
+#define ISMT_MSTR_MCTRL   0x108  /* Master Control */
+#define ISMT_MSTR_MSTS    0x10c  /* Master Status */
+#define ISMT_MSTR_MDS     0x110  /* Master Descriptor Size */
+#define ISMT_MSTR_RPOLICY 0x114  /* Retry Policy */
+
+/* iSMT Miscellaneous Registers */
+#define ISMT_SPGT  0x300  /* SMBus PHY Global Timing */
+
+/* General Control Register (GCTRL) bit definitions */
+#define ISMT_GCTRL_TRST 0x04    /* Target Reset */
+#define ISMT_GCTRL_KILL 0x08    /* Kill */
+#define ISMT_GCTRL_SRST 0x40    /* Soft Reset */
+
+/* Master Control Register (MCTRL) bit definitions */
+#define ISMT_MCTRL_SS    0x01       /* Start/Stop */
+#define ISMT_MCTRL_MEIE  0x10       /* Master Error Interrupt Enable */

drivers/i2c/busses/i2c-ismt.h new file +7 -0

+
+/* MSI Control Register (MSICTL) bit definitions */
+#define ISMT_MSICTL_MSIE 0x01 /* MSI Enable */
+
+/* iSMT Hardware Descriptor */
+struct ismt_desc {
+	u8 tgtaddr_rw; /* target address & r/w bit */

drivers/i2c/busses/i2c-ismt.h new file +4 -0

+	struct completion cmp; /* interrupt completion */
+};
+
+#endif

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Sunday, September 30, 2012 1:45 PM
To: Brown, Bill E
Cc: [email protected]
Subject: Re: [PATCH v2] i2c: Adding support for Intel iSMT SMBus 2.0 host controller

Hi Bill,

Quoted 15 lines · Bill Brown

The iSMT (Intel SMBus Message Transport) supports multi-master I2C/SMBus, as well as IPMI. It's operation is DMA-based and utilizes descriptors to initiate transactions on the bus.

The iSMT hardware can act as both a master and a target, although this driver only supports being a master.

Signed-off-by Bill Brown <[email protected]>

---
 drivers/i2c/busses/Kconfig    |   10 +
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-ismt.c |  845 
+++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-ismt.h |  111 ++++++
 4 files changed, 967 insertions(+), 0 deletions(-)  create mode 

Quoted 2 lines

100644 drivers/i2c/busses/i2c-ismt.c create mode 100644 drivers/i2c/busses/i2c-ismt.h

Here's my review, sorry for the delay. The driver looks overall good and most of my comments are suggestions for improvements, either for code readability or performance. For these it's up to you if you want to follow my advice or not.

There are a few real bugs too though, which you will _have_ to fix. If you're quick we can be in time for kernel 3.7.

Quoted 24 lines

drivers/i2c/busses/Kconfig +10 -0

index 7244c8b..64d5756 100644
@@ -119,6 +119,16 @@ config I2C_ISCH
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-isch.
 
+config I2C_ISMT
+	tristate "Intel iSMT SMBus Controller"
+	depends on PCI
+	help
+	  If you say yes to this option, support will be included for the Intel
+	  iSMT SMBus host controller interface.
+
+	  This driver can also be built as a module.  If so, the module will be
+	  called i2c-ismt.
+
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI

drivers/i2c/busses/Makefile +0 -1

index ce3c2be..694711a 100644
--- a/drivers/i2c/busses/Makefile

drivers/i2c/busses/Makefile obj-$(CONFIG_I2C_SIS630) += i2c-sis630.o · line 25

Quoted 6 lines

+++ b/drivers/i2c/busses/Makefile

drivers/i2c/busses/Makefile +1 -0

@@ -22,6 +22,7 @@ obj-$(CONFIG_I2C_SIS630)	+= i2c-sis630.o
 obj-$(CONFIG_I2C_SIS96X)	+= i2c-sis96x.o
 obj-$(CONFIG_I2C_VIA)		+= i2c-via.o
 obj-$(CONFIG_I2C_VIAPRO)	+= i2c-viapro.o
+obj-$(CONFIG_I2C_ISMT)		+= i2c-ismt.o

In alphabetical order please.

Quoted 12 lines

drivers/i2c/busses/Makefile +0 -0

 # Mac SMBus host controller drivers
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o

(quoted diff) +0 -0

b/drivers/i2c/busses/i2c-ismt.c new file mode 100644 index 0000000..46df042

drivers/i2c/busses/i2c-ismt.c new file +2 -0

@@ -0,0 +1,845 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using 

drivers/i2c/busses/i2c-ismt.c line 7

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright(c) 2012 Intel Corporation. All rights reserved.

Copyright is independent from licensing, so please move the copyright statement outside of the license blocks. This will avoid duplicating the copyright statement.

Quoted 14 lines

drivers/i2c/busses/i2c-ismt.c new file +14 -0

+ *
+ * This program is free software; you can redistribute it and/or 
+ modify
+ * it under the terms of version 2 of the GNU General Public License 
+ as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, 
+ but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License

drivers/i2c/busses/i2c-ismt.c line 27

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ * The full GNU General Public License is included in this 
+ distribution
+ * in the file called LICENSE.GPL.
+ *
+ * BSD LICENSE
+ *
+ * Copyright(c) 2012 Intel Corporation. All rights reserved.
+ * All rights reserved.

Very reserved it seems ;)

Quoted 33 lines

drivers/i2c/busses/i2c-ismt.c new file +32 -0

+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in
+ *     the documentation and/or other materials provided with the
+ *     distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ *     contributors may be used to endorse or promote products derived
+ *     from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 
+ CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS 
+ FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE 
+ COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, 
+ INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF 
+ USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON 
+ ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR 
+ TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE 

drivers/i2c/busses/i2c-ismt.c line 57

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+ USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/*
+  Supports the SMBus Message Transport (SMT) on the following Intel SOCs:

Please use standard comment style for consistency (as above.)

drivers/i2c/busses/i2c-ismt.c line 59

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.c new file +2 -0

+  BWD
+  CTN

Very cryptic names. Can you use something more user-friendly?

drivers/i2c/busses/i2c-ismt.c line 62

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+
+  Features supported by this driver:
+  Software PEC                     no

Does the hardware support this at all? If not, there's no point in mentioning it.

drivers/i2c/busses/i2c-ismt.c line 65

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+  Hardware PEC                     yes
+  Block buffer                     yes
+  Block process call transaction   no

Same question here.

drivers/i2c/busses/i2c-ismt.c line 74

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+  Slave mode                       no
+*/
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/stddef.h>
+#include <linux/delay.h>

It doesn't look like you need this one. OTOH you need <linux/completion.h>, which is missing. Including <linux/dma-mapping.h> would probably be a good idea too.

drivers/i2c/busses/i2c-ismt.c line 81

Quoted 8 lines

drivers/i2c/busses/i2c-ismt.c new file +7 -0

+#include <linux/i2c.h>
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include "i2c-ismt.h"
+
+/**
+ * DEFINE_PCI_DEVICE_TABLE - PCI device IDs supported by this driver

You're describing ismt_ids not DEFINE_PCI_DEVICE_TABLE.

drivers/i2c/busses/i2c-ismt.c

Quoted 13 lines

drivers/i2c/busses/i2c-ismt.c new file +12 -0

+ */
+static const DEFINE_PCI_DEVICE_TABLE(ismt_ids) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT0) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT1) },
+	{ 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, ismt_ids);
+
+/* Master Descriptor control bits */
+static unsigned int stop_on_error = 1; module_param(stop_on_error, 
+uint, S_IRUGO); MODULE_PARM_DESC(stop_on_error, "Stop on Error");

I'd write "error" with no leading capital, for consistency.

drivers/i2c/busses/i2c-ismt.c line 98

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+
+static unsigned int fair = 1;
+module_param(fair, uint, S_IRUGO);
+MODULE_PARM_DESC(fair, "Enable fairness on the SMBus");

It would make sense to make these parameters writable by root in sysfs, so that the settings can be changed at run-time.

You could also make these bools rather than uints, I think this gets you stricter input validation, and maybe some optimizations too.

Both options would be good to document in Documentation/i2c/busses/i2c-ismt, as their effect isn't obvious. See
i2c-i801 in this directory for the formatting. You don't have to be extra verbose if you don't have the time, just write the information that you think is useful for other users and developers.

drivers/i2c/busses/i2c-ismt.c line 107

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+
+#ifdef DEBUG
+/**
+ * ismt_desc_dump() - dump the contents of a descriptor for debug 
+purposes
+ * @adap: the I2C host adapter
+ */
+static void ismt_desc_dump(struct i2c_adapter *adap) {
+	struct ismt_priv *priv = i2c_get_adapdata(adap);

In this function and the 2 other debug functions below, you don't need adap, so you better pass priv as the function parameter.

Quoted 72 lines

drivers/i2c/busses/i2c-ismt.c new file +71 -0

+	struct device *dev = &priv->pci_dev->dev;
+	struct ismt_desc *desc = &priv->hw[priv->head];
+
+	dev_dbg(dev, "Dump of the descriptor struct:  0x%X\n", priv->head);
+	dev_dbg(dev, "\ttgtaddr_rw=0x%02X\n", desc->tgtaddr_rw);
+	dev_dbg(dev, "\twr_len_cmd=0x%02X\n", desc->wr_len_cmd);
+	dev_dbg(dev, "\trd_len=    0x%02X\n", desc->rd_len);
+	dev_dbg(dev, "\tcontrol=   0x%02X\n", desc->control);
+	dev_dbg(dev, "\tstatus=    0x%02X\n", desc->status);
+	dev_dbg(dev, "\tretry=     0x%02X\n", desc->retry);
+	dev_dbg(dev, "\trxbytes=   0x%02X\n", desc->rxbytes);
+	dev_dbg(dev, "\ttxbytes=   0x%02X\n", desc->txbytes);
+	dev_dbg(dev, "\tdptr_low=  0x%08X\n", desc->dptr_low);
+	dev_dbg(dev, "\tdptr_high= 0x%08X\n", desc->dptr_high); }
+
+/**
+ * ismt_gen_reg_dump() - dump the iSMT General Registers
+ * @adap: the I2C host adapter
+ */
+static void ismt_gen_reg_dump(struct i2c_adapter *adap) {
+	struct ismt_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = &priv->pci_dev->dev;
+
+	dev_dbg(dev, "Dump of the iSMT General Registers\n");
+	dev_dbg(dev, "  GCTRL.... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_GCTRL,
+		readl(priv->smba + ISMT_GR_GCTRL));
+	dev_dbg(dev, "  SMTICL... : (0x%p)=0x%016lX\n",
+		priv->smba + ISMT_GR_SMTICL,
+		readq(priv->smba + ISMT_GR_SMTICL));
+	dev_dbg(dev, "  ERRINTMSK : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRINTMSK,
+		readl(priv->smba + ISMT_GR_ERRINTMSK));
+	dev_dbg(dev, "  ERRAERMSK : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRAERMSK,
+		readl(priv->smba + ISMT_GR_ERRAERMSK));
+	dev_dbg(dev, "  ERRSTS... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRSTS,
+		readl(priv->smba + ISMT_GR_ERRSTS));
+	dev_dbg(dev, "  ERRINFO.. : (0x%p)=0x%X\n",
+		priv->smba + ISMT_GR_ERRINFO,
+		readl(priv->smba + ISMT_GR_ERRINFO)); }
+
+/**
+ * ismt_mstr_reg_dump() - dump the iSMT Master Registers
+ * @adap: the I2C host adapter
+ */
+static void ismt_mstr_reg_dump(struct i2c_adapter *adap) {
+	struct ismt_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = &priv->pci_dev->dev;
+
+	dev_dbg(dev, "Dump of the iSMT Master Registers\n");
+	dev_dbg(dev, "  MDBA..... : (0x%p)=0x%016lX\n",
+		priv->smba + ISMT_MSTR_MDBA,
+		readq(priv->smba + ISMT_MSTR_MDBA));
+	dev_dbg(dev, "  MCTRL.... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MCTRL,
+		readl(priv->smba + ISMT_MSTR_MCTRL));
+	dev_dbg(dev, "  MSTS..... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MSTS,
+		readl(priv->smba + ISMT_MSTR_MSTS));
+	dev_dbg(dev, "  MDS...... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_MDS,
+		readl(priv->smba + ISMT_MSTR_MDS));
+	dev_dbg(dev, "  RPOLICY.. : (0x%p)=0x%X\n",
+		priv->smba + ISMT_MSTR_RPOLICY,
+		readl(priv->smba + ISMT_MSTR_RPOLICY));
+	dev_dbg(dev, "  SPGT..... : (0x%p)=0x%X\n",
+		priv->smba + ISMT_SPGT,
+		readl(priv->smba + ISMT_SPGT));
+}

drivers/i2c/busses/i2c-ismt.c line 195

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+
+#else
+static void ismt_desc_dump(struct i2c_adapter *adap) {} static void 
+ismt_gen_reg_dump(struct i2c_adapter *adap) {} static void 
+ismt_mstr_reg_dump(struct i2c_adapter *adap) {} #endif

This prevents using the dynamic debug feature. The 3 debug functions do nothing by default if DEBUG isn't set, so I think they should depend on
defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG). That way they can be included but disabled by default under DYNAMIC_DEBUG, and you can enable specific debugging messages at run-time. Same is done in net/netfilter/nf_conntrack_pptp.c for example.

drivers/i2c/busses/i2c-ismt.c

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+
+/**
+ * ismt_insert_cmd() -  stuff a command into the head of the data 
+buffer

Extra space after dash.

drivers/i2c/busses/i2c-ismt.c line 198

Quoted 7 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+ * @data: data buffer
+ * @command: command to insert
+ */
+static void ismt_insert_cmd(union i2c_smbus_data *data, u8 command) {
+	memmove(&data->block[1], &data->block[0], I2C_SMBUS_BLOCK_MAX);
+	data->block[0] = command;

Ouch, no, you can't do that, sorry. This union is not under your full control. The caller must be able to re-use the same union with no change to issue the same SMBus transaction. As a matter of fact, i2c-core.c:i2c_smbus_xfer() will do exactly this if it detects arbitration loss.

Obviously if you modify the contents like that, it will break. So, to put it short: if you need buffers for your own use, allocate them as part of your private data structure.

As a side note, it may be faster anyway. The memmove() above had to move 32 bytes one by one because the areas overlap, that was certainly slow.

Also note that you're only supposed to access data->block for block transactions. For other transactions it's ->byte or ->word. You probably don't care much about endianness in this specific driver, but if you had to, the above would inevitably fail for word transactions on either endianness.

drivers/i2c/busses/i2c-ismt.c line 208

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+}
+
+/**
+ * ismt_submit_desc() - add a descriptor to the ring
+ * @adap: the i2c host adapter
+ */
+static void ismt_submit_desc(struct i2c_adapter *adap) {
+	int fmhp;
+	int val;

These should be unsigned, as you use them for readl()/writel().

drivers/i2c/busses/i2c-ismt.c line 213

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+	struct ismt_priv *priv = i2c_get_adapdata(adap);
+
+	ismt_desc_dump(adap);
+	ismt_gen_reg_dump(adap);
+	ismt_mstr_reg_dump(adap);

I fail to see the rationale for putting these debug calls here. In this function you don't need adap, so by moving the debug calls back to ismt_access(), you could pass priv directly as a parameter and save a few bytes.

drivers/i2c/busses/i2c-ismt.c line 219

Quoted 7 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+
+	/* Set the FMHP (Firmware Master Head Pointer)*/
+	fmhp = ((priv->head + 1) % ISMT_DESC_ENTRIES) << 16;
+	val = readl(priv->smba + ISMT_MSTR_MCTRL);
+	writel((val & ~(ISMT_MCTRL_FMHP)) | fmhp,
+		(priv->smba + ISMT_MSTR_MCTRL));

Alignment is off by one, and parentheses around (ISMT_MCTRL_FMHP) and (priv->smba + ISMT_MSTR_MCTRL) aren't needed.

drivers/i2c/busses/i2c-ismt.c line 224

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+
+	/* Set the start bit */
+	val = readl(priv->smba + ISMT_MSTR_MCTRL);
+	writel((val | ISMT_MCTRL_SS),
+	       (priv->smba + ISMT_MSTR_MCTRL));

Many unneeded parentheses here too.

drivers/i2c/busses/i2c-ismt.c line 234

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+}
+
+/**
+ * ismt_process_desc() - handle the completion of the descriptor
+ * @adap: the i2c host adapter
+ */
+static int ismt_process_desc(struct i2c_adapter *adap) {
+	struct ismt_desc *desc;
+	struct ismt_priv *priv = i2c_get_adapdata(adap);

All you really need in this function is desc, and you have it at hand on the calling side. So just pass "desc" as the function parameter. This saves 32 binary bytes on x86_64. You can even make desc const as the function doesn't modify it.

drivers/i2c/busses/i2c-ismt.c line 239

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+
+	desc = &priv->hw[priv->head];
+
+	if (desc->status & ISMT_DESC_SCS)
+		return 0;

We can probably assume that this is the most frequent case, and use likely() to help gcc optimize the most frequent path.

drivers/i2c/busses/i2c-ismt.c line 242

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+
+	if ((desc->status & ISMT_DESC_NAK) || (desc->status & ISMT_DESC_CRC))
+		return -ENXIO;

According to Documentation/i2c/fault-codes, CRC (PEC) errors are to be reported with -EBADMSG.

NAK is probably the second most likely case, as it happens on device probing, so again likely() may be good to use.

drivers/i2c/busses/i2c-ismt.c line 247

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+
+	if (desc->status & ISMT_DESC_COL)
+		return -EAGAIN;
+
+	return 0;

Can it really be a success if ISMT_DESC_SCS wasn't set? I doubt it.

Quoted 11 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+}
+
+/**
+ * ismt_access() - process an SMBus command
+ * @adap: the i2c host adapter
+ * @addr: address of the i2c/SMBus target
+ * @flags: command options
+ * @read_write: read from or write to device
+ * @command: the i2c/SMBus command to issue
+ * @size: SMBus transaction type

drivers/i2c/busses/i2c-ismt.c line 264

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+ * @data: read/write data buffer
+ */
+static int ismt_access(struct i2c_adapter *adap, u16 addr,
+		       unsigned short flags, char read_write, u8 command,
+		       int size, union i2c_smbus_data *data) {
+	unsigned int ret = 0; /* return code */

You won't catch (negative!) error codes with an unsigned int.

The comment /* return code */ is pretty redundant, BTW, it's quite clear what this variable is for. And initialization isn't needed either.

drivers/i2c/busses/i2c-ismt.c line 268

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+	dma_addr_t dma_addr = 0; /* address of the data buffer */
+	u8 dma_size = 0;
+	enum dma_data_direction dma_direction = 0;
+	bool map_dma_flag = 0;

Could you do without this flag? AFAICS map_dma_flag always evaluates to the same as dma_size != 0.

drivers/i2c/busses/i2c-ismt.c line 274

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.c new file +2 -0

+	struct ismt_desc *desc;
+	struct ismt_priv *priv = i2c_get_adapdata(adap);

I'd suggest that you also declare:

	struct device *dev = &priv->pci_dev->dev; and use it in all debug and error messages, this will make the code much more readable!

drivers/i2c/busses/i2c-ismt.c line 279

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+
+	desc = &priv->hw[priv->head];
+
+	/* Initialize the descriptor */
+	memset(desc, 0, sizeof(struct ismt_desc));
+	desc->tgtaddr_rw = ISMT_DESC_ADDR_RW(addr, read_write);
+
+	/* Initialize common control bits */
+	desc->control |= ISMT_DESC_INT;

You just zeroed this field with memset(), so "=" will do the same as "|=", and is probably more efficient.

drivers/i2c/busses/i2c-ismt.c line 285

Quoted 7 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+
+	if (stop_on_error)
+		desc->control |= ISMT_DESC_SOE;
+
+	if ((flags & I2C_CLIENT_PEC) && (size != I2C_SMBUS_QUICK)
+		&& (size != I2C_SMBUS_I2C_BLOCK_DATA))

Please align the && on opening parenthesis to avoid confusion between the condition and the action.

Quoted 99 lines

drivers/i2c/busses/i2c-ismt.c new file +98 -0

+		desc->control |= ISMT_DESC_PEC;
+
+	if (fair)
+		desc->control |= ISMT_DESC_FAIR;
+
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_QUICK\n");
+		break;
+
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/*
+			 * Send Byte
+			 * The command field contains the write data
+			 */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE:  WRITE\n");
+			desc->control |= ISMT_DESC_CWRL;
+			desc->wr_len_cmd = command;
+		} else {
+			/* Receive Byte */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE:  READ\n");
+			dma_size = 1;
+			dma_direction = DMA_FROM_DEVICE;
+			map_dma_flag = 1;
+			desc->rd_len = 1;
+		}
+
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/*
+			 * Write Byte
+			 * Command plus 1 data byte
+			 */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:  WRITE\n");
+			desc->wr_len_cmd = 2;
+
+			/* Stuff the command ahead of the data in the buffer */
+			ismt_insert_cmd(data, command);
+			dma_size = 2;
+			dma_direction = DMA_TO_DEVICE;
+		} else {
+			/* Read Byte */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
+			desc->control |= ISMT_DESC_CWRL;
+			desc->wr_len_cmd = command;
+			desc->rd_len = 1;
+			dma_size = 1;
+			dma_direction = DMA_FROM_DEVICE;
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/* Write Word */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:  WRITE\n");
+			desc->wr_len_cmd = 3;
+
+			/* Stuff the command ahead of the data in the buffer */
+			ismt_insert_cmd(data, command);
+			dma_size = 3;
+			dma_direction = DMA_TO_DEVICE;
+		} else {
+			/* Read Word */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_WORD_DATA:  READ\n");
+			desc->wr_len_cmd = command;
+			desc->control |= ISMT_DESC_CWRL;
+			desc->rd_len = 2;
+			dma_size = 2;
+			dma_direction = DMA_FROM_DEVICE;
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_PROC_CALL:
+		dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_PROC_CALL\n");
+		desc->wr_len_cmd = 3;
+		desc->rd_len = 2;
+
+		/* Stuff the command ahead of the data in the buffer */
+		ismt_insert_cmd(data, command);
+		dma_size = 3;
+		dma_direction = DMA_BIDIRECTIONAL;
+		map_dma_flag = 1;
+		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			/* Block Write */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BLOCK_DATA:  WRITE\n");
+			dma_size = data->block[0] + 1;
+			dma_direction = DMA_TO_DEVICE;
+			desc->wr_len_cmd = dma_size;

drivers/i2c/busses/i2c-ismt.c line 389

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+			desc->control |= ISMT_DESC_BLK;
+			ismt_insert_cmd(data, command);
+		} else {
+			/* Block Read */
+			dev_dbg(&priv->pci_dev->dev, "I2C_SMBUS_BLOCK_DATA:  READ\n");
+			dma_size = data->block[0] + 1;

Probably not. For SMBus block reads, the block length is _received_ from the slave, so at this point you don't know it. So you probably have to map the maximum (I2C_SMBUS_BLOCK_MAX == 32 bytes) always.

Quoted 49 lines

drivers/i2c/busses/i2c-ismt.c new file +48 -0

+			dma_direction = DMA_FROM_DEVICE;
+			desc->rd_len = dma_size;
+			desc->wr_len_cmd = command;
+			desc->control |= (ISMT_DESC_BLK | ISMT_DESC_CWRL);
+		}
+
+		map_dma_flag = 1;
+		break;
+
+	default:
+		dev_err(&priv->pci_dev->dev, "Unsupported transaction %d\n",
+			size);
+		return -EOPNOTSUPP;
+	}
+
+	/* map the data buffer */
+	if (map_dma_flag) {
+		dev_dbg(&priv->pci_dev->dev,
+			" &priv->pci_dev->dev=%p\n", &priv->pci_dev->dev);
+		dev_dbg(&priv->pci_dev->dev, " data=%p\n", data);
+		dev_dbg(&priv->pci_dev->dev, " dma_size=%d\n", dma_size);
+		dev_dbg(&priv->pci_dev->dev,
+			" dma_direction=%d\n", dma_direction);
+
+		dma_addr = dma_map_single(&priv->pci_dev->dev,
+				      data,
+				      dma_size,
+				      dma_direction);
+
+		dev_dbg(&priv->pci_dev->dev, " dma_addr = 0x%016llX\n",
+			dma_addr);
+
+		desc->dptr_low = lower_32_bits(dma_addr);
+		desc->dptr_high = upper_32_bits(dma_addr);
+	}
+
+	INIT_COMPLETION(priv->cmp);
+
+	/* Add the descriptor */
+	ismt_submit_desc(adap);
+
+	/* Now we wait for interrupt completion, 1s */
+	ret = wait_for_completion_interruptible_timeout(&priv->cmp, HZ*1);
+
+	/* unmap the data buffer */
+	if (map_dma_flag)
+		dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
+

drivers/i2c/busses/i2c-ismt.c line 443

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+	if (ret < 0) {
+		dev_err(&priv->pci_dev->dev, "completion wait interrupted\n");
+		ret = -EIO;
+	} else if (ret == 0) {
+		dev_err(&priv->pci_dev->dev, "completion wait timed out\n");
+		ret = -ETIMEDOUT;

Both are unlikely and you could help the compiler optimize the most common case.

Quoted 19 lines

drivers/i2c/busses/i2c-ismt.c new file +18 -0

+	} else
+		/* do any post processing of the descriptor here */
+		ret = ismt_process_desc(adap);
+
+	/* Update the ring pointer */
+	priv->head++;
+	priv->head %= ISMT_DESC_ENTRIES;
+
+	return ret;
+}
+
+/**
+ * ismt_func() - report which i2c commands are supported by this 
+adapter
+ * @adap: the i2c host adapter
+ */
+static u32 ismt_func(struct i2c_adapter *adap) {
+	return  I2C_FUNC_SMBUS_QUICK           |

drivers/i2c/busses/i2c-ismt.c line 467

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+		I2C_FUNC_SMBUS_BYTE            |
+		I2C_FUNC_SMBUS_BYTE_DATA       |
+		I2C_FUNC_SMBUS_WORD_DATA       |
+		I2C_FUNC_SMBUS_PROC_CALL       |
+		I2C_FUNC_SMBUS_BLOCK_DATA      |
+		I2C_FUNC_SMBUS_PEC;

Unusual spacing... There should be a single space between return and I2C_FUNC_SMBUS_QUICK, and if you really want to align the "|"s that should be done with tabs, not spaces.

drivers/i2c/busses/i2c-ismt.c

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +5 -0

+}
+
+/**
+ * struct i2c_algorithm - the adapter algorithm and supported 
+functionality

Your structure is actually named smbus_algorithm. Anyway this syntax is meant to describe structures as you define them, not as you instantiate them. I think you have to omit the "struct" here.

Quoted 17 lines

drivers/i2c/busses/i2c-ismt.c new file +16 -0

+ * @smbus_xfer: the adapter algorithm
+ * @functionality: functionality supported by the adapter  */ static 
+const struct i2c_algorithm smbus_algorithm = {
+	.smbus_xfer	= ismt_access,
+	.functionality	= ismt_func,
+};
+
+/**
+ * ismt_handle_isr() - interrupt handler bottom half
+ * @priv: iSMT private data
+ */
+static irqreturn_t ismt_handle_isr(struct ismt_priv *priv) {
+	complete(&priv->cmp);
+
+	return IRQ_HANDLED;
+}

drivers/i2c/busses/i2c-ismt.c line 495

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+
+
+/**
+ * ismt_do_interrupt() - IRQ interrupt handler
+ * @vec: interrupt vector
+ * @data:  iSMT private data

Extra space after colon.

drivers/i2c/busses/i2c-ismt.c line 500

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+ */
+static irqreturn_t ismt_do_interrupt(int vec, void *data) {
+	u32 val;
+	struct ismt_priv *priv = (struct ismt_priv *)data;

You never need to explicitly cast a void * pointer.

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+
+	/*
+	 * check to see it's our interrupt, return IRQ_NONE if not ours

drivers/i2c/busses/i2c-ismt.c line 513

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+	 * since we are sharing interrupt
+	 */
+	val = readl(priv->smba + ISMT_MSTR_MSTS);
+
+	if (!(val & (ISMT_MSTS_MIS | ISMT_MSTS_MEIS)))
+		return IRQ_NONE;
+
+	if (val & ISMT_MSTS_MIS) {
+		/* completed successfully */
+		writel((val | ISMT_MSTS_MIS), priv->smba + ISMT_MSTR_MSTS);

Parentheses not needed. Plus I doubt this is what you want to do... if (val & ISMT_MSTS_MIS) then (val | ISMT_MSTS_MIS) == val. Maybe you really meant "&" instead of "|"?

Could it be that this code was never tested because MSI interrupts were always available? Please inject a pci_enable_msi error in ismt_int_init() to test this code path.

drivers/i2c/busses/i2c-ismt.c line 516

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+	} else {
+		/* completed with errors */
+		writel((val | ISMT_MSTS_MEIS), priv->smba + ISMT_MSTR_MSTS);

Same here.

Furthermore, I would like to see the conditional go. I'm sure you can do without it, and this will speed up the code. Isn't it OK to write back both ISMT_MSTS_MIS and ISMT_MSTS_MEIS unconditionally? This would simplify the code a lot. If this isn't OK, then you can still remember which flag was set and write only that flag back, with:

	val = readl(priv->smba + ISMT_MSTR_MSTS) &
	      (ISMT_MSTS_MIS | ISMT_MSTS_MEIS);

	if (!val)
		return IRQ_NONE;

	writel(val, priv->smba + ISMT_MSTR_MSTS);

This too simplifies the code a lot (-27 binary bytes on x86_64) which is good to keep the latency low.

I was also wondering if it was possible that both flags were set at the same time? My code would deal with this case properly, yours did not.

drivers/i2c/busses/i2c-ismt.c line 525

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+	}
+
+	return ismt_handle_isr(priv);
+}
+
+/**
+ * ismt_do_msi_interrupt() - MSI interrupt handler
+ * @vec: interrupt vector
+ * @data:  iSMT private data

Extra space after colon.

drivers/i2c/busses/i2c-ismt.c line 500

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.c new file +3 -0

+ */
+static irqreturn_t ismt_do_msi_interrupt(int vec, void *data) {
+	struct ismt_priv *priv = (struct ismt_priv *)data;

Here again the cast isn't needed. In fact you can pass data to ismt_handle_isr() directly, so you don't even need to introduce priv.

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+
+	return ismt_handle_isr(priv);
+}
+

drivers/i2c/busses/i2c-ismt.c line 544

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+/**
+ * ismt_hw_init() - initialize the iSMT hardware
+ * @pdev: PCI-Express device
+ */
+static void __devinit ismt_hw_init(struct pci_dev *pdev) {
+	u32 val;
+	struct ismt_priv *priv = pci_get_drvdata(pdev);
+
+	/* initialize the Master Descriptor Base Address (MDBA) */
+	writeq(priv->io_rng_dma, priv->smba + ISMT_MSTR_MDBA);

writeq() doesn't exist on 32-bit architectures, causing a build failure.

If this driver has to work on 32-bit systems, you'll have to come up with a workaround. If not, I suggest we make it depend on CONFIG_X86_64. I doubt this driver is of any use besides X86 anyway, right?

drivers/i2c/busses/i2c-ismt.c line 554

Quoted 11 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+
+	/* initialize the Master Control Register (MCTRL) */
+	writel(ISMT_MCTRL_MEIE, priv->smba + ISMT_MSTR_MCTRL);
+
+	/* initialize the Master Status Register (MSTS) */
+	writel(0, priv->smba + ISMT_MSTR_MSTS);
+
+	/* initialize the Master Descriptor Size (MDS) */
+	val = readl(priv->smba + ISMT_MSTR_MDS);
+	writel((val & ~(ISMT_MDS_MDS)) | (ISMT_DESC_ENTRIES - 1),

Parentheses around ISMT_MDS_MDS aren't needed.

drivers/i2c/busses/i2c-ismt.c line 563

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +9 -0

+		priv->smba + ISMT_MSTR_MDS);
+
+#ifdef DEBUG_SLOW_HW
+	/*
+	 * initialize the SMBus speed to 80KHz for slow HW debuggers
+	 */
+	dev_dbg(&pdev->dev, " Setting SMBus clock to 80KHz\n");
+	val = readl(priv->smba + ISMT_SPGT);
+	writel(((val & ~(ISMT_SPGT_SPD)) | ISMT_SPGT_SPD_80K),

Same here. But I don't think we want to keep this in the upstream driver, at least not in this form. If it's still useful it should be controlled by a module parameter, not a hidden define.

drivers/i2c/busses/i2c-ismt.c line 567

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+	       priv->smba + ISMT_SPGT);
+#endif
+
+	dev_dbg(&pdev->dev, " priv->smba=%p\n", priv->smba);

This would rather belong to the ismt_probe function IMHO. If needed at all...

drivers/i2c/busses/i2c-ismt.c line 574

Quoted 8 lines

drivers/i2c/busses/i2c-ismt.c new file +7 -0

+}
+
+/**
+ * ismt_init() - initialize the iSMT data structures
+ * @pdev: PCI-Express Device
+ */
+static int __devinit ismt_init(struct pci_dev *pdev)

This name is a little confusing with i2c_ismt_init later in the driver. What about ismt_dev_init?

Quoted 25 lines

drivers/i2c/busses/i2c-ismt.c new file +24 -0

+{
+	struct ismt_priv *priv = pci_get_drvdata(pdev);
+
+	priv->entries = ISMT_DESC_ENTRIES;
+
+	/* allocate memory for the descriptor */
+	priv->hw = dmam_alloc_coherent(&pdev->dev,
+				       (ISMT_DESC_ENTRIES
+					       * sizeof(struct ismt_desc)),
+				       &priv->io_rng_dma,
+				       GFP_KERNEL);
+	if (!priv->hw)
+		return -ENOMEM;
+
+	memset(priv->hw, 0, (ISMT_DESC_ENTRIES * sizeof(struct ismt_desc)));
+
+	priv->head = 0;
+	priv->tail = 0;
+	init_completion(&priv->cmp);
+
+	return 0;
+}
+
+/**

drivers/i2c/busses/i2c-ismt.c

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+ * ismt_int_init() - initialize interrupts
+ * @pdev: PCI-Express device
+ * @priv: iSMT private data
+ */
+static int __devinit ismt_int_init(struct pci_dev *pdev, struct 
+ismt_priv *priv)

That's not consistent with the two previous functions where you derive priv from pdev. Please do the same in all 3 functions. Given that gcc is very likely to inline the functions, I suggest passing the priv parameter always, as this is simplified at inlining time.

drivers/i2c/busses/i2c-ismt.c line 609

Quoted 7 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+{
+	int err;
+
+	/* Try using MSI interrupts */
+	err = pci_enable_msi(pdev);
+	if (err)

Maybe a warning message would be good to have, indicating that we're falling back to legacy interrupts? Or is it supposed to happen on many systems?

Quoted 13 lines

drivers/i2c/busses/i2c-ismt.c new file +12 -0

+		goto intx;
+
+	err = devm_request_irq(&pdev->dev,
+			       pdev->irq,
+			       ismt_do_msi_interrupt,
+			       0,
+			       "ismt-msi",
+			       priv);
+
+	if (err) {
+		pci_disable_msi(pdev);
+		goto intx;

drivers/i2c/busses/i2c-ismt.c line 627

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+	}
+
+	goto done;
+
+	/* Try using legacy interrupts */
+intx:

Out of curiosity, what's the "x" for?

Quoted 32 lines

drivers/i2c/busses/i2c-ismt.c new file +31 -0

+	err = devm_request_irq(&pdev->dev,
+			       pdev->irq,
+			       ismt_do_interrupt,
+			       IRQF_SHARED,
+			       "ismt-intx",
+			       priv);
+	if (err) {
+		dev_err(&pdev->dev, "no usable interrupts\n");
+		return -ENODEV;
+	}
+
+done:
+	return 0;
+}
+
+static struct pci_driver ismt_driver;
+
+/**
+ * ismt_probe() - probe for iSMT devices
+ * @pdev: PCI-Express device
+ * @id: PCI-Express device ID
+ */
+static int __devinit
+ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id) {
+	int err;
+	struct ismt_priv *priv;
+	unsigned long start, len;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;

drivers/i2c/busses/i2c-ismt.c line 665

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+
+	pci_set_drvdata(pdev, priv);
+	i2c_set_adapdata(&priv->adapter, priv);
+	priv->adapter.owner = THIS_MODULE;
+
+	priv->adapter.class = I2C_CLASS_HWMON;

Can't there be SPD EEPROMs on this bus?

Quoted 23 lines

drivers/i2c/busses/i2c-ismt.c new file +22 -0

+
+	priv->adapter.algo = &smbus_algorithm;
+	priv->pci_dev = pdev;
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable SMBus PCI device (%d)\n",
+			err);
+		return err;
+	}
+
+	/* enable bus mastering */
+	pci_set_master(pdev);
+
+	/* Determine the address of the SMBus area */
+	start = pci_resource_start(pdev, SMBBAR);
+	if (!start) {
+		dev_err(&pdev->dev,
+			"SMBus base address uninitialized, upgrade BIOS\n");
+		return -ENODEV;
+	}
+

drivers/i2c/busses/i2c-ismt.c line 693

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+	len = pci_resource_len(pdev, SMBBAR);
+	if (len == 0) {
+		dev_err(&pdev->dev,
+			"SMBus base address uninitialized, upgrade BIOS\n");
+		return -ENODEV;
+	}

As you are printing the same error message, you can refactor the two tests to make the code more simple.

Quoted 31 lines

drivers/i2c/busses/i2c-ismt.c new file +30 -0

+
+	dev_dbg(&priv->pci_dev->dev, " start=0x%lX\n", start);
+	dev_dbg(&priv->pci_dev->dev, " len=0x%lX\n", len);
+
+	err = acpi_check_resource_conflict(&pdev->resource[SMBBAR]);
+	if (err) {
+		dev_err(&pdev->dev, "ACPI resource conflict!\n");
+		return err;
+	}
+
+	err = pci_request_region(pdev, SMBBAR, ismt_driver.name);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Failed to request SMBus region 0x%lx-0x%lx\n",
+			start, start + len);
+		return err;
+	}
+
+	priv->smba = pcim_iomap(pdev, SMBBAR, len);
+	if (!priv->smba) {
+		dev_err(&pdev->dev, "Unable to ioremap SMBus BAR\n");
+		err = -ENODEV;
+		goto fail;
+	}
+
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
+	if (err) {
+		err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+		if (err)
+			goto fail;

drivers/i2c/busses/i2c-ismt.c line 733

Quoted 10 lines

drivers/i2c/busses/i2c-ismt.c new file +10 -0

+		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
+	}
+
+	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
+	if (err) {
+		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+		if (err)
+			goto fail;
+		dev_warn(&pdev->dev, "Cannot DMA consistent highmem\n");
+	}

This doesn't look safe to me. What if
pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) succeeds but pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)) fails, or vice-versa? I'd rather do (with a disclaimer: I don't know anything about this "consistent DMA" thing):

	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
	if (err) {
		/* Fall back to 32-bit - lowmem */
		err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
		if (err)
			goto fail;
		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
		if (err)
			goto fail;
		dev_warn(&pdev->dev, "Cannot DMA highmem, falling back to lowmem\n");
	} else {
		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
		if (err)
			goto fail;
	}

(Or the more compact variants found in drivers/scsi/vmw_pvscsi.c or drivers/scsi/bfa/bfad.c for example.)

What do you think? To my candid eyes, setting a different value for
pci_set_consistent_dma_mask() than for pci_set_dma_mask() seems incorrect. OTOH it seems that some other drivers are doing exactly that so maybe I'm just wrong.

drivers/i2c/busses/i2c-ismt.c line 737

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.c new file +4 -0

+
+	err = ismt_init(pdev);
+	if (err) {
+		err = -ENODEV;

Please just return the err value returned by the called function.

Quoted 12 lines

drivers/i2c/busses/i2c-ismt.c new file +11 -0

+		goto fail;
+	}
+
+	ismt_hw_init(pdev);
+
+	err = ismt_int_init(pdev, priv);
+	if (err)
+		goto fail;
+
+	/* set up the sysfs linkage to our parent device */
+	priv->adapter.dev.parent = &pdev->dev;

drivers/i2c/busses/i2c-ismt.c line 754

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+
+	/* number of retries on lost arbitration */
+	priv->adapter.retries = ISMT_MAX_RETRIES;
+
+	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
+		 "SMBus iSMT adapter at %p", priv->smba);

A good I2C adapter name is stable across reboots, this one is not. It's better to use "start" as the unique value, as it will stay the same across reboots.

As a side note, it may make sense, for performance and clarity, to group the initialization of all priv->adapter fields in one place. You initialize 4 fields at the top of the function and 3 fields at the bottom, for no obvious reason.

Quoted 17 lines

drivers/i2c/busses/i2c-ismt.c new file +16 -0

+	err = i2c_add_adapter(&priv->adapter);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to add SMBus iSMT adapter\n");
+		err = -ENODEV;
+		goto fail;
+	}
+	return 0;
+
+fail:
+	pci_release_region(pdev, SMBBAR);
+	return err;
+}
+
+/**
+ * ismt_remove() - release driver resources
+ * @pdev: PCI-Express device

drivers/i2c/busses/i2c-ismt.c line 777

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+ */
+static void __devexit ismt_remove(struct pci_dev *pdev) {
+	struct ismt_priv *priv = pci_get_drvdata(pdev);
+
+	writel(ISMT_GCTRL_SRST, priv->smba + ISMT_GR_GCTRL);
+	i2c_del_adapter(&priv->adapter);

This is most certainly racy, I think you want to delete the I2C adapter first. BTW, what's the rationale for unconditionally resetting the bus here?

Quoted 49 lines

drivers/i2c/busses/i2c-ismt.c new file +48 -0

+	pci_release_region(pdev, SMBBAR);
+}
+
+/**
+ * ismt_suspend() - place the device in suspend
+ * @pdev: PCI-Express device
+ * @mesg: PM message
+ */
+#ifdef CONFIG_PM
+static int ismt_suspend(struct pci_dev *pdev, pm_message_t mesg) {
+	pci_save_state(pdev);
+	pci_set_power_state(pdev, pci_choose_state(pdev, mesg));
+	return 0;
+}
+
+/**
+ * ismt_resume() - PCI resume code
+ * @pdev: PCI-Express device
+ */
+static int ismt_resume(struct pci_dev *pdev) {
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	return pci_enable_device(pdev);
+}
+
+#else
+
+#define ismt_suspend NULL
+#define ismt_resume NULL
+
+#endif
+
+static struct pci_driver ismt_driver = {
+	.name = "ismt_smbus",
+	.id_table = ismt_ids,
+	.probe = ismt_probe,
+	.remove = __devexit_p(ismt_remove),
+	.suspend = ismt_suspend,
+	.resume = ismt_resume,
+};
+
+/**
+ * i2c_ismt_init() - iSMT driver initialization  */ static int __init 
+i2c_ismt_init(void) {
+	pr_debug("Loading the iSMT SMBus driver\n");
+	return pci_register_driver(&ismt_driver);
+}
+

drivers/i2c/busses/i2c-ismt.c line 837

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.c new file +6 -0

+/**
+ * i2c_ismt_exit() - iSMT driver exit code  */ static void __exit 
+i2c_ismt_exit(void) {
+	pr_debug("Unloading iSMT SMBus driver\n");
+	pci_unregister_driver(&ismt_driver);
+}

If you can live without the two debugging messages above (and they don't strike me as terribly useful) you can use module_pci_driver().

drivers/i2c/busses/i2c-ismt.c line 845

Quoted 9 lines

drivers/i2c/busses/i2c-ismt.c new file +8 -0

+
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_AUTHOR("Bill E. Brown <[email protected]>"); 
+MODULE_DESCRIPTION("Intel SMBus Message Transport (iSMT) driver");
+
+module_init(i2c_ismt_init);
+module_exit(i2c_ismt_exit);
+

No blank line at end of file please.

drivers/i2c/busses/i2c-ismt.h

Quoted 6 lines

(quoted diff) +0 -0

b/drivers/i2c/busses/i2c-ismt.h new file mode 100644 index 0000000..e9c10e1

drivers/i2c/busses/i2c-ismt.h new file +0 -0

Is there any reason why you'd ever need to include this file from another driver file than i2c-ismt.c? If not, then please just move all these definitions at the top of i2c-ismt.c. This speeds up the build.

drivers/i2c/busses/i2c-ismt.h line 9

Quoted 11 lines

drivers/i2c/busses/i2c-ismt.h new file +9 -0

@@ -0,0 +1,111 @@
+#ifndef _I2C_ISMT_H_
+#define _I2C_ISMT_H_
+
+/* PCI Address Constants */
+#define SMBBAR		0
+
+/* PCI DIDs for Briarwood's pair of SMBus Message Transport (SMT) 
+Devices */ #define PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT0 0x0c59 #define 
+PCI_DEVICE_ID_INTEL_BWD_SMBUS_SMT1 0x0c5a

Please align IDs using tabs, it will improve readability especially in case we ever have to add more (and history suggests we will.) Same everywhere below, all values and comments (at least within a given block) should be aligned, using tabs.

drivers/i2c/busses/i2c-ismt.h line 11

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.h new file +2 -0

+
+#define ISMT_DESC_ENTRIES 32 /* number of descritor entries */

Typo: descriptor

Quoted 18 lines

drivers/i2c/busses/i2c-ismt.h new file +17 -0

+#define ISMT_MAX_RETRIES 3   /* number of SMBus retries to attempt */
+
+/* Hardware Descriptor Constants - Control Field */
+#define ISMT_DESC_CWRL  0x01    /* Command/Write Length */
+#define ISMT_DESC_BLK   0X04    /* Perform Block Transaction */
+#define ISMT_DESC_FAIR  0x08    /* Set fairness flag upon successful arbit. */
+#define ISMT_DESC_PEC   0x10    /* Packet Error Code */
+#define ISMT_DESC_I2C   0x20    /* I2C Enable */
+#define ISMT_DESC_INT   0x40    /* Interrupt */
+#define ISMT_DESC_SOE   0x80    /* Stop On Error */
+
+/* Hardware Descriptor Constants - Status Field */
+#define ISMT_DESC_SCS   0x01    /* Success */
+#define ISMT_DESC_DLTO  0x04    /* Data Low Time Out */
+#define ISMT_DESC_NAK   0x08    /* NAK Received */
+#define ISMT_DESC_CRC   0x10    /* CRC Error */
+#define ISMT_DESC_CLTO  0x20    /* Clock Low Time Out */

drivers/i2c/busses/i2c-ismt.h line 34

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.h new file +6 -0

+#define ISMT_DESC_COL   0x40    /* Collisions */
+#define ISMT_DESC_LPR   0x80    /* Large Packet Received */
+
+/* Hardware Descriptor Masks */
+#define ISMT_DESC_ADDR  0x7f    /* I2C/SMB address mask */
+#define ISMT_DESC_RW    0x01    /* Read/Write bit mask */

I don't think it's worth introducing these for a single use where the constants would be just as clear.

drivers/i2c/busses/i2c-ismt.h line 38

Quoted 5 lines

drivers/i2c/busses/i2c-ismt.h new file +4 -0

+
+/* Macros */
+#define ISMT_DESC_ADDR_RW(addr, rw) (((addr & ISMT_DESC_ADDR) << 1)	\
+				     | (rw & ISMT_DESC_RW))

The masking doesn't seem to be terribly needed anyway, it would work the same (and be marginally faster) without. Also note that addr and rw must be enclosed in parentheses for your macro to be safe.

drivers/i2c/busses/i2c-ismt.h line 40

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.h new file +2 -0

+
+/* iSMT General Register address offsets (SMBAR + <addr>) */

Typo: SMBBAR

Quoted 31 lines

drivers/i2c/busses/i2c-ismt.h new file +30 -0

+#define ISMT_GR_GCTRL      0x000 /* General Control */
+#define ISMT_GR_SMTICL     0x008 /* SMT Interrupt Cause Location */
+#define ISMT_GR_ERRINTMSK  0x010 /* Error Interrupt Mask */ #define 
+ISMT_GR_ERRAERMSK  0x014 /* Error AER Mask */
+#define ISMT_GR_ERRSTS     0x018 /* Error Status */
+#define ISMT_GR_ERRINFO    0x01c /* Error Information */
+
+/* iSMT Master Registers */
+#define ISMT_MSTR_MDBA    0x100  /* Master Descriptor Base Address */
+#define ISMT_MSTR_MCTRL   0x108  /* Master Control */
+#define ISMT_MSTR_MSTS    0x10c  /* Master Status */
+#define ISMT_MSTR_MDS     0x110  /* Master Descriptor Size */
+#define ISMT_MSTR_RPOLICY 0x114  /* Retry Policy */
+
+/* iSMT Miscellaneous Registers */
+#define ISMT_SPGT  0x300  /* SMBus PHY Global Timing */
+
+/* General Control Register (GCTRL) bit definitions */
+#define ISMT_GCTRL_TRST 0x04    /* Target Reset */
+#define ISMT_GCTRL_KILL 0x08    /* Kill */
+#define ISMT_GCTRL_SRST 0x40    /* Soft Reset */
+
+/* Master Control Register (MCTRL) bit definitions */
+#define ISMT_MCTRL_SS    0x01       /* Start/Stop */
+#define ISMT_MCTRL_MEIE  0x10       /* Master Error Interrupt Enable */
+#define ISMT_MCTRL_FMHP  0x00ff0000 /* Firmware Master Head Pointer 
+(FMHP) */
+
+/* Master Status Register (MSTS) bit definitions */ #define 
+ISMT_MSTS_HMTP  0xff0000 /* HW Master Tail Pointer (HMTP) */

drivers/i2c/busses/i2c-ismt.h

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.h new file +6 -0

+#define ISMT_MSTS_MIS   0x20     /* Master Interrupt Status (MIS) */
+#define ISMT_MSTS_MEIS  0x10     /* Master Error Interrupt Status (MEIS) */
+#define ISMT_MSTS_IP    0x01     /* In Progress */
+
+/* Master Descriptor Size (MDS) bit definitions */ #define 
+ISMT_MDS_MDS  0xFF /* Master Descriptor Size mask (MDS) */

I tend to prefer when masks are clearly identified as such with a trailing _MASK.

drivers/i2c/busses/i2c-ismt.h line 78

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.h new file +3 -0

+
+/* SMBus PHY Global Timing Register (SPGT) bit definitions */
+#define ISMT_SPGT_SPD     0xc0000000   /* SMBus Speed mask */

Same here.

drivers/i2c/busses/i2c-ismt.h line 79

Quoted 2 lines

drivers/i2c/busses/i2c-ismt.h new file +1 -0

+#define ISMT_SPGT_SPD_80K (0x01 << 30) /* 80 KHz */

kHz with lower-case k, please.

Please define the other possible speeds, in case people without the datasheet need to experiment or debug.

Quoted 11 lines

drivers/i2c/busses/i2c-ismt.h new file +10 -0

+
+/* MSI Control Register (MSICTL) bit definitions */ #define 
+ISMT_MSICTL_MSIE 0x01 /* MSI Enable */
+
+/* iSMT Hardware Descriptor */
+struct ismt_desc {
+	u8 tgtaddr_rw; /* target address & r/w bit */
+	u8 wr_len_cmd; /* write length in bytes or a command */
+	u8 rd_len; /* read length */
+	u8 control; /* control bits */

drivers/i2c/busses/i2c-ismt.h

Quoted 6 lines

drivers/i2c/busses/i2c-ismt.h new file +6 -0

+	u8 status; /* status bits */
+	u8 retry; /* collision retry and retry count */
+	u8 rxbytes; /* received bytes */
+	u8 txbytes; /* transmitted bytes */
+	u32 dptr_low; /* lower 32 bit of the data pointer */
+	u32 dptr_high; /* upper 32 bit of the data pointer */ };

Aren't you supposed to declare it with __packed to guarantee it matches the hardware's idea of the descriptor?

drivers/i2c/busses/i2c-ismt.h line 102

Quoted 7 lines

drivers/i2c/busses/i2c-ismt.h new file +6 -0

+
+struct ismt_priv {
+	struct i2c_adapter adapter;
+	void *smba; /* PCI BAR */
+	struct pci_dev *pci_dev;
+	struct ismt_ring_ent **ring; /* housekeeping struct pointer */

This structure isn't defined anywhere, and this member is never used.

drivers/i2c/busses/i2c-ismt.h line 105

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.h new file +3 -0

+	struct ismt_desc *hw; /* virtual base address of the descriptor */
+	dma_addr_t io_rng_dma; /* hardware base address of the descriptor */
+	int entries; /* number of descriptor entries */

This member is assigned once and never used after that. You hard-coded ISMT_DESC_ENTRIES everywhere in the driver. If there's no reason to use another value, just drop priv->entries. If there is a good reason to ever change it, keep and consistently use priv->entries everywhere.

drivers/i2c/busses/i2c-ismt.h line 107

Quoted 3 lines

drivers/i2c/busses/i2c-ismt.h new file +2 -0

+	u8 head; /* ring buffer head pointer */
+	u8 tail; /* ring buffer tail pointer */

Same here, assigned once and never used after that.

drivers/i2c/busses/i2c-ismt.h line 110

Quoted 4 lines

drivers/i2c/busses/i2c-ismt.h new file +3 -0

+	struct completion cmp; /* interrupt completion */ };
+
+#endif

--
Jean Delvare