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

[Dmitry Osipenko]

31.01.2019 5:24, 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
> 

Managed to apply patches this time, was my bad. Please take a look and reply to the rest of my comments before sending a new version, thanks.