> >>> time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> >>> TEGRA_I2C_TIMEOUT);
> >>> tegra_i2c_mask_irq(i2c_dev, int_mask);
> >>>
> >>> if (time_left == 0) {
> >>> dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> >>> + if (dma) {
> >>> + dmaengine_terminate_all(chan);
> >>> + complete(&i2c_dev->dma_complete);
> >>> + }
> >>
> >> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.
> >
> > DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
> > So still need to check for msg_complete timeout.
>
> You're waiting for DMA completion and then for the I2C message completion.
>
> Hence your code is structured like this:
>
> 1. Issue DMA transfer
> 2. Wait for DMA completion
> 3. Wait for message completion
>
> Why do you need to abort DMA in 3 if it was already completed in 2?
Ok, thought you are referring to msg complete timeout check in dma mode. Yes no need for terminating DMA when msg timeout. Will fix it.
> >>> @@ -740,6 +925,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >>> u32 int_mask;
> >>> unsigned long time_left;
> >>> unsigned long flags;
> >>> + size_t xfer_size;
> >>> + u32 *buffer = 0;
> >>> + int ret = 0;
> >>> + bool dma = false;
> >>> +
> >>> + if (msg->flags & I2C_M_RD)
> >>> + xfer_size = msg->len;
> >>> + else
> >>> + xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> >>> +
> >>> + xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> >>> + dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> >>> + if (dma) {
> >>> + if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> >>> + ret = tegra_i2c_init_dma_param(i2c_dev, true);
> >>> + else if (!i2c_dev->tx_dma_chan)
> >>> + ret = tegra_i2c_init_dma_param(i2c_dev, false);
> >>
> >> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
> >>
> >> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
> >>
> >>> + if (ret < 0) {
> >>> + dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> >>> + dma = false;
> >>> + ret = 0;
> >>> + }
> >>> + }
> >>> +
> >>> + i2c_dev->is_curr_dma_xfer = dma;
> >>
> >>
> > Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
> > So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.
> >
> >
>
> Yes, I made that comment, but then corrected myself. Seems you missed the correction: https://lkml.org/lkml/2019/1/26/217
>
> If you're having troubles with the corporate email, maybe you could try to switch to something else like gmail.
>
> I've tried to apply this series locally, but again it fails to apply. What's the kernel base you're using? You should make your patches on top linux-next (preferably) or mainline.
Yeah somehow missed that. Will move it back to probe..
I am using 5.0-rc1
