by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c driver

Hi,

mostly looking good...

> +#define IMX_I2C_DMA_THRESHOLD 16

Have you guessed or measured this value? A comment about this value
would be nice.

>
> +struct imx_i2c_dma {
> + struct dma_chan *chan_tx;
> + struct dma_chan *chan_rx;
> + struct dma_chan *chan_using;
> + struct completion cmd_complete;
> + dma_addr_t dma_buf;
> + unsigned int dma_len;
> + unsigned int dma_transfer_dir;
> + unsigned int dma_data_dir;

Please use proper types as there are things like 'enum
dma_data_direction' and the likes...

> + dmaengine_submit(txdesc);

This can fail, too. Use dma_submit_error() to check.

> + result = wait_for_completion_interruptible_timeout(
> + &i2c_imx->dma->cmd_complete,
> + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));

Have you tested signals extensively? I can't really recommend the
_intrruptible_ here. I don't see any cleaning up to get the bus to a
consistent state again. Most people drop the interruptible sooner or
later.

> + /* Init DMA config if support*/
> + i2c_imx_dma_request(i2c_imx, phy_addr);

Make this function void? DMA is optional anyhow.

Thanks,

Wolfram

Attachments:

(No filename) (1.09 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-11-17 10:42:03

by Yuan Yao

[permalink] [raw]

Subject: RE: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c driver

Thanks for your review.
Your suggestions is really helpful.

I will correct them in v10.

Best Regards,
Yuan Yao

> -----Original Message-----
> From: Wolfram Sang [mailto:[email protected]]
> Sent: Sunday, November 09, 2014 1:35 AM
> To: Yuan Yao-B46683
> Cc: [email protected]; [email protected]; [email protected]; Duan
> Fugang-B38611; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c
> driver
>
> Hi,
>
> mostly looking good...
>
> > +#define IMX_I2C_DMA_THRESHOLD 16
>
> Have you guessed or measured this value? A comment about this value would
> be nice.
>
> >
> > +struct imx_i2c_dma {
> > + struct dma_chan *chan_tx;
> > + struct dma_chan *chan_rx;
> > + struct dma_chan *chan_using;
> > + struct completion cmd_complete;
> > + dma_addr_t dma_buf;
> > + unsigned int dma_len;
> > + unsigned int dma_transfer_dir;
> > + unsigned int dma_data_dir;
>
> Please use proper types as there are things like 'enum
> dma_data_direction' and the likes...
>
> > + dmaengine_submit(txdesc);
>
> This can fail, too. Use dma_submit_error() to check.
>
> > + result = wait_for_completion_interruptible_timeout(
> > + &i2c_imx->dma->cmd_complete,
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
>
> Have you tested signals extensively? I can't really recommend the
> _intrruptible_ here. I don't see any cleaning up to get the bus to a
> consistent state again. Most people drop the interruptible sooner or
> later.
>
> > + /* Init DMA config if support*/
> > + i2c_imx_dma_request(i2c_imx, phy_addr);
>
> Make this function void? DMA is optional anyhow.
>
> Thanks,
>
> Wolfram