[PATCH 0/3] i2c: add DMA support for freescale i2c driver

[PATCH 0/3] i2c: add DMA support for freescale i2c driver

[Yuan Yao]

Added in v1:
- Enable dma if it's support dma and transfer size bigger than the threshold.
- Add device tree bindings for i2c eDMA support.
- Add eDMA support for i2c driver.

[PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Yuan Yao]

Add dma support for i2c. This function depend on DMA driver.
You can turn on it by write both the dmas and dma-name properties in dts node.
And you should set ".has_dma_support" as true for dma support in imx_i2c_hwdata struct.

Signed-off-by: Yuan Yao <yao.yuan-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/i2c/busses/i2c-imx.c | 358 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 344 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index db895fb..6ec392b 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,22 +37,27 @@
 /** Includes *******************************************************************
 *******************************************************************************/
 
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/dmapool.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
-#include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/init.h>
 #include <linux/io.h>
-#include <linux/sched.h>
-#include <linux/platform_device.h>
-#include <linux/clk.h>
-#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/platform_data/i2c-imx.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
 
 /** Defines ********************************************************************
 *******************************************************************************/
@@ -63,6 +68,9 @@
 /* Default value */
 #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
 
+/* enable DMA if transfer size is bigger than this threshold */
+#define IMX_I2C_DMA_THRESHOLD	16
+
 /* IMX I2C registers:
  * the I2C register offset is different between SoCs,
  * to provid support for all these chips, split the
@@ -88,6 +96,7 @@
 #define I2SR_IBB	0x20
 #define I2SR_IAAS	0x40
 #define I2SR_ICF	0x80
+#define I2CR_DMAEN	0x02
 #define I2CR_RSTA	0x04
 #define I2CR_TXAK	0x08
 #define I2CR_MTX	0x10
@@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
 	unsigned		ndivs;
 	unsigned		i2sr_clr_opcode;
 	unsigned		i2cr_ien_opcode;
+	bool			has_dma_support;
+};
+
+struct imx_i2c_dma {
+	struct dma_chan		*chan_tx;
+	struct dma_chan		*chan_rx;
+	dma_addr_t		buf_tx;
+	dma_addr_t		buf_rx;
+	unsigned int		len_tx;
+	unsigned int		len_rx;
+	struct completion	cmd_complete;
 };
 
 struct imx_i2c_struct {
@@ -184,6 +204,9 @@ struct imx_i2c_struct {
 	int			stopped;
 	unsigned int		ifdr; /* IMX_I2C_IFDR */
 	const struct imx_i2c_hwdata	*hwdata;
+
+	bool			use_dma;
+	struct imx_i2c_dma	*dma;
 };
 
 static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
@@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
 	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
 	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
 	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
+	.has_dma_support	= false,
 
 };
 
@@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  = {
 	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
 	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
 	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
+	.has_dma_support	= false,
 
 };
 
@@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
 	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
 	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
 	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
+	.has_dma_support	= true,
 
 };
 
@@ -254,6 +280,155 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
 	return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
 }
 
+/** Functions for DMA support ************************************************
+******************************************************************************/
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, u32 phy_addr)
+{
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+	struct dma_slave_config dma_sconfig;
+	int ret;
+
+	dma->chan_tx = dma_request_slave_channel(&i2c_imx->adapter.dev, "tx");
+	if (!dma->chan_tx) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Dma tx channel request failed!\n");
+		return -ENODEV;
+	}
+
+	dma_sconfig.dst_addr = (dma_addr_t)phy_addr +
+				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_maxburst = 1;
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
+	if (ret < 0) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Dma slave config failed, err = %d\n", ret);
+		dma_release_channel(dma->chan_tx);
+		return ret;
+	}
+
+	dma->chan_rx = dma_request_slave_channel(&i2c_imx->adapter.dev, "rx");
+	if (!dma->chan_rx) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Dma rx channel request failed!\n");
+		return -ENODEV;
+	}
+
+	dma_sconfig.src_addr = (dma_addr_t)phy_addr +
+				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.direction = DMA_DEV_TO_MEM;
+	ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
+	if (ret < 0) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Dma slave config failed, err = %d\n", ret);
+		dma_release_channel(dma->chan_rx);
+		return ret;
+	}
+
+	init_completion(&dma->cmd_complete);
+
+	return 0;
+}
+
+static void i2c_imx_dma_tx_callback(void *arg)
+{
+	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+
+	dma_unmap_single(dma->chan_tx->device->dev, dma->buf_tx,
+			dma->len_tx, DMA_TO_DEVICE);
+	complete(&dma->cmd_complete);
+}
+
+static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+{
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+	struct dma_async_tx_descriptor *txdesc;
+
+	dma->len_tx = msgs->len-1;
+	dma->buf_tx = dma_map_single(dma->chan_tx->device->dev, msgs->buf,
+					dma->len_tx, DMA_TO_DEVICE);
+	if (dma_mapping_error(dma->chan_tx->device->dev, dma->buf_tx)) {
+		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
+		return -EINVAL;
+	}
+
+	txdesc = dmaengine_prep_slave_single(dma->chan_tx, dma->buf_tx,
+					dma->len_tx, DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!txdesc) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Not able to get desc for tx\n");
+		return -EINVAL;
+	}
+
+	txdesc->callback = i2c_imx_dma_tx_callback;
+	txdesc->callback_param = i2c_imx;
+	dmaengine_submit(txdesc);
+	dma_async_issue_pending(dma->chan_tx);
+	
+	return 0;
+}
+
+static void i2c_imx_dma_rx_callback(void *arg)
+{
+	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+
+	dma_unmap_single(dma->chan_rx->device->dev, dma->buf_rx,
+				dma->len_rx, DMA_FROM_DEVICE);
+	complete(&dma->cmd_complete);
+}
+
+static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+{
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+	struct dma_async_tx_descriptor *txdesc;
+
+	dma->len_rx = msgs->len - 2;
+	dma->buf_rx = dma_map_single(dma->chan_rx->device->dev, msgs->buf,
+					dma->len_rx, DMA_FROM_DEVICE);
+	if (dma_mapping_error(dma->chan_rx->device->dev, dma->buf_rx)) {
+		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
+		return -EINVAL;
+	}
+
+	txdesc = dmaengine_prep_slave_single(dma->chan_rx, dma->buf_rx,
+					dma->len_rx, DMA_DEV_TO_MEM,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!txdesc) {
+		dev_err(&i2c_imx->adapter.dev,
+				"Not able to get desc for rx\n");
+		return -EINVAL;
+	}
+
+	txdesc->callback = i2c_imx_dma_rx_callback;
+	txdesc->callback_param = i2c_imx;
+	dmaengine_submit(txdesc);
+	dma_async_issue_pending(dma->chan_rx);
+	
+	return 0;
+}
+static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
+{
+	struct imx_i2c_dma *dma = i2c_imx->dma;
+	struct dma_chan *dma_chan;
+
+	dma_chan = dma->chan_tx;
+	dma->chan_tx = NULL;
+	dma->buf_tx = 0;
+	dma->len_tx = 0;
+	dma_release_channel(dma_chan);
+
+	dma_chan = dma->chan_rx;
+	dma->chan_tx = NULL;
+	dma->buf_rx = 0;
+	dma->len_rx = 0;
+	dma_release_channel(dma_chan);
+}
 /** Functions for IMX I2C adapter driver ***************************************
 *******************************************************************************/
 
@@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
+							struct i2c_msg *msgs)
 {
 	int i, result;
 
@@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 	return 0;
 }
 
-static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
+							struct i2c_msg *msgs)
+{
+	int result, timeout=1000;
+	unsigned int temp = 0;
+
+	reinit_completion(&i2c_imx->dma->cmd_complete);
+	result = i2c_imx_dma_tx(i2c_imx, msgs);
+	if(result)
+		return result;
+
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp |= I2CR_DMAEN;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+	/* write slave address */
+	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
+	result = wait_for_completion_interruptible_timeout(
+						&i2c_imx->dma->cmd_complete,
+						msecs_to_jiffies(1000));
+	if (result == 0)
+		return  -ETIMEDOUT;
+
+	/* waiting for Transfer complete. */
+	while(timeout--) {
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+		if (temp & 0x80)
+			break;
+		udelay(10);
+	}
+
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp &= ~I2CR_DMAEN;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+	/* write the last byte */
+	imx_i2c_write_reg(msgs->buf[msgs->len-1], i2c_imx, IMX_I2C_I2DR);
+	result = i2c_imx_trx_complete(i2c_imx);
+	if (result)
+		return result;
+
+	result = i2c_imx_acked(i2c_imx);
+	if (result)
+		return result;
+
+	return 0;
+}
+
+static int i2c_imx_pio_read(struct imx_i2c_struct *i2c_imx,
+							struct i2c_msg *msgs)
 {
 	int i, result;
 	unsigned int temp;
@@ -518,6 +743,80 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 	return 0;
 }
 
+static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
+							struct i2c_msg *msgs)
+{
+	int result, timeout=1000;
+	unsigned int temp;
+
+	/* write slave address */
+	imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR);
+	result = i2c_imx_trx_complete(i2c_imx);
+	if (result)
+		return result;
+
+	result = i2c_imx_acked(i2c_imx);
+	if (result)
+		return result;
+
+	/* setup bus to read data */
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp &= ~I2CR_MTX;
+	if (msgs->len - 1)
+		temp &= ~I2CR_TXAK;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
+
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp |= I2CR_DMAEN;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+	reinit_completion(&i2c_imx->dma->cmd_complete);
+	result = i2c_imx_dma_rx(i2c_imx, msgs);
+	if(result)
+		return result;
+
+	result = wait_for_completion_interruptible_timeout(
+						&i2c_imx->dma->cmd_complete,
+						msecs_to_jiffies(1000));
+	if (result == 0)
+		return  -ETIMEDOUT;
+
+	/* waiting for Transfer complete. */
+	while(timeout--) {
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+		if (temp & 0x80)
+			break;
+		udelay(10);
+	}
+
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp &= ~I2CR_DMAEN;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+	/* read n-1 byte data */
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp |= I2CR_TXAK;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+	/* read n byte data */
+	result = i2c_imx_trx_complete(i2c_imx);
+	if (result)
+		return result;
+
+	/* It must generate STOP before read I2DR to prevent
+	   controller from generating another clock cycle */
+	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	temp &= ~(I2CR_MSTA | I2CR_MTX);
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+	i2c_imx_bus_busy(i2c_imx, 0);
+	i2c_imx->stopped = 1;
+	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+
+	return 0;
+}
+
 static int i2c_imx_xfer(struct i2c_adapter *adapter,
 						struct i2c_msg *msgs, int num)
 {
@@ -563,10 +862,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
 			(temp & I2SR_RXAK ? 1 : 0));
 #endif
-		if (msgs[i].flags & I2C_M_RD)
-			result = i2c_imx_read(i2c_imx, &msgs[i]);
-		else
-			result = i2c_imx_write(i2c_imx, &msgs[i]);
+		if(i2c_imx->use_dma && msgs[i].len >= IMX_I2C_DMA_THRESHOLD) {
+			if (msgs[i].flags & I2C_M_RD)
+				result = i2c_imx_dma_read(i2c_imx, &msgs[i]);
+			else
+				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
+		} else {
+			if (msgs[i].flags & I2C_M_RD)
+				result = i2c_imx_pio_read(i2c_imx, &msgs[i]);
+			else
+				result = i2c_imx_pio_write(i2c_imx, &msgs[i]);
+		}
+
 		if (result)
 			goto fail0;
 	}
@@ -601,6 +908,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	void __iomem *base;
 	int irq, ret;
 	u32 bitrate;
+	u32 phy_addr;
 
 	dev_dbg(&pdev->dev, "<%s>\n", __func__);
 
@@ -611,6 +919,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	phy_addr = res->start;
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -696,6 +1005,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		i2c_imx->adapter.name);
 	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 
+	/* Init DMA config if support*/
+	if (i2c_imx->hwdata->has_dma_support) {
+		dev_info(&pdev->dev, "has_dma_support_begin.\n");
+		i2c_imx->dma = devm_kzalloc(&pdev->dev, sizeof(struct imx_i2c_dma),
+				GFP_KERNEL);
+		if (!i2c_imx->dma) {
+			dev_info(&pdev->dev, "can't allocate dma struct faild use dma.\n");
+			i2c_imx->use_dma = false;
+		}
+		else if (i2c_imx_dma_request(i2c_imx, phy_addr)) {
+			dev_info(&pdev->dev, "can't request dma chan, faild use dma.\n");
+			i2c_imx->use_dma = false;
+		}
+		else {
+			i2c_imx->use_dma = true;
+		}
+	}
+
 	return 0;   /* Return OK */
 }
 
@@ -707,6 +1034,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
 	i2c_del_adapter(&i2c_imx->adapter);
 
+	if (i2c_imx->use_dma)
+		i2c_imx_dma_free(i2c_imx);
+
 	/* setup chip registers to defaults */
 	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
 	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
-- 
1.8.4

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Shawn Guo]

On Thu, Feb 27, 2014 at 02:05:14PM +0800, Yuan Yao wrote:
> Add dma support for i2c. This function depend on DMA driver.
> You can turn on it by write both the dmas and dma-name properties in dts node.
> And you should set ".has_dma_support" as true for dma support in imx_i2c_hwdata struct.
> 
> Signed-off-by: Yuan Yao <yao.yuan-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Just added a few people who might be interested.

Shawn

> ---
>  drivers/i2c/busses/i2c-imx.c | 358 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 344 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index db895fb..6ec392b 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -37,22 +37,27 @@
>  /** Includes *******************************************************************
>  *******************************************************************************/
>  
> -#include <linux/init.h>
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> -#include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/init.h>
>  #include <linux/io.h>
> -#include <linux/sched.h>
> -#include <linux/platform_device.h>
> -#include <linux/clk.h>
> -#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_dma.h>
>  #include <linux/platform_data/i2c-imx.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
>  
>  /** Defines ********************************************************************
>  *******************************************************************************/
> @@ -63,6 +68,9 @@
>  /* Default value */
>  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
>  
> +/* enable DMA if transfer size is bigger than this threshold */
> +#define IMX_I2C_DMA_THRESHOLD	16
> +
>  /* IMX I2C registers:
>   * the I2C register offset is different between SoCs,
>   * to provid support for all these chips, split the
> @@ -88,6 +96,7 @@
>  #define I2SR_IBB	0x20
>  #define I2SR_IAAS	0x40
>  #define I2SR_ICF	0x80
> +#define I2CR_DMAEN	0x02
>  #define I2CR_RSTA	0x04
>  #define I2CR_TXAK	0x08
>  #define I2CR_MTX	0x10
> @@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
>  	unsigned		ndivs;
>  	unsigned		i2sr_clr_opcode;
>  	unsigned		i2cr_ien_opcode;
> +	bool			has_dma_support;
> +};
> +
> +struct imx_i2c_dma {
> +	struct dma_chan		*chan_tx;
> +	struct dma_chan		*chan_rx;
> +	dma_addr_t		buf_tx;
> +	dma_addr_t		buf_rx;
> +	unsigned int		len_tx;
> +	unsigned int		len_rx;
> +	struct completion	cmd_complete;
>  };
>  
>  struct imx_i2c_struct {
> @@ -184,6 +204,9 @@ struct imx_i2c_struct {
>  	int			stopped;
>  	unsigned int		ifdr; /* IMX_I2C_IFDR */
>  	const struct imx_i2c_hwdata	*hwdata;
> +
> +	bool			use_dma;
> +	struct imx_i2c_dma	*dma;
>  };
>  
>  static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
>  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +	.has_dma_support	= false,
>  
>  };
>  
> @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  = {
>  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +	.has_dma_support	= false,
>  
>  };
>  
> @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
>  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> +	.has_dma_support	= true,
>  
>  };
>  
> @@ -254,6 +280,155 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
>  	return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
>  }
>  
> +/** Functions for DMA support ************************************************
> +******************************************************************************/
> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, u32 phy_addr)
> +{
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +	struct dma_slave_config dma_sconfig;
> +	int ret;
> +
> +	dma->chan_tx = dma_request_slave_channel(&i2c_imx->adapter.dev, "tx");
> +	if (!dma->chan_tx) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Dma tx channel request failed!\n");
> +		return -ENODEV;
> +	}
> +
> +	dma_sconfig.dst_addr = (dma_addr_t)phy_addr +
> +				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> +	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconfig.dst_maxburst = 1;
> +	dma_sconfig.direction = DMA_MEM_TO_DEV;
> +	ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
> +	if (ret < 0) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Dma slave config failed, err = %d\n", ret);
> +		dma_release_channel(dma->chan_tx);
> +		return ret;
> +	}
> +
> +	dma->chan_rx = dma_request_slave_channel(&i2c_imx->adapter.dev, "rx");
> +	if (!dma->chan_rx) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Dma rx channel request failed!\n");
> +		return -ENODEV;
> +	}
> +
> +	dma_sconfig.src_addr = (dma_addr_t)phy_addr +
> +				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> +	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconfig.src_maxburst = 1;
> +	dma_sconfig.direction = DMA_DEV_TO_MEM;
> +	ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
> +	if (ret < 0) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Dma slave config failed, err = %d\n", ret);
> +		dma_release_channel(dma->chan_rx);
> +		return ret;
> +	}
> +
> +	init_completion(&dma->cmd_complete);
> +
> +	return 0;
> +}
> +
> +static void i2c_imx_dma_tx_callback(void *arg)
> +{
> +	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +
> +	dma_unmap_single(dma->chan_tx->device->dev, dma->buf_tx,
> +			dma->len_tx, DMA_TO_DEVICE);
> +	complete(&dma->cmd_complete);
> +}
> +
> +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> +{
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +	struct dma_async_tx_descriptor *txdesc;
> +
> +	dma->len_tx = msgs->len-1;
> +	dma->buf_tx = dma_map_single(dma->chan_tx->device->dev, msgs->buf,
> +					dma->len_tx, DMA_TO_DEVICE);
> +	if (dma_mapping_error(dma->chan_tx->device->dev, dma->buf_tx)) {
> +		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
> +		return -EINVAL;
> +	}
> +
> +	txdesc = dmaengine_prep_slave_single(dma->chan_tx, dma->buf_tx,
> +					dma->len_tx, DMA_MEM_TO_DEV,
> +					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +	if (!txdesc) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Not able to get desc for tx\n");
> +		return -EINVAL;
> +	}
> +
> +	txdesc->callback = i2c_imx_dma_tx_callback;
> +	txdesc->callback_param = i2c_imx;
> +	dmaengine_submit(txdesc);
> +	dma_async_issue_pending(dma->chan_tx);
> +	
> +	return 0;
> +}
> +
> +static void i2c_imx_dma_rx_callback(void *arg)
> +{
> +	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +
> +	dma_unmap_single(dma->chan_rx->device->dev, dma->buf_rx,
> +				dma->len_rx, DMA_FROM_DEVICE);
> +	complete(&dma->cmd_complete);
> +}
> +
> +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> +{
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +	struct dma_async_tx_descriptor *txdesc;
> +
> +	dma->len_rx = msgs->len - 2;
> +	dma->buf_rx = dma_map_single(dma->chan_rx->device->dev, msgs->buf,
> +					dma->len_rx, DMA_FROM_DEVICE);
> +	if (dma_mapping_error(dma->chan_rx->device->dev, dma->buf_rx)) {
> +		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
> +		return -EINVAL;
> +	}
> +
> +	txdesc = dmaengine_prep_slave_single(dma->chan_rx, dma->buf_rx,
> +					dma->len_rx, DMA_DEV_TO_MEM,
> +					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +	if (!txdesc) {
> +		dev_err(&i2c_imx->adapter.dev,
> +				"Not able to get desc for rx\n");
> +		return -EINVAL;
> +	}
> +
> +	txdesc->callback = i2c_imx_dma_rx_callback;
> +	txdesc->callback_param = i2c_imx;
> +	dmaengine_submit(txdesc);
> +	dma_async_issue_pending(dma->chan_rx);
> +	
> +	return 0;
> +}
> +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> +{
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +	struct dma_chan *dma_chan;
> +
> +	dma_chan = dma->chan_tx;
> +	dma->chan_tx = NULL;
> +	dma->buf_tx = 0;
> +	dma->len_tx = 0;
> +	dma_release_channel(dma_chan);
> +
> +	dma_chan = dma->chan_rx;
> +	dma->chan_tx = NULL;
> +	dma->buf_rx = 0;
> +	dma->len_rx = 0;
> +	dma_release_channel(dma_chan);
> +}
>  /** Functions for IMX I2C adapter driver ***************************************
>  *******************************************************************************/
>  
> @@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	return IRQ_NONE;
>  }
>  
> -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> +static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
>  {
>  	int i, result;
>  
> @@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  	return 0;
>  }
>  
> -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
> +{
> +	int result, timeout=1000;
> +	unsigned int temp = 0;
> +
> +	reinit_completion(&i2c_imx->dma->cmd_complete);
> +	result = i2c_imx_dma_tx(i2c_imx, msgs);
> +	if(result)
> +		return result;
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp |= I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	/* write slave address */
> +	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> +	result = wait_for_completion_interruptible_timeout(
> +						&i2c_imx->dma->cmd_complete,
> +						msecs_to_jiffies(1000));
> +	if (result == 0)
> +		return  -ETIMEDOUT;
> +
> +	/* waiting for Transfer complete. */
> +	while(timeout--) {
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +		if (temp & 0x80)
> +			break;
> +		udelay(10);
> +	}
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	/* write the last byte */
> +	imx_i2c_write_reg(msgs->buf[msgs->len-1], i2c_imx, IMX_I2C_I2DR);
> +	result = i2c_imx_trx_complete(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	result = i2c_imx_acked(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	return 0;
> +}
> +
> +static int i2c_imx_pio_read(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
>  {
>  	int i, result;
>  	unsigned int temp;
> @@ -518,6 +743,80 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  	return 0;
>  }
>  
> +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
> +{
> +	int result, timeout=1000;
> +	unsigned int temp;
> +
> +	/* write slave address */
> +	imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR);
> +	result = i2c_imx_trx_complete(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	result = i2c_imx_acked(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	/* setup bus to read data */
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~I2CR_MTX;
> +	if (msgs->len - 1)
> +		temp &= ~I2CR_TXAK;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp |= I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	reinit_completion(&i2c_imx->dma->cmd_complete);
> +	result = i2c_imx_dma_rx(i2c_imx, msgs);
> +	if(result)
> +		return result;
> +
> +	result = wait_for_completion_interruptible_timeout(
> +						&i2c_imx->dma->cmd_complete,
> +						msecs_to_jiffies(1000));
> +	if (result == 0)
> +		return  -ETIMEDOUT;
> +
> +	/* waiting for Transfer complete. */
> +	while(timeout--) {
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +		if (temp & 0x80)
> +			break;
> +		udelay(10);
> +	}
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	/* read n-1 byte data */
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp |= I2CR_TXAK;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +	/* read n byte data */
> +	result = i2c_imx_trx_complete(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	/* It must generate STOP before read I2DR to prevent
> +	   controller from generating another clock cycle */
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~(I2CR_MSTA | I2CR_MTX);
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	i2c_imx_bus_busy(i2c_imx, 0);
> +	i2c_imx->stopped = 1;
> +	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +
> +	return 0;
> +}
> +
>  static int i2c_imx_xfer(struct i2c_adapter *adapter,
>  						struct i2c_msg *msgs, int num)
>  {
> @@ -563,10 +862,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>  			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
>  			(temp & I2SR_RXAK ? 1 : 0));
>  #endif
> -		if (msgs[i].flags & I2C_M_RD)
> -			result = i2c_imx_read(i2c_imx, &msgs[i]);
> -		else
> -			result = i2c_imx_write(i2c_imx, &msgs[i]);
> +		if(i2c_imx->use_dma && msgs[i].len >= IMX_I2C_DMA_THRESHOLD) {
> +			if (msgs[i].flags & I2C_M_RD)
> +				result = i2c_imx_dma_read(i2c_imx, &msgs[i]);
> +			else
> +				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
> +		} else {
> +			if (msgs[i].flags & I2C_M_RD)
> +				result = i2c_imx_pio_read(i2c_imx, &msgs[i]);
> +			else
> +				result = i2c_imx_pio_write(i2c_imx, &msgs[i]);
> +		}
> +
>  		if (result)
>  			goto fail0;
>  	}
> @@ -601,6 +908,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	int irq, ret;
>  	u32 bitrate;
> +	u32 phy_addr;
>  
>  	dev_dbg(&pdev->dev, "<%s>\n", __func__);
>  
> @@ -611,6 +919,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	}
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	phy_addr = res->start;
>  	base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> @@ -696,6 +1005,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		i2c_imx->adapter.name);
>  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>  
> +	/* Init DMA config if support*/
> +	if (i2c_imx->hwdata->has_dma_support) {
> +		dev_info(&pdev->dev, "has_dma_support_begin.\n");
> +		i2c_imx->dma = devm_kzalloc(&pdev->dev, sizeof(struct imx_i2c_dma),
> +				GFP_KERNEL);
> +		if (!i2c_imx->dma) {
> +			dev_info(&pdev->dev, "can't allocate dma struct faild use dma.\n");
> +			i2c_imx->use_dma = false;
> +		}
> +		else if (i2c_imx_dma_request(i2c_imx, phy_addr)) {
> +			dev_info(&pdev->dev, "can't request dma chan, faild use dma.\n");
> +			i2c_imx->use_dma = false;
> +		}
> +		else {
> +			i2c_imx->use_dma = true;
> +		}
> +	}
> +
>  	return 0;   /* Return OK */
>  }
>  
> @@ -707,6 +1034,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
>  	i2c_del_adapter(&i2c_imx->adapter);
>  
> +	if (i2c_imx->use_dma)
> +		i2c_imx_dma_free(i2c_imx);
> +
>  	/* setup chip registers to defaults */
>  	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
>  	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> -- 
> 1.8.4
> 
> 

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Shawn Guo]

On Thu, Feb 27, 2014 at 08:55:45PM +0800, Shawn Guo wrote:
> On Thu, Feb 27, 2014 at 02:05:14PM +0800, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in dts node.
> > And you should set ".has_dma_support" as true for dma support in imx_i2c_hwdata struct.
> > 
> > Signed-off-by: Yuan Yao <yao.yuan@freescale.com>
> 
> Just added a few people who might be interested.

Still missed one - Marek.  Sorry.

Shawn

> 
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 358 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 344 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index db895fb..6ec392b 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -37,22 +37,27 @@
> >  /** Includes *******************************************************************
> >  *******************************************************************************/
> >  
> > -#include <linux/init.h>
> > -#include <linux/kernel.h>
> > -#include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dmapool.h>
> >  #include <linux/errno.h>
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/init.h>
> >  #include <linux/io.h>
> > -#include <linux/sched.h>
> > -#include <linux/platform_device.h>
> > -#include <linux/clk.h>
> > -#include <linux/slab.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_dma.h>
> >  #include <linux/platform_data/i2c-imx.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> >  
> >  /** Defines ********************************************************************
> >  *******************************************************************************/
> > @@ -63,6 +68,9 @@
> >  /* Default value */
> >  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> >  
> > +/* enable DMA if transfer size is bigger than this threshold */
> > +#define IMX_I2C_DMA_THRESHOLD	16
> > +
> >  /* IMX I2C registers:
> >   * the I2C register offset is different between SoCs,
> >   * to provid support for all these chips, split the
> > @@ -88,6 +96,7 @@
> >  #define I2SR_IBB	0x20
> >  #define I2SR_IAAS	0x40
> >  #define I2SR_ICF	0x80
> > +#define I2CR_DMAEN	0x02
> >  #define I2CR_RSTA	0x04
> >  #define I2CR_TXAK	0x08
> >  #define I2CR_MTX	0x10
> > @@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
> >  	unsigned		ndivs;
> >  	unsigned		i2sr_clr_opcode;
> >  	unsigned		i2cr_ien_opcode;
> > +	bool			has_dma_support;
> > +};
> > +
> > +struct imx_i2c_dma {
> > +	struct dma_chan		*chan_tx;
> > +	struct dma_chan		*chan_rx;
> > +	dma_addr_t		buf_tx;
> > +	dma_addr_t		buf_rx;
> > +	unsigned int		len_tx;
> > +	unsigned int		len_rx;
> > +	struct completion	cmd_complete;
> >  };
> >  
> >  struct imx_i2c_struct {
> > @@ -184,6 +204,9 @@ struct imx_i2c_struct {
> >  	int			stopped;
> >  	unsigned int		ifdr; /* IMX_I2C_IFDR */
> >  	const struct imx_i2c_hwdata	*hwdata;
> > +
> > +	bool			use_dma;
> > +	struct imx_i2c_dma	*dma;
> >  };
> >  
> >  static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> > @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> >  
> >  };
> >  
> > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  = {
> >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> >  
> >  };
> >  
> > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > +	.has_dma_support	= true,
> >  
> >  };
> >  
> > @@ -254,6 +280,155 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
> >  	return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
> >  }
> >  
> > +/** Functions for DMA support ************************************************
> > +******************************************************************************/
> > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, u32 phy_addr)
> > +{
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_slave_config dma_sconfig;
> > +	int ret;
> > +
> > +	dma->chan_tx = dma_request_slave_channel(&i2c_imx->adapter.dev, "tx");
> > +	if (!dma->chan_tx) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Dma tx channel request failed!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dma_sconfig.dst_addr = (dma_addr_t)phy_addr +
> > +				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> > +	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> > +	dma_sconfig.dst_maxburst = 1;
> > +	dma_sconfig.direction = DMA_MEM_TO_DEV;
> > +	ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
> > +	if (ret < 0) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Dma slave config failed, err = %d\n", ret);
> > +		dma_release_channel(dma->chan_tx);
> > +		return ret;
> > +	}
> > +
> > +	dma->chan_rx = dma_request_slave_channel(&i2c_imx->adapter.dev, "rx");
> > +	if (!dma->chan_rx) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Dma rx channel request failed!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dma_sconfig.src_addr = (dma_addr_t)phy_addr +
> > +				(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> > +	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> > +	dma_sconfig.src_maxburst = 1;
> > +	dma_sconfig.direction = DMA_DEV_TO_MEM;
> > +	ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
> > +	if (ret < 0) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Dma slave config failed, err = %d\n", ret);
> > +		dma_release_channel(dma->chan_rx);
> > +		return ret;
> > +	}
> > +
> > +	init_completion(&dma->cmd_complete);
> > +
> > +	return 0;
> > +}
> > +
> > +static void i2c_imx_dma_tx_callback(void *arg)
> > +{
> > +	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +
> > +	dma_unmap_single(dma->chan_tx->device->dev, dma->buf_tx,
> > +			dma->len_tx, DMA_TO_DEVICE);
> > +	complete(&dma->cmd_complete);
> > +}
> > +
> > +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> > +{
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_async_tx_descriptor *txdesc;
> > +
> > +	dma->len_tx = msgs->len-1;
> > +	dma->buf_tx = dma_map_single(dma->chan_tx->device->dev, msgs->buf,
> > +					dma->len_tx, DMA_TO_DEVICE);
> > +	if (dma_mapping_error(dma->chan_tx->device->dev, dma->buf_tx)) {
> > +		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	txdesc = dmaengine_prep_slave_single(dma->chan_tx, dma->buf_tx,
> > +					dma->len_tx, DMA_MEM_TO_DEV,
> > +					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > +	if (!txdesc) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Not able to get desc for tx\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	txdesc->callback = i2c_imx_dma_tx_callback;
> > +	txdesc->callback_param = i2c_imx;
> > +	dmaengine_submit(txdesc);
> > +	dma_async_issue_pending(dma->chan_tx);
> > +	
> > +	return 0;
> > +}
> > +
> > +static void i2c_imx_dma_rx_callback(void *arg)
> > +{
> > +	struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +
> > +	dma_unmap_single(dma->chan_rx->device->dev, dma->buf_rx,
> > +				dma->len_rx, DMA_FROM_DEVICE);
> > +	complete(&dma->cmd_complete);
> > +}
> > +
> > +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> > +{
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_async_tx_descriptor *txdesc;
> > +
> > +	dma->len_rx = msgs->len - 2;
> > +	dma->buf_rx = dma_map_single(dma->chan_rx->device->dev, msgs->buf,
> > +					dma->len_rx, DMA_FROM_DEVICE);
> > +	if (dma_mapping_error(dma->chan_rx->device->dev, dma->buf_rx)) {
> > +		dev_err(&i2c_imx->adapter.dev, "dma_map_single tx failed\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	txdesc = dmaengine_prep_slave_single(dma->chan_rx, dma->buf_rx,
> > +					dma->len_rx, DMA_DEV_TO_MEM,
> > +					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > +	if (!txdesc) {
> > +		dev_err(&i2c_imx->adapter.dev,
> > +				"Not able to get desc for rx\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	txdesc->callback = i2c_imx_dma_rx_callback;
> > +	txdesc->callback_param = i2c_imx;
> > +	dmaengine_submit(txdesc);
> > +	dma_async_issue_pending(dma->chan_rx);
> > +	
> > +	return 0;
> > +}
> > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> > +{
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_chan *dma_chan;
> > +
> > +	dma_chan = dma->chan_tx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_tx = 0;
> > +	dma->len_tx = 0;
> > +	dma_release_channel(dma_chan);
> > +
> > +	dma_chan = dma->chan_rx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_rx = 0;
> > +	dma->len_rx = 0;
> > +	dma_release_channel(dma_chan);
> > +}
> >  /** Functions for IMX I2C adapter driver ***************************************
> >  *******************************************************************************/
> >  
> > @@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> >  	return IRQ_NONE;
> >  }
> >  
> > -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> > +static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> >  {
> >  	int i, result;
> >  
> > @@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> >  	return 0;
> >  }
> >  
> > -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> > +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> > +{
> > +	int result, timeout=1000;
> > +	unsigned int temp = 0;
> > +
> > +	reinit_completion(&i2c_imx->dma->cmd_complete);
> > +	result = i2c_imx_dma_tx(i2c_imx, msgs);
> > +	if(result)
> > +		return result;
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp |= I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	/* write slave address */
> > +	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > +	result = wait_for_completion_interruptible_timeout(
> > +						&i2c_imx->dma->cmd_complete,
> > +						msecs_to_jiffies(1000));
> > +	if (result == 0)
> > +		return  -ETIMEDOUT;
> > +
> > +	/* waiting for Transfer complete. */
> > +	while(timeout--) {
> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > +		if (temp & 0x80)
> > +			break;
> > +		udelay(10);
> > +	}
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp &= ~I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	/* write the last byte */
> > +	imx_i2c_write_reg(msgs->buf[msgs->len-1], i2c_imx, IMX_I2C_I2DR);
> > +	result = i2c_imx_trx_complete(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	result = i2c_imx_acked(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	return 0;
> > +}
> > +
> > +static int i2c_imx_pio_read(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> >  {
> >  	int i, result;
> >  	unsigned int temp;
> > @@ -518,6 +743,80 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> >  	return 0;
> >  }
> >  
> > +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> > +{
> > +	int result, timeout=1000;
> > +	unsigned int temp;
> > +
> > +	/* write slave address */
> > +	imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR);
> > +	result = i2c_imx_trx_complete(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	result = i2c_imx_acked(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	/* setup bus to read data */
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp &= ~I2CR_MTX;
> > +	if (msgs->len - 1)
> > +		temp &= ~I2CR_TXAK;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp |= I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	reinit_completion(&i2c_imx->dma->cmd_complete);
> > +	result = i2c_imx_dma_rx(i2c_imx, msgs);
> > +	if(result)
> > +		return result;
> > +
> > +	result = wait_for_completion_interruptible_timeout(
> > +						&i2c_imx->dma->cmd_complete,
> > +						msecs_to_jiffies(1000));
> > +	if (result == 0)
> > +		return  -ETIMEDOUT;
> > +
> > +	/* waiting for Transfer complete. */
> > +	while(timeout--) {
> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > +		if (temp & 0x80)
> > +			break;
> > +		udelay(10);
> > +	}
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp &= ~I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	/* read n-1 byte data */
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp |= I2CR_TXAK;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> > +	/* read n byte data */
> > +	result = i2c_imx_trx_complete(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	/* It must generate STOP before read I2DR to prevent
> > +	   controller from generating another clock cycle */
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp &= ~(I2CR_MSTA | I2CR_MTX);
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +	i2c_imx_bus_busy(i2c_imx, 0);
> > +	i2c_imx->stopped = 1;
> > +	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> > +
> > +	return 0;
> > +}
> > +
> >  static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >  						struct i2c_msg *msgs, int num)
> >  {
> > @@ -563,10 +862,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >  			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
> >  			(temp & I2SR_RXAK ? 1 : 0));
> >  #endif
> > -		if (msgs[i].flags & I2C_M_RD)
> > -			result = i2c_imx_read(i2c_imx, &msgs[i]);
> > -		else
> > -			result = i2c_imx_write(i2c_imx, &msgs[i]);
> > +		if(i2c_imx->use_dma && msgs[i].len >= IMX_I2C_DMA_THRESHOLD) {
> > +			if (msgs[i].flags & I2C_M_RD)
> > +				result = i2c_imx_dma_read(i2c_imx, &msgs[i]);
> > +			else
> > +				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
> > +		} else {
> > +			if (msgs[i].flags & I2C_M_RD)
> > +				result = i2c_imx_pio_read(i2c_imx, &msgs[i]);
> > +			else
> > +				result = i2c_imx_pio_write(i2c_imx, &msgs[i]);
> > +		}
> > +
> >  		if (result)
> >  			goto fail0;
> >  	}
> > @@ -601,6 +908,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> >  	void __iomem *base;
> >  	int irq, ret;
> >  	u32 bitrate;
> > +	u32 phy_addr;
> >  
> >  	dev_dbg(&pdev->dev, "<%s>\n", __func__);
> >  
> > @@ -611,6 +919,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	phy_addr = res->start;
> >  	base = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(base))
> >  		return PTR_ERR(base);
> > @@ -696,6 +1005,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
> >  		i2c_imx->adapter.name);
> >  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
> >  
> > +	/* Init DMA config if support*/
> > +	if (i2c_imx->hwdata->has_dma_support) {
> > +		dev_info(&pdev->dev, "has_dma_support_begin.\n");
> > +		i2c_imx->dma = devm_kzalloc(&pdev->dev, sizeof(struct imx_i2c_dma),
> > +				GFP_KERNEL);
> > +		if (!i2c_imx->dma) {
> > +			dev_info(&pdev->dev, "can't allocate dma struct faild use dma.\n");
> > +			i2c_imx->use_dma = false;
> > +		}
> > +		else if (i2c_imx_dma_request(i2c_imx, phy_addr)) {
> > +			dev_info(&pdev->dev, "can't request dma chan, faild use dma.\n");
> > +			i2c_imx->use_dma = false;
> > +		}
> > +		else {
> > +			i2c_imx->use_dma = true;
> > +		}
> > +	}
> > +
> >  	return 0;   /* Return OK */
> >  }
> >  
> > @@ -707,6 +1034,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
> >  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> >  	i2c_del_adapter(&i2c_imx->adapter);
> >  
> > +	if (i2c_imx->use_dma)
> > +		i2c_imx_dma_free(i2c_imx);
> > +
> >  	/* setup chip registers to defaults */
> >  	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> >  	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
> > -- 
> > 1.8.4
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:

[...]

> *****/ @@ -63,6 +68,9 @@
>  /* Default value */
>  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> 
> +/* enable DMA if transfer size is bigger than this threshold */
> +#define IMX_I2C_DMA_THRESHOLD	16

So what's the unit here , potatoes or beers or what ? I suppose it's bytes , but 
please make it explicit in the comment ...

[...]

>  static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
>  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +	.has_dma_support	= false,
> 
>  };
> 
> @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  =
> { .ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +	.has_dma_support	= false,
> 
>  };
> 
> @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
>  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
>  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
>  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> +	.has_dma_support	= true,

So why exactly don't we have a DT prop for determining whether the controller 
has DMA support ?

What about the other controllers, do they not support DMA for some specific 
reason? Please elaborate on that, thank you !
[...]

> +static void i2c_imx_dma_tx_callback(void *arg)
[...]
> +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct i2c_msg
> *msgs) +{
[...]
> +static void i2c_imx_dma_rx_callback(void *arg)
[...]
> +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct i2c_msg
> *msgs) +{
[...]

Looks like there's quite a bit of code duplication in the four functions above, 
can you not unify them ?

Also, can the DMA not do full-duplex operation ? What I see here is just half-
duplex operations , one for RX and the other one for TX .

> +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> +{
> +	struct imx_i2c_dma *dma = i2c_imx->dma;
> +	struct dma_chan *dma_chan;
> +
> +	dma_chan = dma->chan_tx;
> +	dma->chan_tx = NULL;
> +	dma->buf_tx = 0;
> +	dma->len_tx = 0;
> +	dma_release_channel(dma_chan);
> +
> +	dma_chan = dma->chan_rx;
> +	dma->chan_tx = NULL;
> +	dma->buf_rx = 0;
> +	dma->len_rx = 0;
> +	dma_release_channel(dma_chan);

You must make _DEAD_ _SURE_ this function is not ever called while the DMA is 
still active. In your case, I have a feeling that's not handled.

> +}
>  /** Functions for IMX I2C adapter driver
> ***************************************
> **************************************************************************
> *****/
> 
> @@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	return IRQ_NONE;
>  }
> 
> -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg
> *msgs) +static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
>  {
>  	int i, result;
> 
> @@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct
> *i2c_imx, struct i2c_msg *msgs) return 0;
>  }
> 
> -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg
> *msgs) +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
> +{
> +	int result, timeout=1000;
> +	unsigned int temp = 0;
> +
> +	reinit_completion(&i2c_imx->dma->cmd_complete);
> +	result = i2c_imx_dma_tx(i2c_imx, msgs);
> +	if(result)
> +		return result;
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp |= I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	/* write slave address */
> +	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> +	result = wait_for_completion_interruptible_timeout(
> +						&i2c_imx->dma->cmd_complete,
> +						msecs_to_jiffies(1000));

Pull the magic constant of 1000 out and #define it as some I2C_IMX_DMA_TIMEOUT 
please .

> +	if (result == 0)
> +		return  -ETIMEDOUT;
> +
> +	/* waiting for Transfer complete. */
> +	while(timeout--) {
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +		if (temp & 0x80)
> +			break;
> +		udelay(10);
> +	}
> +
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~I2CR_DMAEN;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> +	/* write the last byte */
> +	imx_i2c_write_reg(msgs->buf[msgs->len-1], i2c_imx, IMX_I2C_I2DR);
> +	result = i2c_imx_trx_complete(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	result = i2c_imx_acked(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	return 0;
> +}
> +
> +static int i2c_imx_pio_read(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
>  {
>  	int i, result;
>  	unsigned int temp;
> @@ -518,6 +743,80 @@ static int i2c_imx_read(struct imx_i2c_struct
> *i2c_imx, struct i2c_msg *msgs) return 0;
>  }
> 
> +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> +							struct i2c_msg *msgs)
> +{

Looks like almost an duplication as well...

Besides, full-duplex DMA operation is missing, please explain why.

THanks!

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Shawn Guo]

On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> > 
> >  };
> > 
> > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  =
> > { .ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> > 
> >  };
> > 
> > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > +	.has_dma_support	= true,
> 
> So why exactly don't we have a DT prop for determining whether the controller 
> has DMA support ?

Using DMA or PIO is a decision that should be made by driver on its own,
not device tree.

Shawn

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Shawn Guo]

On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
> On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > > @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> > >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > > +	.has_dma_support	= false,
> > > 
> > >  };
> > > 
> > > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata  =
> > > { .ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > > +	.has_dma_support	= false,
> > > 
> > >  };
> > > 
> > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> > >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > > +	.has_dma_support	= true,
> > 
> > So why exactly don't we have a DT prop for determining whether the controller 
> > has DMA support ?
> 
> Using DMA or PIO is a decision that should be made by driver on its own,
> not device tree.

Sorry.  I misunderstood it.  Yes, we can look at the DT property 'dmas'
to know if the controller has DMA capability.

Shawn

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Friday, February 28, 2014 at 03:23:52 AM, Shawn Guo wrote:
> On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
> > On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > > > @@ -193,6 +216,7 @@ static const struct imx_i2c_hwdata
> > > > imx1_i2c_hwdata  = {
> > > > 
> > > >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> > > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> > > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > > > 
> > > > +	.has_dma_support	= false,
> > > > 
> > > >  };
> > > > 
> > > > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata
> > > > imx21_i2c_hwdata  = { .ndivs			= 
ARRAY_SIZE(imx_i2c_clk_div),
> > > > 
> > > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> > > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > > > 
> > > > +	.has_dma_support	= false,
> > > > 
> > > >  };
> > > > 
> > > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> > > > 
> > > >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> > > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> > > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > > > 
> > > > +	.has_dma_support	= true,
> > > 
> > > So why exactly don't we have a DT prop for determining whether the
> > > controller has DMA support ?
> > 
> > Using DMA or PIO is a decision that should be made by driver on its own,
> > not device tree.
> 
> Sorry.  I misunderstood it.  Yes, we can look at the DT property 'dmas'
> to know if the controller has DMA capability.

You're right. Whether or not does the hardware support DMA transfers is a 
property of the hardware, that's why it should be in DT.

Best regards,
Marek Vasut

RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Yao Yuan]

Hi Marek,

Thank you very much for your suggestion.

> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: Friday, February 28, 2014 4:40 AM
> To: linux-arm-kernel@lists.infradead.org
> Cc: Yuan Yao-B46683; wsa@the-dreams.de; mark.rutland@arm.com;
> shawn.guo@linaro.org; linux-kernel@vger.kernel.org; linux-
> i2c@vger.kernel.org
> Subject: Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
> 
> On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:
> 
> [...]
> 
> > *****/ @@ -63,6 +68,9 @@
> >  /* Default value */
> >  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> >
> > +/* enable DMA if transfer size is bigger than this threshold */
> > +#define IMX_I2C_DMA_THRESHOLD	16
> 
> So what's the unit here , potatoes or beers or what ? I suppose it's
> bytes , but please make it explicit in the comment ...
> 

Yes it's bytes. I will make it explicit in the comment.

> [...]
> 
> >  static const struct imx_i2c_hwdata imx1_i2c_hwdata  = { @@ -193,6
> > +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata  = {
> >  	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> >
> >  };
> >
> > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata
> =
> > { .ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> > +	.has_dma_support	= false,
> >
> >  };
> >
> > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > +	.has_dma_support	= true,
> 
> So why exactly don't we have a DT prop for determining whether the
> controller has DMA support ?
> 
> What about the other controllers, do they not support DMA for some
> specific reason? Please elaborate on that, thank you !

Sorry for my fault. I will modify it.

> [...]
> 
> > +static void i2c_imx_dma_tx_callback(void *arg)
> [...]
> > +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct
> > +i2c_msg
> > *msgs) +{
> [...]
> > +static void i2c_imx_dma_rx_callback(void *arg)
> [...]
> > +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct
> > +i2c_msg
> > *msgs) +{
> [...]
> 
> Looks like there's quite a bit of code duplication in the four functions
> above, can you not unify them ?
> 

Yes, There's looks like quite a bit of code duplication in the four functions above.
I also hate quite a bit of code duplication.
But there are many differences in fact.
If unify them we should add many conditional statements and auxiliary variable.
I think it's superfluous and will damage the readability.
So, I am very confused. And if you think unify them will be better I will modify it. 
Thanks for your suggestion and looking forward to hearing from you.


> Also, can the DMA not do full-duplex operation ? What I see here is just
> half- duplex operations , one for RX and the other one for TX .
> 

Yes, here have two dma channels, one for RX and the other one for TX.
When we request the channel we should determine it for TX or RX.

> > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_chan *dma_chan;
> > +
> > +	dma_chan = dma->chan_tx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_tx = 0;
> > +	dma->len_tx = 0;
> > +	dma_release_channel(dma_chan);
> > +
> > +	dma_chan = dma->chan_rx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_rx = 0;
> > +	dma->len_rx = 0;
> > +	dma_release_channel(dma_chan);
> 
> You must make _DEAD_ _SURE_ this function is not ever called while the
> DMA is still active. In your case, I have a feeling that's not handled.
>

I think this function will not called while the DMA is still 
active because of the Linux synchronization mechanism - completion. 
I used it in the dma function.

> > +}
> >  /** Functions for IMX I2C adapter driver
> > ***************************************
> > **********************************************************************
> > ****
> > *****/
> >
> > @@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void
> *dev_id)
> >  	return IRQ_NONE;
> >  }
> >
> > -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) +static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> >  {
> >  	int i, result;
> >
> > @@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct
> > *i2c_imx, struct i2c_msg *msgs) return 0;  }
> >
> > -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> > +{
> > +	int result, timeout=1000;
> > +	unsigned int temp = 0;
> > +
> > +	reinit_completion(&i2c_imx->dma->cmd_complete);
> > +	result = i2c_imx_dma_tx(i2c_imx, msgs);
> > +	if(result)
> > +		return result;
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp |= I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	/* write slave address */
> > +	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > +	result = wait_for_completion_interruptible_timeout(
> > +						&i2c_imx->dma->cmd_complete,
> > +						msecs_to_jiffies(1000));
> 
> Pull the magic constant of 1000 out and #define it as some
> I2C_IMX_DMA_TIMEOUT please .
> 

Thanks, I will modify it.

> > +	if (result == 0)
> > +		return  -ETIMEDOUT;
> > +
> > +	/* waiting for Transfer complete. */
> > +	while(timeout--) {
> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > +		if (temp & 0x80)
> > +			break;
> > +		udelay(10);
> > +	}
> > +
> > +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	temp &= ~I2CR_DMAEN;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +	/* write the last byte */
> > +	imx_i2c_write_reg(msgs->buf[msgs->len-1], i2c_imx, IMX_I2C_I2DR);
> > +	result = i2c_imx_trx_complete(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	result = i2c_imx_acked(i2c_imx);
> > +	if (result)
> > +		return result;
> > +
> > +	return 0;
> > +}
> > +
> > +static int i2c_imx_pio_read(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> >  {
> >  	int i, result;
> >  	unsigned int temp;
> > @@ -518,6 +743,80 @@ static int i2c_imx_read(struct imx_i2c_struct
> > *i2c_imx, struct i2c_msg *msgs) return 0;  }
> >
> > +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> > +							struct i2c_msg *msgs)
> > +{
> 
> Looks like almost an duplication as well...
> 

Considering the symmetric with them i2c_imx_dma_write.
i2c_imx_dma_write and i2c_imx_pio_write have many differences. So I separate them.
But i2c_imx_dma_read and i2c_imx_pio_read is the same at first part. I may should unify them.
But it's will not symmetric with them i2c_imx_dma_write if unified them. So I don't know which will be better?
Looking forward to hearing from you.

> Besides, full-duplex DMA operation is missing, please explain why.
> 
> THanks!
> 

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:

[...]

> > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> > > 
> > >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > > 
> > > +	.has_dma_support	= true,
> > 
> > So why exactly don't we have a DT prop for determining whether the
> > controller has DMA support ?
> > 
> > What about the other controllers, do they not support DMA for some
> > specific reason? Please elaborate on that, thank you !
> 
> Sorry for my fault. I will modify it.

I would prefer if you could explain why other controllers do have DMA disabled 
even if the hardware does support the DMA operation.
 
> > [...]
> > 
> > > +static void i2c_imx_dma_tx_callback(void *arg)
> > 
> > [...]
> > 
> > > +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct
> > > +i2c_msg
> > > *msgs) +{
> > 
> > [...]
> > 
> > > +static void i2c_imx_dma_rx_callback(void *arg)
> > 
> > [...]
> > 
> > > +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct
> > > +i2c_msg
> > > *msgs) +{
> > 
> > [...]
> > 
> > Looks like there's quite a bit of code duplication in the four functions
> > above, can you not unify them ?
> 
> Yes, There's looks like quite a bit of code duplication in the four
> functions above. I also hate quite a bit of code duplication.
> But there are many differences in fact.
> If unify them we should add many conditional statements and auxiliary
> variable. I think it's superfluous and will damage the readability.
> So, I am very confused. And if you think unify them will be better I will
> modify it. Thanks for your suggestion and looking forward to hearing from
> you.

I'd say try it, the RX and TX callback look almost the same. So does the 
i2c_imx_dma_rx() and i2c_imx_dma_tx() .

> > Also, can the DMA not do full-duplex operation ? What I see here is just
> > half- duplex operations , one for RX and the other one for TX .
> 
> Yes, here have two dma channels, one for RX and the other one for TX.
> When we request the channel we should determine it for TX or RX.

Sorry, I don't quite understand this. If you have two DMA channels, can you not 
use them both to do full-duplex SPI transfer ?

> > > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > > +	struct dma_chan *dma_chan;
> > > +
> > > +	dma_chan = dma->chan_tx;
> > > +	dma->chan_tx = NULL;
> > > +	dma->buf_tx = 0;
> > > +	dma->len_tx = 0;
> > > +	dma_release_channel(dma_chan);
> > > +
> > > +	dma_chan = dma->chan_rx;
> > > +	dma->chan_tx = NULL;
> > > +	dma->buf_rx = 0;
> > > +	dma->len_rx = 0;
> > > +	dma_release_channel(dma_chan);
> > 
> > You must make _DEAD_ _SURE_ this function is not ever called while the
> > DMA is still active. In your case, I have a feeling that's not handled.
> 
> I think this function will not called while the DMA is still
> active because of the Linux synchronization mechanism - completion.
> I used it in the dma function.

This doesn't check whether the completion is actually finished anywhere. I don't 
quite understand how this is safe .

[...]

> > > +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> > > +							struct i2c_msg *msgs)
> > > +{
> > 
> > Looks like almost an duplication as well...
> 
> Considering the symmetric with them i2c_imx_dma_write.
> i2c_imx_dma_write and i2c_imx_pio_write have many differences. So I
> separate them. But i2c_imx_dma_read and i2c_imx_pio_read is the same at
> first part. I may should unify them. But it's will not symmetric with them
> i2c_imx_dma_write if unified them. So I don't know which will be better?
> Looking forward to hearing from you.

The dma_read() looks almost like dma_write(), so I'd also try merging them 
together.

> > Besides, full-duplex DMA operation is missing, please explain why.
> > 
> > THanks!

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Lothar Waßmann]

Hi,

Marek Vasut wrote:
> On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:
> 
> [...]
> > Yes, here have two dma channels, one for RX and the other one for TX.
> > When we request the channel we should determine it for TX or RX.
> 
> Sorry, I don't quite understand this. If you have two DMA channels, can you not 
> use them both to do full-duplex SPI transfer ?
> 
SPI != I2C


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info-AvR2QvxeiV7DiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org
___________________________________________________________

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Friday, February 28, 2014 at 11:59:25 AM, Lothar Waßmann wrote:
> Hi,
> 
> Marek Vasut wrote:
> > On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:
> > 
> > [...]
> > 
> > > Yes, here have two dma channels, one for RX and the other one for TX.
> > > When we request the channel we should determine it for TX or RX.
> > 
> > Sorry, I don't quite understand this. If you have two DMA channels, can
> > you not use them both to do full-duplex SPI transfer ?
> 
> SPI != I2C

You got me, sorry.

Best regards,
Marek Vasut

RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Yao Yuan]

Hi Marek,
> On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:
> 
> [...]
> 
> > > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata
> > > > = {
> > > >
> > > >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> > > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> > > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > > >
> > > > +	.has_dma_support	= true,
> > >
> > > So why exactly don't we have a DT prop for determining whether the
> > > controller has DMA support ?
> > >
> > > What about the other controllers, do they not support DMA for some
> > > specific reason? Please elaborate on that, thank you !
> >
> > Sorry for my fault. I will modify it.
> 
> I would prefer if you could explain why other controllers do have DMA
> disabled even if the hardware does support the DMA operation.
> 

Well, Because of the I2C in I.MX hardware don't support the DMA operation. 
But here I also think has_dma_support isn't necessary.

> > > Also, can the DMA not do full-duplex operation ? What I see here is
> > > just
> > > half- duplex operations , one for RX and the other one for TX .
> >
> > Yes, here have two dma channels, one for RX and the other one for TX.
> > When we request the channel we should determine it for TX or RX.
> 
> Sorry, I don't quite understand this. If you have two DMA channels, can
> you not use them both to do full-duplex SPI transfer ?
> 

Sorry, There are also hard for me. I don't understand what is full-duplex for dma? 
A DMA engine can only read or write at the same time. And a dma channel request for only DMA_MEM_TO_DEV or DMA_DEV_TO_MEM almost.
Also i2c is a half-duplex bus.

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Friday, February 28, 2014 at 12:36:01 PM, Yao Yuan wrote:
> Hi Marek,
> 
> > On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:
> > 
> > [...]
> > 
> > > > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata
> > > > > = {
> > > > > 
> > > > >  	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> > > > >  	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> > > > >  	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> > > > > 
> > > > > +	.has_dma_support	= true,
> > > > 
> > > > So why exactly don't we have a DT prop for determining whether the
> > > > controller has DMA support ?
> > > > 
> > > > What about the other controllers, do they not support DMA for some
> > > > specific reason? Please elaborate on that, thank you !
> > > 
> > > Sorry for my fault. I will modify it.
> > 
> > I would prefer if you could explain why other controllers do have DMA
> > disabled even if the hardware does support the DMA operation.
> 
> Well, Because of the I2C in I.MX hardware don't support the DMA operation.
> But here I also think has_dma_support isn't necessary.

OK, got it now. Thanks!

> > > > Also, can the DMA not do full-duplex operation ? What I see here is
> > > > just
> > > > half- duplex operations , one for RX and the other one for TX .
> > > 
> > > Yes, here have two dma channels, one for RX and the other one for TX.
> > > When we request the channel we should determine it for TX or RX.
> > 
> > Sorry, I don't quite understand this. If you have two DMA channels, can
> > you not use them both to do full-duplex SPI transfer ?
> 
> Sorry, There are also hard for me. I don't understand what is full-duplex
> for dma?

Sorry, nevermind. I was confused by this and some SPI patches review. Like 
Lothar (thanks!) pointed out, using only half-duplex operation is OK.

Best regards,
Marek Vasut

RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Yao Yuan]

Hi, Marek

Marek Vasut wrote:
> On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:
> 
> [...]
> 
> > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_chan *dma_chan;
> > +
> > +	dma_chan = dma->chan_tx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_tx = 0;
> > +	dma->len_tx = 0;
> > +	dma_release_channel(dma_chan);
> > +
> > +	dma_chan = dma->chan_rx;
> > +	dma->chan_tx = NULL;
> > +	dma->buf_rx = 0;
> > +	dma->len_rx = 0;
> > +	dma_release_channel(dma_chan);
> 
> You must make _DEAD_ _SURE_ this function is not ever called while the
> DMA is still active. In your case, I have a feeling that's not handled.
>

Thanks for your attention.
This few days I look up the code for the realization of dma_release_channel(). 
I found that it will disable the dma request first. So it's may safe. 
And drivers hadn't check whether dma is still active before dma_release_channel() as a usually usage.
Because it will be disabled automatic.

The only problem is that, it will be forced to cancel the transfer which was not yet completed.
Looking forward to hearing from you.

Best regards,
Yuan Yao

Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver

[Marek Vasut]

On Monday, March 03, 2014 at 11:23:33 AM, Yao Yuan wrote:
> Hi, Marek
> 
> Marek Vasut wrote:
> > On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:
> > 
> > [...]
> > 
> > > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > > +	struct dma_chan *dma_chan;
> > > +
> > > +	dma_chan = dma->chan_tx;
> > > +	dma->chan_tx = NULL;
> > > +	dma->buf_tx = 0;
> > > +	dma->len_tx = 0;
> > > +	dma_release_channel(dma_chan);
> > > +
> > > +	dma_chan = dma->chan_rx;
> > > +	dma->chan_tx = NULL;
> > > +	dma->buf_rx = 0;
> > > +	dma->len_rx = 0;
> > > +	dma_release_channel(dma_chan);
> > 
> > You must make _DEAD_ _SURE_ this function is not ever called while the
> > DMA is still active. In your case, I have a feeling that's not handled.
> 
> Thanks for your attention.
> This few days I look up the code for the realization of
> dma_release_channel(). I found that it will disable the dma request first.
> So it's may safe. And drivers hadn't check whether dma is still active
> before dma_release_channel() as a usually usage. Because it will be
> disabled automatic.
> 
> The only problem is that, it will be forced to cancel the transfer which
> was not yet completed. Looking forward to hearing from you.

OK

Best regards,
Marek Vasut

[PATCH 3/3] Documentation:add DMA support for freescale i2c driver

[Yuan Yao]

Add i2c dts node properties for eDMA support, them depend on the eDMA driver.

Signed-off-by: Yuan Yao <yao.yuan@freescale.com>
---
 Documentation/devicetree/bindings/i2c/i2c-imx.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
index 4a8513e..52d37fd 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
@@ -11,6 +11,8 @@ Required properties:
 Optional properties:
 - clock-frequency : Constains desired I2C/HS-I2C bus clock frequency in Hz.
   The absence of the propoerty indicates the default frequency 100 kHz.
+- dmas: A list of two dma specifiers, one for each entry in dma-names.
+- dma-names: should contain "tx" and "rx".
 
 Examples:
 
@@ -26,3 +28,12 @@ i2c@70038000 { /* HS-I2C on i.MX51 */
 	interrupts = <64>;
 	clock-frequency = <400000>;
 };
+
+i2c0: i2c@40066000 { /* i2c0 on vf610 */
+	compatible = "fsl,vf610-i2c";
+	reg = <0x40066000 0x1000>;
+	interrupts =<0 71 0x04>;
+	dmas = <&edma0 0 50>,
+		<&edma0 0 51>;
+	dma-names = "rx","tx";
+};
-- 
1.8.4