Hi Carlos,
Thanks for your patch, sorry for having taken so much time, looks
good, just some nitpicks.
...
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 976d43f73f38..530ca5d76403 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -8,6 +8,8 @@
> #include <linux/clk.h>
> #include <linux/completion.h>
> #include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
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
