Dear Dong Aisheng,

On Fri, Jun 29, 2012 at 04:24:03PM +0800, Marek Vasut wrote:

quoted

This patch implements DMA support into mxs-i2c. DMA transfers are now
enabled via DT. The DMA operation is enabled by default.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
CC: Fabio Estevam <redacted>
Cc: Linux ARM kernel <redacted>
Cc: linux-i2c at vger.kernel.org
CC: Sascha Hauer <s.hauer@pengutronix.de>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
CC: Uwe Kleine-K?nig <redacted>
Cc: Wolfgang Denk <redacted>
Cc: Wolfram Sang <redacted>
---

 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    4 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |  267
 +++++++++++++++++++-- 3 files changed, 251 insertions(+), 22
 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt

b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index
d2bf750..9497ee0 100644

--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt

@@ -5,6 +5,10 @@ Required properties:
 - reg: Should contain registers location and length
 - interrupts: Should contain ERROR and DMA interrupts
 - clock-frequency: desired I2C bus clock frequency in Hz.

+- fsl,i2c-dma-channel: APBX DMA channel for the I2C

Shoundn't this be optional?

DMA channel? No, why?

quoted

+	/*
+	 * 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;

look strange why return 1;

Because it failed. -Esomething might be better ?

quoted

+
+

One more unnecessary line?

quoted

+/* Write failpath. */
+write_init_dma_fail:
+	dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+write_init_pio_fail:
+	return 1;
+}
+

..

quoted

@@ -291,6 +459,7 @@ 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);

Shared with pio?

Doesn't dma_unmap_sg() call check if the buffer is there or not? Therefore it 
shouldn't have any effect on PIO ops. But it can be indeed if-ed.

quoted

 	mxs_i2c_reset(i2c);
 	return -ETIMEDOUT;
 
 }

Regards
Dong Aisheng

Best regards,
Marek Vasut