RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Sowjanya Komatineni]

> >>>  	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