On 2018-09-10 19:43, Ajay Gupta wrote:
> Hi Peter,
>
>>>>>>> +
>>>>>>> + if (msgs[i].flags & I2C_M_RD) {
>>>>>>> + /* gpu_i2c_read has implicit start and stop */
>>>>>>> + status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>>>>>> + if (status < 0)
>>>>>>> + return status;
>>>>>>> + } else {
>>>>>>> + /* start on first write message */
>>>>>>> + if (i == 0) {
>>>>>>
>>>>>> This "if (i == 0)" test is completely bogus to me. I fail to see
>>>>>> why the meat of the block should not happen for both writes in a
>>>>>> double-write transfer.
>>>>>>
>>>>>> If the second message is a write, you do not issue any start nor do
>>>>>> you write out the address for the second message. You want to
>>>>>> generate the following for a transfer consisting of 2 write-messages:
>>>>>>
>>>>>> S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>>>> =============
>>>>>>
>>>>>> (where "..." denotes further optional "Data [A]" instances)
>>>>>>
>>>>>> As is, the stuff underlined by equal signs are not generated, at
>>>>>> least as I read the code.
>>>>>>
>>>>>> This is what I meant in my comment around this area for the v9 patch.
>>>>>
>>>>> Oh, I just realized, this probably means that the ccg_write function
>>>>> in patch
>>>>> 2/2 asks for the wrong thing. If this code actually works, the
>>>>> client driver should probably ask for a single-message transfer
>>>>> consisting of the 2-byte rab concatenated with the data buffer.
>>>>> And that actually makes sense, there is no reason to split the two
>>>>> (dependent) parts of the write into separate messages.
>>>> That would require to create new buffer and copy data for each write
>>>> request from UCSI core driver for sending UCSI command. This doesn't
>>>> look proper way of doing it.
>>>
>>> Well, that's the way master_xfer works, so you will just have to live
>>> with it if you do not want a stop in the middle.
>>
>> Bzzt, s/stop/restart/
> Obviously, a stop in middle will not work. For any read or write request,
Yes, with "s/stop/restart/", I meant "replace stop with restart", and not
"stop followed by restart". That's pretty standard notation, but sorry if
that was not clear.
> 2 byte rab write has to go first. We anyway have to separate rab part
> in read transactions so write should be okay to have separate rab write
> message.
That is not at all certain. The two below transfers need not mean the same
thing at all.
S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P
S Addr Wr [A] rab1 [A] rab2 [A] S Addr Wr [A] Data [A] ... P
In fact, I strongly suspect that for the latter case, the device will
interpret the first two Data bytes as rab and that you therefore have to
make sure that the first transfer (w/o restart) is what you generate in
ccg_write.
> All the client driver of this needs to create messages in pairs where first
> one will always be a rab write and second one either read or write.
> I can add a check for this condition in master_xfer if needed.
>
> Do you still see any issue with this?
Yes, this is very much an issue. master_xfer has to start each message with
a start for the first message, and a restart for subsequent messages. And
after the final message there should be a stop. All starts/restarts should
be followed by the address and the read/write bit. Then data in the given
direction. The master_xfer function should never do any device specific
checks for device specific conditions. It simply should not care about
which client driver is currently using it, and instead just stick to
implementing the master_xfer api so that all client drivers know what to
expect. No cludges.
Cheers,
Peter
