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(>dev, sizeof(struct imx_i2c_dma), > + GFP_KERNEL); > + if (!i2c_imx->dma) { > + dev_info(>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(>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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 08:08:28 AM, Yao Yuan wrote: > On Wednesday, March 26, 2014 at 02:27:46 PM, Marek Vasut wrote: > > On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: > > > On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: > > > > On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: > > > > > > > > [...] > > > > > > > > > > > + i2c_imx->use_dma = false; > > > > > > > + } else if (i2c_imx_dma_request(i2c_imx, > > > > > > > +(dma_addr_t)phy_addr)) > > > > > > > > { > > > > > > > > > > > + dev_info(>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. > > > > > > > > You are just wasting space and duplicating data, unless I am wrong. > > > > > > Well, Do you have a better idea? Although I think it's necessary. > > > > 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(>dev, sizeof(struct imx_i2c_dma), + GFP_KERNEL); + if (!i2c_imx->dma) { + dev_info(>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(>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 ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 02:27:46 PM, Marek Vasut wrote: > On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: > > On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: > > > On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: > > > > > > [...] > > > > > > > > > + i2c_imx->use_dma = false; > > > > > > + } else if (i2c_imx_dma_request(i2c_imx, > > > > > > +(dma_addr_t)phy_addr)) > > > > > > { > > > > > > > > > + dev_info(>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. > > > > > > You are just wasting space and duplicating data, unless I am wrong. > > > > Well, Do you have a better idea? Although I think it's necessary. > > 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()"
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: > On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: > > On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: > > > > [...] > > > > > > > + i2c_imx->use_dma = false; > > > > > + } else if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr)) > > > > { > > > > > > > + dev_info(>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. > > > > You are just wasting space and duplicating data, unless I am wrong. > > Well, Do you have a better idea? Although I think it's necessary. I think we disconnected here, sorry. Why can you not use (i2c_imx->dma != NULL) instead of (i2c_imx->use_dma == true) please ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] + 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. You are just wasting space and duplicating data, unless I am wrong. Well, Do you have a better idea? Although I think it's necessary. I think we disconnected here, sorry. Why can you not use (i2c_imx-dma != NULL) instead of (i2c_imx-use_dma == true) please ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 02:27:46 PM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] + 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. You are just wasting space and duplicating data, unless I am wrong. Well, Do you have a better idea? Although I think it's necessary. 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()
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 08:08:28 AM, Yao Yuan wrote: On Wednesday, March 26, 2014 at 02:27:46 PM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote: On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] + 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. You are just wasting space and duplicating data, unless I am wrong. Well, Do you have a better idea? Although I think it's necessary. 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 ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: > On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: > > [...] > > > > > + i2c_imx->use_dma = false; > > > > + } else if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr)) > { > > > > + dev_info(>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. > > You are just wasting space and duplicating data, unless I am wrong. > Well, Do you have a better idea? Although I think it's necessary. Best regards, Yuan Yao N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a��� 0��h���i
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] > > > + i2c_imx->use_dma = false; > > > + } else if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr)) { > > > + dev_info(>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. You are just wasting space and duplicating data, unless I am wrong. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
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 > > --- > > 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 = _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(_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( > > + _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(_imx->adapter.dev, "<%s> write data\n", __func__); > > + > > + /* write data */ > > + for (i = 0; i < msgs->len; i++) { > > + dev_dbg(_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); >
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
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.y...@freescale.com --- 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
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] + 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. You are just wasting space and duplicating data, unless I am wrong. Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote: On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote: [...] + 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. You are just wasting space and duplicating data, unless I am wrong. Well, Do you have a better idea? Although I think it's necessary. Best regards, Yuan Yao N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a��� 0��h���i
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
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 > --- > drivers/i2c/busses/i2c-imx.c | 354 > +-- 1 file changed, 306 > insertions(+), 48 deletions(-) Changelog is missing. [...] > +/* 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 = _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. [...] > 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(_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( > + _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 ... > + 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(_imx->adapter.dev, "<%s> write data\n", __func__); > + > + /* write data */ > + for (i = 0; i < msgs->len; i++) { > + dev_dbg(_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 ? > + 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(_imx->adapter.dev, "IMX I2C adapter registered\n"); > > + /* Init DMA config if support*/ > +
Re: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
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.y...@freescale.com --- drivers/i2c/busses/i2c-imx.c | 354 +-- 1 file changed, 306 insertions(+), 48 deletions(-) Changelog is missing. [...] +/* 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. [...] 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 ... + 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 ? + 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); +