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.
