RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver

[Yao Yuan]

On Sunday, March 23, 2014 @ 11:50:00 AM, Marek Vasut wrote:
> On Thursday, March 13, 2014 at 02:47:56 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in
> > dts node.
> >
> > Signed-off-by: Yuan Yao <yao.yuan@xxxxxxxxxxxxx>
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 354
> > +++++++++++++++++++++++++++++++++++++------ 1 file changed, 306
> > insertions(+), 48 deletions(-)
> 
> Changelog is missing.

Sorry for this, Maybe the changelog is in the junk box. It's named "[PATCH v3 0/2] i2c: add DMA support for freescale i2c driver". 

> [...]
> 
> > +/* Functions for DMA support */
> > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > +						dma_addr_t phy_addr)
> > +{
> > +	struct imx_i2c_dma *dma = i2c_imx->dma;
> > +	struct dma_slave_config dma_sconfig;
> > +	struct device *dev = &i2c_imx->adapter.dev;
> > +	int ret;
> > +
> > +	dma->chan_tx = dma_request_slave_channel(dev, "tx");
> > +	if (!dma->chan_tx) {
> > +		dev_err(dev, "Dma tx channel request failed!\n");
> 
> DMA is written in all caps, it's an abbrev. for Direct Memory Access .
> Please fix globally.
> 

Ok, I will fix it globally.

> [...]
> 
> >  static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) {
> 
> [...]
> 
> > -	/* write data */
> > -	for (i = 0; i < msgs->len; i++) {
> > -		dev_dbg(&i2c_imx->adapter.dev,
> > -			"<%s> write byte: B%d=0x%X\n",
> > -			__func__, i, msgs->buf[i]);
> > -		imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +		temp |= I2CR_DMAEN;
> > +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +		/* write slave address */
> > +		imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > +		result = wait_for_completion_interruptible_timeout(
> > +					&i2c_imx->dma->cmd_complete,
> > +					msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> > +		if (result <= 0) {
> > +			dmaengine_terminate_all(dma->chan_using);
> > +			if (result)
> > +				return result;
> > +			else
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		/* waiting for Transfer complete. */
> > +		while (timeout--) {
> > +			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > +			if (temp & I2SR_ICF)
> > +				break;
> > +			udelay(10);
> > +		}
> 
> Do you ever check if this really timed out and handle such case at all ?
> I don't see it , but I might be wrong ...
> 

Oh, It's a bug, Thank you very much.

> > +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +		temp &= ~I2CR_DMAEN;
> > +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > +		/* write the last byte */
> > +		imx_i2c_write_reg(msgs->buf[msgs->len-1],
> > +					i2c_imx, IMX_I2C_I2DR);
> >  		result = i2c_imx_trx_complete(i2c_imx);
> >  		if (result)
> >  			return result;
> > +
> >  		result = i2c_imx_acked(i2c_imx);
> >  		if (result)
> >  			return result;
> > +	} else {
> > +		/* write slave address */
> > +		imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > +		result = i2c_imx_trx_complete(i2c_imx);
> > +		if (result)
> > +			return result;
> > +
> > +		result = i2c_imx_acked(i2c_imx);
> > +		if (result)
> > +			return result;
> > +
> > +		dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
> > +
> > +		/* write data */
> > +		for (i = 0; i < msgs->len; i++) {
> > +			dev_dbg(&i2c_imx->adapter.dev,
> > +				"<%s> write byte: B%d=0x%X\n",
> > +				__func__, i, msgs->buf[i]);
> 
> Can you not just have a variable $dev here and avoid having such a long
> noodle in the dev_dbg() call ?
> 

Ok, And I don't think the dev_dbg() is very helpful here now. 

> > +			imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> > +			result = i2c_imx_trx_complete(i2c_imx);
> > +			if (result)
> > +				return result;
> > +			result = i2c_imx_acked(i2c_imx);
> > +			if (result)
> > +				return result;
> > +		}
> >  	}
> >  	return 0;
> >  }
> [...]
> 
> > +		/* waiting for Transfer complete. */
> > +		while (timeout--) {
> > +			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > +			if (temp & I2SR_ICF)
> > +				break;
> > +			udelay(10);
> > +		}
> 
> Timeout handling here as well ...
> 
> [...]
> 
> > @@ -696,6 +936,21 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >  		i2c_imx->adapter.name);
> >  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
> >
> > +	/* 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");
> 
> Or just have $dev variable and you won't have to break the line at all ...
> 
> > +		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;
> > +	}
> 
> Can you not just check if i2c_imx->dma is valid pointer or NULL pointer ?
> Then you won't need a separate variable, for this purpose ... right ?

Sorry and I think what I know is just to check whether it is NULL.
Then for the second question, maybe there are some other ways, But I think it is more tidy and easier 
understanding for using a separate variable, for this purpose.

> [...]
> 

Best regards,
Yuan Yao