Hi Carlos,

Thanks for your patch, sorry for having taken so much time, looks
good, just some nitpicks.

...

please sort in alphabetical order

 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>

...

+struct lpi2c_imx_dma {
+	bool			using_pio_mode;
+	u8			rx_cmd_buf_len;
+	u8			*dma_buf;
+	u16			*rx_cmd_buf;
+	unsigned int	dma_len;
+	unsigned int	tx_burst_num;
+	unsigned int	rx_burst_num;
+	unsigned long	dma_msg_flag;
+	resource_size_t		phy_addr;
+	dma_addr_t		dma_tx_addr;
+	dma_addr_t		dma_addr;
+	enum dma_data_direction dma_direction;
+	struct dma_chan		*chan_tx;
+	struct dma_chan		*chan_rx;
+};

The alignment here is a bit off

...

+static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
+{
+	if (!lpi2c_imx->can_use_dma)
+		return false;
+
+	/*
+	 * When the length of data is less than I2C_DMA_THRESHOLD,
+	 * cpu mode is used directly to avoid low performance.
+	 */
+	if (msg->len < I2C_DMA_THRESHOLD)
+		return false;
+
+	return true;

You could do

	return !(msg->len < I2C_DMA_THRESHOLD);

Just a matter of taste, your choice.

+}
+
+static int lpi2c_imx_pio_xfer(struct lpi2c_imx_struct *lpi2c_imx,
+				 struct i2c_msg *msg)
+{
+	int ret;
+
+	reinit_completion(&lpi2c_imx->complete);
+
+	if (msg->flags & I2C_M_RD)
+		lpi2c_imx_read(lpi2c_imx, msg);
+	else
+		lpi2c_imx_write(lpi2c_imx, msg);
+
+	ret = lpi2c_imx_pio_msg_complete(lpi2c_imx);
+	if (ret)
+		return ret;
+
+	return 0;

You could do

	return lpi2c_imx_pio_msg_complete(lpi2c_imx);

Purely taste, your choice, still.

+}

...

+static void lpi2c_cleanup_rx_cmd_dma(struct lpi2c_imx_dma *dma)
+{
+	dmaengine_terminate_sync(dma->chan_tx);
+	dma_unmap_single(dma->chan_tx->device->dev, dma->dma_tx_addr,
+				dma->rx_cmd_buf_len, DMA_TO_DEVICE);

alignment

+}

...

+static int lpi2c_dma_rx_cmd_submit(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	struct lpi2c_imx_dma *dma = lpi2c_imx->dma;
+	struct dma_chan *txchan = dma->chan_tx;
+	struct dma_async_tx_descriptor *rx_cmd_desc;
+	dma_cookie_t cookie;
+
+	dma->dma_tx_addr = dma_map_single(txchan->device->dev,
+						 dma->rx_cmd_buf,
+						 dma->rx_cmd_buf_len, DMA_TO_DEVICE);
+	if (dma_mapping_error(txchan->device->dev, dma->dma_tx_addr)) {
+		dev_err(&lpi2c_imx->adapter.dev, "dma map failed, use pio\n");

/dma/DMA/ and it's valid for every time you have used "dma".

+		return -EINVAL;
+	}
+
+	rx_cmd_desc = dmaengine_prep_slave_single(txchan, dma->dma_tx_addr,
+				 dma->rx_cmd_buf_len, DMA_MEM_TO_DEV,
+				 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

alignment.

+	if (!rx_cmd_desc) {
+		dma_unmap_single(txchan->device->dev, dma->dma_tx_addr,
+				 dma->rx_cmd_buf_len, DMA_TO_DEVICE);

put dma_unmap_single() in a goto exit path.

+		dev_err(&lpi2c_imx->adapter.dev, "dma prep slave sg failed, use pio\n");
+		return -EINVAL;
+	}
+
+	cookie = dmaengine_submit(rx_cmd_desc);
+	if (dma_submit_error(cookie)) {
+		dma_unmap_single(txchan->device->dev, dma->dma_tx_addr,
+				 dma->rx_cmd_buf_len, DMA_TO_DEVICE);
+		dmaengine_desc_free(rx_cmd_desc);
+		dev_err(&lpi2c_imx->adapter.dev, "submitting dma failed, use pio\n");
+		return -EINVAL;
+	}
+
+	dma_async_issue_pending(txchan);
+
+	return 0;
+}
+
+static int lpi2c_dma_submit(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	struct lpi2c_imx_dma *dma = lpi2c_imx->dma;
+	bool read = dma->dma_msg_flag & I2C_M_RD;
+	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	struct dma_chan *chan = read ? dma->chan_rx : dma->chan_tx;

I generally prefer the assignment to be done after the
declaration. It looks more clear.

+	struct dma_async_tx_descriptor *desc;
+	dma_cookie_t cookie;
+
+	dma->dma_direction = dir;
+	dma->dma_addr = dma_map_single(chan->device->dev,
+					     dma->dma_buf,
+					     dma->dma_len, dir);

alignment is off.

+	if (dma_mapping_error(chan->device->dev, dma->dma_addr)) {
+		dev_err(&lpi2c_imx->adapter.dev, "dma map failed, use pio\n");

/dma/DMA/

+		return -EINVAL;
+	}
+
+	desc = dmaengine_prep_slave_single(chan, dma->dma_addr,
+					 dma->dma_len, read ?
+					 DMA_DEV_TO_MEM : DMA_MEM_TO_DEV,
+					 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

alignment off.

+	if (!desc) {
+		dev_err(&lpi2c_imx->adapter.dev, "dma prep slave sg failed, use pio\n");
+		lpi2c_dma_unmap(dma);

put lpi2c_dma_unmape under a goto exit path.

+		return -EINVAL;
+	}
+
+	reinit_completion(&lpi2c_imx->complete);
+	desc->callback = lpi2c_dma_callback;
+	desc->callback_param = (void *)lpi2c_imx;

the cast is not needed.

+	cookie = dmaengine_submit(desc);
+	if (dma_submit_error(cookie)) {
+		dev_err(&lpi2c_imx->adapter.dev, "submitting dma failed, use pio\n");
+		lpi2c_dma_unmap(dma);
+		dmaengine_desc_free(desc);
+		return -EINVAL;
+	}
+
+	/* Can't switch to PIO mode when DMA have started transfer */
+	dma->using_pio_mode = false;
+
+	dma_async_issue_pending(chan);
+
+	return 0;
+}
+
+static int lpi2c_imx_find_max_burst_num(unsigned int fifosize, unsigned int len)
+{
+	unsigned int i;
+
+	for (i = fifosize / 2; i > 0; i--) {
+		if (!(len % i))
+			break;
+	}

braces are not needed

+
+	return i;
+}
+
+/*
+ * For a highest DMA efficiency, tx/rx burst number should be calculated according
+ * to the FIFO depth.
+ */
+static void lpi2c_imx_dma_burst_num_calculate(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	struct lpi2c_imx_dma *dma = lpi2c_imx->dma;
+	unsigned int cmd_num;
+
+	if (dma->dma_msg_flag & I2C_M_RD) {
+		/*
+		 * One RX cmd word can trigger DMA receive no more than 256 bytes.
+		 * The number of RX cmd words should be calculated based on the data
+		 * length.
+		 */
+		cmd_num = DIV_ROUND_UP(dma->dma_len, CHUNK_DATA);
+		dma->tx_burst_num = lpi2c_imx_find_max_burst_num(lpi2c_imx->txfifosize,
+				 cmd_num);
+		dma->rx_burst_num = lpi2c_imx_find_max_burst_num(lpi2c_imx->rxfifosize,
+				 dma->dma_len);
+	} else {
+		dma->tx_burst_num = lpi2c_imx_find_max_burst_num(lpi2c_imx->txfifosize,
+				 dma->dma_len);

Alignment is off.

+	}
+}

...

+/*
+ * When lpi2c in TX DMA mode we can use one DMA TX channel to write

/in/is in/

+ * data word into TXFIFO, but in RX DMA mode it is different.
+ *
+ * LPI2C MTDR register is a command data and transmit data register.

/LPI2C/The LPI2C/

+ * Bit 8-10 is command data field and Bit 0-7 is transmit data field.

/Bit 8-10 is/Bits 8-10 are the/
/Bit 0-7 is/ Bits 0-7 are the/

+ * When the LPI2C master needs to read data, the data number to read

/data number/number of bytes/

+ * should be set in transmit data field and RECV_DATA should be set
+ * into the command data field to receive (DATA[7:0] + 1) bytes. The
+ * recv data command word is made of RECV_DATA in command data field

/in command/in the command/

+ * and the data number to read in transmit data field. When the length

/data number/number of bytes/

+ * of data that needs to be read exceeds 256 bytes, recv data command

/data that needs to be read/data to be read/

+ * word needs to be written to TXFIFO multiple times.
+ *
+ * So when in RX DMA mode, the TX channel also needs to be configured
+ * additionally to send RX command words and the RX command word need

/additionally//
/need/must/

+ * be set in advance before transmitting.
+ */
+static int lpi2c_imx_dma_xfer(struct lpi2c_imx_struct *lpi2c_imx,
+			 struct i2c_msg *msg)

The alignemnt here is off (did you run checkpatch.pl?)

+{
+	struct lpi2c_imx_dma *dma = lpi2c_imx->dma;
+	int ret;
+
+	/* When DMA mode failed before transferring, CPU mode can be used. */

/failed/fails/

+	dma->using_pio_mode = true;
+
+	dma->dma_len = msg->len;
+	dma->dma_msg_flag = msg->flags;
+	dma->dma_buf = i2c_get_dma_safe_msg_buf(msg, I2C_DMA_THRESHOLD);
+	if (!dma->dma_buf)
+		return -ENOMEM;
+
+	ret = lpi2c_dma_config(lpi2c_imx);
+	if (ret) {
+		dev_err(&lpi2c_imx->adapter.dev, "DMA Config Fail, error %d\n", ret);

Please rephrase as:

	... "Failed to configure DMA (%d)\n", ret);

+		goto disable_dma;
+	}
+
+	lpi2c_dma_enable(lpi2c_imx);
+
+	ret = lpi2c_dma_submit(lpi2c_imx);
+	if (ret) {
+		dev_err(&lpi2c_imx->adapter.dev, "DMA submit Fail, error %d\n", ret);

Please rephrase as:

	... "DMA submission failed (%d)\n", ret);

+		goto disable_dma;
+	}
+
+	if (dma->dma_msg_flag & I2C_M_RD) {
+		ret = lpi2c_imx_alloc_rx_cmd_buf(lpi2c_imx);
+		if (ret) {
+			lpi2c_cleanup_dma(dma);
+			goto disable_dma;
+		}
+
+		ret = lpi2c_dma_rx_cmd_submit(lpi2c_imx);
+		if (ret) {
+			lpi2c_cleanup_dma(dma);
+			goto disable_dma;
+		}
+	}
+
+	ret = lpi2c_imx_dma_msg_complete(lpi2c_imx);
+	if (ret) {
+		if (dma->dma_msg_flag & I2C_M_RD)
+			lpi2c_cleanup_rx_cmd_dma(dma);
+		lpi2c_cleanup_dma(dma);
+		goto disable_dma;
+	}
+
+	/* When meet NACK in transfer, cleanup all DMA transfer */

Please rephrase as:

/* When encountering NACK in transfer, clean up all DMA transfers */

+	if ((readl(lpi2c_imx->base + LPI2C_MSR) & MSR_NDF) && !ret) {
+		if (dma->dma_msg_flag & I2C_M_RD)
+			lpi2c_cleanup_rx_cmd_dma(dma);
+		lpi2c_cleanup_dma(dma);
+		ret = -EIO;
+		goto disable_dma;
+	}
+
+	if (dma->dma_msg_flag & I2C_M_RD)
+		dma_unmap_single(dma->chan_tx->device->dev, dma->dma_tx_addr,
+					 dma->rx_cmd_buf_len, DMA_TO_DEVICE);
+	lpi2c_dma_unmap(dma);
+

you could add here:

disable_cleanup_dma:
	lpi2c_cleanup_dma(dma);

and goto here instead of calling lpi2c_cleanup_dma(dma) at each
phase.

+disable_dma:
+	/* Disable I2C DMA function */
+	writel(0, lpi2c_imx->base + LPI2C_MDER);
+
+	if (dma->dma_msg_flag & I2C_M_RD)
+		kfree(dma->rx_cmd_buf);
+
+	if (ret)
+		i2c_put_dma_safe_msg_buf(dma->dma_buf, msg, false);
+	else
+		i2c_put_dma_safe_msg_buf(dma->dma_buf, msg, true);

I could leave a blank line here to put some space between
if...else and return.

+	return ret;
+}

...

Thanks,
Andi