[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

This sets the bus to run at 400kHz, prior to this,
the bus frequency was undefined.

Cc: Linux I2C <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 3d471d5..0d8e485 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -43,6 +43,10 @@
 #define MXS_I2C_CTRL0_DIRECTION			0x00010000
 #define MXS_I2C_CTRL0_XFER_COUNT(v)		((v) & 0x0000FFFF)
 
+#define MXS_I2C_TIMING0		(0x10)
+#define MXS_I2C_TIMING1		(0x20)
+#define MXS_I2C_TIMING2		(0x30)
+
 #define MXS_I2C_CTRL1		(0x40)
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
@@ -118,6 +122,12 @@ struct mxs_i2c_dev {
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 {
 	mxs_reset_block(i2c->regs);
+
+	/* I2C is running at 400kHz */
+	writel(0x000f0007, i2c->regs + MXS_I2C_TIMING0);
+	writel(0x001f000f, i2c->regs + MXS_I2C_TIMING1);
+	writel(0x0015000d, i2c->regs + MXS_I2C_TIMING2);
+
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
 	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
-- 
1.7.10

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

This sets the bus to run at 400kHz, prior to this,
the bus frequency was undefined.

Cc: Linux I2C <linux-i2c@vger.kernel.org>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/i2c/busses/i2c-mxs.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 3d471d5..0d8e485 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -43,6 +43,10 @@
 #define MXS_I2C_CTRL0_DIRECTION			0x00010000
 #define MXS_I2C_CTRL0_XFER_COUNT(v)		((v) & 0x0000FFFF)
 
+#define MXS_I2C_TIMING0		(0x10)
+#define MXS_I2C_TIMING1		(0x20)
+#define MXS_I2C_TIMING2		(0x30)
+
 #define MXS_I2C_CTRL1		(0x40)
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
@@ -118,6 +122,12 @@ struct mxs_i2c_dev {
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 {
 	mxs_reset_block(i2c->regs);
+
+	/* I2C is running at 400kHz */
+	writel(0x000f0007, i2c->regs + MXS_I2C_TIMING0);
+	writel(0x001f000f, i2c->regs + MXS_I2C_TIMING1);
+	writel(0x0015000d, i2c->regs + MXS_I2C_TIMING2);
+
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
 	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
-- 
1.7.10

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

This patch implements DMA support into mxs-i2c. DMA transfers are now enabled by
default, though it is possible for example for debugging purposes, to disable
them via i2c->dma_mode.

Cc: Linux I2C <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
 arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
 drivers/i2c/busses/i2c-mxs.c                    |  261 +++++++++++++++++++++--
 3 files changed, 245 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
index 79222ec..a94f308 100644
--- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
+++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
@@ -14,6 +14,7 @@
 	{								\
 		.id = _id,						\
 		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
+		.dma	= soc ## _DMA_I2C ## _id,			\
 		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
 		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
 	}
@@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
 			.end = data->iobase + SZ_8K - 1,
 			.flags = IORESOURCE_MEM,
 		}, {
+			.start	= data->dma,
+			.end	= data->dma,
+			.flags	= IORESOURCE_DMA,
+		}, {
 			.start = data->errirq,
 			.end = data->errirq,
 			.flags = IORESOURCE_IRQ,
diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
index f2e3839..791d99c 100644
--- a/arch/arm/mach-mxs/include/mach/devices-common.h
+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
@@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data *pdata,
 struct mxs_mxs_i2c_data {
 	int id;
 	resource_size_t iobase;
+	resource_size_t dma;
 	resource_size_t errirq;
 	resource_size_t dmairq;
 };
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 0d8e485..b364b8b 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -26,6 +26,9 @@
 #include <linux/platform_device.h>
 #include <linux/jiffies.h>
 #include <linux/io.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/fsl/mxs-dma.h>
 
 #include <mach/common.h>
 
@@ -113,6 +116,16 @@ struct mxs_i2c_dev {
 	struct completion cmd_complete;
 	u32 cmd_err;
 	struct i2c_adapter adapter;
+
+	/* DMA support components */
+	bool				dma_mode;
+	struct resource			*dmares;
+	struct dma_chan         	*dmach;
+	struct mxs_dma_data		dma_data;
+	uint32_t			pio_data[2];
+	uint32_t			addr_data;
+	struct scatterlist		sg_io[2];
+	bool				dma_read;
 };
 
 /*
@@ -129,7 +142,12 @@ static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 	writel(0x0015000d, i2c->regs + MXS_I2C_TIMING2);
 
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
-	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+
+	if (i2c->dma_mode)
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
+	else
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
 }
 
@@ -204,7 +222,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
 
 static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 {
-	u32 data;
+	u32 data = 0;
 	int i;
 
 	for (i = 0; i < len; i++) {
@@ -220,9 +238,155 @@ static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 	return 0;
 }
 
+static void mxs_i2c_dma_finish(struct mxs_i2c_dev *i2c)
+{
+	if (i2c->dma_read) {
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+	} else {
+		dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+	}
+}
+
+static void mxs_i2c_dma_irq_callback(void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	complete(&i2c->cmd_complete);
+	mxs_i2c_dma_finish(i2c);
+}
+
+static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msg, uint32_t flags)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
+
+	if (msg->flags & I2C_M_RD) {
+		i2c->dma_read = 1;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_READ;
+
+		/*
+		 * SELECT command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = MXS_CMD_I2C_SELECT;
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[0], &i2c->addr_data, 1);
+		dma_map_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[0], 1,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/*
+		 * READ command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[1] = flags | MXS_CMD_I2C_READ |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[1],
+					1, DMA_TRANS_NONE, DMA_PREP_INTERRUPT);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[1], 1,
+					DMA_DEV_TO_MEM,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto read_init_dma_fail;
+		}
+	} else {
+		i2c->dma_read = 0;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_WRITE;
+
+		/*
+		 * WRITE command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto write_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_table(i2c->sg_io, 2);
+		sg_set_buf(&i2c->sg_io[0], &i2c->addr_data, 1);
+		sg_set_buf(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, i2c->sg_io, 2,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto write_init_dma_fail;
+		}
+	}
+
+	/*
+	 * The last descriptor must have this callback,
+	 * to finish the DMA transaction.
+	 */
+	desc->callback = mxs_i2c_dma_irq_callback;
+	desc->callback_param = i2c;
+
+	/* Start the transfer. */
+	dmaengine_submit(desc);
+	dma_async_issue_pending(i2c->dmach);
+	return 0;
+
+/* Read failpath. */
+read_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+select_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+select_init_pio_fail:
+	return 1;
+
+
+/* Write failpath. */
+write_init_dma_fail:
+	dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+write_init_pio_fail:
+	return 1;
+}
+
 /*
  * Low level master read/write transaction.
  */
+
 static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 				int stop)
 {
@@ -230,6 +394,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	int ret;
 	int flags;
 
+	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
+
 	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
 		msg->addr, msg->len, msg->flags, stop);
 
@@ -238,23 +404,30 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 	init_completion(&i2c->cmd_complete);
 
-	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
-
-	if (msg->flags & I2C_M_RD)
-		mxs_i2c_pioq_setup_read(i2c, msg->addr, msg->len, flags);
-	else
-		mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf, msg->len,
-					flags);
+	if (i2c->dma_mode) {
+		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
+		if (ret)
+			return -EINVAL;
+	} else {
+		if (msg->flags & I2C_M_RD) {
+			mxs_i2c_pioq_setup_read(i2c, msg->addr,
+						msg->len, flags);
+		} else {
+			mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf,
+						msg->len, flags);
+		}
 
-	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+		writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
+	}
 
 	ret = wait_for_completion_timeout(&i2c->cmd_complete,
 						msecs_to_jiffies(1000));
+
 	if (ret == 0)
 		goto timeout;
 
-	if ((!i2c->cmd_err) && (msg->flags & I2C_M_RD)) {
+	if (!i2c->dma_mode && (!i2c->cmd_err) && (msg->flags & I2C_M_RD)) {
 		ret = mxs_i2c_finish_read(i2c, msg->buf, msg->len);
 		if (ret)
 			goto timeout;
@@ -269,7 +442,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 timeout:
 	dev_dbg(i2c->dev, "Timeout!\n");
+	mxs_i2c_dma_finish(i2c);
 	mxs_i2c_reset(i2c);
+
 	return -ETIMEDOUT;
 }
 
@@ -312,11 +487,13 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
 	else
 		i2c->cmd_err = 0;
 
-	is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
-		MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
+	if (!i2c->dma_mode) {
+		is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
+			MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
 
-	if (is_last_cmd || i2c->cmd_err)
-		complete(&i2c->cmd_complete);
+		if (!i2c->dma_mode && (is_last_cmd || i2c->cmd_err))
+			complete(&i2c->cmd_complete);
+	}
 
 	writel(stat, i2c->regs + MXS_I2C_CTRL1_CLR);
 
@@ -328,21 +505,41 @@ static const struct i2c_algorithm mxs_i2c_algo = {
 	.functionality = mxs_i2c_func,
 };
 
+static bool mxs_i2c_dma_filter(struct dma_chan *chan, void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	if (!mxs_dma_is_apbx(chan))
+		return false;
+
+	if (chan->chan_id != i2c->dmares->start)
+		return false;
+
+	chan->private = &i2c->dma_data;
+
+	return true;
+}
+
 static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mxs_i2c_dev *i2c;
 	struct i2c_adapter *adap;
-	struct resource *res;
+	struct resource *res, *dmares;
 	resource_size_t res_size;
-	int err, irq;
+	int err, irq, dmairq;
+	dma_cap_mask_t mask;
 
 	i2c = devm_kzalloc(dev, sizeof(struct mxs_i2c_dev), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
+	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+	irq = platform_get_irq(pdev, 0);
+	dmairq = platform_get_irq(pdev, 1);
+
+	if (!res || !dmares || irq < 0 || dmairq < 0)
 		return -ENOENT;
 
 	res_size = resource_size(res);
@@ -353,15 +550,32 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 	if (!i2c->regs)
 		return -EBUSY;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	err = devm_request_irq(dev, irq, mxs_i2c_isr, 0, dev_name(dev), i2c);
 	if (err)
 		return err;
 
 	i2c->dev = dev;
+
+	/*
+	 * The MXS I2C DMA mode is prefered and enabled by default.
+	 * The PIO mode is still supported, but should be used only
+	 * for debuging purposes etc.
+	 */
+	i2c->dma_mode = 1;
+
+	/* Setup the DMA */
+	if (i2c->dma_mode) {
+		i2c->dmares = dmares;
+		dma_cap_zero(mask);
+		dma_cap_set(DMA_SLAVE, mask);
+		i2c->dma_data.chan_irq = dmairq;
+		i2c->dmach = dma_request_channel(mask, mxs_i2c_dma_filter, i2c);
+		if (!i2c->dmach) {
+			dev_err(dev, "Failed to request dma\n");
+			return -ENODEV;
+		}
+	}
+
 	platform_set_drvdata(pdev, i2c);
 
 	/* Do reset to enforce correct startup after pinmuxing */
@@ -394,6 +608,9 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev)
 	if (ret)
 		return -EBUSY;
 
+	if (i2c->dmach)
+		dma_release_channel(i2c->dmach);
+
 	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
 			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
 	writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET);
-- 
1.7.10

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

This patch implements DMA support into mxs-i2c. DMA transfers are now enabled by
default, though it is possible for example for debugging purposes, to disable
them via i2c->dma_mode.

Cc: Linux I2C <linux-i2c@vger.kernel.org>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Wolfram Sang <w.sang@pengutronix.de>
---
 arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
 arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
 drivers/i2c/busses/i2c-mxs.c                    |  261 +++++++++++++++++++++--
 3 files changed, 245 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
index 79222ec..a94f308 100644
--- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
+++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
@@ -14,6 +14,7 @@
 	{								\
 		.id = _id,						\
 		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
+		.dma	= soc ## _DMA_I2C ## _id,			\
 		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
 		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
 	}
@@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
 			.end = data->iobase + SZ_8K - 1,
 			.flags = IORESOURCE_MEM,
 		}, {
+			.start	= data->dma,
+			.end	= data->dma,
+			.flags	= IORESOURCE_DMA,
+		}, {
 			.start = data->errirq,
 			.end = data->errirq,
 			.flags = IORESOURCE_IRQ,
diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
index f2e3839..791d99c 100644
--- a/arch/arm/mach-mxs/include/mach/devices-common.h
+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
@@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data *pdata,
 struct mxs_mxs_i2c_data {
 	int id;
 	resource_size_t iobase;
+	resource_size_t dma;
 	resource_size_t errirq;
 	resource_size_t dmairq;
 };
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 0d8e485..b364b8b 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -26,6 +26,9 @@
 #include <linux/platform_device.h>
 #include <linux/jiffies.h>
 #include <linux/io.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/fsl/mxs-dma.h>
 
 #include <mach/common.h>
 
@@ -113,6 +116,16 @@ struct mxs_i2c_dev {
 	struct completion cmd_complete;
 	u32 cmd_err;
 	struct i2c_adapter adapter;
+
+	/* DMA support components */
+	bool				dma_mode;
+	struct resource			*dmares;
+	struct dma_chan         	*dmach;
+	struct mxs_dma_data		dma_data;
+	uint32_t			pio_data[2];
+	uint32_t			addr_data;
+	struct scatterlist		sg_io[2];
+	bool				dma_read;
 };
 
 /*
@@ -129,7 +142,12 @@ static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 	writel(0x0015000d, i2c->regs + MXS_I2C_TIMING2);
 
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
-	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+
+	if (i2c->dma_mode)
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
+	else
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
 }
 
@@ -204,7 +222,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
 
 static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 {
-	u32 data;
+	u32 data = 0;
 	int i;
 
 	for (i = 0; i < len; i++) {
@@ -220,9 +238,155 @@ static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 	return 0;
 }
 
+static void mxs_i2c_dma_finish(struct mxs_i2c_dev *i2c)
+{
+	if (i2c->dma_read) {
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+	} else {
+		dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+	}
+}
+
+static void mxs_i2c_dma_irq_callback(void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	complete(&i2c->cmd_complete);
+	mxs_i2c_dma_finish(i2c);
+}
+
+static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msg, uint32_t flags)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
+
+	if (msg->flags & I2C_M_RD) {
+		i2c->dma_read = 1;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_READ;
+
+		/*
+		 * SELECT command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = MXS_CMD_I2C_SELECT;
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[0], &i2c->addr_data, 1);
+		dma_map_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[0], 1,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/*
+		 * READ command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[1] = flags | MXS_CMD_I2C_READ |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[1],
+					1, DMA_TRANS_NONE, DMA_PREP_INTERRUPT);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[1], 1,
+					DMA_DEV_TO_MEM,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto read_init_dma_fail;
+		}
+	} else {
+		i2c->dma_read = 0;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_WRITE;
+
+		/*
+		 * WRITE command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto write_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_table(i2c->sg_io, 2);
+		sg_set_buf(&i2c->sg_io[0], &i2c->addr_data, 1);
+		sg_set_buf(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, i2c->sg_io, 2,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto write_init_dma_fail;
+		}
+	}
+
+	/*
+	 * The last descriptor must have this callback,
+	 * to finish the DMA transaction.
+	 */
+	desc->callback = mxs_i2c_dma_irq_callback;
+	desc->callback_param = i2c;
+
+	/* Start the transfer. */
+	dmaengine_submit(desc);
+	dma_async_issue_pending(i2c->dmach);
+	return 0;
+
+/* Read failpath. */
+read_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+select_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+select_init_pio_fail:
+	return 1;
+
+
+/* Write failpath. */
+write_init_dma_fail:
+	dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+write_init_pio_fail:
+	return 1;
+}
+
 /*
  * Low level master read/write transaction.
  */
+
 static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 				int stop)
 {
@@ -230,6 +394,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	int ret;
 	int flags;
 
+	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
+
 	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
 		msg->addr, msg->len, msg->flags, stop);
 
@@ -238,23 +404,30 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 	init_completion(&i2c->cmd_complete);
 
-	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
-
-	if (msg->flags & I2C_M_RD)
-		mxs_i2c_pioq_setup_read(i2c, msg->addr, msg->len, flags);
-	else
-		mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf, msg->len,
-					flags);
+	if (i2c->dma_mode) {
+		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
+		if (ret)
+			return -EINVAL;
+	} else {
+		if (msg->flags & I2C_M_RD) {
+			mxs_i2c_pioq_setup_read(i2c, msg->addr,
+						msg->len, flags);
+		} else {
+			mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf,
+						msg->len, flags);
+		}
 
-	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+		writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
+	}
 
 	ret = wait_for_completion_timeout(&i2c->cmd_complete,
 						msecs_to_jiffies(1000));
+
 	if (ret == 0)
 		goto timeout;
 
-	if ((!i2c->cmd_err) && (msg->flags & I2C_M_RD)) {
+	if (!i2c->dma_mode && (!i2c->cmd_err) && (msg->flags & I2C_M_RD)) {
 		ret = mxs_i2c_finish_read(i2c, msg->buf, msg->len);
 		if (ret)
 			goto timeout;
@@ -269,7 +442,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 timeout:
 	dev_dbg(i2c->dev, "Timeout!\n");
+	mxs_i2c_dma_finish(i2c);
 	mxs_i2c_reset(i2c);
+
 	return -ETIMEDOUT;
 }
 
@@ -312,11 +487,13 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
 	else
 		i2c->cmd_err = 0;
 
-	is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
-		MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
+	if (!i2c->dma_mode) {
+		is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
+			MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
 
-	if (is_last_cmd || i2c->cmd_err)
-		complete(&i2c->cmd_complete);
+		if (!i2c->dma_mode && (is_last_cmd || i2c->cmd_err))
+			complete(&i2c->cmd_complete);
+	}
 
 	writel(stat, i2c->regs + MXS_I2C_CTRL1_CLR);
 
@@ -328,21 +505,41 @@ static const struct i2c_algorithm mxs_i2c_algo = {
 	.functionality = mxs_i2c_func,
 };
 
+static bool mxs_i2c_dma_filter(struct dma_chan *chan, void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	if (!mxs_dma_is_apbx(chan))
+		return false;
+
+	if (chan->chan_id != i2c->dmares->start)
+		return false;
+
+	chan->private = &i2c->dma_data;
+
+	return true;
+}
+
 static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mxs_i2c_dev *i2c;
 	struct i2c_adapter *adap;
-	struct resource *res;
+	struct resource *res, *dmares;
 	resource_size_t res_size;
-	int err, irq;
+	int err, irq, dmairq;
+	dma_cap_mask_t mask;
 
 	i2c = devm_kzalloc(dev, sizeof(struct mxs_i2c_dev), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
+	dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+	irq = platform_get_irq(pdev, 0);
+	dmairq = platform_get_irq(pdev, 1);
+
+	if (!res || !dmares || irq < 0 || dmairq < 0)
 		return -ENOENT;
 
 	res_size = resource_size(res);
@@ -353,15 +550,32 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 	if (!i2c->regs)
 		return -EBUSY;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	err = devm_request_irq(dev, irq, mxs_i2c_isr, 0, dev_name(dev), i2c);
 	if (err)
 		return err;
 
 	i2c->dev = dev;
+
+	/*
+	 * The MXS I2C DMA mode is prefered and enabled by default.
+	 * The PIO mode is still supported, but should be used only
+	 * for debuging purposes etc.
+	 */
+	i2c->dma_mode = 1;
+
+	/* Setup the DMA */
+	if (i2c->dma_mode) {
+		i2c->dmares = dmares;
+		dma_cap_zero(mask);
+		dma_cap_set(DMA_SLAVE, mask);
+		i2c->dma_data.chan_irq = dmairq;
+		i2c->dmach = dma_request_channel(mask, mxs_i2c_dma_filter, i2c);
+		if (!i2c->dmach) {
+			dev_err(dev, "Failed to request dma\n");
+			return -ENODEV;
+		}
+	}
+
 	platform_set_drvdata(pdev, i2c);
 
 	/* Do reset to enforce correct startup after pinmuxing */
@@ -394,6 +608,9 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev)
 	if (ret)
 		return -EBUSY;
 
+	if (i2c->dmach)
+		dma_release_channel(i2c->dmach);
+
 	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
 			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
 	writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET);
-- 
1.7.10

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Mon, Apr 30, 2012 at 12:36:08AM +0200, Marek Vasut wrote:
> This sets the bus to run at 400kHz, prior to this,
> the bus frequency was undefined.

Not exactly. The default values let it run at 100kHz. Since not all
slaves support 400kHz, I have to NACK this one. Making it configurable
is the way to go here.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

On Mon, Apr 30, 2012 at 12:36:08AM +0200, Marek Vasut wrote:
> This sets the bus to run at 400kHz, prior to this,
> the bus frequency was undefined.

Not exactly. The default values let it run at 100kHz. Since not all
slaves support 400kHz, I have to NACK this one. Making it configurable
is the way to go here.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/ce7d39c1/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]

On Mon, Apr 30, 2012 at 12:36:09AM +0200, Marek Vasut wrote:
> This patch implements DMA support into mxs-i2c. DMA transfers are now enabled by
> default, though it is possible for example for debugging purposes, to disable
> them via i2c->dma_mode.
> 
> Cc: Linux I2C <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Looks promising, will give it a shot this week! Thanks.
Very quick review.

> ---
>  arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
>  arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
>  drivers/i2c/busses/i2c-mxs.c                    |  261 +++++++++++++++++++++--
>  3 files changed, 245 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> index 79222ec..a94f308 100644
> --- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> +++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> @@ -14,6 +14,7 @@
>  	{								\
>  		.id = _id,						\
>  		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
> +		.dma	= soc ## _DMA_I2C ## _id,			\
>  		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
>  		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
>  	}
> @@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
>  			.end = data->iobase + SZ_8K - 1,
>  			.flags = IORESOURCE_MEM,
>  		}, {
> +			.start	= data->dma,
> +			.end	= data->dma,
> +			.flags	= IORESOURCE_DMA,
> +		}, {
>  			.start = data->errirq,
>  			.end = data->errirq,
>  			.flags = IORESOURCE_IRQ,
> diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> index f2e3839..791d99c 100644
> --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> @@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data *pdata,
>  struct mxs_mxs_i2c_data {
>  	int id;
>  	resource_size_t iobase;
> +	resource_size_t dma;
>  	resource_size_t errirq;
>  	resource_size_t dmairq;
>  };

You need to either split this and send via alkml or I need an ACK from
Shawn if it shall go in via I2C. You don't have Shawn on CC, please use
scripts/get_maintainer.pl on your patchesr; you have been missing
people/lists at least for the third time now.

>  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
>  {
> -	u32 data;
> +	u32 data = 0;

Unrelated, useless change.

>  	int i;
> +
> +	/*
> +	 * The MXS I2C DMA mode is prefered and enabled by default.
> +	 * The PIO mode is still supported, but should be used only
> +	 * for debuging purposes etc.
> +	 */

For exactness, should be "PIOQUEUE" instead of PIO. There is a PIO mode,
but we don't use it.

> +	i2c->dma_mode = 1;

Have you measured the overhead of DMA? I'd still think that 

	if (MX23 || msg->len > 24)
		do_dma
	else
		do_pioqueue

might be worth considered.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

On Mon, Apr 30, 2012 at 12:36:09AM +0200, Marek Vasut wrote:
> This patch implements DMA support into mxs-i2c. DMA transfers are now enabled by
> default, though it is possible for example for debugging purposes, to disable
> them via i2c->dma_mode.
> 
> Cc: Linux I2C <linux-i2c@vger.kernel.org>
> Cc: Detlev Zundel <dzu@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Wolfram Sang <w.sang@pengutronix.de>

Looks promising, will give it a shot this week! Thanks.
Very quick review.

> ---
>  arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
>  arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
>  drivers/i2c/busses/i2c-mxs.c                    |  261 +++++++++++++++++++++--
>  3 files changed, 245 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> index 79222ec..a94f308 100644
> --- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> +++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> @@ -14,6 +14,7 @@
>  	{								\
>  		.id = _id,						\
>  		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
> +		.dma	= soc ## _DMA_I2C ## _id,			\
>  		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
>  		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
>  	}
> @@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
>  			.end = data->iobase + SZ_8K - 1,
>  			.flags = IORESOURCE_MEM,
>  		}, {
> +			.start	= data->dma,
> +			.end	= data->dma,
> +			.flags	= IORESOURCE_DMA,
> +		}, {
>  			.start = data->errirq,
>  			.end = data->errirq,
>  			.flags = IORESOURCE_IRQ,
> diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> index f2e3839..791d99c 100644
> --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> @@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data *pdata,
>  struct mxs_mxs_i2c_data {
>  	int id;
>  	resource_size_t iobase;
> +	resource_size_t dma;
>  	resource_size_t errirq;
>  	resource_size_t dmairq;
>  };

You need to either split this and send via alkml or I need an ACK from
Shawn if it shall go in via I2C. You don't have Shawn on CC, please use
scripts/get_maintainer.pl on your patchesr; you have been missing
people/lists at least for the third time now.

>  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
>  {
> -	u32 data;
> +	u32 data = 0;

Unrelated, useless change.

>  	int i;
> +
> +	/*
> +	 * The MXS I2C DMA mode is prefered and enabled by default.
> +	 * The PIO mode is still supported, but should be used only
> +	 * for debuging purposes etc.
> +	 */

For exactness, should be "PIOQUEUE" instead of PIO. There is a PIO mode,
but we don't use it.

> +	i2c->dma_mode = 1;

Have you measured the overhead of DMA? I'd still think that 

	if (MX23 || msg->len > 24)
		do_dma
	else
		do_pioqueue

might be worth considered.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/58e70bbb/attachment.sig>

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Mon, Apr 30, 2012 at 12:36:08AM +0200, Marek Vasut wrote:
> > This sets the bus to run at 400kHz, prior to this,
> > the bus frequency was undefined.
> 
> Not exactly. The default values let it run at 100kHz.

Have you tried dumping the default values and comparing it with the values for 
100kHz in the manual?

> Since not all
> slaves support 400kHz, I have to NACK this one. Making it configurable
> is the way to go here.

All right, that's a good point.
> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Mon, Apr 30, 2012 at 12:36:08AM +0200, Marek Vasut wrote:
> > This sets the bus to run at 400kHz, prior to this,
> > the bus frequency was undefined.
> 
> Not exactly. The default values let it run at 100kHz.

Have you tried dumping the default values and comparing it with the values for 
100kHz in the manual?

> Since not all
> slaves support 400kHz, I have to NACK this one. Making it configurable
> is the way to go here.

All right, that's a good point.
> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Mon, Apr 30, 2012 at 12:36:09AM +0200, Marek Vasut wrote:
> > This patch implements DMA support into mxs-i2c. DMA transfers are now
> > enabled by default, though it is possible for example for debugging
> > purposes, to disable them via i2c->dma_mode.
> > 
> > Cc: Linux I2C <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
> > Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
> > Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> Looks promising, will give it a shot this week! Thanks.
> Very quick review.
> 
> > ---
> > 
> >  arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
> >  arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
> >  drivers/i2c/busses/i2c-mxs.c                    |  261
> >  +++++++++++++++++++++-- 3 files changed, 245 insertions(+), 22
> >  deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c index 79222ec..a94f308
> > 100644
> > --- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > +++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > @@ -14,6 +14,7 @@
> > 
> >  	{								\
> >  	
> >  		.id = _id,						\
> >  		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
> > 
> > +		.dma	= soc ## _DMA_I2C ## _id,			\
> > 
> >  		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
> >  		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
> >  	
> >  	}
> > 
> > @@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
> > 
> >  			.end = data->iobase + SZ_8K - 1,
> >  			.flags = IORESOURCE_MEM,
> >  		
> >  		}, {
> > 
> > +			.start	= data->dma,
> > +			.end	= data->dma,
> > +			.flags	= IORESOURCE_DMA,
> > +		}, {
> > 
> >  			.start = data->errirq,
> >  			.end = data->errirq,
> >  			.flags = IORESOURCE_IRQ,
> > 
> > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h
> > b/arch/arm/mach-mxs/include/mach/devices-common.h index f2e3839..791d99c
> > 100644
> > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > @@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data
> > *pdata,
> > 
> >  struct mxs_mxs_i2c_data {
> >  
> >  	int id;
> >  	resource_size_t iobase;
> > 
> > +	resource_size_t dma;
> > 
> >  	resource_size_t errirq;
> >  	resource_size_t dmairq;
> >  
> >  };
> 
> You need to either split this and send via alkml or I need an ACK from
> Shawn if it shall go in via I2C. You don't have Shawn on CC, please use
> scripts/get_maintainer.pl on your patchesr; you have been missing
> people/lists at least for the third time now.
> 
> >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> >  len) {
> > 
> > -	u32 data;
> > +	u32 data = 0;
> 
> Unrelated, useless change.

Have you tried compiling the code with gcc 4.7?

> 
> >  	int i;
> > 
> > +
> > +	/*
> > +	 * The MXS I2C DMA mode is prefered and enabled by default.
> > +	 * The PIO mode is still supported, but should be used only
> > +	 * for debuging purposes etc.
> > +	 */
> 
> For exactness, should be "PIOQUEUE" instead of PIO. There is a PIO mode,
> but we don't use it.

Ok, it indeed should.

> 
> > +	i2c->dma_mode = 1;
> 
> Have you measured the overhead of DMA? I'd still think that
> 
> 	if (MX23 || msg->len > 24)
> 		do_dma
> 	else
> 		do_pioqueue
> 
> might be worth considered.

It might, but in the 2.6.35.3 I have from FSL, it only does:

WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");

Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might as 
well make this dma/pioqueue mode configurable as well as speed via platform data 
until someone comes up with a patch that can mix pioqueue and dma?
> 
> Regards,
> 
>    Wolfram

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Mon, Apr 30, 2012 at 12:36:09AM +0200, Marek Vasut wrote:
> > This patch implements DMA support into mxs-i2c. DMA transfers are now
> > enabled by default, though it is possible for example for debugging
> > purposes, to disable them via i2c->dma_mode.
> > 
> > Cc: Linux I2C <linux-i2c@vger.kernel.org>
> > Cc: Detlev Zundel <dzu@denx.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Wolfram Sang <w.sang@pengutronix.de>
> 
> Looks promising, will give it a shot this week! Thanks.
> Very quick review.
> 
> > ---
> > 
> >  arch/arm/mach-mxs/devices/platform-mxs-i2c.c    |    5 +
> >  arch/arm/mach-mxs/include/mach/devices-common.h |    1 +
> >  drivers/i2c/busses/i2c-mxs.c                    |  261
> >  +++++++++++++++++++++-- 3 files changed, 245 insertions(+), 22
> >  deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c index 79222ec..a94f308
> > 100644
> > --- a/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > +++ b/arch/arm/mach-mxs/devices/platform-mxs-i2c.c
> > @@ -14,6 +14,7 @@
> > 
> >  	{								\
> >  	
> >  		.id = _id,						\
> >  		.iobase = soc ## _I2C ## _id ## _BASE_ADDR,		\
> > 
> > +		.dma	= soc ## _DMA_I2C ## _id,			\
> > 
> >  		.errirq = soc ## _INT_I2C ## _id ## _ERROR,		\
> >  		.dmairq = soc ## _INT_I2C ## _id ## _DMA,		\
> >  	
> >  	}
> > 
> > @@ -37,6 +38,10 @@ struct platform_device *__init mxs_add_mxs_i2c(
> > 
> >  			.end = data->iobase + SZ_8K - 1,
> >  			.flags = IORESOURCE_MEM,
> >  		
> >  		}, {
> > 
> > +			.start	= data->dma,
> > +			.end	= data->dma,
> > +			.flags	= IORESOURCE_DMA,
> > +		}, {
> > 
> >  			.start = data->errirq,
> >  			.end = data->errirq,
> >  			.flags = IORESOURCE_IRQ,
> > 
> > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h
> > b/arch/arm/mach-mxs/include/mach/devices-common.h index f2e3839..791d99c
> > 100644
> > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > @@ -80,6 +80,7 @@ mxs_add_gpmi_nand(const struct gpmi_nand_platform_data
> > *pdata,
> > 
> >  struct mxs_mxs_i2c_data {
> >  
> >  	int id;
> >  	resource_size_t iobase;
> > 
> > +	resource_size_t dma;
> > 
> >  	resource_size_t errirq;
> >  	resource_size_t dmairq;
> >  
> >  };
> 
> You need to either split this and send via alkml or I need an ACK from
> Shawn if it shall go in via I2C. You don't have Shawn on CC, please use
> scripts/get_maintainer.pl on your patchesr; you have been missing
> people/lists at least for the third time now.
> 
> >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> >  len) {
> > 
> > -	u32 data;
> > +	u32 data = 0;
> 
> Unrelated, useless change.

Have you tried compiling the code with gcc 4.7?

> 
> >  	int i;
> > 
> > +
> > +	/*
> > +	 * The MXS I2C DMA mode is prefered and enabled by default.
> > +	 * The PIO mode is still supported, but should be used only
> > +	 * for debuging purposes etc.
> > +	 */
> 
> For exactness, should be "PIOQUEUE" instead of PIO. There is a PIO mode,
> but we don't use it.

Ok, it indeed should.

> 
> > +	i2c->dma_mode = 1;
> 
> Have you measured the overhead of DMA? I'd still think that
> 
> 	if (MX23 || msg->len > 24)
> 		do_dma
> 	else
> 		do_pioqueue
> 
> might be worth considered.

It might, but in the 2.6.35.3 I have from FSL, it only does:

WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");

Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might as 
well make this dma/pioqueue mode configurable as well as speed via platform data 
until someone comes up with a patch that can mix pioqueue and dma?
> 
> Regards,
> 
>    Wolfram

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]


> > > This sets the bus to run at 400kHz, prior to this,
> > > the bus frequency was undefined.
> > 
> > Not exactly. The default values let it run at 100kHz.
> 
> Have you tried dumping the default values and comparing it with the values for 
> 100kHz in the manual?

Yes, sure. Doesn't it work for you? I just rechecked and the values are
okay. That being said, if you make it configurable for 400kHz, it would
in deed be better to rewrite the values for 100kHz, too.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

> > > This sets the bus to run at 400kHz, prior to this,
> > > the bus frequency was undefined.
> > 
> > Not exactly. The default values let it run at 100kHz.
> 
> Have you tried dumping the default values and comparing it with the values for 
> 100kHz in the manual?

Yes, sure. Doesn't it work for you? I just rechecked and the values are
okay. That being said, if you make it configurable for 400kHz, it would
in deed be better to rewrite the values for 100kHz, too.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/450aee0f/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]


> > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> > >  len) {
> > > 
> > > -	u32 data;
> > > +	u32 data = 0;
> > 
> > Unrelated, useless change.
> 
> Have you tried compiling the code with gcc 4.7?

What has the compiler version to do with the relevance? :) This has
nothing to do with adding DMA support. Plus, nobody so far could tell me
what path gcc is seeing there.

> > > +	i2c->dma_mode = 1;
> > 
> > Have you measured the overhead of DMA? I'd still think that
> > 
> > 	if (MX23 || msg->len > 24)
> > 		do_dma
> > 	else
> > 		do_pioqueue
> > 
> > might be worth considered.
> 
> It might, but in the 2.6.35.3 I have from FSL, it only does:
> 
> WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");

Well, we are back to the "FSL code is just a reference" topic. I truly
hope that part was implemented because of laziness and not because of
hardware limitations.

> Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might as 
> well make this dma/pioqueue mode configurable as well as speed via platform data 
> until someone comes up with a patch that can mix pioqueue and dma?

User configuration is not an option here IMO, because PIOQUEUE would still
fail for bigger transfers. "Always DMA" might be better then. But I will
try to switch modes this week, that's just too tempting.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> > >  len) {
> > > 
> > > -	u32 data;
> > > +	u32 data = 0;
> > 
> > Unrelated, useless change.
> 
> Have you tried compiling the code with gcc 4.7?

What has the compiler version to do with the relevance? :) This has
nothing to do with adding DMA support. Plus, nobody so far could tell me
what path gcc is seeing there.

> > > +	i2c->dma_mode = 1;
> > 
> > Have you measured the overhead of DMA? I'd still think that
> > 
> > 	if (MX23 || msg->len > 24)
> > 		do_dma
> > 	else
> > 		do_pioqueue
> > 
> > might be worth considered.
> 
> It might, but in the 2.6.35.3 I have from FSL, it only does:
> 
> WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");

Well, we are back to the "FSL code is just a reference" topic. I truly
hope that part was implemented because of laziness and not because of
hardware limitations.

> Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might as 
> well make this dma/pioqueue mode configurable as well as speed via platform data 
> until someone comes up with a patch that can mix pioqueue and dma?

User configuration is not an option here IMO, because PIOQUEUE would still
fail for bigger transfers. "Always DMA" might be better then. But I will
try to switch modes this week, that's just too tempting.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/b81d51a3/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> > > >  len) {
> > > > 
> > > > -	u32 data;
> > > > +	u32 data = 0;
> > > 
> > > Unrelated, useless change.
> > 
> > Have you tried compiling the code with gcc 4.7?
> 
> What has the compiler version to do with the relevance? :) This has
> nothing to do with adding DMA support. Plus, nobody so far could tell me
> what path gcc is seeing there.

It complains that data might be undefined, do you want it in a separate patch 
then ?

> 
> > > > +	i2c->dma_mode = 1;
> > > 
> > > Have you measured the overhead of DMA? I'd still think that
> > > 
> > > 	if (MX23 || msg->len > 24)
> > > 	
> > > 		do_dma
> > > 	
> > > 	else
> > > 	
> > > 		do_pioqueue
> > > 
> > > might be worth considered.
> > 
> > It might, but in the 2.6.35.3 I have from FSL, it only does:
> > 
> > WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");
> 
> Well, we are back to the "FSL code is just a reference" topic.

Obviously.

> I truly
> hope that part was implemented because of laziness and not because of
> hardware limitations.

I still havent figured out how to flip between pioqueue mode and dma mode 
without the controller getting confused, so I prefer to postpone it into 
subsequent patch. Eventually, I agree it'd be cool to be able to do this 
switching.

> > Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might
> > as well make this dma/pioqueue mode configurable as well as speed via
> > platform data until someone comes up with a patch that can mix pioqueue
> > and dma?
> 
> User configuration is not an option here IMO, because PIOQUEUE would still
> fail for bigger transfers. "Always DMA" might be better then.

Yes, that's what I have in V2.

> But I will
> try to switch modes this week, that's just too tempting.

I'll have a V2 patch for you by ... in a bit ;-) Also, you can grab latest 
version here (will be updated before I submit V2): 
http://git.kernel.org/?p=linux/kernel/git/marex/linux-2.6.git;a=shortlog;h=refs/heads/mxs-
i2c

> 
> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> > > >  len) {
> > > > 
> > > > -	u32 data;
> > > > +	u32 data = 0;
> > > 
> > > Unrelated, useless change.
> > 
> > Have you tried compiling the code with gcc 4.7?
> 
> What has the compiler version to do with the relevance? :) This has
> nothing to do with adding DMA support. Plus, nobody so far could tell me
> what path gcc is seeing there.

It complains that data might be undefined, do you want it in a separate patch 
then ?

> 
> > > > +	i2c->dma_mode = 1;
> > > 
> > > Have you measured the overhead of DMA? I'd still think that
> > > 
> > > 	if (MX23 || msg->len > 24)
> > > 	
> > > 		do_dma
> > > 	
> > > 	else
> > > 	
> > > 		do_pioqueue
> > > 
> > > might be worth considered.
> > 
> > It might, but in the 2.6.35.3 I have from FSL, it only does:
> > 
> > WARN_ONCE(len > 24, "choose DMA mode if xfer len > 24 bytes\n");
> 
> Well, we are back to the "FSL code is just a reference" topic.

Obviously.

> I truly
> hope that part was implemented because of laziness and not because of
> hardware limitations.

I still havent figured out how to flip between pioqueue mode and dma mode 
without the controller getting confused, so I prefer to postpone it into 
subsequent patch. Eventually, I agree it'd be cool to be able to do this 
switching.

> > Also, I tried switching PIOQUEUE/DMA, but didn't get it working. We might
> > as well make this dma/pioqueue mode configurable as well as speed via
> > platform data until someone comes up with a patch that can mix pioqueue
> > and dma?
> 
> User configuration is not an option here IMO, because PIOQUEUE would still
> fail for bigger transfers. "Always DMA" might be better then.

Yes, that's what I have in V2.

> But I will
> try to switch modes this week, that's just too tempting.

I'll have a V2 patch for you by ... in a bit ;-) Also, you can grab latest 
version here (will be updated before I submit V2): 
http://git.kernel.org/?p=linux/kernel/git/marex/linux-2.6.git;a=shortlog;h=refs/heads/mxs-
i2c

> 
> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > This sets the bus to run at 400kHz, prior to this,
> > > > the bus frequency was undefined.
> > > 
> > > Not exactly. The default values let it run at 100kHz.
> > 
> > Have you tried dumping the default values and comparing it with the
> > values for 100kHz in the manual?
> 
> Yes, sure. Doesn't it work for you? I just rechecked and the values are
> okay. That being said, if you make it configurable for 400kHz, it would
> in deed be better to rewrite the values for 100kHz, too.

They were different for me ... did you configure the i2c bus in your bootloader 
possibly? Those values might have been written there since then ... I didn't run 
i2c in u-boot before booting linux.

Best regards,
Marek Vasut

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > This sets the bus to run at 400kHz, prior to this,
> > > > the bus frequency was undefined.
> > > 
> > > Not exactly. The default values let it run at 100kHz.
> > 
> > Have you tried dumping the default values and comparing it with the
> > values for 100kHz in the manual?
> 
> Yes, sure. Doesn't it work for you? I just rechecked and the values are
> okay. That being said, if you make it configurable for 400kHz, it would
> in deed be better to rewrite the values for 100kHz, too.

They were different for me ... did you configure the i2c bus in your bootloader 
possibly? Those values might have been written there since then ... I didn't run 
i2c in u-boot before booting linux.

Best regards,
Marek Vasut

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]


> > > > > This sets the bus to run at 400kHz, prior to this,
> > > > > the bus frequency was undefined.
> > > > 
> > > > Not exactly. The default values let it run at 100kHz.
> > > 
> > > Have you tried dumping the default values and comparing it with the
> > > values for 100kHz in the manual?
> > 
> > Yes, sure. Doesn't it work for you? I just rechecked and the values are
> > okay. That being said, if you make it configurable for 400kHz, it would
> > in deed be better to rewrite the values for 100kHz, too.
> 
> They were different for me ... did you configure the i2c bus in your bootloader 
> possibly? Those values might have been written there since then ... I didn't run 
> i2c in u-boot before booting linux.

Nope, no I2C involved in the bootloader at all. Do you get the correct
values after soft-resetting the device?

It doesn't really matter, though. Just reinit them to the proper values
depending on the user setting.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

> > > > > This sets the bus to run at 400kHz, prior to this,
> > > > > the bus frequency was undefined.
> > > > 
> > > > Not exactly. The default values let it run at 100kHz.
> > > 
> > > Have you tried dumping the default values and comparing it with the
> > > values for 100kHz in the manual?
> > 
> > Yes, sure. Doesn't it work for you? I just rechecked and the values are
> > okay. That being said, if you make it configurable for 400kHz, it would
> > in deed be better to rewrite the values for 100kHz, too.
> 
> They were different for me ... did you configure the i2c bus in your bootloader 
> possibly? Those values might have been written there since then ... I didn't run 
> i2c in u-boot before booting linux.

Nope, no I2C involved in the bootloader at all. Do you get the correct
values after soft-resetting the device?

It doesn't really matter, though. Just reinit them to the proper values
depending on the user setting.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/b5db05cd/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]


> > What has the compiler version to do with the relevance? :) This has
> > nothing to do with adding DMA support. Plus, nobody so far could tell me
> > what path gcc is seeing there.
> 
> It complains that data might be undefined, do you want it in a separate patch 
> then ?

Yes, but only if you can prove that the compiler is right.

> I still havent figured out how to flip between pioqueue mode and dma mode 
> without the controller getting confused, so I prefer to postpone it into 
> subsequent patch. Eventually, I agree it'd be cool to be able to do this 
> switching.

Yes, send V2 with DMA by default, and I will give a try on the
mode-switching later, too.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > What has the compiler version to do with the relevance? :) This has
> > nothing to do with adding DMA support. Plus, nobody so far could tell me
> > what path gcc is seeing there.
> 
> It complains that data might be undefined, do you want it in a separate patch 
> then ?

Yes, but only if you can prove that the compiler is right.

> I still havent figured out how to flip between pioqueue mode and dma mode 
> without the controller getting confused, so I prefer to postpone it into 
> subsequent patch. Eventually, I agree it'd be cool to be able to do this 
> switching.

Yes, send V2 with DMA by default, and I will give a try on the
mode-switching later, too.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/49e0a24c/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > What has the compiler version to do with the relevance? :) This has
> > > nothing to do with adding DMA support. Plus, nobody so far could tell
> > > me what path gcc is seeing there.
> > 
> > It complains that data might be undefined, do you want it in a separate
> > patch then ?
> 
> Yes, but only if you can prove that the compiler is right.

It's not right, because that variable is always initialized, but this at least 
squashes the compile warning.

> > I still havent figured out how to flip between pioqueue mode and dma mode
> > without the controller getting confused, so I prefer to postpone it into
> > subsequent patch. Eventually, I agree it'd be cool to be able to do this
> > switching.
> 
> Yes, send V2 with DMA by default, and I will give a try on the
> mode-switching later, too.

I'd prefer to have PIOQ by default, to avoid breaking some of the boards. Or can 
you test on all of them?

> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > What has the compiler version to do with the relevance? :) This has
> > > nothing to do with adding DMA support. Plus, nobody so far could tell
> > > me what path gcc is seeing there.
> > 
> > It complains that data might be undefined, do you want it in a separate
> > patch then ?
> 
> Yes, but only if you can prove that the compiler is right.

It's not right, because that variable is always initialized, but this at least 
squashes the compile warning.

> > I still havent figured out how to flip between pioqueue mode and dma mode
> > without the controller getting confused, so I prefer to postpone it into
> > subsequent patch. Eventually, I agree it'd be cool to be able to do this
> > switching.
> 
> Yes, send V2 with DMA by default, and I will give a try on the
> mode-switching later, too.

I'd prefer to have PIOQ by default, to avoid breaking some of the boards. Or can 
you test on all of them?

> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > > > This sets the bus to run at 400kHz, prior to this,
> > > > > > the bus frequency was undefined.
> > > > > 
> > > > > Not exactly. The default values let it run at 100kHz.
> > > > 
> > > > Have you tried dumping the default values and comparing it with the
> > > > values for 100kHz in the manual?
> > > 
> > > Yes, sure. Doesn't it work for you? I just rechecked and the values are
> > > okay. That being said, if you make it configurable for 400kHz, it would
> > > in deed be better to rewrite the values for 100kHz, too.
> > 
> > They were different for me ... did you configure the i2c bus in your
> > bootloader possibly? Those values might have been written there since
> > then ... I didn't run i2c in u-boot before booting linux.
> 
> Nope, no I2C involved in the bootloader at all. Do you get the correct
> values after soft-resetting the device?
> 
> It doesn't really matter, though. Just reinit them to the proper values
> depending on the user setting.

Indeed. Default to 100kHz might be a good idea too, in case user entered some 
weird value. Eventually, it'd be nice if we could calculate the timing values 
for all possible frequencies, but I didn't got to deriving such formula ... 
dunno if it's worth it either, doubt so.

Best regards,
Marek Vasut

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > > > This sets the bus to run at 400kHz, prior to this,
> > > > > > the bus frequency was undefined.
> > > > > 
> > > > > Not exactly. The default values let it run at 100kHz.
> > > > 
> > > > Have you tried dumping the default values and comparing it with the
> > > > values for 100kHz in the manual?
> > > 
> > > Yes, sure. Doesn't it work for you? I just rechecked and the values are
> > > okay. That being said, if you make it configurable for 400kHz, it would
> > > in deed be better to rewrite the values for 100kHz, too.
> > 
> > They were different for me ... did you configure the i2c bus in your
> > bootloader possibly? Those values might have been written there since
> > then ... I didn't run i2c in u-boot before booting linux.
> 
> Nope, no I2C involved in the bootloader at all. Do you get the correct
> values after soft-resetting the device?
> 
> It doesn't really matter, though. Just reinit them to the proper values
> depending on the user setting.

Indeed. Default to 100kHz might be a good idea too, in case user entered some 
weird value. Eventually, it'd be nice if we could calculate the timing values 
for all possible frequencies, but I didn't got to deriving such formula ... 
dunno if it's worth it either, doubt so.

Best regards,
Marek Vasut

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]


> > > It complains that data might be undefined, do you want it in a separate
> > > patch then ?
> > 
> > Yes, but only if you can prove that the compiler is right.
> 
> It's not right, because that variable is always initialized, but this at least 
> squashes the compile warning.

The compiler needs to be fixed, not the kernel.

> > Yes, send V2 with DMA by default, and I will give a try on the
> > mode-switching later, too.
> 
> I'd prefer to have PIOQ by default, to avoid breaking some of the boards. Or can 
> you test on all of them?

? What kind of breakage do you expect? I think it is okay if we pull it
in during the merge window; we can fix issues then till the relase (and
maybe mode switching works, after all).

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > > It complains that data might be undefined, do you want it in a separate
> > > patch then ?
> > 
> > Yes, but only if you can prove that the compiler is right.
> 
> It's not right, because that variable is always initialized, but this at least 
> squashes the compile warning.

The compiler needs to be fixed, not the kernel.

> > Yes, send V2 with DMA by default, and I will give a try on the
> > mode-switching later, too.
> 
> I'd prefer to have PIOQ by default, to avoid breaking some of the boards. Or can 
> you test on all of them?

? What kind of breakage do you expect? I think it is okay if we pull it
in during the merge window; we can fix issues then till the relase (and
maybe mode switching works, after all).

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/2d34f440/attachment.sig>

Re: [PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

> Indeed. Default to 100kHz might be a good idea too, in case user entered some 
> weird value. Eventually, it'd be nice if we could calculate the timing values 
> for all possible frequencies, but I didn't got to deriving such formula ... 
> dunno if it's worth it either, doubt so.

Would be awesome to have it; but we can leave it for later when it is
actually needed.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/2] I2C: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

> Indeed. Default to 100kHz might be a good idea too, in case user entered some 
> weird value. Eventually, it'd be nice if we could calculate the timing values 
> for all possible frequencies, but I didn't got to deriving such formula ... 
> dunno if it's worth it either, doubt so.

Would be awesome to have it; but we can leave it for later when it is
actually needed.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120430/904f919e/attachment.sig>

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > It complains that data might be undefined, do you want it in a
> > > > separate patch then ?
> > > 
> > > Yes, but only if you can prove that the compiler is right.
> > 
> > It's not right, because that variable is always initialized, but this at
> > least squashes the compile warning.
> 
> The compiler needs to be fixed, not the kernel.
> 
> > > Yes, send V2 with DMA by default, and I will give a try on the
> > > mode-switching later, too.
> > 
> > I'd prefer to have PIOQ by default, to avoid breaking some of the boards.
> > Or can you test on all of them?
> 
> ? What kind of breakage do you expect? I think it is okay if we pull it
> in during the merge window; we can fix issues then till the relase (and
> maybe mode switching works, after all).

Ok, so be it then.

Best regards,
Marek Vasut

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > > It complains that data might be undefined, do you want it in a
> > > > separate patch then ?
> > > 
> > > Yes, but only if you can prove that the compiler is right.
> > 
> > It's not right, because that variable is always initialized, but this at
> > least squashes the compile warning.
> 
> The compiler needs to be fixed, not the kernel.
> 
> > > Yes, send V2 with DMA by default, and I will give a try on the
> > > mode-switching later, too.
> > 
> > I'd prefer to have PIOQ by default, to avoid breaking some of the boards.
> > Or can you test on all of them?
> 
> ? What kind of breakage do you expect? I think it is okay if we pull it
> in during the merge window; we can fix issues then till the relase (and
> maybe mode switching works, after all).

Ok, so be it then.

Best regards,
Marek Vasut

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Shawn Guo]

On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> 
> > > > It complains that data might be undefined, do you want it in a separate
> > > > patch then ?
> > > 
> > > Yes, but only if you can prove that the compiler is right.
> > 
> > It's not right, because that variable is always initialized, but this at least 
> > squashes the compile warning.
> 
> The compiler needs to be fixed, not the kernel.
> 
Heh, this is coming up again.  I'm not convincing you to take the
change, but just curious what if this is not a compile warning but
an error for the same cause that compiler is not right.

-- 
Regards,
Shawn

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Shawn Guo]

On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> 
> > > > It complains that data might be undefined, do you want it in a separate
> > > > patch then ?
> > > 
> > > Yes, but only if you can prove that the compiler is right.
> > 
> > It's not right, because that variable is always initialized, but this at least 
> > squashes the compile warning.
> 
> The compiler needs to be fixed, not the kernel.
> 
Heh, this is coming up again.  I'm not convincing you to take the
change, but just curious what if this is not a compile warning but
an error for the same cause that compiler is not right.

-- 
Regards,
Shawn

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Shawn Guo,

> On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > It complains that data might be undefined, do you want it in a
> > > > > separate patch then ?
> > > > 
> > > > Yes, but only if you can prove that the compiler is right.
> > > 
> > > It's not right, because that variable is always initialized, but this
> > > at least squashes the compile warning.
> > 
> > The compiler needs to be fixed, not the kernel.

I second this.

> Heh, this is coming up again.  I'm not convincing you to take the
> change, but just curious what if this is not a compile warning but
> an error for the same cause that compiler is not right.

Can you elaborate?

Best regards,
Marek Vasut

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Shawn Guo,

> On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > It complains that data might be undefined, do you want it in a
> > > > > separate patch then ?
> > > > 
> > > > Yes, but only if you can prove that the compiler is right.
> > > 
> > > It's not right, because that variable is always initialized, but this
> > > at least squashes the compile warning.
> > 
> > The compiler needs to be fixed, not the kernel.

I second this.

> Heh, this is coming up again.  I'm not convincing you to take the
> change, but just curious what if this is not a compile warning but
> an error for the same cause that compiler is not right.

Can you elaborate?

Best regards,
Marek Vasut

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Shawn Guo]

On Tue, May 01, 2012 at 04:02:56PM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > > It complains that data might be undefined, do you want it in a
> > > > > > separate patch then ?
> > > > > 
> > > > > Yes, but only if you can prove that the compiler is right.
> > > > 
> > > > It's not right, because that variable is always initialized, but this
> > > > at least squashes the compile warning.
> > > 
> > > The compiler needs to be fixed, not the kernel.
> 
> I second this.
> 
I agree on this too.

> > Heh, this is coming up again.  I'm not convincing you to take the
> > change, but just curious what if this is not a compile warning but
> > an error for the same cause that compiler is not right.
> 
> Can you elaborate?
> 
Will we have the kernel not compilable for a few cycles until the
compiler fixing is available, or we work it around to keep kernel
compiling and working before we have the compiler fixed?

-- 
Regards,
Shawn

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Shawn Guo]

On Tue, May 01, 2012 at 04:02:56PM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > > It complains that data might be undefined, do you want it in a
> > > > > > separate patch then ?
> > > > > 
> > > > > Yes, but only if you can prove that the compiler is right.
> > > > 
> > > > It's not right, because that variable is always initialized, but this
> > > > at least squashes the compile warning.
> > > 
> > > The compiler needs to be fixed, not the kernel.
> 
> I second this.
> 
I agree on this too.

> > Heh, this is coming up again.  I'm not convincing you to take the
> > change, but just curious what if this is not a compile warning but
> > an error for the same cause that compiler is not right.
> 
> Can you elaborate?
> 
Will we have the kernel not compilable for a few cycles until the
compiler fixing is available, or we work it around to keep kernel
compiling and working before we have the compiler fixed?

-- 
Regards,
Shawn

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Shawn Guo,

> On Tue, May 01, 2012 at 04:02:56PM +0200, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > > > It complains that data might be undefined, do you want it in a
> > > > > > > separate patch then ?
> > > > > > 
> > > > > > Yes, but only if you can prove that the compiler is right.
> > > > > 
> > > > > It's not right, because that variable is always initialized, but
> > > > > this at least squashes the compile warning.
> > > > 
> > > > The compiler needs to be fixed, not the kernel.
> > 
> > I second this.
> 
> I agree on this too.
> 
> > > Heh, this is coming up again.  I'm not convincing you to take the
> > > change, but just curious what if this is not a compile warning but
> > > an error for the same cause that compiler is not right.
> > 
> > Can you elaborate?
> 
> Will we have the kernel not compilable for a few cycles until the
> compiler fixing is available, or we work it around to keep kernel
> compiling and working before we have the compiler fixed?

This is kind of lacmus paper showing if the compiler was fixed. On the other 
hand, many people won't have a fixed compiler for a long time, so I'd be for 
applying this patch.

Best regards,
Marek Vasut

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Shawn Guo,

> On Tue, May 01, 2012 at 04:02:56PM +0200, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > > > > > > It complains that data might be undefined, do you want it in a
> > > > > > > separate patch then ?
> > > > > > 
> > > > > > Yes, but only if you can prove that the compiler is right.
> > > > > 
> > > > > It's not right, because that variable is always initialized, but
> > > > > this at least squashes the compile warning.
> > > > 
> > > > The compiler needs to be fixed, not the kernel.
> > 
> > I second this.
> 
> I agree on this too.
> 
> > > Heh, this is coming up again.  I'm not convincing you to take the
> > > change, but just curious what if this is not a compile warning but
> > > an error for the same cause that compiler is not right.
> > 
> > Can you elaborate?
> 
> Will we have the kernel not compilable for a few cycles until the
> compiler fixing is available, or we work it around to keep kernel
> compiling and working before we have the compiler fixed?

This is kind of lacmus paper showing if the compiler was fixed. On the other 
hand, many people won't have a fixed compiler for a long time, so I'd be for 
applying this patch.

Best regards,
Marek Vasut

Re: [PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Russell King - ARM Linux]

On Tue, May 01, 2012 at 09:58:03PM +0800, Shawn Guo wrote:
> On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > 
> > > > > It complains that data might be undefined, do you want it in a separate
> > > > > patch then ?
> > > > 
> > > > Yes, but only if you can prove that the compiler is right.
> > > 
> > > It's not right, because that variable is always initialized, but this at least 
> > > squashes the compile warning.
> > 
> > The compiler needs to be fixed, not the kernel.
> > 
> Heh, this is coming up again.  I'm not convincing you to take the
> change, but just curious what if this is not a compile warning but
> an error for the same cause that compiler is not right.

It's a compiler warning.

Please look at the wording of the warning.  The compiler uses "may" not
"is".  That means it can't come to a conclusion about it.

But we, as humans with our biological inference engines, can see that
the code is correct, and we _can_ choose to ignore the warning -
just like I do with:

drivers/video/omap2/displays/panel-taal.c: In function 'taal_num_errors_show':
drivers/video/omap2/displays/panel-taal.c:605: warning: 'errors' may be used uninitialized in this function

static int taal_dcs_read_1(struct taal_data *td, u8 dcs_cmd, u8 *data)
{
        int r;
        u8 buf[1];
        r = dsi_vc_dcs_read(td->dssdev, td->channel, dcs_cmd, buf, 1);
        if (r < 0)
                return r;
        *data = buf[0];  
        return 0;
}

        u8 errors;

        mutex_lock(&td->lock);
        if (td->enabled) {
                dsi_bus_lock(dssdev);
                r = taal_wake_up(dssdev);
                if (!r) 
                        r = taal_dcs_read_1(td, DCS_READ_NUM_ERRORS, &errors);
                dsi_bus_unlock(dssdev);
        } else {
                r = -ENODEV;
        }
        mutex_unlock(&td->lock);

        if (r)
                return r;

        return snprintf(buf, PAGE_SIZE, "%d\n", errors);

See?  We can infer from the above that that 'errors' will always be set
if r is zero - but the compiler is saying it's not clever enough to do
that automatically.

This doesn't mean the code _has_ to be changed - we can see that the
above warning is false.  Though, if there is a way to rewrite it to make
it easier for us humans to read and that makes the warning go away, it
_might_ be a good idea to make the change - but only for a maintainability
reason.

The reverse is also true - if trying to fix a warning like this makes
the code more difficult to understand, then the warning _should_ stay.

That's actually the point of programming languages - the ability to
express our instructions to a computer in a way that both we and the
compiler can readily understand.  The most important part of that is
that _we_ understand what's been written.

[PATCH 2/2] I2C: Implement DMA support into mxs-i2c

[Russell King - ARM Linux]

On Tue, May 01, 2012 at 09:58:03PM +0800, Shawn Guo wrote:
> On Mon, Apr 30, 2012 at 11:01:08PM +0200, Wolfram Sang wrote:
> > 
> > > > > It complains that data might be undefined, do you want it in a separate
> > > > > patch then ?
> > > > 
> > > > Yes, but only if you can prove that the compiler is right.
> > > 
> > > It's not right, because that variable is always initialized, but this at least 
> > > squashes the compile warning.
> > 
> > The compiler needs to be fixed, not the kernel.
> > 
> Heh, this is coming up again.  I'm not convincing you to take the
> change, but just curious what if this is not a compile warning but
> an error for the same cause that compiler is not right.

It's a compiler warning.

Please look at the wording of the warning.  The compiler uses "may" not
"is".  That means it can't come to a conclusion about it.

But we, as humans with our biological inference engines, can see that
the code is correct, and we _can_ choose to ignore the warning -
just like I do with:

drivers/video/omap2/displays/panel-taal.c: In function 'taal_num_errors_show':
drivers/video/omap2/displays/panel-taal.c:605: warning: 'errors' may be used uninitialized in this function

static int taal_dcs_read_1(struct taal_data *td, u8 dcs_cmd, u8 *data)
{
        int r;
        u8 buf[1];
        r = dsi_vc_dcs_read(td->dssdev, td->channel, dcs_cmd, buf, 1);
        if (r < 0)
                return r;
        *data = buf[0];  
        return 0;
}

        u8 errors;

        mutex_lock(&td->lock);
        if (td->enabled) {
                dsi_bus_lock(dssdev);
                r = taal_wake_up(dssdev);
                if (!r) 
                        r = taal_dcs_read_1(td, DCS_READ_NUM_ERRORS, &errors);
                dsi_bus_unlock(dssdev);
        } else {
                r = -ENODEV;
        }
        mutex_unlock(&td->lock);

        if (r)
                return r;

        return snprintf(buf, PAGE_SIZE, "%d\n", errors);

See?  We can infer from the above that that 'errors' will always be set
if r is zero - but the compiler is saying it's not clever enough to do
that automatically.

This doesn't mean the code _has_ to be changed - we can see that the
above warning is false.  Though, if there is a way to rewrite it to make
it easier for us humans to read and that makes the warning go away, it
_might_ be a good idea to make the change - but only for a maintainability
reason.

The reverse is also true - if trying to fix a warning like this makes
the code more difficult to understand, then the warning _should_ stay.

That's actually the point of programming languages - the ability to
express our instructions to a computer in a way that both we and the
compiler can readily understand.  The most important part of that is
that _we_ understand what's been written.