I2C driver supporting Moorestown and Medfield platform

(And the correct patch attached this time)

From: Wen Wang <[email protected]>

Initial release of the driver. Updated and verified on hardware.

Cleaned up as follows

Alan Cox:
Squash down the switches into tables, and use the PCI ident field. We could perhaps take this further and put the platform and port number into this.
uint32t -> u32
bracketing of case statements
spacing and '!' usage
Check the speed (which is now 0/1/2) is valid and ignore otherwise.

Arjan van de Ven:
Initial power management hooks

Signed-off-by Wen Wang <[email protected]>

Signed-off-by Arjan van de Ven <arjan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

---

 drivers/i2c/busses/Kconfig    |    9 
 drivers/i2c/busses/Makefile   |    1 
 drivers/i2c/busses/i2c-mrst.c | 1082 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1092 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-mrst.c

drivers/i2c/busses/Kconfig +2 -0

index 15a9702..46b9acb 100644
@@ -420,6 +420,15 @@ config I2C_IXP2000
 	  This driver is deprecated and will be dropped soon. Use i2c-gpio
 	  instead.
 
+config I2C_MRST
+	tristate "Intel Moorestown/Medfield Platform I2C controller"

hardware/software resources, but IMO, MRST is not easy to imagine that it represents "Moorstone". But this is not such a big deal.

Quoted 2 lines

+ * Copyright (c) 2009 Intel Corporation.
+ * Copyright (c) 2009 Synopsys. Inc.

Having a quick glance of the patch, the driver is similar to the existing I2C_DESIGNWARE driver. Presumably Intel people have already checkekd the i2c-designware.c and made a decision that it would be reasonable to create a new I2C driver to incorporate its requirements (such as PCI resource handlings, power management code, etc.), rather than to extend the existing one. Is my understanding correct?

I'm not objecting to the patch, but would like to know the background of this driver, as one of the DW users.

There are various little differences along with the PCI, power management, timing and configuration side. I've also looked at the two

side by side and agreed with the original Intel authors that it seemed best to keep it as two drivers. Otherwise you drag a lot of PCI and PCI pm logic into a simpler driver that is used on non PCI platforms.

(And the correct patch attached this time)

From: Wen Wang <[email protected]>

Initial release of the driver. Updated and verified on hardware.

Cleaned up as follows

Alan Cox:
Squash down the switches into tables, and use the PCI ident field. We could perhaps take this further and put the platform and port number into this.
uint32t -> u32
bracketing of case statements
spacing and '!' usage
Check the speed (which is now 0/1/2) is valid and ignore otherwise.

Arjan van de Ven:
Initial power management hooks

Signed-off-by Wen Wang <[email protected]>

Signed-off-by Arjan van de Ven <arjan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

---

 drivers/i2c/busses/Kconfig    |    9 
 drivers/i2c/busses/Makefile   |    1 
 drivers/i2c/busses/i2c-mrst.c | 1082 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1092 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-mrst.c

drivers/i2c/busses/Kconfig +0 -0

index 15a9702..46b9acb 100644
@@ -420,6 +420,15 @@ config I2C_IXP2000
 	  This driver is deprecated and will be dropped soon. Use i2c-gpio

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

+struct mrst_i2c_private {
+	struct i2c_adapter adap;
+	/* Register base address */
+	void __iomem *base;
+	/* Speed mode */
+	int speed;
+	int pm_state;
+	struct completion complete;

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

+
+static inline void mrst_i2c_write(void __iomem *reg, u32 val)
+{
+	__raw_writel(val, reg);
+}
+
 
+/**
+ * mrst_i2c_address_neq - To check if the addresses for different i2c messages
+ * are equal.
+ * @p1: first i2c_msg
+ * @p2: second i2c_msg
+ *
+ * Return Values:
+ * 0	 if addresses are equal
+ * 1	 if not equal
+ *
+ * Within a single transfer, I2C client may need to send its address more
+ * than one time. So a check if the addresses match  is needed.

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

+
+/**
+ * xfer_write - Internal function to implement master write transfer.
+ * @adap: i2c_adapter struct pointer
+ * @buf: buffer in i2c_msg
+ * @length: number of bytes to be read
+ *
+ * Return Values:
+ * 0	if the read transfer succeeds
+ * -ETIMEDOUT	if cannot read the "raw" interrupt register
+ * -EINVAL	if transfer abort occured
+ *
+ * For every byte, a "WRITE" command will be loaded into IC_DATA_CMD prior to
+ * data transfer. The actual "write" operation will be performed when the
+ * RX_FULL interrupt signal occurs.
+ *
+ * Note there may be two interrupt signals captured, one should read
+ * IC_RAW_INTR_STAT to separate between errors and actual data.
+ */
+static int xfer_write(struct i2c_adapter *adap,
+		      unsigned char *buf, int length)
+{
+	struct mrst_i2c_private *i2c = i2c_get_adapdata(adap);
+	int i, err;
+
+	mrst_i2c_write(i2c->base + IC_INTR_MASK, 0x0050);
+	mrst_i2c_read(i2c->base + IC_CLR_INTR);
+

drivers/i2c/busses/i2c-mrst.c new file +116 -0

+	i2c->status = STATUS_WRITE_START;
+	err = wait_for_completion_interruptible_timeout(&i2c->complete, HZ);
+	if (!err) {
+		dev_err(&adap->dev, "Timeout for ACK from I2C slave device\n");
+		mrst_i2c_hwinit(i2c);
+		return -ETIMEDOUT;
+	} else {
+		if (i2c->status == STATUS_WRITE_SUCCESS)
+			return 0;
+		else
+			return -EIO;
+	}
+}
 
 
+
+/**
+ * mrst_i2c_probe - I2C controller initialization routine
+ * @dev: pci device
+ * @id: device id
+ *
+ * Return Values:
+ * 0		success
+ * -ENODEV	If cannot allocate pci resource
+ * -ENOMEM	If the register base remapping failed, or
+ *		if kzalloc failed
+ *
+ * Initialization steps:
+ * 1. Request for PCI resource
+ * 2. Remap the start address of PCI resource to register base
+ * 3. Request for device memory region
+ * 4. Fill in the struct members of mrst_i2c_private
+ * 5. Call mrst_i2c_hwinit() for hardware initialization
+ * 6. Register I2C adapter in i2c-core
+ */
+static int __devinit mrst_i2c_probe(struct pci_dev *dev,
+				    const struct pci_device_id *id)
+{
+	struct mrst_i2c_private *mrst;
+	unsigned long start, len;
+	int err, busnum;
+	void __iomem *base = NULL;
+
+	dev_dbg(&dev->dev, "Get into probe function for I2C\n");
+	err = pci_enable_device(dev);
+	if (err) {
+		dev_err(&dev->dev, "Failed to enable I2C PCI device (%d)\n",
+			err);
+		goto exit;
+	}
+
+	/* Determine the address of the I2C area */
+	start = pci_resource_start(dev, 0);
+	len = pci_resource_len(dev, 0);
+	if (!start || len == 0) {
+		dev_err(&dev->dev, "base address not set\n");
+		err = -ENODEV;
+		goto exit;
+	}
+	dev_dbg(&dev->dev, "%s i2c resource start 0x%lx, len=%ld\n",
+		PLATFORM, start, len);
+
+	err = pci_request_region(dev, 0, DRIVER_NAME);
+	if (err) {
+		dev_err(&dev->dev, "failed to request I2C region "
+			"0x%lx-0x%lx\n", start,
+			(unsigned long)pci_resource_end(dev, 0));
+		goto exit;
+	}
+
+	base = ioremap_nocache(start, len);
+	if (!base) {
+		dev_err(&dev->dev, "I/O memory remapping failed\n");
+		err = -ENOMEM;
+		goto fail0;
+	}
+
+	/* Allocate the per-device data structure, mrst_i2c_private */
+	mrst = kzalloc(sizeof(struct mrst_i2c_private), GFP_KERNEL);
+	if (mrst == NULL) {
+		dev_err(&dev->dev, "can't allocate interface\n");
+		err = -ENOMEM;
+		goto fail1;
+	}
+
+	/* Initialize struct members */
+	snprintf(mrst->adap.name, sizeof(&mrst->adap.name),
+		"MRST/Medfield I2C at %lx", start);
+	mrst->adap.owner = THIS_MODULE;
+	mrst->adap.algo = &mrst_i2c_algorithm;
+	mrst->adap.dev.parent = &dev->dev;
+	mrst->base = base;
+	mrst->speed = STANDARD;
+	mrst->pm_state = ACTIVE;
+	mrst->abort = 0;
+	mrst->rx_buf_len = 0;
+	mrst->status = STATUS_IDLE;
+
+	pci_set_drvdata(dev, mrst);
+	i2c_set_adapdata(&mrst->adap, mrst);
+
+	mrst->adap.nr = busnum = id->driver_data;
+	if (dev->device <= 0x0804)
+		mrst->platform = MOORESTOWN;
+	else
+		mrst->platform = MEDFIELD;
+
+	dev_dbg(&dev->dev, "I2C%d\n", busnum);
+
+	if (ctl_num > busnum) {
+		if (speed_mode[busnum] < 0 || speed_mode[busnum] >= NUM_SPEEDS)
+			dev_warn(&dev->dev, "invalid speed %d ignored.\n",
+							speed_mode[busnum]);
+		else
+			mrst->speed = speed_mode[busnum];
+	}
+
+	/* Initialize i2c controller */

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

+	mrst_i2c_disable(&mrst->adap);
+	if (i2c_del_adapter(&mrst->adap))
+		dev_err(&dev->dev, "Failed to delete i2c adapter");
+
+	free_irq(dev->irq, mrst);
+	pci_set_drvdata(dev, NULL);
+	iounmap(mrst->base);
+	kfree(mrst);
+	pci_release_region(dev, 0);
+}
+
+static struct pci_device_id mrst_i2c_ids[] = {
+	/* Moorestown */
+	{PCI_VDEVICE(INTEL, 0x0802), 0 },

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

+};
+MODULE_DEVICE_TABLE(pci, mrst_i2c_ids);

I would much prefer this to be called i2c-moorsetown, we have modern systems which can handle >8 character names.

Hmm, if this is a synopsys block, then is it a standard one and if so can we get some standard support?

drivers/i2c/busses/i2c-mrst.c line 293

Quoted 4 lines

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

+static inline u32 mrst_i2c_read(void __iomem *reg)
+{
+	return __raw_readl(reg);
+}

ioremap, but using __raw_read and __raw_write? readl/writel should be used. if they can't be used, then please explain why.

drivers/i2c/busses/i2c-mrst.c line 432

Quoted 4 lines

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

+	if ((p1->flags ^ p2->flags) & I2C_M_TEN)
+		return 1;
+	return 0;
+}

would have been better to return bool.

drivers/i2c/busses/i2c-mrst.c line 580

Quoted 3 lines

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

+	for (i = 0; i < length; i++)
+		mrst_i2c_write(i2c->base + IC_DATA_CMD,
+			       (uint16_t)(*(buf + i)));

you say length in bytes, but write u16?
also, would be neater to have a u16 *buf?

drivers/i2c/busses/i2c-mrst.c line 990

Quoted 4 lines

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

+	mutex_init(&mrst->lock);
+	init_completion(&mrst->complete);
+	err = request_irq(dev->irq, mrst_i2c_isr, IRQF_DISABLED,
+			  mrst->adap.name, mrst);

are you sure you want this, IRQF_DISABLED is marked DEPRECATED.

drivers/i2c/busses/i2c-mrst.c line 1031

Quoted 2 lines

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

+	struct mrst_i2c_private *mrst = (struct mrst_i2c_private *)
+					pci_get_drvdata(dev);

no need to cast.

drivers/i2c/busses/i2c-mrst.c line 1055

Quoted 3 lines

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

+	{PCI_VDEVICE(INTEL, 0x082E), 2 },
+	{0,}

style, "{ "

Quoted 2 lines

I would much prefer this to be called i2c-moorsetown, we have modern systems which can handle >8 character names.

Moorestown. Everything else in the kernel uses 'mrst' so this would make the driver differ from the rest of the tree. I don't care too much what its called. Given its now for Medfield etc as well it probably should be i2c_intel_mid to match the rest of the kernel

Quoted 2 lines

Hmm, if this is a synopsys block, then is it a standard one and if so can we get some standard support?

See other reply on this one.

Quoted 6 lines

drivers/i2c/busses/i2c-mrst.c line 432

Quoted 4 lines

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

+	if ((p1->flags ^ p2->flags) & I2C_M_TEN)
+		return 1;
+	return 0;
+}

would have been better to return bool.

Can do but most of the kernel never uses bool so again it would be a quirky style. If its preferred i2c style no problem.

Quoted 7 lines

drivers/i2c/busses/i2c-mrst.c line 580

Quoted 4 lines

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

+	for (i = 0; i < length; i++)
+		mrst_i2c_write(i2c->base + IC_DATA_CMD,
+			       (uint16_t)(*(buf + i)));

you say length in bytes, but write u16?
also, would be neater to have a u16 *buf?

Ermm no - that would be most peculiar as you'd then only send every alternate byte of data. Then again the cast ought to be implied by the types of mrst_i2c_write - will review