[PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Guennadi Liakhovetski]

This is an updated and cleaned up i2c driver for i.MX3x from Freescale.

Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>

---

Please comment, I'll also have to obtain ARM Acks for the arch/arm part, 
or, maybe, split it into two parts.

diff --git a/arch/arm/mach-mx3/devices.c b/arch/arm/mach-mx3/devices.c
index 3ef1227..7c66f7c 100644
--- a/arch/arm/mach-mx3/devices.c
+++ b/arch/arm/mach-mx3/devices.c
@@ -23,6 +23,7 @@
 #include <linux/gpio.h>
 
 #include <mach/hardware.h>
+#include <mach/i2c.h>
 #include <mach/imx-uart.h>
 #include <mach/ipu.h>
 #include <mach/mxcfb.h>
@@ -229,3 +230,94 @@ static int __init register_ipu(void)
 
 /* This is needed for the framebuffer, so, initialise early */
 arch_initcall(register_ipu);
+
+/*
+ * Resource definition for the I2C1
+ */
+static struct resource mxci2c1_resources[] = {
+	[0] = {
+		.start = I2C_BASE_ADDR,
+		.end = I2C_BASE_ADDR + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	}, [1] = {
+		.start = MXC_INT_I2C,
+		.end = MXC_INT_I2C,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+/*
+ * Resource definition for the I2C2
+ */
+static struct resource mxci2c2_resources[] = {
+	[0] = {
+		.start = I2C2_BASE_ADDR,
+		.end = I2C2_BASE_ADDR + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	}, [1] = {
+		.start = MXC_INT_I2C2,
+		.end = MXC_INT_I2C2,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+/*
+ * Resource definition for the I2C3
+ */
+static struct resource mxci2c3_resources[] = {
+	[0] = {
+		.start = I2C3_BASE_ADDR,
+		.end = I2C3_BASE_ADDR + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	}, [1] = {
+		.start = MXC_INT_I2C3,
+		.end = MXC_INT_I2C3,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+/* Device definition for MXC I2C */
+static struct platform_device mxci2c_devices[] = {
+	[0] = {
+		.name = "mxc-i2c",
+		.id = 0,
+		.num_resources = ARRAY_SIZE(mxci2c1_resources),
+		.resource = mxci2c1_resources,
+	}, [1] = {
+		.name = "mxc-i2c",
+		.id = 1,
+		.num_resources = ARRAY_SIZE(mxci2c2_resources),
+		.resource = mxci2c2_resources,
+	}, [2] = {
+		.name = "mxc-i2c",
+		.id = 2,
+		.num_resources = ARRAY_SIZE(mxci2c3_resources),
+		.resource = mxci2c3_resources,
+	},
+};
+
+int mxc_i2c_register_adapters(struct mxc_i2c_board_data *data, int n)
+{
+	int i;
+
+	if (n > 3 || !data) {
+		printk(KERN_ERR "i2c-mxc: Wrong platform data!\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < n; i++) {
+		unsigned int id = data[i].id;
+		if (id > 2) {
+			printk(KERN_ERR
+			       "i2c-mxc: Bad platform data #%d ID: %u!\n",
+			       i, id);
+			return -EINVAL;
+		}
+		mxci2c_devices[id].dev.platform_data = &data[i].data;
+		if (platform_device_register(&mxci2c_devices[id]) < 0)
+			printk(KERN_ERR
+			       "i2c-mxc: Failed to register bus %u\n", id);
+	}
+
+	return 0;
+}
diff --git a/arch/arm/plat-mxc/include/mach/i2c.h b/arch/arm/plat-mxc/include/mach/i2c.h
new file mode 100644
index 0000000..ce50af5
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/i2c.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 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
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#ifndef __ASM_ARCH_I2C_H__
+#define __ASM_ARCH_I2C_H__
+
+struct mxc_i2c_platform_data {
+	u32 i2c_clk;
+};
+
+struct mxc_i2c_board_data {
+	struct mxc_i2c_platform_data	data;
+	unsigned int			id;
+};
+
+extern int mxc_i2c_register_adapters(struct mxc_i2c_board_data *, int);
+
+#endif
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 7f95905..055a33a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -401,6 +401,15 @@ config I2C_MV64XXX
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mv64xxx.
 
+config I2C_MXC
+	tristate "MXC I2C support"
+	depends on I2C && ARCH_MXC
+	help
+	  If you say yes to this option, support will be included for Freescale
+	  MXC I2C modules.
+
+	  This driver can also be built as a module.
+
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	depends on EXPERIMENTAL
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 0c2c4b2..495f040 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
+obj-$(CONFIG_I2C_MXC)		+= i2c-mxc.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
diff --git a/drivers/i2c/busses/i2c-mxc.c b/drivers/i2c/busses/i2c-mxc.c
new file mode 100644
index 0000000..8b4e52f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mxc.c
@@ -0,0 +1,744 @@
+/*
+ * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#include <mach/i2c.h>
+#include <mach/iomux-mx3.h>
+
+/* Address offsets of the I2C registers */
+#define MXC_IADR                0x00	/* Address Register */
+#define MXC_IFDR                0x04	/* Freq div register */
+#define MXC_I2CR                0x08	/* Control regsiter */
+#define MXC_I2SR                0x0C	/* Status register */
+#define MXC_I2DR                0x10	/* Data I/O register */
+
+/* Bit definitions of I2CR */
+#define MXC_I2CR_IEN            0x0080
+#define MXC_I2CR_IIEN           0x0040
+#define MXC_I2CR_MSTA           0x0020
+#define MXC_I2CR_MTX            0x0010
+#define MXC_I2CR_TXAK           0x0008
+#define MXC_I2CR_RSTA           0x0004
+
+/* Bit definitions of I2SR */
+#define MXC_I2SR_ICF            0x0080
+#define MXC_I2SR_IAAS           0x0040
+#define MXC_I2SR_IBB            0x0020
+#define MXC_I2SR_IAL            0x0010
+#define MXC_I2SR_SRW            0x0004
+#define MXC_I2SR_IIF            0x0002
+#define MXC_I2SR_RXAK           0x0001
+
+#define MXC_ADAPTER_NAME        "MXC I2C Adapter"
+
+/*
+ * In case the MXC device has multiple I2C modules, this structure is used to
+ * store information specific to each I2C module.
+ */
+struct mxc_i2c_device {
+	/*
+	 * This structure is used to identify the physical i2c bus along with
+	 * the access algorithms necessary to access it.
+	 */
+	struct i2c_adapter adap;
+
+	/*
+	 * This waitqueue is used to wait for the data transfer to complete.
+	 */
+	wait_queue_head_t wq;
+
+	/*
+	 * The base address of the I2C device.
+	 */
+	void __iomem *membase;
+
+	/*
+	 * The interrupt number used by the I2C device.
+	 */
+	int irq;
+
+	/*
+	 * The default clock divider value to be used.
+	 */
+	unsigned int clkdiv;
+
+	/*
+	 * The clock source for the device.
+	 */
+	struct clk *clk;
+
+	/*
+	 * The current power state of the device
+	 */
+	bool low_power;
+
+	/*
+	 * Boolean to indicate if data was transferred
+	 */
+	bool transfer_done;
+
+	/*
+	 * Boolean to indicate if we received an ACK for the data transmitted
+	 */
+	bool tx_success;
+};
+
+struct clk_div_table {
+	int reg_value;
+	int div;
+};
+
+static const struct clk_div_table i2c_clk_table[] = {
+	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
+	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
+	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
+	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
+	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
+	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
+	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
+	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
+	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
+	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
+	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
+	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
+	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
+	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
+	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
+	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
+	{0, 0}
+};
+
+static void gpio_i2c_active(int i2c_num)
+{
+	switch (i2c_num) {
+	case 0:
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_CLK, IOMUX_CONFIG_FUNC));
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_DAT, IOMUX_CONFIG_FUNC));
+		break;
+	case 1:
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MOSI, IOMUX_CONFIG_ALT1));
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MISO, IOMUX_CONFIG_ALT1));
+		break;
+	case 2:
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SS2, IOMUX_CONFIG_ALT1));
+		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SCLK, IOMUX_CONFIG_ALT1));
+	}
+}
+
+static void gpio_i2c_inactive(int i2c_num)
+{
+	/* The one who needs the pins should configure them */
+}
+
+/**
+ * Transmit a \b STOP signal to the slave device.
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ */
+static void mxc_i2c_stop(struct mxc_i2c_device * dev)
+{
+	unsigned int cr;
+	int retry = 200;
+
+	cr = readw(dev->membase + MXC_I2CR);
+	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
+	writew(cr, dev->membase + MXC_I2CR);
+
+	/*
+	 * Make sure STOP meets setup requirement.
+	 */
+	for (;;) {
+		unsigned int sr = readw(dev->membase + MXC_I2SR);
+		if ((sr & MXC_I2SR_IBB) == 0)
+			break;
+		if (retry-- <= 0) {
+			printk(KERN_DEBUG "Bus busy\n");
+			break;
+		}
+		udelay(3);
+	}
+}
+
+/**
+ * Wait for the transmission of the data byte to complete. This function waits
+ * till we get a signal from the interrupt service routine indicating completion
+ * of the address cycle or we time out.
+ *
+ * @param   dev         the mxc i2c structure used to get to the right i2c device
+ * @param   trans_flag  transfer flag
+ *
+ *
+ * @return  The function returns 0 on success or -1 if an ack was not received
+ */
+
+static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
+{
+	int retry = 16;
+
+	while (retry-- && !dev->transfer_done)
+		wait_event_interruptible_timeout(dev->wq,
+						 dev->transfer_done,
+						 dev->adap.timeout);
+
+	dev->transfer_done = false;
+
+	if (retry <= 0) {
+		/* Unable to send data */
+		dev_warn(&dev->adap.dev, "Data not transmitted\n");
+		return -ETIMEDOUT;
+	}
+
+	if (!dev->tx_success) {
+		/* An ACK was not received for transmitted byte */
+		dev_dbg(&dev->adap.dev, "ACK not received \n");
+		return -EREMOTEIO;
+	}
+
+	return 0;
+}
+
+/**
+ * Transmit a \b START signal to the slave device.
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ * @param   *msg  pointer to a message structure that contains the slave
+ *                address
+ */
+static void mxc_i2c_start(struct mxc_i2c_device * dev, struct i2c_msg *msg)
+{
+	unsigned int cr, sr;
+	unsigned int addr_trans;
+	int retry = 16;
+
+	/*
+	 * Set the slave address and the requested transfer mode
+	 * in the data register
+	 */
+	addr_trans = msg->addr << 1;
+	if (msg->flags & I2C_M_RD)
+		addr_trans |= 0x01;
+
+	/* Set the Master bit */
+	cr = readw(dev->membase + MXC_I2CR);
+	cr |= MXC_I2CR_MSTA;
+	writew(cr, dev->membase + MXC_I2CR);
+
+	/* Wait till the Bus Busy bit is set */
+	sr = readw(dev->membase + MXC_I2SR);
+	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
+		udelay(3);
+		sr = readw(dev->membase + MXC_I2SR);
+	}
+	if (retry <= 0)
+		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");
+
+	/* Set the Transmit bit */
+	cr = readw(dev->membase + MXC_I2CR);
+	cr |= MXC_I2CR_MTX;
+	writew(cr, dev->membase + MXC_I2CR);
+
+	writew(addr_trans, dev->membase + MXC_I2DR);
+}
+
+/**
+ * Transmit a \b REPEAT START to the slave device
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ * @param   *msg  pointer to a message structure that contains the slave
+ *                address
+ */
+static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
+{
+	unsigned int cr;
+	unsigned int addr_trans;
+
+	/*
+	 * Set the slave address and the requested transfer mode
+	 * in the data register
+	 */
+	addr_trans = msg->addr << 1;
+	if (msg->flags & I2C_M_RD)
+		addr_trans |= 0x01;
+
+	cr = readw(dev->membase + MXC_I2CR);
+	cr |= MXC_I2CR_RSTA;
+	writew(cr, dev->membase + MXC_I2CR);
+	udelay(3);
+	writew(addr_trans, dev->membase + MXC_I2DR);
+}
+
+/**
+ * Read the received data. The function waits till data is available or times
+ * out. Generates a stop signal if this is the last message to be received.
+ * Sends an ack for all the bytes received except the last byte.
+ *
+ * @param  dev       the mxc i2c structure used to get to the right i2c device
+ * @param  *msg      pointer to a message structure that contains the slave
+ *                   address and a pointer to the receive buffer
+ * @param  last      indicates that this is the last message to be received
+ * @param  addr_comp flag indicates that we just finished the address cycle
+ *
+ * @return  The function returns the number of bytes read or -1 on time out.
+ */
+static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
+			     int last, int addr_comp)
+{
+	int i;
+	char *buf = msg->buf;
+	int len = msg->len;
+	unsigned int cr;
+
+	cr = readw(dev->membase + MXC_I2CR);
+	/*
+	 * Clear MTX to switch to receive mode.
+	 */
+	cr &= ~MXC_I2CR_MTX;
+	/*
+	 * Clear the TXAK bit to gen an ack when receiving only one byte.
+	 */
+	if (len == 1)
+		cr |= MXC_I2CR_TXAK;
+	else
+		cr &= ~MXC_I2CR_TXAK;
+
+	writew(cr, dev->membase + MXC_I2CR);
+	/*
+	 * Dummy read only at the end of an address cycle
+	 */
+	if (addr_comp > 0)
+		readw(dev->membase + MXC_I2DR);
+
+	for (i = 0; i < len; i++) {
+		int ret;
+		/*
+		 * Wait for data transmission to complete
+		 */
+		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
+		if (ret < 0) {
+			mxc_i2c_stop(dev);
+			return ret;
+		}
+		/*
+		 * Do not generate an ACK for the last byte
+		 */
+		if (i == len - 2) {
+			cr = readw(dev->membase + MXC_I2CR);
+			cr |= MXC_I2CR_TXAK;
+			writew(cr, dev->membase + MXC_I2CR);
+		} else if (i == len - 1) {
+			if (last)
+				mxc_i2c_stop(dev);
+		}
+		/* Read the data */
+		*buf++ = readw(dev->membase + MXC_I2DR);
+	}
+
+	return i;
+}
+
+/**
+ * Write the data to the data register. Generates a stop signal if this is
+ * the last message to be sent or if no ack was received for the data sent.
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ * @param   *msg  pointer to a message structure that contains the slave
+ *                address and data to be sent
+ * @param   last  indicates that this is the last message to be received
+ *
+ * @return  The function returns the number of bytes written or -1 on time out
+ *          or if no ack was received for the data that was sent.
+ */
+static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
+			      int last)
+{
+	int i;
+	char *buf = msg->buf;
+	int len = msg->len;
+	unsigned int cr;
+
+	cr = readw(dev->membase + MXC_I2CR);
+	/* Set MTX to switch to transmit mode */
+	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);
+
+	for (i = 0; i < len; i++) {
+		int ret;
+		/*
+		 * Write the data
+		 */
+		writew(*buf++, dev->membase + MXC_I2DR);
+		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
+		if (ret < 0) {
+			mxc_i2c_stop(dev);
+			return ret;
+		}
+	}
+	if (last > 0)
+		mxc_i2c_stop(dev);
+
+	return i;
+}
+
+/**
+ * Function enables the I2C module and initializes the registers.
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ * @param   trans_flag  transfer flag
+ */
+static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
+{
+	clk_enable(dev->clk);
+	/* Set the frequency divider */
+	writew(dev->clkdiv, dev->membase + MXC_IFDR);
+	/* Clear the status register */
+	writew(0x0, dev->membase + MXC_I2SR);
+	/* Enable I2C and its interrupts */
+	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
+	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
+}
+
+/**
+ * Disables the I2C module.
+ *
+ * @param   dev   the mxc i2c structure used to get to the right i2c device
+ */
+static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
+{
+	writew(0x0, dev->membase + MXC_I2CR);
+	clk_disable(dev->clk);
+}
+
+/**
+ * The function is registered in the adapter structure. It is called when an MXC
+ * driver wishes to transfer data to a device connected to the I2C device.
+ *
+ * @param   adap   adapter structure for the MXC i2c device
+ * @param   msgs[] array of messages to be transferred to the device
+ * @param   num    number of messages to be transferred to the device
+ *
+ * @return  The function returns the number of messages transferred,
+ *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
+ *          less than 0.
+ */
+static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
+	int i, ret = 0, addr_comp = 0;
+	unsigned int sr;
+
+	if (dev->low_power) {
+		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
+		return -EREMOTEIO;
+	}
+
+	if (num < 1)
+		return 0;
+
+	mxc_i2c_module_en(dev, msgs[0].flags);
+	sr = readw(dev->membase + MXC_I2SR);
+	/*
+	 * Check bus state
+	 */
+	if (sr & MXC_I2SR_IBB) {
+		mxc_i2c_module_dis(dev);
+		printk(KERN_DEBUG "Bus busy\n");
+		return -EREMOTEIO;
+	}
+
+	dev->transfer_done = false;
+	dev->tx_success = false;
+	for (i = 0; i < num && ret >= 0; i++) {
+		addr_comp = 0;
+		/*
+		 * Send the slave address and transfer direction in the
+		 * address cycle
+		 */
+		if (i == 0) {
+			/*
+			 * Send a start or repeat start signal
+			 */
+			mxc_i2c_start(dev, &msgs[0]);
+			/* Wait for the address cycle to complete */
+			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
+				mxc_i2c_stop(dev);
+				mxc_i2c_module_dis(dev);
+				return -EREMOTEIO;
+			}
+			addr_comp = 1;
+		} else {
+			/*
+			 * Generate repeat start only if required i.e. the
+			 * address changed or the transfer direction changed
+			 */
+			if ((msgs[i].addr != msgs[i - 1].addr) ||
+			    ((msgs[i].flags & I2C_M_RD) !=
+			     (msgs[i - 1].flags & I2C_M_RD))) {
+				mxc_i2c_repstart(dev, &msgs[i]);
+				/* Wait for the address cycle to complete */
+				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
+					mxc_i2c_stop(dev);
+					mxc_i2c_module_dis(dev);
+					return -EREMOTEIO;
+				}
+				addr_comp = 1;
+			}
+		}
+
+		/* Transfer the data */
+		if (msgs[i].flags & I2C_M_RD) {
+			/* Read the data */
+			ret = mxc_i2c_readbytes(dev, &msgs[i], (i + 1 == num),
+						addr_comp);
+			if (ret < 0) {
+				printk(KERN_ERR "mxc_i2c_readbytes: fail.\n");
+				break;
+			}
+		} else {
+			/* Write the data */
+			ret = mxc_i2c_writebytes(dev, &msgs[i], (i + 1 == num));
+			if (ret < 0) {
+				printk(KERN_ERR "mxc_i2c_writebytes: fail.\n");
+				break;
+			}
+		}
+	}
+
+	mxc_i2c_module_dis(dev);
+	/*
+	 * Decrease by 1 as we do not want Start message to be included in
+	 * the count
+	 */
+	return i - 1;
+}
+
+/**
+ * Returns the i2c functionality supported by this driver.
+ *
+ * @param   adap adapter structure for this i2c device
+ *
+ * @return Returns the functionality that is supported.
+ */
+static u32 mxc_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm mxc_i2c_algorithm = {
+	.master_xfer = mxc_i2c_xfer,
+	.functionality = mxc_i2c_func
+};
+
+/*
+ * Interrupt Service Routine. It signals to the process about the data transfer
+ * completion. Also sets a flag if bus arbitration is lost.
+ */
+static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
+{
+	struct mxc_i2c_device *dev = dev_id;
+	unsigned int sr, cr;
+
+	sr = readw(dev->membase + MXC_I2SR);
+	cr = readw(dev->membase + MXC_I2CR);
+
+	/*
+	 * Clear the interrupt bit
+	 */
+	writew(0x0, dev->membase + MXC_I2SR);
+
+	if (sr & MXC_I2SR_IAL) {
+		printk(KERN_DEBUG "Bus Arbitration lost\n");
+	} else {
+		/* Interrupt due byte transfer completion */
+		dev->tx_success = true;
+		/* Check if RXAK is received in Transmit mode */
+		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
+			dev->tx_success = false;
+
+		dev->transfer_done = true;
+		wake_up_interruptible(&dev->wq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
+	unsigned int sr;
+
+	if (mxcdev == NULL)
+		return -ENODEV;
+
+	/* Prevent further calls to be processed */
+	mxcdev->low_power = true;
+	/* Wait till we finish the current transfer */
+	sr = readw(mxcdev->membase + MXC_I2SR);
+	while (sr & MXC_I2SR_IBB) {
+		msleep(10);
+		sr = readw(mxcdev->membase + MXC_I2SR);
+	}
+	gpio_i2c_inactive(mxcdev->adap.nr);
+
+	return 0;
+}
+
+static int mxci2c_resume(struct platform_device *pdev)
+{
+	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
+
+	if (mxcdev == NULL)
+		return -ENODEV;
+
+	mxcdev->low_power = false;
+	gpio_i2c_active(mxcdev->adap.nr);
+
+	return 0;
+}
+
+static int mxci2c_probe(struct platform_device *pdev)
+{
+	struct mxc_i2c_device *mxc_i2c;
+	struct mxc_i2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
+	struct resource *res;
+	int id = pdev->id;
+	u32 clk_freq;
+	int ret;
+	int i;
+
+	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
+	if (!mxc_i2c)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto err1;
+	}
+	mxc_i2c->membase = IO_ADDRESS(res->start);
+
+	/*
+	 * Request the I2C interrupt
+	 */
+	mxc_i2c->irq = platform_get_irq(pdev, 0);
+	if (mxc_i2c->irq < 0) {
+		ret = mxc_i2c->irq;
+		goto err1;
+	}
+
+	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
+			  0, pdev->name, mxc_i2c);
+	if (ret < 0)
+		goto err1;
+
+	init_waitqueue_head(&mxc_i2c->wq);
+
+	mxc_i2c->low_power	= false;
+	mxc_i2c->transfer_done	= false;
+	mxc_i2c->tx_success	= false;
+
+	gpio_i2c_active(id);
+
+	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");
+	clk_freq = clk_get_rate(mxc_i2c->clk);
+	mxc_i2c->clkdiv = -1;
+	if (i2c_plat_data->i2c_clk) {
+		/* Calculate divider and round up any fractional part */
+		int div = (clk_freq + i2c_plat_data->i2c_clk - 1) /
+		    i2c_plat_data->i2c_clk;
+		for (i = 0; i2c_clk_table[i].div != 0; i++) {
+			if (i2c_clk_table[i].div >= div) {
+				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
+				break;
+			}
+		}
+	}
+	if (mxc_i2c->clkdiv == -1) {
+		i = ARRAY_SIZE(i2c_clk_table) - 2;
+		mxc_i2c->clkdiv = 0x1F;	/* Use max divider */
+	}
+	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
+		clk_freq, i2c_clk_table[i].div,
+		clk_freq / i2c_clk_table[i].div, mxc_i2c->clkdiv);
+
+	/*
+	 * Set the adapter information
+	 */
+	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
+	mxc_i2c->adap.nr	= id;
+	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
+	mxc_i2c->adap.timeout	= 1;
+	mxc_i2c->adap.dev.parent= &pdev->dev;
+	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	platform_set_drvdata(pdev, mxc_i2c);
+	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
+	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
+		goto err2;
+
+	return 0;
+
+err2:
+	free_irq(mxc_i2c->irq, mxc_i2c);
+	gpio_i2c_inactive(id);
+err1:
+	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
+	kfree(mxc_i2c);
+	return ret;
+}
+
+static int mxci2c_remove(struct platform_device *pdev)
+{
+	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
+	int id = pdev->id;
+
+	free_irq(mxc_i2c->irq, mxc_i2c);
+	i2c_del_adapter(&mxc_i2c->adap);
+	gpio_i2c_inactive(id);
+	clk_put(mxc_i2c->clk);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver mxci2c_driver = {
+	.driver = {
+		   .name = "mxc-i2c",
+		   .owner = THIS_MODULE,
+	},
+	.probe = mxci2c_probe,
+	.remove = __exit_p(mxci2c_remove),
+	.suspend = mxci2c_suspend,
+	.resume = mxci2c_resume,
+};
+
+static int __init mxc_i2c_init(void)
+{
+	return platform_driver_register(&mxci2c_driver);
+}
+
+static void __exit mxc_i2c_exit(void)
+{
+	platform_driver_unregister(&mxci2c_driver);
+}
+
+subsys_initcall(mxc_i2c_init);
+module_exit(mxc_i2c_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("MXC I2C driver");
+MODULE_LICENSE("GPL");

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Sascha Hauer]

Hi Guennadi,


Some comments inline...

Sascha

On Fri, Nov 07, 2008 at 07:01:26PM +0100, Guennadi Liakhovetski wrote:
> This is an updated and cleaned up i2c driver for i.MX3x from Freescale.
> 
> Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>
> 
> ---
> 
> Please comment, I'll also have to obtain ARM Acks for the arch/arm part, 
> or, maybe, split it into two parts.
> 
> diff --git a/arch/arm/mach-mx3/devices.c b/arch/arm/mach-mx3/devices.c
> index 3ef1227..7c66f7c 100644
> --- a/arch/arm/mach-mx3/devices.c
> +++ b/arch/arm/mach-mx3/devices.c
> @@ -23,6 +23,7 @@
>  #include <linux/gpio.h>
>  
>  #include <mach/hardware.h>
> +#include <mach/i2c.h>
>  #include <mach/imx-uart.h>
>  #include <mach/ipu.h>
>  #include <mach/mxcfb.h>
> @@ -229,3 +230,94 @@ static int __init register_ipu(void)
>  
>  /* This is needed for the framebuffer, so, initialise early */
>  arch_initcall(register_ipu);
> +
> +/*
> + * Resource definition for the I2C1
> + */
> +static struct resource mxci2c1_resources[] = {
> +	[0] = {
> +		.start = I2C_BASE_ADDR,
> +		.end = I2C_BASE_ADDR + SZ_4K - 1,
> +		.flags = IORESOURCE_MEM,
> +	}, [1] = {
> +		.start = MXC_INT_I2C,
> +		.end = MXC_INT_I2C,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};

Can we drop the [0] and [1]? gcc knows how to count.

> +
> +/*
> + * Resource definition for the I2C2
> + */
> +static struct resource mxci2c2_resources[] = {
> +	[0] = {
> +		.start = I2C2_BASE_ADDR,
> +		.end = I2C2_BASE_ADDR + SZ_4K - 1,
> +		.flags = IORESOURCE_MEM,
> +	}, [1] = {
> +		.start = MXC_INT_I2C2,
> +		.end = MXC_INT_I2C2,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +/*
> + * Resource definition for the I2C3
> + */
> +static struct resource mxci2c3_resources[] = {
> +	[0] = {
> +		.start = I2C3_BASE_ADDR,
> +		.end = I2C3_BASE_ADDR + SZ_4K - 1,
> +		.flags = IORESOURCE_MEM,
> +	}, [1] = {
> +		.start = MXC_INT_I2C3,
> +		.end = MXC_INT_I2C3,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +/* Device definition for MXC I2C */
> +static struct platform_device mxci2c_devices[] = {
> +	[0] = {
> +		.name = "mxc-i2c",
> +		.id = 0,
> +		.num_resources = ARRAY_SIZE(mxci2c1_resources),
> +		.resource = mxci2c1_resources,
> +	}, [1] = {
> +		.name = "mxc-i2c",
> +		.id = 1,
> +		.num_resources = ARRAY_SIZE(mxci2c2_resources),
> +		.resource = mxci2c2_resources,
> +	}, [2] = {
> +		.name = "mxc-i2c",
> +		.id = 2,
> +		.num_resources = ARRAY_SIZE(mxci2c3_resources),
> +		.resource = mxci2c3_resources,
> +	},
> +};
> +
> +int mxc_i2c_register_adapters(struct mxc_i2c_board_data *data, int n)
> +{
> +	int i;
> +
> +	if (n > 3 || !data) {
> +		printk(KERN_ERR "i2c-mxc: Wrong platform data!\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < n; i++) {
> +		unsigned int id = data[i].id;
> +		if (id > 2) {
> +			printk(KERN_ERR
> +			       "i2c-mxc: Bad platform data #%d ID: %u!\n",
> +			       i, id);
> +			return -EINVAL;
> +		}
> +		mxci2c_devices[id].dev.platform_data = &data[i].data;
> +		if (platform_device_register(&mxci2c_devices[id]) < 0)
> +			printk(KERN_ERR
> +			       "i2c-mxc: Failed to register bus %u\n", id);
> +	}
> +
> +	return 0;
> +}

Why not use mxc_register_device() instead? The need of an extra struct
mxc_i2c_board_data would be removed.

> diff --git a/arch/arm/plat-mxc/include/mach/i2c.h b/arch/arm/plat-mxc/include/mach/i2c.h
> new file mode 100644
> index 0000000..ce50af5
> --- /dev/null
> +++ b/arch/arm/plat-mxc/include/mach/i2c.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 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
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301, USA.
> + */
> +
> +#ifndef __ASM_ARCH_I2C_H__
> +#define __ASM_ARCH_I2C_H__
> +
> +struct mxc_i2c_platform_data {
> +	u32 i2c_clk;
> +};
> +
> +struct mxc_i2c_board_data {
> +	struct mxc_i2c_platform_data	data;
> +	unsigned int			id;
> +};
> +
> +extern int mxc_i2c_register_adapters(struct mxc_i2c_board_data *, int);
> +
> +#endif
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 7f95905..055a33a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -401,6 +401,15 @@ config I2C_MV64XXX
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mv64xxx.
>  
> +config I2C_MXC
> +	tristate "MXC I2C support"
> +	depends on I2C && ARCH_MXC
> +	help
> +	  If you say yes to this option, support will be included for Freescale
> +	  MXC I2C modules.
> +
> +	  This driver can also be built as a module.
> +
>  config I2C_OCORES
>  	tristate "OpenCores I2C Controller"
>  	depends on EXPERIMENTAL
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 0c2c4b2..495f040 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
>  obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
>  obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
>  obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
> +obj-$(CONFIG_I2C_MXC)		+= i2c-mxc.o
>  obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
>  obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>  obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
> diff --git a/drivers/i2c/busses/i2c-mxc.c b/drivers/i2c/busses/i2c-mxc.c
> new file mode 100644
> index 0000000..8b4e52f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-mxc.c
> @@ -0,0 +1,744 @@
> +/*
> + * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +
> +#include <asm/irq.h>
> +#include <asm/io.h>
> +
> +#include <mach/i2c.h>
> +#include <mach/iomux-mx3.h>
> +
> +/* Address offsets of the I2C registers */
> +#define MXC_IADR                0x00	/* Address Register */
> +#define MXC_IFDR                0x04	/* Freq div register */
> +#define MXC_I2CR                0x08	/* Control regsiter */
> +#define MXC_I2SR                0x0C	/* Status register */
> +#define MXC_I2DR                0x10	/* Data I/O register */
> +
> +/* Bit definitions of I2CR */
> +#define MXC_I2CR_IEN            0x0080
> +#define MXC_I2CR_IIEN           0x0040
> +#define MXC_I2CR_MSTA           0x0020
> +#define MXC_I2CR_MTX            0x0010
> +#define MXC_I2CR_TXAK           0x0008
> +#define MXC_I2CR_RSTA           0x0004
> +
> +/* Bit definitions of I2SR */
> +#define MXC_I2SR_ICF            0x0080
> +#define MXC_I2SR_IAAS           0x0040
> +#define MXC_I2SR_IBB            0x0020
> +#define MXC_I2SR_IAL            0x0010
> +#define MXC_I2SR_SRW            0x0004
> +#define MXC_I2SR_IIF            0x0002
> +#define MXC_I2SR_RXAK           0x0001
> +
> +#define MXC_ADAPTER_NAME        "MXC I2C Adapter"
> +
> +/*
> + * In case the MXC device has multiple I2C modules, this structure is used to
> + * store information specific to each I2C module.
> + */
> +struct mxc_i2c_device {
> +	/*
> +	 * This structure is used to identify the physical i2c bus along with
> +	 * the access algorithms necessary to access it.
> +	 */
> +	struct i2c_adapter adap;
> +
> +	/*
> +	 * This waitqueue is used to wait for the data transfer to complete.
> +	 */
> +	wait_queue_head_t wq;
> +
> +	/*
> +	 * The base address of the I2C device.
> +	 */
> +	void __iomem *membase;
> +
> +	/*
> +	 * The interrupt number used by the I2C device.
> +	 */
> +	int irq;
> +
> +	/*
> +	 * The default clock divider value to be used.
> +	 */
> +	unsigned int clkdiv;
> +
> +	/*
> +	 * The clock source for the device.
> +	 */
> +	struct clk *clk;
> +
> +	/*
> +	 * The current power state of the device
> +	 */
> +	bool low_power;
> +
> +	/*
> +	 * Boolean to indicate if data was transferred
> +	 */
> +	bool transfer_done;
> +
> +	/*
> +	 * Boolean to indicate if we received an ACK for the data transmitted
> +	 */
> +	bool tx_success;
> +};
> +
> +struct clk_div_table {
> +	int reg_value;
> +	int div;
> +};
> +
> +static const struct clk_div_table i2c_clk_table[] = {
> +	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
> +	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
> +	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
> +	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
> +	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
> +	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
> +	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
> +	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
> +	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
> +	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
> +	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
> +	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
> +	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
> +	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
> +	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
> +	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
> +	{0, 0}
> +};
> +
> +static void gpio_i2c_active(int i2c_num)
> +{
> +	switch (i2c_num) {
> +	case 0:
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_CLK, IOMUX_CONFIG_FUNC));
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_DAT, IOMUX_CONFIG_FUNC));
> +		break;
> +	case 1:
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MOSI, IOMUX_CONFIG_ALT1));
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MISO, IOMUX_CONFIG_ALT1));
> +		break;
> +	case 2:
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SS2, IOMUX_CONFIG_ALT1));
> +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SCLK, IOMUX_CONFIG_ALT1));
> +	}
> +}

This should be moved to the platform part. You make the driver MX31
specific with this. Also, care to update the macros in iomux-mx3.h
instead of constructing the pin definitions directly using IOMUX_MODE?
(btw how is the general feeling about these macros? I really like it
using these macros since I don't have to read the datasheet to lookup up
a specific pin, but I may be the only one)

> +
> +static void gpio_i2c_inactive(int i2c_num)
> +{
> +	/* The one who needs the pins should configure them */
> +}
> +
> +/**
> + * Transmit a \b STOP signal to the slave device.
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + */
> +static void mxc_i2c_stop(struct mxc_i2c_device * dev)
> +{
> +	unsigned int cr;
> +	int retry = 200;
> +
> +	cr = readw(dev->membase + MXC_I2CR);
> +	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
> +	writew(cr, dev->membase + MXC_I2CR);
> +
> +	/*
> +	 * Make sure STOP meets setup requirement.
> +	 */
> +	for (;;) {
> +		unsigned int sr = readw(dev->membase + MXC_I2SR);
> +		if ((sr & MXC_I2SR_IBB) == 0)
> +			break;
> +		if (retry-- <= 0) {
> +			printk(KERN_DEBUG "Bus busy\n");

use dev_dbg instead. Several other occurences follow.

> +			break;
> +		}
> +		udelay(3);
> +	}
> +}
> +
> +/**
> + * Wait for the transmission of the data byte to complete. This function waits
> + * till we get a signal from the interrupt service routine indicating completion
> + * of the address cycle or we time out.
> + *
> + * @param   dev         the mxc i2c structure used to get to the right i2c device
> + * @param   trans_flag  transfer flag
> + *
> + *
> + * @return  The function returns 0 on success or -1 if an ack was not received
> + */
> +
> +static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
> +{
> +	int retry = 16;
> +
> +	while (retry-- && !dev->transfer_done)
> +		wait_event_interruptible_timeout(dev->wq,
> +						 dev->transfer_done,
> +						 dev->adap.timeout);
> +
> +	dev->transfer_done = false;
> +
> +	if (retry <= 0) {
> +		/* Unable to send data */
> +		dev_warn(&dev->adap.dev, "Data not transmitted\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (!dev->tx_success) {
> +		/* An ACK was not received for transmitted byte */
> +		dev_dbg(&dev->adap.dev, "ACK not received \n");
> +		return -EREMOTEIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * Transmit a \b START signal to the slave device.
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + * @param   *msg  pointer to a message structure that contains the slave
> + *                address
> + */
> +static void mxc_i2c_start(struct mxc_i2c_device * dev, struct i2c_msg *msg)
> +{
> +	unsigned int cr, sr;
> +	unsigned int addr_trans;
> +	int retry = 16;
> +
> +	/*
> +	 * Set the slave address and the requested transfer mode
> +	 * in the data register
> +	 */
> +	addr_trans = msg->addr << 1;
> +	if (msg->flags & I2C_M_RD)
> +		addr_trans |= 0x01;
> +
> +	/* Set the Master bit */
> +	cr = readw(dev->membase + MXC_I2CR);
> +	cr |= MXC_I2CR_MSTA;
> +	writew(cr, dev->membase + MXC_I2CR);
> +
> +	/* Wait till the Bus Busy bit is set */
> +	sr = readw(dev->membase + MXC_I2SR);
> +	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
> +		udelay(3);
> +		sr = readw(dev->membase + MXC_I2SR);
> +	}
> +	if (retry <= 0)
> +		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");
> +
> +	/* Set the Transmit bit */
> +	cr = readw(dev->membase + MXC_I2CR);
> +	cr |= MXC_I2CR_MTX;
> +	writew(cr, dev->membase + MXC_I2CR);
> +
> +	writew(addr_trans, dev->membase + MXC_I2DR);
> +}
> +
> +/**
> + * Transmit a \b REPEAT START to the slave device
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + * @param   *msg  pointer to a message structure that contains the slave
> + *                address
> + */
> +static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
> +{
> +	unsigned int cr;
> +	unsigned int addr_trans;
> +
> +	/*
> +	 * Set the slave address and the requested transfer mode
> +	 * in the data register
> +	 */
> +	addr_trans = msg->addr << 1;
> +	if (msg->flags & I2C_M_RD)
> +		addr_trans |= 0x01;
> +
> +	cr = readw(dev->membase + MXC_I2CR);
> +	cr |= MXC_I2CR_RSTA;
> +	writew(cr, dev->membase + MXC_I2CR);
> +	udelay(3);
> +	writew(addr_trans, dev->membase + MXC_I2DR);
> +}
> +
> +/**
> + * Read the received data. The function waits till data is available or times
> + * out. Generates a stop signal if this is the last message to be received.
> + * Sends an ack for all the bytes received except the last byte.
> + *
> + * @param  dev       the mxc i2c structure used to get to the right i2c device
> + * @param  *msg      pointer to a message structure that contains the slave
> + *                   address and a pointer to the receive buffer
> + * @param  last      indicates that this is the last message to be received
> + * @param  addr_comp flag indicates that we just finished the address cycle
> + *
> + * @return  The function returns the number of bytes read or -1 on time out.
> + */
> +static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> +			     int last, int addr_comp)
> +{
> +	int i;
> +	char *buf = msg->buf;
> +	int len = msg->len;
> +	unsigned int cr;
> +
> +	cr = readw(dev->membase + MXC_I2CR);
> +	/*
> +	 * Clear MTX to switch to receive mode.
> +	 */
> +	cr &= ~MXC_I2CR_MTX;
> +	/*
> +	 * Clear the TXAK bit to gen an ack when receiving only one byte.
> +	 */
> +	if (len == 1)
> +		cr |= MXC_I2CR_TXAK;
> +	else
> +		cr &= ~MXC_I2CR_TXAK;
> +
> +	writew(cr, dev->membase + MXC_I2CR);
> +	/*
> +	 * Dummy read only at the end of an address cycle
> +	 */
> +	if (addr_comp > 0)
> +		readw(dev->membase + MXC_I2DR);
> +
> +	for (i = 0; i < len; i++) {
> +		int ret;
> +		/*
> +		 * Wait for data transmission to complete
> +		 */
> +		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> +		if (ret < 0) {
> +			mxc_i2c_stop(dev);
> +			return ret;
> +		}
> +		/*
> +		 * Do not generate an ACK for the last byte
> +		 */
> +		if (i == len - 2) {
> +			cr = readw(dev->membase + MXC_I2CR);
> +			cr |= MXC_I2CR_TXAK;
> +			writew(cr, dev->membase + MXC_I2CR);
> +		} else if (i == len - 1) {
> +			if (last)
> +				mxc_i2c_stop(dev);
> +		}
> +		/* Read the data */
> +		*buf++ = readw(dev->membase + MXC_I2DR);
> +	}
> +
> +	return i;
> +}
> +
> +/**
> + * Write the data to the data register. Generates a stop signal if this is
> + * the last message to be sent or if no ack was received for the data sent.
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + * @param   *msg  pointer to a message structure that contains the slave
> + *                address and data to be sent
> + * @param   last  indicates that this is the last message to be received
> + *
> + * @return  The function returns the number of bytes written or -1 on time out
> + *          or if no ack was received for the data that was sent.
> + */
> +static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> +			      int last)
> +{
> +	int i;
> +	char *buf = msg->buf;
> +	int len = msg->len;
> +	unsigned int cr;
> +
> +	cr = readw(dev->membase + MXC_I2CR);
> +	/* Set MTX to switch to transmit mode */
> +	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);
> +
> +	for (i = 0; i < len; i++) {
> +		int ret;
> +		/*
> +		 * Write the data
> +		 */
> +		writew(*buf++, dev->membase + MXC_I2DR);
> +		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> +		if (ret < 0) {
> +			mxc_i2c_stop(dev);
> +			return ret;
> +		}
> +	}
> +	if (last > 0)
> +		mxc_i2c_stop(dev);
> +
> +	return i;
> +}
> +
> +/**
> + * Function enables the I2C module and initializes the registers.
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + * @param   trans_flag  transfer flag
> + */
> +static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> +{
> +	clk_enable(dev->clk);
> +	/* Set the frequency divider */
> +	writew(dev->clkdiv, dev->membase + MXC_IFDR);

Can't we do this only once during probe()?

> +	/* Clear the status register */
> +	writew(0x0, dev->membase + MXC_I2SR);
> +	/* Enable I2C and its interrupts */
> +	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
> +	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
> +}
> +
> +/**
> + * Disables the I2C module.
> + *
> + * @param   dev   the mxc i2c structure used to get to the right i2c device
> + */
> +static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
> +{
> +	writew(0x0, dev->membase + MXC_I2CR);
> +	clk_disable(dev->clk);
> +}
> +
> +/**
> + * The function is registered in the adapter structure. It is called when an MXC
> + * driver wishes to transfer data to a device connected to the I2C device.
> + *
> + * @param   adap   adapter structure for the MXC i2c device
> + * @param   msgs[] array of messages to be transferred to the device
> + * @param   num    number of messages to be transferred to the device
> + *
> + * @return  The function returns the number of messages transferred,
> + *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
> + *          less than 0.
> + */
> +static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
> +	int i, ret = 0, addr_comp = 0;
> +	unsigned int sr;
> +
> +	if (dev->low_power) {
> +		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
> +		return -EREMOTEIO;
> +	}
> +
> +	if (num < 1)
> +		return 0;
> +
> +	mxc_i2c_module_en(dev, msgs[0].flags);
> +	sr = readw(dev->membase + MXC_I2SR);
> +	/*
> +	 * Check bus state
> +	 */
> +	if (sr & MXC_I2SR_IBB) {
> +		mxc_i2c_module_dis(dev);
> +		printk(KERN_DEBUG "Bus busy\n");
> +		return -EREMOTEIO;
> +	}
> +
> +	dev->transfer_done = false;
> +	dev->tx_success = false;
> +	for (i = 0; i < num && ret >= 0; i++) {
> +		addr_comp = 0;
> +		/*
> +		 * Send the slave address and transfer direction in the
> +		 * address cycle
> +		 */
> +		if (i == 0) {
> +			/*
> +			 * Send a start or repeat start signal
> +			 */
> +			mxc_i2c_start(dev, &msgs[0]);
> +			/* Wait for the address cycle to complete */
> +			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
> +				mxc_i2c_stop(dev);
> +				mxc_i2c_module_dis(dev);
> +				return -EREMOTEIO;
> +			}
> +			addr_comp = 1;
> +		} else {
> +			/*
> +			 * Generate repeat start only if required i.e. the
> +			 * address changed or the transfer direction changed
> +			 */
> +			if ((msgs[i].addr != msgs[i - 1].addr) ||
> +			    ((msgs[i].flags & I2C_M_RD) !=
> +			     (msgs[i - 1].flags & I2C_M_RD))) {
> +				mxc_i2c_repstart(dev, &msgs[i]);
> +				/* Wait for the address cycle to complete */
> +				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
> +					mxc_i2c_stop(dev);
> +					mxc_i2c_module_dis(dev);
> +					return -EREMOTEIO;
> +				}
> +				addr_comp = 1;
> +			}
> +		}
> +
> +		/* Transfer the data */
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* Read the data */
> +			ret = mxc_i2c_readbytes(dev, &msgs[i], (i + 1 == num),
> +						addr_comp);
> +			if (ret < 0) {
> +				printk(KERN_ERR "mxc_i2c_readbytes: fail.\n");
> +				break;
> +			}
> +		} else {
> +			/* Write the data */
> +			ret = mxc_i2c_writebytes(dev, &msgs[i], (i + 1 == num));
> +			if (ret < 0) {
> +				printk(KERN_ERR "mxc_i2c_writebytes: fail.\n");
> +				break;
> +			}
> +		}
> +	}
> +
> +	mxc_i2c_module_dis(dev);
> +	/*
> +	 * Decrease by 1 as we do not want Start message to be included in
> +	 * the count
> +	 */
> +	return i - 1;
> +}
> +
> +/**
> + * Returns the i2c functionality supported by this driver.
> + *
> + * @param   adap adapter structure for this i2c device
> + *
> + * @return Returns the functionality that is supported.
> + */
> +static u32 mxc_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm mxc_i2c_algorithm = {
> +	.master_xfer = mxc_i2c_xfer,
> +	.functionality = mxc_i2c_func
> +};
> +
> +/*
> + * Interrupt Service Routine. It signals to the process about the data transfer
> + * completion. Also sets a flag if bus arbitration is lost.
> + */
> +static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
> +{
> +	struct mxc_i2c_device *dev = dev_id;
> +	unsigned int sr, cr;
> +
> +	sr = readw(dev->membase + MXC_I2SR);
> +	cr = readw(dev->membase + MXC_I2CR);
> +
> +	/*
> +	 * Clear the interrupt bit
> +	 */
> +	writew(0x0, dev->membase + MXC_I2SR);
> +
> +	if (sr & MXC_I2SR_IAL) {
> +		printk(KERN_DEBUG "Bus Arbitration lost\n");
> +	} else {
> +		/* Interrupt due byte transfer completion */
> +		dev->tx_success = true;
> +		/* Check if RXAK is received in Transmit mode */
> +		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
> +			dev->tx_success = false;
> +
> +		dev->transfer_done = true;
> +		wake_up_interruptible(&dev->wq);
> +	}


> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> +	unsigned int sr;
> +
> +	if (mxcdev == NULL)
> +		return -ENODEV;
> +
> +	/* Prevent further calls to be processed */
> +	mxcdev->low_power = true;
> +	/* Wait till we finish the current transfer */
> +	sr = readw(mxcdev->membase + MXC_I2SR);
> +	while (sr & MXC_I2SR_IBB) {
> +		msleep(10);
> +		sr = readw(mxcdev->membase + MXC_I2SR);
> +	}
> +	gpio_i2c_inactive(mxcdev->adap.nr);
> +
> +	return 0;
> +}
> +
> +static int mxci2c_resume(struct platform_device *pdev)
> +{
> +	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> +
> +	if (mxcdev == NULL)
> +		return -ENODEV;
> +
> +	mxcdev->low_power = false;
> +	gpio_i2c_active(mxcdev->adap.nr);
> +
> +	return 0;
> +}
> +
> +static int mxci2c_probe(struct platform_device *pdev)
> +{
> +	struct mxc_i2c_device *mxc_i2c;
> +	struct mxc_i2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> +	struct resource *res;
> +	int id = pdev->id;
> +	u32 clk_freq;
> +	int ret;
> +	int i;
> +
> +	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
> +	if (!mxc_i2c)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL) {
> +		ret = -ENODEV;
> +		goto err1;
> +	}
> +	mxc_i2c->membase = IO_ADDRESS(res->start);

Please use ioremap instead.

> +
> +	/*
> +	 * Request the I2C interrupt
> +	 */
> +	mxc_i2c->irq = platform_get_irq(pdev, 0);
> +	if (mxc_i2c->irq < 0) {
> +		ret = mxc_i2c->irq;
> +		goto err1;
> +	}
> +
> +	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
> +			  0, pdev->name, mxc_i2c);
> +	if (ret < 0)
> +		goto err1;
> +
> +	init_waitqueue_head(&mxc_i2c->wq);
> +
> +	mxc_i2c->low_power	= false;
> +	mxc_i2c->transfer_done	= false;
> +	mxc_i2c->tx_success	= false;
> +
> +	gpio_i2c_active(id);
> +
> +	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");

The corresponding clk_put is missing in the error path.

> +	clk_freq = clk_get_rate(mxc_i2c->clk);
> +	mxc_i2c->clkdiv = -1;
> +	if (i2c_plat_data->i2c_clk) {
> +		/* Calculate divider and round up any fractional part */
> +		int div = (clk_freq + i2c_plat_data->i2c_clk - 1) /
> +		    i2c_plat_data->i2c_clk;
> +		for (i = 0; i2c_clk_table[i].div != 0; i++) {
> +			if (i2c_clk_table[i].div >= div) {
> +				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
> +				break;
> +			}
> +		}
> +	}
> +	if (mxc_i2c->clkdiv == -1) {
> +		i = ARRAY_SIZE(i2c_clk_table) - 2;
> +		mxc_i2c->clkdiv = 0x1F;	/* Use max divider */
> +	}
> +	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
> +		clk_freq, i2c_clk_table[i].div,
> +		clk_freq / i2c_clk_table[i].div, mxc_i2c->clkdiv);
> +
> +	/*
> +	 * Set the adapter information
> +	 */
> +	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
> +	mxc_i2c->adap.nr	= id;
> +	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
> +	mxc_i2c->adap.timeout	= 1;
> +	mxc_i2c->adap.dev.parent= &pdev->dev;
> +	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	platform_set_drvdata(pdev, mxc_i2c);
> +	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
> +	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
> +		goto err2;
> +
> +	return 0;
> +
> +err2:
> +	free_irq(mxc_i2c->irq, mxc_i2c);
> +	gpio_i2c_inactive(id);
> +err1:
> +	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
> +	kfree(mxc_i2c);
> +	return ret;
> +}
> +
> +static int mxci2c_remove(struct platform_device *pdev)
> +{
> +	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
> +	int id = pdev->id;
> +
> +	free_irq(mxc_i2c->irq, mxc_i2c);
> +	i2c_del_adapter(&mxc_i2c->adap);
> +	gpio_i2c_inactive(id);
> +	clk_put(mxc_i2c->clk);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct platform_driver mxci2c_driver = {
> +	.driver = {
> +		   .name = "mxc-i2c",
> +		   .owner = THIS_MODULE,
> +	},
> +	.probe = mxci2c_probe,
> +	.remove = __exit_p(mxci2c_remove),
> +	.suspend = mxci2c_suspend,
> +	.resume = mxci2c_resume,
> +};
> +
> +static int __init mxc_i2c_init(void)
> +{
> +	return platform_driver_register(&mxci2c_driver);
> +}
> +
> +static void __exit mxc_i2c_exit(void)
> +{
> +	platform_driver_unregister(&mxci2c_driver);
> +}
> +
> +subsys_initcall(mxc_i2c_init);
> +module_exit(mxc_i2c_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("MXC I2C driver");
> +MODULE_LICENSE("GPL");
> 

-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Guennadi Liakhovetski]

Hi Sascha,

Thanks for the comments,

On Mon, 10 Nov 2008, Sascha Hauer wrote:

> > +static struct resource mxci2c1_resources[] = {
> > +	[0] = {
> > +		.start = I2C_BASE_ADDR,
> > +		.end = I2C_BASE_ADDR + SZ_4K - 1,
> > +		.flags = IORESOURCE_MEM,
> > +	}, [1] = {
> > +		.start = MXC_INT_I2C,
> > +		.end = MXC_INT_I2C,
> > +		.flags = IORESOURCE_IRQ,
> > +	},
> > +};
> 
> Can we drop the [0] and [1]? gcc knows how to count.

I certainly hope so too:-) I normally only use indices in array 
initialisations when they are important, however, here this is not the 
case. Will drop.

> > +{
> > +	switch (i2c_num) {
> > +	case 0:
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_CLK, IOMUX_CONFIG_FUNC));
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_DAT, IOMUX_CONFIG_FUNC));
> > +		break;
> > +	case 1:
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MOSI, IOMUX_CONFIG_ALT1));
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MISO, IOMUX_CONFIG_ALT1));
> > +		break;
> > +	case 2:
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SS2, IOMUX_CONFIG_ALT1));
> > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SCLK, IOMUX_CONFIG_ALT1));
> > +	}
> > +}
> 
> This should be moved to the platform part. You make the driver MX31
> specific with this.

First, even though the driver is called (also in Freescale sources) "mxc," 
I am not sure which other *mx* SoCs have the same i2c controller, do you 
know that? So, it might anyway _be_ i.MX3* specific. Next, I could just 
configure pins in i2c platform device registration code, but I don't know 
if anyone ever comes up with an idea to dynamically switch between, say, 
I2C #2 and SPI #2... Shall I make this a platform hook?

> Also, care to update the macros in iomux-mx3.h
> instead of constructing the pin definitions directly using IOMUX_MODE?
> (btw how is the general feeling about these macros? I really like it
> using these macros since I don't have to read the datasheet to lookup up
> a specific pin, but I may be the only one)

You mean these:

/*
 * Convenience values for use with mxc_iomux_mode()
 *
 * Format here is MX31_PIN_(pin name)__(function)
 */
#define MX31_PIN_CSPI3_MOSI__RXD3	IOMUX_MODE(MX31_PIN_CSPI3_MOSI, IOMUX_CONFIG_ALT1)
...

? Well, I usually only define macros if they are used more than once. If a 
value is used only once I usually don't bother... But if you like, I can 
define them, sure.

> > +/**
> > + * Function enables the I2C module and initializes the registers.
> > + *
> > + * @param   dev   the mxc i2c structure used to get to the right i2c device
> > + * @param   trans_flag  transfer flag
> > + */
> > +static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> > +{
> > +	clk_enable(dev->clk);
> > +	/* Set the frequency divider */
> > +	writew(dev->clkdiv, dev->membase + MXC_IFDR);
> 
> Can't we do this only once during probe()?

Hm, would have to test...

I'll address your other comments in the next version too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Sascha Hauer]

On Mon, Nov 10, 2008 at 12:30:07PM +0100, Guennadi Liakhovetski wrote:
> Hi Sascha,
> 
> Thanks for the comments,
> 
> On Mon, 10 Nov 2008, Sascha Hauer wrote:
> 
> > > +static struct resource mxci2c1_resources[] = {
> > > +	[0] = {
> > > +		.start = I2C_BASE_ADDR,
> > > +		.end = I2C_BASE_ADDR + SZ_4K - 1,
> > > +		.flags = IORESOURCE_MEM,
> > > +	}, [1] = {
> > > +		.start = MXC_INT_I2C,
> > > +		.end = MXC_INT_I2C,
> > > +		.flags = IORESOURCE_IRQ,
> > > +	},
> > > +};
> > 
> > Can we drop the [0] and [1]? gcc knows how to count.
> 
> I certainly hope so too:-) I normally only use indices in array 
> initialisations when they are important, however, here this is not the 
> case. Will drop.
> 
> > > +{
> > > +	switch (i2c_num) {
> > > +	case 0:
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_CLK, IOMUX_CONFIG_FUNC));
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_DAT, IOMUX_CONFIG_FUNC));
> > > +		break;
> > > +	case 1:
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MOSI, IOMUX_CONFIG_ALT1));
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MISO, IOMUX_CONFIG_ALT1));
> > > +		break;
> > > +	case 2:
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SS2, IOMUX_CONFIG_ALT1));
> > > +		mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SCLK, IOMUX_CONFIG_ALT1));
> > > +	}
> > > +}
> > 
> > This should be moved to the platform part. You make the driver MX31
> > specific with this.
> 
> First, even though the driver is called (also in Freescale sources) "mxc," 
> I am not sure which other *mx* SoCs have the same i2c controller, do you 
> know that? So, it might anyway _be_ i.MX3* specific.

It works at least on MX2 (where I use a variant of this driver) and
should work on MX1 which has the same register layout.

> Next, I could just 
> configure pins in i2c platform device registration code, but I don't know 
> if anyone ever comes up with an idea to dynamically switch between, say, 
> I2C #2 and SPI #2...

Sounds unlikely.

> Shall I make this a platform hook?

I asked myself whether it was worth the effort to implement a platform
hook for each and every device instead of just creating a long list of
pins and just configure them during board initialization. But this might
get interesting when it comes to powermanagement. So a platform hook
seems good, boards are still free to ignore the platform hooks and use a
list instead.

> 
> > Also, care to update the macros in iomux-mx3.h
> > instead of constructing the pin definitions directly using IOMUX_MODE?
> > (btw how is the general feeling about these macros? I really like it
> > using these macros since I don't have to read the datasheet to lookup up
> > a specific pin, but I may be the only one)
> 
> You mean these:
> 
> /*
>  * Convenience values for use with mxc_iomux_mode()
>  *
>  * Format here is MX31_PIN_(pin name)__(function)
>  */
> #define MX31_PIN_CSPI3_MOSI__RXD3	IOMUX_MODE(MX31_PIN_CSPI3_MOSI, IOMUX_CONFIG_ALT1)
> ...
> 
> ? Well, I usually only define macros if they are used more than once. If a 
> value is used only once I usually don't bother... But if you like, I can 
> define them, sure.

Well, they are used once for every board implementing I2C.

> 
> > > +/**
> > > + * Function enables the I2C module and initializes the registers.
> > > + *
> > > + * @param   dev   the mxc i2c structure used to get to the right i2c device
> > > + * @param   trans_flag  transfer flag
> > > + */
> > > +static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> > > +{
> > > +	clk_enable(dev->clk);
> > > +	/* Set the frequency divider */
> > > +	writew(dev->clkdiv, dev->membase + MXC_IFDR);
> > 
> > Can't we do this only once during probe()?
> 
> Hm, would have to test...
> 
> I'll address your other comments in the next version too.

Thank you

Sascha

-- 
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Darius]

Guennadi Liakhovetski wrote:
> This is an updated and cleaned up i2c driver for i.MX3x from Freescale.
> 
> Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>
> 
> ---
> 
> Please comment, I'll also have to obtain ARM Acks for the arch/arm part, 
> or, maybe, split it into two parts.
> 

Hi, I have ready driver for MX1. Is it reason to make two separate drivers for MX2/MX3 and MX1?
btw, pending patches for MX1 is main reason why I2C for imx driver is not in kernel.

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Guennadi Liakhovetski]

(please, use reply-to-all, thanks)

On Mon, 10 Nov 2008, Darius wrote:

> Guennadi Liakhovetski wrote:
> > This is an updated and cleaned up i2c driver for i.MX3x from Freescale.
> > 
> > Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>
> > 
> > ---
> > 
> > Please comment, I'll also have to obtain ARM Acks for the arch/arm part, or,
> > maybe, split it into two parts.
> > 
> 
> Hi, I have ready driver for MX1. Is it reason to make two separate drivers for
> MX2/MX3 and MX1?
> btw, pending patches for MX1 is main reason why I2C for imx driver is not in
> kernel.

If it works with mx3 - great! I did see it, but I didn't realise it would 
work with mx3. Please, confirm.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] i2c: add i2c adapter driver for i.MX3x SoCs

[Darius]

Guennadi Liakhovetski wrote:
> (please, use reply-to-all, thanks)
> 
> On Mon, 10 Nov 2008, Darius wrote:
> 
>> Guennadi Liakhovetski wrote:
>>> This is an updated and cleaned up i2c driver for i.MX3x from Freescale.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
>>>
>>> ---
>>>
>>> Please comment, I'll also have to obtain ARM Acks for the arch/arm part, or,
>>> maybe, split it into two parts.
>>>
>> Hi, I have ready driver for MX1. Is it reason to make two separate drivers for
>> MX2/MX3 and MX1?
>> btw, pending patches for MX1 is main reason why I2C for imx driver is not in
>> kernel.
> 
> If it works with mx3 - great! I did see it, but I didn't realise it would 
> work with mx3. Please, confirm.

It is tested only with i.MXL.
We can see in mx1 and in mx3 user manuals, that both they has the same controller.
mx3 differs only in register width (16bit instead 32bit in mx1) and has 3 I2C devices (mx1 only one).
I don't have possibility to test it on mx3. Maybe you have?

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer