2014-04-03 07:37:19

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver

On Wed, Mar 26, 2014 at 08:26:27AM +0100, Marek Vasut wrote:
> > > I think we disconnected here, sorry. Why can you not use (i2c_imx->dma !=
> > > NULL) instead of (i2c_imx->use_dma == true) please ?
> >
> > But there are two judge conditions. But only the "i2c_imx->dma", but also
> > whether "i2c_imx_dma_request" success.
> >
> > "i2c_imx->use_dma == true" be equivalent to "i2c_imx->dma != NULL &&
> > !i2c_imx_dma_request()"
>
> + /* Init DMA config if support*/
> + 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, (dma_addr_t)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;
> + }
>
> OK, looking at this one more time, why don't you wrap the allocation of i2c_imx-
> >dma into i2c_imx_dma_request() ? Even better, you can allocate *dma as a local
> variable in i2c_imx_dma_request() and then assign it into i2c_imx->dma only at
> the end of the i2c_imx_dma_request() function , at the point where you are sure
> nothing failed. Then you can check i2c_imx->dma != NULL throughout the code to
> check if the DMA is available, no ?
>
> Shawn, Wolfram, am I talking nonsense or am I just not connecting ?

I'm with you, Marek.

Shawn